Skip to content
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

Model child node properties better #13

Closed
KingOfBrian opened this issue Nov 12, 2016 · 7 comments
Closed

Model child node properties better #13

KingOfBrian opened this issue Nov 12, 2016 · 7 comments
Assignees

Comments

@KingOfBrian
Copy link
Contributor

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
Copy link
Contributor

SquaredTiki commented Nov 26, 2016

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
Copy link
Contributor

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

@KingOfBrian
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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 KingOfBrian self-assigned this Nov 28, 2016
@KingOfBrian
Copy link
Contributor Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants