search

Rightpoint / Eject

An eject button for Interface Builder to generate swift code
MIT License
522 stars 24 forks source link

Model child node properties better #13

Closed KingOfBrian closed 8 years ago

KingOfBrian commented 8 years ago

Eject models XML attributes to properties pretty well, but it does not model XML elements to properties very well. This usually just works, so it's not a big deal, but currently there's no way to inject a node value into the constructor. This is mostly causing issues for frame, but also for passing in target actions into UIBarButtonItem.

SquaredTiki commented 8 years ago

Perhaps it is necessary to allow property values that are XML elements to be specified as Propertys for ObjectDefinitions also (with a new ValueFormat) whilst maintaining their current creation logic. That way that can be specified as initialiser parameters. Thoughts?

SquaredTiki commented 8 years ago

I think ideally properties that are represented by XML elements should be treated equally as those represented by XML attributes.

KingOfBrian commented 8 years ago

Yea, exactly. Adding a ValueFormat like node should make our model declare all the information we. A few notes with some more of my thoughts:

  1. There are tons of tree associations, and not modeling these properties really helps reduce scope. It would be nice to have a pattern that allowed specifying the node associations if needed (for ignoring and injecting), but for it to default to the current behavior so we don't have to model everything.

  2. The pattern of specifying builders is separate from the ObjectDefinition, since not all nodes define objects. I did some initial work, where a Builder could also conform to BuilderLookup, and if it did, would define a scope of valid for sub-builders, but I realized that I could avoid the behavior for now. The key-value contract is pretty robust here, although I currently have to do the exclusions globally which is ugly, but not a huge concern.

  3. I'd love to gather more cases where this is needed for now. frame is the only one I remember, and I'm disabling frame generation so it's not a big problem.

SquaredTiki commented 8 years ago

Interesting, thanks for explaining some of your thoughts.

  1. I see where you are coming from, it's a trade up between verbosity and automation/dynamic behaviour. I agree that such optional specification falling back to the current behaviour would be the best for now. The only concern would be that it would not be immediately clear from looking at the builders why some element-based properties are specified and others are not compared to attribute-based properties where all are.

  2. What would be the advantage of scoped builders? I currently like the implementation where builders are global and need not be defined per ObjectDefinition. Perhaps it would be best to scope exclusions as opposed to the inverse of scoping builders? (assuming I understand correctly)

  3. There's frame, target/action for UIBarButtonItem and effect for UIVisualEffectView. Probably more. Though all of these don't have to be set in the initialiser.

KingOfBrian commented 8 years ago

Yea, I think there are easier ways here than expanding BuilderLookup after looking things over. I think defering this logic to parent.definition and then having the parent determine the association via the property associated with the key path should be a lot more concise. Also, I think the XIBDocument.Declaration class can get removed and tracked via Property.AssociationContextmore consistently. If you have any other thoughts I'm all ears!

I'm probably not going to get a chance to work on this for a while, feel free to take a stab at it if you're interested. My next chunk of time is probably going to be focusing on enhancing the high level code generation API's and integrating with stencil.

SquaredTiki commented 8 years ago

Agreed, perhaps such logic would be best handled by the parent. I'll have a think about it and if I do get some time I might take a look and PR my changes.

Looking forward to seeing this project evolve, will definitely be keeping a close eye on things and contributing where I can!

KingOfBrian commented 8 years ago

I pushed up some improvements here, such that the property can provide a mapping for a child node. Turned out to be pretty easy, and I needed to do it to clean up the modeling of commands a bit in preparation for Stencil.

I was able to get rid of a global hack for fontDescriptor -> font, but haven't looked at the bar item actions or visual effect. I'll log separate bugs for those.

Thanks for talking me through it though!