Add Canvas widget#1519
Conversation
Co-authored-by: Olivier FAURE <couteaubleu@gmail.com>
Returning childids now
Made no op as it previosly did not exist
Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com>
Co-authored-by: Muhammad Ragib Hasin <ragib.hasin@gmail.com>
| /// That Scene is then used as the canvas' contents. | ||
| #[derive(Default)] | ||
| pub struct Canvas { | ||
| alt_text: ArcStr, |
There was a problem hiding this comment.
As mentioned in #1519, "empty alt text" and "no alt text" are different semantic values, so we should make this an option.
… and None, and fix smaller issues
PoignardAzur
left a comment
There was a problem hiding this comment.
Sorry, I saw your commit but forgot to re-review.
I haven't followed the whole Arc<dyn Fn> debate closely, but I'm a little hesitant about the CanvasSizeChanged solution. It seems like it could lead to performance footguns?
Still, it's probably going to be fine in the short term. We can re-evaluate if people complain. And I'd really like us to settle on something, and this seems as good a solution as any.
Aside from that, LGTM.
Could you further specify what that might be? The event should only be emitted on layout changes. Do you mean that we end in a feedback loop, when the user has effectful code directly in the app logic that changes the layout/size of the canvas (which indeed might be an issue, but it's something we should discourage anyways)? |
|
Well it seems like the widget would send one action per time the layout pass is ran. If the layout pass is ran multiple times in a frame, the Xilem view will get multiple actions and render multiple scenes, even though only one is used. |
Unless the canvas size changes multiple times it should still only result in one event because of: if self.size != size {
self.size = size;
ctx.submit_action::<Self::Action>(CanvasSizeChanged { size });
}We could think about batching or coalescing events (similar as I'll keep this PR open until the end of the week to give others the chance to review as I don't think a lot of merge-conflicts will occur anytime soon. |
Alternative to #1506 (actually it builds on top of it again and applies the review comments/suggestions, quite a group effort :))
I was bugged by the
Arc<dyn Fn>and the way it was handled.IMO it's a little bit better layered this way and more predictable when the canvas draw function is actually changed, and allows the app state in the canvas draw function by using
Fn(Arg<'_, State>, &mut Scene, Size)instead of justArc<dyn Fn(&mut Scene, Size)>which would require capturing in any realistic use-case (accessing the user state) and thus theArcwould be somewhat useless, as always a new one needs to be created.Additionally I've added an example (as I think this is good practice when adding new widgets).
(also to check whether this results in latency, i.e. when pressing the button, that the scene is updated immediately instead of a frame-delay (fortunately this doesn't result in frame-delay))