Skip to content
This repository has been archived by the owner on Sep 6, 2018. It is now read-only.

Unify the iOS and macOS storyboard templates #57

Merged
merged 5 commits into from
Jul 7, 2017

Conversation

djbe
Copy link
Member

@djbe djbe commented May 31, 2017

Wait on #54 as we'll have conflicts.

@djbe djbe added this to the 2.0.0 milestone May 31, 2017
@djbe
Copy link
Member Author

djbe commented May 31, 2017

The output code changes are minimal, the only change of which I can't tell the impact is for macOS, going from:

extension NSWindowController {
  func performSegue<S: StoryboardSegueType>(segue: S, sender: Any? = nil) where S.RawValue == String {
    performSegue(withIdentifier: segue.rawValue, sender: sender)
  }
}
extension NSViewController {
  func performSegue<S: StoryboardSegueType>(segue: S, sender: Any? = nil) where S.RawValue == String {
    performSegue(withIdentifier: segue.rawValue, sender: sender)
  }
}

To:

extension NSSeguePerforming {
  func perform<S: StoryboardSegueType>(segue: S, sender: Any? = nil) where S.RawValue == String {
    performSegue?(withIdentifier: segue.rawValue, sender: sender)
  }
}

@djbe
Copy link
Member Author

djbe commented May 31, 2017

I was also thinking of getting rid of the

extension StoryboardSceneType {
  static func initial{{controller}}() -> {{baseType}} {
    guard let controller = storyboard().instantiateInitial{{controller}}() else {
      fatalError("Failed to instantiate initial{{controller}} for \(self.storyboardName)")
    }
    return controller
  }
}

It would simplify the template code for initial scenes in each storyboard, and it is overwritten anyway for AppKit storyboards and initial scenes with a custom type.

@djbe djbe force-pushed the feature/breaking/unify-storyboard-templates branch from a2c72e1 to dabe066 Compare May 31, 2017 17:12
@djbe
Copy link
Member Author

djbe commented May 31, 2017

Another thing to consider: should we get rid of the enum cases for scenes and leave only the static functions? And rename those functions to sceneName() instead of instantiateSceneName().

@djbe djbe force-pushed the feature/breaking/unify-storyboard-templates branch from f50eff7 to 1f600cf Compare June 4, 2017 22:00
}
return vc
static func storyboard() -> {{prefix}}Storyboard {
return {{prefix}}Storyboard(name: self.storyboardName, bundle: Bundle(for: BundleToken.self))
}
}

extension StoryboardSceneType where Self: RawRepresentable, Self.RawValue == String {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AliSoftware thoughts on that last comment (about removing the cases and using static vars)? It would also mean removing these functions, as we'd directly call .storyboard().instantiateViewController(withIdentifier: ...) and cast if needed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah good idea (except that these should be static let not static var 😛 ), for the sam reason we removed enums case in the other templates. We shouldn't use case if it doesn't make sense to switch over them, and should prefer static let in those conditions indeed 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, we want to be able to instantiate new instances of each scene, that's why I chose for static var (only get, no set)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 Wait, for Segue that actually does make sense to iterate / switch over them, especially for prepare(for segue:) — maybe we're missing use cases where it's also useful to iterate over scenes too? Not seing one rn, but given there is one for Segues maybe think about it twice if we didn't miss a use case for Scenes before starting to change everything?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but for segues we have a string based api to check them during for example prepare(for segue:) and shouldPerform(segue:). Afaik, you can't ask a viewController what it's scene identifier is, nor is it used anywhere else except during instantiation.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But even for scenes we might want to have a reference to a scene without instantiating the VC just yet?

Example enum usage for Segues

https://github.com/SwiftGen/SwiftGen/blob/master/SwiftGen.playground/Pages/Storyboards-Demo.xcplaygroundpage/Contents.swift#L12-L129

override func prepareForSegue(_ segue: UIStoryboardSegue, sender sender: AnyObject?) {
  switch UIStoryboard.Segue.Message(rawValue: segue.identifier)! {
  case .Custom:
    // Prepare for your custom segue transition
  case .NonCustom:
    // Pass in information to the destination View Controller
  }
}

(which reminds me we should update the playgrounds one day…)

Example enum usage for Scenes

For Scenes it might makes sense for people to have a reference to a specific ViewController ref without instantiating right away

func navigate(to scene: StoryboardSceneType) {
  self.navigationController.push(scene.viewController)
}

private static let scenes = [StoryboardScene.Foo.foo, StoryboardScene.Foo.bar]

@IBAction func navigate() {
  go(to: scenes[picker.selectedIndex])
}

Copy link
Member Author

@djbe djbe Jun 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about this then?
https://gist.github.com/djbe/a5b8cc73098a215e83ee3d5240c42b03

(copied from playground)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice, I like the use of small structs instead of cases.

We're sure that this wouldn't that prevent cases where we actually need to switch over scenes, like maybe switch over Scene(rawValue: segue.destination.sceneIdentifier) check which scene is the destination without instantiating a new UIViewController when building that Scene instance?

tbh as I use the Coordinator pattern in all my projects now, and don't use segues at all anymore, I might be forgetting the patterns and use cases we can encounter when dealing with segues, source & destination VCs and peformSegue implementations…

Copy link
Member Author

@djbe djbe Jun 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but destination is an UIViewController instance, which doesn't have a sceneIdentifier property. Unfortunately controllers, once instantiated, can't tell where they come from (they have a storyboard property, but no identifier property).

In a prepareForSegue, you already get the instantiated view controller, no identifier. In shouldPerformSegue, you only get the segue identifier, no destination info.

Segues use identifiers at multiple points, so they should remain enumerable.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, just re read these UIStoryboardSegue doc, confirms that.
Then let's keep segue as enums but move scenes as structs as you suggested!

@djbe djbe force-pushed the feature/breaking/unify-storyboard-templates branch 3 times, most recently from 52aec99 to 6ceb4db Compare June 6, 2017 00:40
@djbe djbe removed the status: WIP label Jun 6, 2017
@djbe djbe force-pushed the feature/breaking/unify-storyboard-templates branch from f97f544 to 31ce737 Compare June 6, 2017 01:51
@djbe djbe mentioned this pull request Jun 6, 2017
23 tasks
@djbe djbe force-pushed the feature/breaking/unify-storyboard-templates branch from 31ce737 to bfbf5e6 Compare June 8, 2017 02:02
@AliSoftware AliSoftware merged commit 607e831 into master Jul 7, 2017
@AliSoftware AliSoftware deleted the feature/breaking/unify-storyboard-templates branch July 7, 2017 17:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants