Skip to content

Add Canvas widget#1519

Merged
Philipp-M merged 56 commits intolinebender:mainfrom
Philipp-M:canvas_widget_4
Dec 12, 2025
Merged

Add Canvas widget#1519
Philipp-M merged 56 commits intolinebender:mainfrom
Philipp-M:canvas_widget_4

Conversation

@Philipp-M
Copy link
Copy Markdown
Member

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 just Arc<dyn Fn(&mut Scene, Size)> which would require capturing in any realistic use-case (accessing the user state) and thus the Arc would 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))

richard-uk1 and others added 30 commits November 30, 2025 12:37
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>
Comment thread xilem_masonry/src/view/canvas.rs Outdated
Co-authored-by: Muhammad Ragib Hasin <ragib.hasin@gmail.com>
Comment thread masonry/src/widgets/canvas.rs Outdated
/// That Scene is then used as the canvas' contents.
#[derive(Default)]
pub struct Canvas {
alt_text: ArcStr,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As mentioned in #1519, "empty alt text" and "no alt text" are different semantic values, so we should make this an option.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Should be solved

Copy link
Copy Markdown
Contributor

@PoignardAzur PoignardAzur left a comment

Choose a reason for hiding this comment

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

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.

@Philipp-M
Copy link
Copy Markdown
Member Author

It seems like it could lead to performance footguns?

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)?

@PoignardAzur
Copy link
Copy Markdown
Contributor

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.

@Philipp-M
Copy link
Copy Markdown
Member Author

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 pointerrawupdate vs pointermove for pointer events in the web), but that's orthogonal to this PR.

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.

@Philipp-M Philipp-M changed the title Add Canvas widget - take 4 Add Canvas widget Dec 12, 2025
@Philipp-M Philipp-M enabled auto-merge December 12, 2025 14:45
@Philipp-M Philipp-M added this pull request to the merge queue Dec 12, 2025
Merged via the queue into linebender:main with commit 0b1105b Dec 12, 2025
17 checks passed
@Philipp-M Philipp-M deleted the canvas_widget_4 branch December 12, 2025 15:03
@PoignardAzur PoignardAzur mentioned this pull request Dec 13, 2025
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.

5 participants