feat: introduce WidgetMut::find_mut #1031
Conversation
Finding widgets by their id in the RenderRoot::edit_root_widget callback makes the code more maintainable because you can rearrange the widgets without having to update the callback. And with the unsafe tree arena find_mut from the root is O(1). To make this pattern convenient this commit also introduces a Default impl for WidgetId that just returns WidgetId::next().
not-my-profile
force-pushed
the
MutateCtx-find_mut
branch
from
f4d9238 to
a064d91
Compare
PoignardAzur
left a comment
PoignardAzur
left a comment
There was a problem hiding this comment.
I get the idea behind this, and it's something we'll eventually support first-class, but right now it doesn't really work.
| /// Return a [`WidgetMut`] to a child widget by its id. | ||
| pub fn find_mut(&mut self, child_id: WidgetId) -> WidgetMut<'_, dyn Widget> { | ||
| let child_state_mut = self | ||
| .widget_state_children | ||
| .reborrow_mut() | ||
| .find_mut(child_id) | ||
| .expect("find_mut: child not found"); |
There was a problem hiding this comment.
So, correct me if I'm missing something, but the problem with that implementation is that WidgetMut will not correctly update the flags of any intermediary nodes between the current widget and the descendant widget.
Like, say you have a A -> B -> C -> D -> E chain and you do A.ctx.find_mut(E) and then update some stuff; the flags for A and E will be updated, but not the flags for B, C and D.
You need something like #1011 to make this method possible.
There was a problem hiding this comment.
Oh, right yeah. Forgot about that.
| impl Default for WidgetId { | ||
| fn default() -> Self { | ||
| Self::next() | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Big no to this. Default implementations that return different things if you call them twice are a huge code smell.
| ctx.render_root().edit_root_widget(|mut root| { | ||
| let mut root = root.downcast::<RootWidget>(); | ||
| let mut flex = RootWidget::child_mut(&mut root); | ||
| let mut flex = flex.downcast(); | ||
| let mut label = Flex::child_mut(&mut flex, 1).unwrap(); | ||
| let mut label = root.ctx.find_mut(self.ids.display); | ||
| let mut label = label.downcast::<Label>(); | ||
| Label::set_text(&mut label, &*self.value); | ||
| }); |
There was a problem hiding this comment.
Note that if you already know the id of the widget you're trying to mutate, you don't need a new method:
ctx.render_root().edit_widget(self.ids.display, |mut label| {
let mut label = label.downcast::<Label>();
Label::set_text(&mut label, &*self.value);
};There was a problem hiding this comment.
By the way, my medium-term plan for this kind of use-case is to create a new kind of string id (something like WidgetKey) that could be assigned to a widget at construction and let you identify them, basically an equivalent of the DOM id attribute.
There was a problem hiding this comment.
you don't need a new method
Oh right yeah. I'll create a new PR to use that in the examples.
my medium-term plan for this kind of use-case is to create a new kind of string id
I really don't see how that would be better than just using WidgetIds. If we have sth else I don't think it should be strings because of type-safety.
| #[derive(Default, Clone)] | ||
| struct WidgetIds { | ||
| display: WidgetId, | ||
| } |
There was a problem hiding this comment.
This doesn't need to implement Default. You could add a new() or create() method.
|
Closing this for now since it needs sth like #1011. I'll open a new PR to make the examples use |
|
Meh calling |
|
Honestly, that doesn't matter. We're talking about doing two Edit: Although I guess we're running all the other passes too, so I see your point. On the other hand, those passes are fairly cheap as long as we don't change layout. |
|
Okay, so having re-read the code:
The examples do change layout (adding buttons or changing text counts), so doubling rewrite passes isn't great for performance. Of course they're small enough that performance doesn't matter, but I get that this isn't the pattern we want to encourage. I'm thinking this definitely needs to be documented somewhere. |
2 participants
Finding widgets by their id in the RenderRoot::edit_root_widget callback makes the code more maintainable because you can rearrange the widgets without having to update the callback. And with the unsafe tree arena find_mut from the root is O(1).
To make this pattern convenient this commit also introduces a Default impl for WidgetId that just returns WidgetId::next().