-
Notifications
You must be signed in to change notification settings - Fork 204
Add Canvas widget - take 3 #1506
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
Changes from all commits
43856af
1baa22e
0d43e8a
df55e68
3e973d2
43a6cfd
2a0f296
afd330a
a30deae
62ca993
2e03fad
2f8957f
053ff93
aaa629d
07f2eb3
593cf85
8e36ac6
0d2dedc
288d062
2af781d
47300c5
ed36469
7d1f65e
4891caf
bd83e3b
3b3cf61
d162f63
1e722dd
ffdd9cf
e5ce6ae
ce63927
b65db9e
d5da439
a6f35cd
2e045f1
5310240
b3f677a
21ee038
82e54b6
554c799
d3a35b9
f26556f
af646b4
403ecd7
404cdd2
c61251e
c56864c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,231 @@ | ||
| // Copyright 2025 the Xilem Authors and the Druid Authors | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| //! A canvas widget. | ||
|
|
||
| use std::sync::Arc; | ||
|
|
||
| use accesskit::{Node, Role}; | ||
| use masonry_core::core::{ChildrenIds, NoAction}; | ||
| use tracing::{Span, trace_span}; | ||
| use vello::Scene; | ||
| use vello::kurbo::Size; | ||
|
|
||
| use crate::core::{ | ||
| AccessCtx, AccessEvent, BoxConstraints, EventCtx, LayoutCtx, PaintCtx, PointerEvent, | ||
| PropertiesMut, PropertiesRef, RegisterCtx, TextEvent, Update, UpdateCtx, Widget, WidgetId, | ||
| WidgetMut, | ||
| }; | ||
|
|
||
| // TODO - Add background color? | ||
|
|
||
| /// A widget allowing custom drawing. | ||
| /// | ||
| /// A canvas takes a painter callback; every time the canvas is repainted, that callback | ||
| /// in run with a [`Scene`]. | ||
| /// That Scene is then used as the canvas' contents. | ||
| pub struct Canvas { | ||
| draw: Arc<dyn Fn(&mut Scene, Size) + Send + Sync + 'static>, | ||
| alt_text: Option<String>, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't hurt to have an
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The thing is, "alt text is unset" and "alt text is set to empty string" have different semantic meanings, though I'm not sure whether screen readers really interpret them that differently in practice. |
||
| } | ||
|
|
||
| // --- MARK: BUILDERS | ||
| impl Canvas { | ||
| /// Create a new canvas with the given draw function. | ||
| pub fn new(draw: impl Fn(&mut Scene, Size) + Send + Sync + 'static) -> Self { | ||
| Self::from_arc(Arc::new(draw)) | ||
| } | ||
|
|
||
| /// Create a new canvas from a function already contained in an [`Arc`]. | ||
| pub fn from_arc(draw: Arc<dyn Fn(&mut Scene, Size) + Send + Sync + 'static>) -> Self { | ||
| Self { | ||
| draw, | ||
| alt_text: None, | ||
| } | ||
| } | ||
|
|
||
| /// Set the text that will describe the canvas to screen readers. | ||
| /// | ||
| /// 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 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 with_alt_text(mut self, alt_text: impl Into<String>) -> Self { | ||
| self.alt_text = Some(alt_text.into()); | ||
| self | ||
| } | ||
| } | ||
|
|
||
| // --- MARK: WIDGETMUT | ||
| impl Canvas { | ||
| /// Update the draw function. | ||
| pub fn set_painter( | ||
| this: &mut WidgetMut<'_, Self>, | ||
| draw: impl Fn(&mut Scene, Size) + Send + Sync + 'static, | ||
| ) { | ||
| Self::set_painter_arc(this, Arc::new(draw)); | ||
| } | ||
|
|
||
| /// Update the draw function. | ||
| pub fn set_painter_arc( | ||
| this: &mut WidgetMut<'_, Self>, | ||
| draw: Arc<dyn Fn(&mut Scene, Size) + Send + Sync + 'static>, | ||
| ) { | ||
| this.widget.draw = draw; | ||
| this.ctx.request_render(); | ||
|
Comment on lines
+76
to
+77
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Something I'm wondering for a longer time already, is whether we should check within these setters whether really something changed (i.e. in this case if the arc pointer has changed), and only then request a render. As we often (always?) do this in Xilem it's not necessary here for Xilem at least, but I think it might be nice to have this built into masonry? See also https://github.com/ricvelozo/xilem/pull/1/files#diff-7d717e7fd640e50cef624a6fa2afb9c20968470e5849c8b3b0abe9782499814eR67-R72
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know. In general, that kind of reasoning is how you end up with multiple layers of caching in your app that all check whether some parameter changed. In practice, repainting is usually cheap and having Masonry always repaint something when a parameter is set is more predictable, so I'd lean towards the current design. |
||
| } | ||
|
|
||
| /// Set the text that will describe the canvas to screen readers. | ||
| /// | ||
| /// See [`Canvas::with_alt_text`] for details. | ||
| pub fn set_alt_text(mut this: WidgetMut<'_, Self>, alt_text: String) { | ||
| this.widget.alt_text = Some(alt_text); | ||
| this.ctx.request_accessibility_update(); | ||
| } | ||
|
|
||
| /// Remove the canvas' alt text. | ||
| /// | ||
| /// See [`Canvas::with_alt_text`] for details. | ||
| pub fn remove_alt_text(mut this: WidgetMut<'_, Self>) { | ||
| this.widget.alt_text = None; | ||
| this.ctx.request_accessibility_update(); | ||
| } | ||
|
Comment on lines
+80
to
+94
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See the comment below, I think we can just sum this up in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we might just want to pass an |
||
| } | ||
|
|
||
| // --- MARK: IMPL WIDGET | ||
| impl Widget for Canvas { | ||
| type Action = NoAction; | ||
|
|
||
| fn on_pointer_event( | ||
| &mut self, | ||
| _ctx: &mut EventCtx<'_>, | ||
| _props: &mut PropertiesMut<'_>, | ||
| _event: &PointerEvent, | ||
| ) { | ||
| } | ||
|
Comment on lines
+101
to
+107
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't methods with default impls be omitted? |
||
|
|
||
| // TODO - Do we want the Canvas to be transparent to pointer events? | ||
| fn accepts_pointer_interaction(&self) -> bool { | ||
| true | ||
| } | ||
|
|
||
| fn on_text_event( | ||
| &mut self, | ||
| _ctx: &mut EventCtx<'_>, | ||
| _props: &mut PropertiesMut<'_>, | ||
| _event: &TextEvent, | ||
| ) { | ||
| } | ||
|
|
||
| fn on_access_event( | ||
| &mut self, | ||
| _ctx: &mut EventCtx<'_>, | ||
| _props: &mut PropertiesMut<'_>, | ||
| _event: &AccessEvent, | ||
| ) { | ||
| } | ||
|
|
||
| fn register_children(&mut self, _ctx: &mut RegisterCtx<'_>) {} | ||
|
|
||
| fn update( | ||
| &mut self, | ||
| _ctx: &mut UpdateCtx<'_>, | ||
| _props: &mut PropertiesMut<'_>, | ||
| _event: &Update, | ||
| ) { | ||
| } | ||
|
|
||
| fn layout( | ||
| &mut self, | ||
| ctx: &mut LayoutCtx<'_>, | ||
| _props: &mut PropertiesMut<'_>, | ||
| bc: &BoxConstraints, | ||
| ) -> Size { | ||
| // We use all the available space as possible. | ||
| let size = bc.max(); | ||
|
|
||
| // We clip the contents we draw. | ||
| ctx.set_clip_path(size.to_rect()); | ||
|
|
||
| size | ||
| } | ||
|
|
||
| fn paint(&mut self, ctx: &mut PaintCtx<'_>, _props: &PropertiesRef<'_>, scene: &mut Scene) { | ||
| (self.draw)(scene, ctx.size()); | ||
| } | ||
|
|
||
| fn accessibility_role(&self) -> Role { | ||
| Role::Canvas | ||
| } | ||
|
|
||
| fn accessibility( | ||
| &mut self, | ||
| _ctx: &mut AccessCtx<'_>, | ||
| _props: &PropertiesRef<'_>, | ||
| node: &mut Node, | ||
| ) { | ||
| if let Some(text) = &self.alt_text { | ||
| node.set_description(text.clone()); | ||
| } | ||
| } | ||
|
|
||
| fn children_ids(&self) -> ChildrenIds { | ||
| ChildrenIds::new() | ||
| } | ||
|
|
||
| fn make_trace_span(&self, widget_id: WidgetId) -> Span { | ||
| trace_span!("Canvas", id = widget_id.trace()) | ||
| } | ||
|
|
||
| fn get_debug_text(&self) -> Option<String> { | ||
| self.alt_text.clone() | ||
| } | ||
| } | ||
|
|
||
| // --- MARK: TESTS | ||
| #[cfg(test)] | ||
| mod tests { | ||
| use masonry_core::core::{DefaultProperties, Properties}; | ||
| use masonry_testing::assert_render_snapshot; | ||
| use vello::kurbo::{Affine, BezPath, Stroke}; | ||
| use vello::peniko::{Color, Fill}; | ||
|
|
||
| use super::*; | ||
| use crate::testing::TestHarness; | ||
|
|
||
| #[test] | ||
| fn simple_canvas() { | ||
| let canvas = Canvas::new(|scene, size| { | ||
| let scale = Affine::scale_non_uniform(size.width, size.height); | ||
| let mut path = BezPath::new(); | ||
| path.move_to((0.1, 0.1)); | ||
| path.line_to((0.9, 0.9)); | ||
| path.line_to((0.9, 0.1)); | ||
| path.close_path(); | ||
| path = scale * path; | ||
| scene.fill( | ||
| Fill::NonZero, | ||
| Affine::IDENTITY, | ||
| Color::from_rgb8(100, 240, 150), | ||
| None, | ||
| &path, | ||
| ); | ||
| scene.stroke( | ||
| &Stroke::new(4.), | ||
| Affine::IDENTITY, | ||
| Color::from_rgb8(200, 140, 50), | ||
| None, | ||
| &path, | ||
| ); | ||
| }); | ||
|
|
||
| let mut harness = TestHarness::create( | ||
| DefaultProperties::default(), | ||
| canvas.with_props(Properties::default()), | ||
| ); | ||
|
|
||
| assert_render_snapshot!(harness, "canvas_simple"); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,112 @@ | ||
| // Copyright 2024 the Xilem Authors | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| use std::sync::Arc; | ||
|
|
||
| use masonry::widgets; | ||
| use vello::Scene; | ||
| use vello::kurbo::Size; | ||
|
|
||
| use crate::core::{Arg, MessageCtx, MessageResult, Mut, View, ViewArgument, ViewMarker}; | ||
| use crate::{Pod, ViewCtx}; | ||
|
|
||
| /// # Example | ||
| /// | ||
| /// ``` | ||
| /// use xilem::masonry::kurbo::{Rect, Size, Affine}; | ||
| /// use xilem::masonry::peniko::Fill; | ||
| /// use xilem::masonry::vello::Scene; | ||
| /// use xilem::view::canvas; | ||
| /// use std::sync::Arc; | ||
| /// # use xilem::WidgetView; | ||
| /// | ||
| /// # fn fill_canvas() -> impl WidgetView<()> + use<> { | ||
| /// let my_canvas = canvas(Arc::new(|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 | ||
| /// # } | ||
| /// ``` | ||
| pub fn canvas(draw: Arc<dyn Fn(&mut Scene, Size) + Send + Sync + 'static>) -> Canvas { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Btw. I think we should probably specify in the doc-comment, that this
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we can also use a function pointer instead, to avoid allocs and (mostly) unnecessary generics? |
||
| Canvas { | ||
| draw, | ||
| alt_text: None, | ||
| } | ||
| } | ||
|
|
||
| /// The [`View`] created by [`canvas`]. | ||
| #[must_use = "View values do nothing unless provided to Xilem."] | ||
| pub struct Canvas { | ||
| draw: Arc<dyn Fn(&mut Scene, Size) + Send + Sync + 'static>, | ||
| alt_text: Option<String>, | ||
| } | ||
|
|
||
| impl Canvas { | ||
| /// Sets alt text for the contents of the canvas. | ||
| /// | ||
| /// Users are strongly encouraged to provide alt text for accessibility tools | ||
| /// to use. | ||
| pub fn alt_text(mut self, alt_text: String) -> Self { | ||
| self.alt_text = Some(alt_text); | ||
| self | ||
| } | ||
| } | ||
|
|
||
| impl ViewMarker for Canvas {} | ||
|
|
||
| impl<State: ViewArgument, Action> View<State, Action, ViewCtx> for Canvas { | ||
| type Element = Pod<widgets::Canvas>; | ||
| type ViewState = (); | ||
|
|
||
| fn build(&self, ctx: &mut ViewCtx, _: Arg<'_, State>) -> (Self::Element, Self::ViewState) { | ||
| let mut widget = widgets::Canvas::from_arc(self.draw.clone()); | ||
|
|
||
| if let Some(alt_text) = &self.alt_text { | ||
| widget = widget.with_alt_text(alt_text.to_owned()); | ||
| } | ||
|
|
||
| let widget_pod = ctx.create_pod(widget); | ||
| (widget_pod, ()) | ||
| } | ||
|
|
||
| fn rebuild( | ||
| &self, | ||
| prev: &Self, | ||
| (): &mut Self::ViewState, | ||
| _ctx: &mut ViewCtx, | ||
| mut element: Mut<'_, Self::Element>, | ||
| _: Arg<'_, State>, | ||
| ) { | ||
| if !Arc::ptr_eq(&self.draw, &prev.draw) { | ||
| widgets::Canvas::set_painter_arc(&mut element, self.draw.clone()); | ||
| } | ||
| if self.alt_text != prev.alt_text | ||
| && let Some(alt_text) = &self.alt_text | ||
| { | ||
| widgets::Canvas::set_alt_text(element, alt_text.clone()); | ||
| } | ||
|
Comment on lines
+90
to
+94
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is safe? When alt_test was |
||
| } | ||
|
|
||
| fn teardown(&self, (): &mut Self::ViewState, _: &mut ViewCtx, _: Mut<'_, Self::Element>) {} | ||
|
|
||
| fn message( | ||
| &self, | ||
| (): &mut Self::ViewState, | ||
| message: &mut MessageCtx, | ||
| _element: Mut<'_, Self::Element>, | ||
| _app_state: Arg<'_, State>, | ||
| ) -> MessageResult<Action> { | ||
| tracing::error!( | ||
| ?message, | ||
| "Message arrived in Canvas::message, but Canvas doesn't consume any messages, this is a bug" | ||
| ); | ||
| MessageResult::Stale | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better done in the
drawfunction?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I would just remove TODO, I think it's a good default to draw nothing when nothing is specified in
draw.