Skip to content
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

xilem_masonry: Add Label widget #226

Merged
merged 3 commits into from
Apr 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions crates/masonry/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ pub use box_constraints::BoxConstraints;
pub use contexts::{EventCtx, LayoutCtx, LifeCycleCtx, PaintCtx, WidgetCtx};
pub use event::{InternalLifeCycle, LifeCycle, PointerEvent, StatusChange, TextEvent, WindowTheme};
pub use kurbo::{Affine, Insets, Point, Rect, Size, Vec2};
pub use parley::layout::Alignment as TextAlignment;
pub use util::{AsAny, Handled};
pub use vello::peniko::{Color, Gradient};
pub use widget::{BackgroundBrush, Widget, WidgetId, WidgetPod, WidgetState};
Expand Down
21 changes: 12 additions & 9 deletions crates/masonry/src/widget/flex.rs
Original file line number Diff line number Diff line change
Expand Up @@ -246,30 +246,35 @@ impl Flex {
// --- Mutate live Flex - WidgetMut ---

impl<'a> FlexMut<'a> {
/// Set the flex direction (see [`Axis`]).
///
/// [`Axis`]: enum.Axis.html
pub fn set_direction(&mut self, direction: Axis) {
self.widget.direction = direction;
self.ctx.request_layout();
}

/// Set the childrens' [`CrossAxisAlignment`].
///
/// [`CrossAxisAlignment`]: enum.CrossAxisAlignment.html
pub fn set_cross_axis_alignment(&mut self, alignment: CrossAxisAlignment) {
self.widget.cross_alignment = alignment;
// TODO
self.ctx.widget_state.needs_layout = true;
self.ctx.request_layout();
}

/// Set the childrens' [`MainAxisAlignment`].
///
/// [`MainAxisAlignment`]: enum.MainAxisAlignment.html
pub fn set_main_axis_alignment(&mut self, alignment: MainAxisAlignment) {
self.widget.main_alignment = alignment;
// TODO
self.ctx.widget_state.needs_layout = true;
self.ctx.request_layout();
}

/// Set whether the container must expand to fill the available space on
/// its main axis.
pub fn set_must_fill_main_axis(&mut self, fill: bool) {
self.widget.fill_major_axis = fill;
// TODO
self.ctx.widget_state.needs_layout = true;
self.ctx.request_layout();
}

/// Add a non-flex child widget.
Expand Down Expand Up @@ -317,9 +322,7 @@ impl<'a> FlexMut<'a> {
}
};
self.widget.children.push(child);
// TODO
self.ctx.widget_state.children_changed = true;
self.ctx.widget_state.needs_layout = true;
self.ctx.children_changed();
}

/// Add a spacer widget with a standard size.
Expand Down
13 changes: 13 additions & 0 deletions crates/masonry/src/widget/label.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,12 @@ impl Label {
self
}

/// Builder-style method to disable this label.
pub fn with_disabled(mut self, disabled: bool) -> Self {
self.disabled = disabled;
self
}

/// Return the current value of the label's text.
pub fn text(&self) -> ArcStr {
self.current_text.clone()
Expand Down Expand Up @@ -180,6 +186,13 @@ impl LabelMut<'_> {
self.widget.alignment = alignment;
self.ctx.request_layout();
}

/// Set disabled
Copy link
Member

Choose a reason for hiding this comment

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

What does it even mean for a label to be disabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess it makes more sense in combination with a checkbox. But basically, that it's not clickable in HTML jargon at least

Copy link
Member

Choose a reason for hiding this comment

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

Right, so e.g. the contents can't be clicked and dragged? I guess it also would have consequences for the accessibility metadata

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I guess that is indeed the case.

pub fn set_disabled(&mut self, disabled: bool) {
self.widget.disabled = disabled;
// TODO: only request_paint necessary?
self.ctx.request_layout();
}
}

// --- TRAIT IMPLS ---
Expand Down
42 changes: 29 additions & 13 deletions crates/xilem_masonry/examples/mason.rs
Original file line number Diff line number Diff line change
@@ -1,45 +1,61 @@
// On Windows platform, don't show a console when opening the app.
#![windows_subsystem = "windows"]

use xilem_masonry::view::{button, checkbox, flex};
use xilem_masonry::{BoxedMasonryView, MasonryView, Xilem};
use xilem_masonry::view::{button, checkbox, flex, label};
use xilem_masonry::{Axis, BoxedMasonryView, Color, MasonryView, TextAlignment, Xilem};

fn app_logic(data: &mut AppData) -> impl MasonryView<AppData> {
// here's some logic, deriving state for the view from our state
let count = data.count;
let label = if count == 1 {
let button_label = if count == 1 {
"clicked 1 time".to_string()
} else {
format!("clicked {count} times")
};

// The actual UI Code starts here

let axis = if data.active {
Axis::Horizontal
} else {
Axis::Vertical
};

let sequence = (0..count)
.map(|x| button(format!("+{x}"), move |data: &mut AppData| data.count += x))
.collect::<Vec<_>>();
flex((
button(label, |data: &mut AppData| data.count += 1),
flex((
label("Label")
.color(Color::REBECCA_PURPLE)
.alignment(TextAlignment::Start),
label("Disabled label").disabled(),
))
.direction(Axis::Horizontal),
button(button_label, |data: &mut AppData| data.count += 1),
checkbox("Check me", data.active, |data: &mut AppData, checked| {
data.active = checked;
}),
toggleable(data),
button("Decrement", |data: &mut AppData| data.count -= 1),
button("Reset", |data: &mut AppData| data.count = 0),
sequence,
flex(sequence).direction(axis),
))
}

fn toggleable(data: &mut AppData) -> impl MasonryView<AppData> {
let inner_view: BoxedMasonryView<_, _> = if data.active {
Box::new(flex((
button("Deactivate", |data: &mut AppData| {
data.active = false;
}),
button("Unlimited Power", |data: &mut AppData| {
data.count = -1_000_000;
}),
)))
Box::new(
flex((
button("Deactivate", |data: &mut AppData| {
data.active = false;
}),
button("Unlimited Power", |data: &mut AppData| {
data.count = -1_000_000;
}),
))
.direction(Axis::Horizontal),
Copy link
Member

Choose a reason for hiding this comment

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

Should we have a dynamically toggling axis in the same widget here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure, it jumps around the layout, which I didn't find satisfying, but I added it for the +n buttons when data.active changes, which is less annoying.

)
} else {
Box::new(button("Activate", |data: &mut AppData| data.active = true))
};
Expand Down
1 change: 1 addition & 0 deletions crates/xilem_masonry/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use masonry::{
BoxConstraints, EventCtx, LayoutCtx, LifeCycle, LifeCycleCtx, PaintCtx, Point, PointerEvent,
Size, StatusChange, TextEvent, Widget, WidgetId, WidgetPod,
};
pub use masonry::{widget::Axis, Color, TextAlignment};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm shamelessly reexporting all these attributes to the root of the library (since I think they're mostly unambiguous), not sure what our policy here is.

Copy link
Member

Choose a reason for hiding this comment

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

This is probably #223 again. I think this addition is fine, but we need to reason out a final policy for this, rather than making ad-hoc decisions

use smallvec::SmallVec;
use vello::Scene;
use winit::{
Expand Down
32 changes: 24 additions & 8 deletions crates/xilem_masonry/src/view/flex.rs
Original file line number Diff line number Diff line change
@@ -1,25 +1,34 @@
use std::marker::PhantomData;

use masonry::{
widget::{self, WidgetMut},
widget::{self, Axis, WidgetMut},
Widget, WidgetPod,
};

use crate::{ElementSplice, MasonryView, VecSplice, ViewSequence};
use crate::{ChangeFlags, ElementSplice, MasonryView, VecSplice, ViewSequence};

// TODO: Allow configuring flex properties. I think this actually needs its own view trait?
pub fn flex<VT, Marker>(sequence: VT) -> Flex<VT, Marker> {
Flex {
phantom: PhantomData,
sequence,
axis: Axis::Vertical,
}
}

pub struct Flex<VT, Marker> {
sequence: VT,
axis: Axis,
phantom: PhantomData<fn() -> Marker>,
}

impl<VT, Marker> Flex<VT, Marker> {
pub fn direction(mut self, axis: Axis) -> Self {
self.axis = axis;
self
}
}

impl<State, Action, Marker: 'static, Seq> MasonryView<State, Action> for Flex<Seq, Marker>
where
Seq: ViewSequence<State, Action, Marker>,
Expand All @@ -35,7 +44,7 @@ where
let mut scratch = Vec::new();
let mut splice = VecSplice::new(&mut elements, &mut scratch);
let seq_state = self.sequence.build(cx, &mut splice);
let mut view = widget::Flex::column();
let mut view = widget::Flex::for_axis(self.axis);
Copy link
Member

Choose a reason for hiding this comment

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

I didn't expose this intentionally, because Masonry doesn't support changing the axis at runtime.

We do not want to have views which give different final trees in build and rebuild

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, haven't checked that closely yet. That's unfortunate, and I agree, all attributes that are able to set in the view should of course change the widget tree.
In that case maybe stick to something like h_flex or h_stack something a like (open for naming suggestions)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or, well just implement it in masonry, checking that now...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I've implemented it in masonry, and checked it via

let axis = if count % 2 == 0 {
    Axis::Horizontal
} else {
    Axis::Vertical
};

...
flex(()).direction(axis)

(but have hardcoded the values in the example)

debug_assert!(
scratch.is_empty(),
// TODO: Not at all confident about this, but linear_layout makes this assumption
Expand Down Expand Up @@ -63,11 +72,19 @@ where
view_state: &mut Self::ViewState,
cx: &mut crate::ViewCx,
prev: &Self,
element: widget::WidgetMut<Self::Element>,
) -> crate::ChangeFlags {
mut element: widget::WidgetMut<Self::Element>,
) -> ChangeFlags {
let mut changeflags = ChangeFlags::UNCHANGED;
if prev.axis != self.axis {
element.set_direction(self.axis);
changeflags.changed |= ChangeFlags::CHANGED.changed;
}
let mut splice = FlexSplice { ix: 0, element };
self.sequence
changeflags.changed |= self
.sequence
.rebuild(view_state, cx, &prev.sequence, &mut splice)
.changed;
changeflags
}
}

Expand Down Expand Up @@ -121,7 +138,6 @@ impl ElementSplice for FlexSplice<'_> {
}

fn len(&self) -> usize {
// This is not correct because of the spacer items. Is `len` actually needed?
self.element.len() - self.ix
self.ix
Copy link
Member

Choose a reason for hiding this comment

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

Sure! I suspect that we'll probably just remove len, anyway; but this is more correct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah probably makes sense, doesn't seem to be needed anymore (I'll probably look into this more deeply, whether that is indeed the case).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's used in old xilem::view::List, so I guess we still need this, in case we don't exactly know the size of a ViewSequence in the view logic (e.g. lazy loading of items in a list, not sure currently whether that should happen in the xilem reactive layer?)

Copy link
Member

Choose a reason for hiding this comment

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

I don't think its use in List is necessary, but I haven't remade the List ViewSequence yet, so I'm not certain

}
}
93 changes: 93 additions & 0 deletions crates/xilem_masonry/src/view/label.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
use masonry::{widget::WidgetMut, ArcStr, WidgetPod};

use crate::{ChangeFlags, Color, MasonryView, MessageResult, TextAlignment, ViewCx, ViewId};

pub fn label(label: impl Into<ArcStr>) -> Label {
Label {
label: label.into(),
text_color: Color::BLACK,
alignment: TextAlignment::default(),
disabled: false,
}
}

pub struct Label {
label: ArcStr,
text_color: Color,
alignment: TextAlignment,
disabled: bool,
// TODO: add more attributes of `masonry::widget::Label`
}

impl Label {
pub fn color(mut self, color: Color) -> Self {
self.text_color = color;
self
}

pub fn alignment(mut self, alignment: TextAlignment) -> Self {
self.alignment = alignment;
self
}

pub fn disabled(mut self) -> Self {
self.disabled = true;
self
}
}

impl<State, Action> MasonryView<State, Action> for Label {
type Element = masonry::widget::Label;
type ViewState = ();

fn build(&self, _cx: &mut ViewCx) -> (WidgetPod<Self::Element>, Self::ViewState) {
(
WidgetPod::new(
masonry::widget::Label::new(self.label.clone())
.with_text_color(self.text_color)
.with_text_alignment(self.alignment)
.with_disabled(self.disabled),
),
(),
)
}

fn rebuild(
&self,
_view_state: &mut Self::ViewState,
_cx: &mut ViewCx,
prev: &Self,
mut element: WidgetMut<Self::Element>,
) -> crate::ChangeFlags {
let mut changeflags = ChangeFlags::UNCHANGED;

if prev.label != self.label {
element.set_text(self.label.clone());
changeflags.changed |= ChangeFlags::CHANGED.changed;
}
if prev.disabled != self.disabled {
element.set_disabled(self.disabled);
changeflags.changed |= ChangeFlags::CHANGED.changed;
}
if prev.text_color != self.text_color {
element.set_text_color(self.text_color);
changeflags.changed |= ChangeFlags::CHANGED.changed;
}
if prev.alignment != self.alignment {
element.set_text_alignment(self.alignment);
changeflags.changed |= ChangeFlags::CHANGED.changed;
}
changeflags
}

fn message(
&self,
_view_state: &mut Self::ViewState,
_id_path: &[ViewId],
message: Box<dyn std::any::Any>,
_app_state: &mut State,
) -> crate::MessageResult<Action> {
tracing::error!("Message arrived in Label::message, but Label doesn't consume any messages, this is a bug");
MessageResult::Stale(message)
Philipp-M marked this conversation as resolved.
Show resolved Hide resolved
}
}
3 changes: 3 additions & 0 deletions crates/xilem_masonry/src/view/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,6 @@ pub use checkbox::*;

mod flex;
pub use flex::*;

mod label;
pub use label::*;