-
Notifications
You must be signed in to change notification settings - Fork 739
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Segues support #8
Conversation
@@ -86,4 +101,12 @@ validateVC.title | |||
not known by the storyboard. But it should work correctly in a real project. */ | |||
// let cgu = UIStoryboard.Name.Wizzard.createAccountViewController | |||
|
|||
let segue = UIStoryboard.Segue.Message(rawValue: "CustomSegue")! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that your string associated with the .CustomSegue
case above is "Custom"
not "CustomSegue"
I guess this doesn't actually work? Probably forgot to update your example then? (reading that from GitHub so not actually tested here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are 💯% correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be a good idea to add an example with real-world code BTW, like how you would use that enum to typically implement prepareForSegue
(even in case you need to comment that code if it doesn't work in a Playground context, that'd still be useful for the reader that opens the playground to understand typical usages)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this to the end of that Storyboard
/*******************************************************************************
This is a «real world» example of how you can benefit from the generated enum;
you can easily switch or directly compare the passed int `segue` with the corresponding
segues for a specific storyboard.
*******************************************************************************/
//override func prepareForSegue(_ segue: UIStoryboardSegue, sender sender: AnyObject?) {
// switch UIStoryboard.Segue.Message(rawValue: segue.identifier)! {
// case .CustomSegue:
// // Prepare for your custom segue transition
// case .NonCustomSegue:
// // Pass in information to the destination View Controller
// }
//}
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except for the typo ("the passed int segue
"), seems 👌
readyForFirstObject = false | ||
case "connections": | ||
readyForConnections = true | ||
case _ where readyForConnections: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not case "segue" where readyForConnections
, just to be sure to only grab segue
nodes here?
Note that I used _
for the objects
case above because they can actually be of various type, like viewController
or tableViewController
etc, but I don't think that could actually be the case for segue
… And I even wonder if that <connection>
node doesn't also contain IBOutlets
if any, may be worth adding some in the demo storyboard to check that out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would have to add then a custom view controller class to connect the IBOutlet
s there which in turn would «bloat» the Storyboard
.
But is worth checking that the <connections>
won't ever contain stuff other than segue
s and perhaps clean up that piece of the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good point. Probably don't do it in the storyboard actually shipped with the playground & code, but still worth checking in a storyboard created aside 😉
(That's why we need #2 to add tests 😄 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checked on another project of mine and here's a Storyboard
scene with outlets
<connections>
<outlet property="hairlineView" destination="VRg-OY-4IC" id="PuB-XN-okm"/>
<outlet property="portTextField" destination="6b3-TB-J53" id="Ks7-gi-ZD7"/>
<outlet property="serverTextField" destination="Lfy-mJ-Msa" id="apH-fb-2cT"/>
<segue destination="Z5I-uz-FR0" kind="show" identifier="ShowClientsSegue" id="cml-0d-ha0"/>
</connections>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I knew I've seen those outlets here some times back! Better add that "segue"
in the case
then!
@@ -4,7 +4,8 @@ import Foundation | |||
|
|||
public final class SwiftGenStoryboardEnumBuilder { | |||
private typealias Scene = (storyboardID: String, customClass: String?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably rename that tuple to something else for consistency, to give it a name that can both fit for Scenes and Segues (and then use identifier
for the first entry name, instead of storyboardID
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argh… seems like I discarded that change…
I had it like StoryboardElement
or UIElement
can't remember and the first parameter was actually identifier
:/
Which name would you rather use? or none of the above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
StoryboardElement
seems ok. Entry
could do it too I guess.
I see the idea behind UIElement
but think it's took generic (and somehow makes me think about AccessibilityElement
, not sure why)
Or… we could also add another typealias
, even if it's equal to the same tuple type, just to clarify things (and to be more flexible if some day we need to add another parameter to that tuple for scenes but not segues or the other way around)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I explored adding an extra typealias
but thought it would be weird.
Honestly you suggesting it kinds of validates my first thought.
Let me add it and commit that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💃
Added for readability since both tuples are exactly the same but makes reading the code easier.
@@ -70,17 +83,19 @@ public final class SwiftGenStoryboardEnumBuilder { | |||
} | |||
} | |||
|
|||
public func build(enumName enumName: String? = "Name", indentation indent : SwiftGenIndentation = .Spaces(4)) -> String { | |||
public func build(scenesEnumName enumName: String? = "Scene", seguesEnumName: String? = "Segue", indentation indent : SwiftGenIndentation = .Spaces(4)) -> String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we have two different enums
, it doesn't even make sense anymore to let the option to the user to not provide an enumName
(which so far was possible and generated all the enums
for the Scenes
at the root of the UIStoryboard
extension). So we better migrate those two parameters to non-optional String
s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed here 3983edc
I left default values though just in case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, those default values are still good to keep 😃
Since there's 2 different enums now it is «required» to pass the names of both of them.
- Added back the `scenesEnumName` explicit name to the `build` method. - Added a `case segue` to the `switch` due to the `outlets` added in the `connections` node.
Seems good from here now, will merge tomorrow after some good sleep. |
for (name, scenes) in storyboards { | ||
|
||
/// Scenes | ||
let s = t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that those params are not optional anymore and those intermediate enums can't be omitted, this s
can go away and be replaced by t
everywhere
As noted by @AliSoftware it is not necessary since its «derived» from the `enum`s name. (most likely).
- Replaced uses of `s` in favor of all `t`s - As suggested by @AliSoftware assigning the `rawValue` of the segue enum cases to `segue.segueID`
[SwiftGen-Storyboard] Add Segues to the generated enums
Fix #3
Done:
Storyboard
fileScenes
extension forUIStoryboard
and 1Segues
extensionStoryboards
that have at least 1 (others are ignored)Scene
enum andSegue
enum are now used instead ofName
Generated code: