Add Canvas widget - cont. by philocalyst · Pull Request #1445 · linebender/xilem

philocalyst · 2025-10-31T23:24:58Z

No description provided.

Co-authored-by: Olivier FAURE <couteaubleu@gmail.com>

Returning childids now

Made no op as it previosly did not exist

Now exposed in other modules

PoignardAzur · 2025-11-01T22:29:20Z

To be clear: this is based on #875.

PoignardAzur

Thanks for picking up this work!

masonry/src/testing/harness.rs

masonry/src/widgets/screenshots/masonry__widgets__canvas__tests__hello.png

PoignardAzur · 2025-11-01T22:33:17Z

masonry/src/widgets/snapshots/masonry__widgets__canvas__tests__simple_canvas.snap

We almost never do these kinds of snapshot tests anymore. You should remove this file.

masonry/src/widgets/canvas.rs

Philipp-M · 2025-11-03T10:53:48Z

xilem/src/view/canvas.rs

+/// ```ignore
+/// use xilem::view::canvas;
+/// use masonry::{Scene, Size};
+///
+/// let my_canvas = canvas(|scene: &mut Scene, size: Size| {
+///     // Drawing a simple rectangle that fills the canvas.
+///     scene.fill_rect((0.0, 0.0, size.width, size.height), [0.2, 0.4, 0.8, 1.0]);
+/// });


I think we should strive towards not ignoring the examples in doc-comments.
This has the tendency to quickly be out of sync, I think you can already see that in the diff.

Suggested change

/// ```ignore

/// use xilem::view::canvas;

/// use masonry::{Scene, Size};

///

/// let my_canvas = canvas(|scene: &mut Scene, size: Size| {

/// // Drawing a simple rectangle that fills the canvas.

/// scene.fill_rect((0.0, 0.0, size.width, size.height), [0.2, 0.4, 0.8, 1.0]);

/// });

/// ```

/// use xilem::masonry::{kurbo::{Rect, Size}, peniko::Fill, vello::Scene};

/// use xilem::{Affine, view::canvas};

/// # use xilem::WidgetView;

///

/// # fn fill_canvas() -> impl WidgetView<()> + use<> {

/// let my_canvas = canvas(|scene: &mut Scene, size: Size| {

/// // Drawing a simple rectangle that fills the canvas.

/// scene.fill(

/// Fill::NonZero,

/// Affine::IDENTITY,

/// xilem::palette::css::AQUA,

/// None,

/// &Rect::new(0.0, 0.0, size.width, size.height),

/// );

/// });

/// # my_canvas

/// # }

DJMcNab

Thanks for reviving this! It's an important capability to have.
Thanks also for retaining Richard's commits, so this will have properly attributed authors when it lands.

Most of my comments are the same as in #875, for things which still need addressing.

DJMcNab · 2025-11-03T16:09:23Z

xilem/src/view/canvas.rs

+    fn build(&self, ctx: &mut ViewCtx, _state: &mut State) -> (Self::Element, Self::ViewState) {
+        let widget = widgets::Canvas::from_arc(self.draw.clone());
+
+        let widget_pod = ctx.create_pod(widget);
+        (widget_pod, ())
+    }
+    fn rebuild(
+        &self,
+        prev: &Self,
+        (): &mut Self::ViewState,
+        _ctx: &mut ViewCtx,
+        element: Mut<'_, Self::Element>,
+        _state: &mut State,
+    ) {
+        if !Arc::ptr_eq(&self.draw, &prev.draw) {
+            widgets::Canvas::set_painter_arc(element, self.draw.clone());
+        }
+    }


There are two answers here.

Firstly, the WidgetMut methods on the Canvas widget should be accepting &mut WidgetMut<Self>, rather than just plain WidgetMut. For example, you can look in:

xilem/masonry/src/widgets/button.rs

Lines 77 to 83 in d408dd6

// --- MARK: WIDGETMUT

impl Button {

/// Get a mutable reference to the label.

pub fn child_mut<'t>(this: &'t mut WidgetMut<'_, Self>) -> WidgetMut<'t, dyn Widget> {

this.ctx.get_mut(&mut this.widget.child)

}

}

That would solve this issue.

However, there is also a trick for when you do need to fork a WidgetMut. That's the reborrow_mut method.

DJMcNab · 2025-11-03T16:12:37Z

masonry/src/widgets/canvas.rs

+    /// Users are encouraged to set alt text for the canvas.
+    /// If possible, the alt-text should succinctly describe what the canvas represents.
+    ///
+    /// If the canvas is decorative or too hard to describe through text, users should set alt text to `""`.


Again, the same comment applies as in #875, namely:

Suggested change

/// If the canvas is decorative or too hard to describe through text, users should set alt text to `""`.

/// If the canvas is decorative users should set alt text to `""`.

/// If it's too hard to describe through text, the alt text should be left unset.

/// This allows accessibility clients to know that there is no accessible description of the canvas content.

DJMcNab · 2025-11-03T16:13:15Z

masonry/src/widgets/canvas.rs

+
+// --- MARK: WIDGETMUT ---
+impl Canvas {
+    /// Update the draw function


Suggested change

/// Update the draw function

/// Update the draw function.

And the same for the other items. We want all doc comments to end with a full stop, for consistency.
Unfortunately, this currently isn't linted for.

masonry/src/widgets/canvas.rs

DJMcNab · 2025-11-03T16:17:15Z

masonry/src/widgets/canvas.rs

+    }
+
+    fn accepts_pointer_interaction(&self) -> bool {
+        true


Previous discussion at #875 (comment).

The only case this really impacts is using a canvas within a button; see for example #1429. I think that in most cases, we would want this to be false, as was my position in that original thread. But fixing this can be a follow-up; we certainly don't want to block on this.

That is concretely, please leave this as-is!

DJMcNab · 2025-11-03T16:20:39Z

masonry/src/widgets/canvas.rs

+    type Action
+        = NoAction
+    where
+        Self: Sized;


Suggested change

type Action

= NoAction

where

Self: Sized;

type Action = NoAction;

For consistency with the rest of the widgets, I'd probably also move this to the start of this implementation. But that isn't blocking.

DJMcNab · 2025-11-03T16:23:34Z

xilem/src/view/canvas.rs

+///     );
+/// });
+/// ```
+pub fn canvas(draw: impl Fn(&mut Scene, Size) + Send + Sync + 'static) -> Canvas {


The same comment applies as in #875, that this signature is wrong.
This signature can only be:

Suggested change

pub fn canvas(draw: impl Fn(&mut Scene, Size) + Send + Sync + 'static) -> Canvas {

pub fn canvas(draw: Arc<dyn Fn(&mut Scene, Size) + Send + Sync + 'static>) -> Canvas

// or

pub fn canvas(draw: &Arc<dyn Fn(&mut Scene, Size) + Send + Sync + 'static>) -> Canvas

Because we want this to allow not resetting the draw function every rebuild. Currently the !Arc::ptr_eq line will always return true (i.e. that the items are not equal) because a new Arc is allocated in each call to canvas.

Philipp-M · 2025-11-05T19:41:19Z

xilem/src/view/canvas.rs

+///
+/// ```
+/// use xilem::view::canvas;
+/// use vello::{


More of a nit, and I see reasons for using vello directly, my suggestion of using vello via xilem was intentional, as it's for one not necessary to have vello as direct dependency, and it avoids possible version mismatch.

I don't see it re-exported anywhere though... Should I do that?

Check out my original suggestion (which I actually tested locally, and includes the suggestion below as well)

Now included :) Apologies, I should have integrated the suggestion at the time as I quickly forgot...

Philipp-M · 2025-11-05T19:41:22Z

xilem/src/view/canvas.rs

+///     Scene,
+/// };
+///
+/// let my_canvas = canvas(|scene: &mut Scene, size| {


Same here, wrapping this in a function (even if not visible in the doc for the user) returning impl WidgetView has the advantage of checking whether this is actually a view (i.e. test on the type-checking-level).

paulzhang5511 · 2025-11-10T04:28:47Z

How to save a canvas as an image

DJMcNab · 2025-11-11T10:56:53Z

How to save a canvas as an image

This will need to be a follow-up. To avoid locking up the entire app, it would require a lot of care.

Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com>

…uilds

philocalyst · 2025-11-12T00:54:13Z

Welp... I picked these changes up again and I'm confused what was added? There's a new bound for ViewArgument and &mut state is no longer allowed as a parameter...

Philipp-M · 2025-11-12T10:22:31Z

These changes came from #1444

philocalyst · 2025-11-12T15:03:08Z

Thank you! It's not impossible to work around, I was just overwhelmed trying to figure out its source. ▰▰▰▰▰ Miles Wirht 🙃

…

From: Philipp Mildenberger ***@***.***> Sent: 12 November 2025 02:22 To: linebender/xilem ***@***.***> Cc: Miles Wirht ***@***.***>, Mention ***@***.***> Subject: Re: [linebender/xilem] Add Canvas widget - cont. (PR #1445) Philipp-M left a comment (linebender/xilem#1445) These changes came from #1444 -- Reply to this email directly or view it on GitHub: #1445 (comment) You are receiving this because you were mentioned. Message ID: ***@***.***>

This is shorter, plays better with rustfmt, and is less likely to lead to merge conflicts if two people add a module in parallel.

Unneeded &mut

I can't find the vello rexport :(

@Philipp-M

Thanks to @Philipp-M!!! I forgot about the suggestion

richard-uk1 and others added 23 commits March 10, 2025 15:15

add canvas widget

3967c6e

impl suggestions and add xilem view for canvas

e30fcfc

Apply suggestions from code review

b4dfea6

Co-authored-by: Olivier FAURE <couteaubleu@gmail.com>

implement suggestions

ec5c653

fix after rebase

033e5bd

fix fmt

b63a2c8

Merge branch 'canvas_widget'

39e178b

Removed smallvec reliancy

81bd724

Returning childids now

fixed signature, replacing queryctx with widgetid

0c20108

Made explicit hidden lifetimes

80f5026

fixed test harness import

e01277d

Fixed the harness running method, by pulling in props

e33f0d2

Added the associated type Action

0141e2f

Made no op as it previosly did not exist

removed needless imports

085b383

Now exposed in other modules

Fixed function signatures

68fc4a8

Changed name from new pod to create pod

787b131

Removed unused imports and parameters

a2d1136

Documentation for the canvas accessibility methods

bf710e5

Properly reexported checkbox and buttonpress for access in xilem view

1af0887

removed needless qualification

32bc30b

fixed test snapshot name

964355d

Renamed hello.new to hello_new

05e8eef

Fixed the harness name (whoops)

54fef9d

PoignardAzur changed the title ~~Small fixes for compatability, based off of the original canvas_widget branch~~ Add Canvas widget - contd Nov 1, 2025

PoignardAzur changed the title ~~Add Canvas widget - contd~~ Add Canvas widget - cont Nov 1, 2025

PoignardAzur changed the title ~~Add Canvas widget - cont~~ Add Canvas widget - cont. Nov 1, 2025

PoignardAzur requested changes Nov 1, 2025

View reviewed changes

philocalyst added 2 commits November 1, 2025 19:03

Removed unused files

a3e9942

Removed unwanted debug snapshot test

6b06356

Philipp-M reviewed Nov 3, 2025

View reviewed changes

philocalyst added 2 commits November 3, 2025 10:43

Fixed the example and enabled as required doc test

4d6c88e

Added alt text support on the "build" method

2c13fab

DJMcNab reviewed Nov 3, 2025

View reviewed changes

Philipp-M reviewed Nov 5, 2025

View reviewed changes

philocalyst and others added 6 commits November 11, 2025 19:36

Update masonry/src/widgets/canvas.rs

0020cee

Co-authored-by: Daniel McNab <36049421+DJMcNab@users.noreply.github.com>

Moved associated top to the beginning of impl

38b11e7

Fixed the fucntion signature of canvas

98c43dc

Using vello through xilem

07cd12d

Modified signatures and calls to effectively handle Canvas during reb…

ab4e1f7

…uilds

Merge branch 'main' into canvas_widget_again

5c40096

richard-uk1 and others added 12 commits November 27, 2025 18:48

add canvas widget

d52a528

Reorder reexports in xilem crates (linebender#1459)

989d2cb

This is shorter, plays better with rustfmt, and is less likely to lead to merge conflicts if two people add a module in parallel.

add canvas widget

0199af7

Reorder reexports in xilem crates (linebender#1459)

26310d8

This is shorter, plays better with rustfmt, and is less likely to lead to merge conflicts if two people add a module in parallel.

Fixed view location mismatch

cc85d51

Rebase

f2cc252

Fixed exports

8b33331

Renamed to messagectx

c0dc096

Fixed clippy lint

95fdb3d

Unneeded &mut

Updated signatures

4419765

Fixed example

86a78a3

I can't find the vello rexport :(

Incorporated suggestions for type-checked return value

2966586

Thanks to @Philipp-M!!! I forgot about the suggestion

PoignardAzur mentioned this pull request Nov 30, 2025

Add Canvas widget - take 3 #1506

Closed

philocalyst closed this Dec 1, 2025

-/// ```ignore
-/// use xilem::view::canvas;
-/// use masonry::{Scene, Size};
-///
-/// let my_canvas = canvas(|scene: &mut Scene, size: Size| {
-///     // Drawing a simple rectangle that fills the canvas.
-///     scene.fill_rect((0.0, 0.0, size.width, size.height), [0.2, 0.4, 0.8, 1.0]);
-/// });
+/// ```
+/// use xilem::masonry::{kurbo::{Rect, Size}, peniko::Fill, vello::Scene};
+/// use xilem::{Affine, view::canvas};
+/// # use xilem::WidgetView;
+///
+/// # fn fill_canvas() -> impl WidgetView<()> + use<> {
+/// let my_canvas = canvas(|scene: &mut Scene, size: Size| {
+///     // Drawing a simple rectangle that fills the canvas.
+///     scene.fill(
+///         Fill::NonZero,
+///         Affine::IDENTITY,
+///         xilem::palette::css::AQUA,
+///         None,
+///         &Rect::new(0.0, 0.0, size.width, size.height),
+///     );
+/// });
+/// # my_canvas
+/// # }

	// --- MARK: WIDGETMUT
	impl Button {
	/// Get a mutable reference to the label.
	pub fn child_mut<'t>(this: &'t mut WidgetMut<'_, Self>) -> WidgetMut<'t, dyn Widget> {
	this.ctx.get_mut(&mut this.widget.child)
	}
	}

-    /// If the canvas is decorative or too hard to describe through text, users should set alt text to `""`.
+    /// If the canvas is decorative users should set alt text to `""`.
+    /// If it's too hard to describe through text, the alt text should be left unset.
+    /// This allows accessibility clients to know that there is no accessible description of the canvas content.

-pub fn canvas(draw: impl Fn(&mut Scene, Size) + Send + Sync + 'static) -> Canvas {
+pub fn canvas(draw: Arc<dyn Fn(&mut Scene, Size) + Send + Sync + 'static>) -> Canvas
+// or
+pub fn canvas(draw: &Arc<dyn Fn(&mut Scene, Size) + Send + Sync + 'static>) -> Canvas

Conversation

philocalyst commented Oct 31, 2025

Uh oh!

PoignardAzur commented Nov 1, 2025

Uh oh!

PoignardAzur left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DJMcNab left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulzhang5511 commented Nov 10, 2025

Uh oh!

DJMcNab commented Nov 11, 2025

Uh oh!

philocalyst commented Nov 12, 2025

Uh oh!

Philipp-M commented Nov 12, 2025

Uh oh!

philocalyst commented Nov 12, 2025 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants