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

Add widget wrapper and implementation macros. #1511

Merged
merged 1 commit into from
Jan 7, 2021

Conversation

rjwittams
Copy link
Collaborator

To support bindings and other cases where access and mutation of existing widgets is desired.

Note (can be ignored):

It is quite involved to actually make use of this access and mutation in the way you would expect (e.g changing a font of a label by clicking a button - the font not being in the data argument but rather a member variable of the label).

This is because of the ownership of widgets in druid. Widgets that are in scope during construction can't be directly fiddled with (e.g in onclick) because they will have moved by the time any events are processed. Effectively you need to send a command up to some parent, and it needs to navigate down to the thing it needs to change. This is not particularly succinct and theres no obvious mapping between the lexical name of the widget as you've constructed it, and the navigation you need to do.

That is what BindingHost in druid_bindings does - decouples this by pushing changes up into app data, and then down from app data into member variables.

To support bindings and other cases where access and mutation of existing widgets is desired.
@rjwittams rjwittams requested a review from cmyr January 6, 2021 15:16
Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

I agree with the overall project of exposing the inner child of a single-child container, but ideally we would keep that code as simple & dumb as possible. In general I like to avoid hanging methods off of traits, because it means the trait needs to be in scope to use the method.

If there's a particular reason to prefer the trait I'm definitely open to hearing it.

Thanks!

@@ -0,0 +1,47 @@
/// A trait for widgets that wrap a single child to expose that child for access and mutation
pub trait WidgetWrapper {
Copy link
Member

Choose a reason for hiding this comment

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

Do we actually make use of the trait-ness of this trait anywhere?

My inclination would be to just have independent methods on each widget, instead of a trait (which needs to be in scope). That way we would have just Container::child and Container::child_mut methods.

An annoying consideration here is also that we are inconsistent about monomorphizing widget params; that is, some single-child containers are parameterized over their child type, and some just box unilaterally.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we don't have the trait, I need to implement them all over again in bindings in order to treat this generically, and so would any other scheme trying to do the same thing. I guess coherence sometimes forces things like that anyway, but it didn't yet.
The lack of traits in Contexts does cause real problems with duplicated code.

Also, these methods are fairly useless for anything that isn't some kind of general scheme for access.
To use them in a dynamic way (which is the only real use case, otherwise you would set it up in construction), you need to implement so much boiler plate (controllers, commands, navigation, threading through widget ids), that an import seems like the last of our worries.

I have to admit I don't agree with the 'hanging methods off of traits' objection really. Importing is just normal, and IDEs will do it for you.

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

Although you've mentioned it a few times I have no clear sense of the design you have in mind for bindings, so it's hard for me to really consider that case. If we think of this patch as explicitly adding some functionality that will let you experiment with some other things out-of-tree, that makes sense to me.

@rjwittams
Copy link
Collaborator Author

Yes that is the main point of it - I'm sure all this will change in future (esp with crochet/ druid becoming more dom-ish? ). Really this is just an attempt at something minimally invasive and the widget wrapper import maybe stops too many people depending on it.

@rjwittams rjwittams merged commit 497be22 into linebender:master Jan 7, 2021
@rjwittams rjwittams deleted the widget-wrapper branch January 7, 2021 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants