xilem_masonry: Add Label widget by Philipp-M · Pull Request #226 · linebender/xilem

Philipp-M · 2024-04-27T13:18:40Z

Adds the label widget to xilem_masonry with some (but not all) attribute setters, and methods to disable the label in masonry.
Also slightly extends the Flex view, by being able to specify the direction (and a small fix in FlexSplice).

Philipp-M · 2024-04-27T13:20:17Z

crates/xilem_masonry/src/lib.rs

    BoxConstraints, EventCtx, LayoutCtx, LifeCycle, LifeCycleCtx, PaintCtx, Point, PointerEvent,
    Size, StatusChange, TextEvent, Widget, WidgetId, WidgetPod,
 };
+pub use masonry::{widget::Axis, Color, TextAlignment};


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.

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

DJMcNab · 2024-04-27T13:30:51Z

crates/xilem_masonry/src/view/flex.rs

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


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

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

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

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)

DJMcNab · 2024-04-27T13:31:33Z

crates/xilem_masonry/src/view/flex.rs

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


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

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

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

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

DJMcNab · 2024-04-27T13:32:35Z

crates/xilem_masonry/src/view/label.rs

+pub fn label(label: impl Into<ArcStr>) -> Label {
+    Label {
+        label: label.into(),
+        text_color: Color::default(),


Hmm, Color implementing Default is weird - the default would surely depend on the use case?

I can instead make it black (which seems to be the default) to be more clear. I agree, looking at it more closely, because default for the alpha value seems to be 0 (which I don't think is a good default). Maybe remove Default for peniko::Color alltogether (or make it explicitely black with opaque alpha)?

Yeah, removing that Default is worth a PR, at worst to get a reason it should stay

I've made it Color::BLACK for now in this PR.

if we kept Default, this could just use functional record update syntax,
since label is the only non-default field. Agree about the alpha value.

Label { label: label.into(), ..Default::default() }

I'm not sure I follow. Label: Default would not be a good API, because there's nearly no use for an empty label.

(Plus, I think that would allocate without need, which isn't great.)

Of course, I just momentarily forgot functional record update syntax requires the structure itself to be Default not just the fields.

crates/xilem_masonry/src/view/label.rs

DJMcNab · 2024-04-27T13:34:19Z

crates/masonry/src/widget/label.rs

        self.ctx.request_layout();
    }
+
+    /// Set disabled


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

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

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

Yeah I guess that is indeed the case.

…asonry`)

DJMcNab

I still think we need some way to check the flex direction change works as expected.

Are you willing to add a Masonry level test for it? I've not dug into the test suite myself, so I'm not sure what that would involve.
If not, I think we can just merge this for expedience.

We do also need to add some xilem_masonry level tests. It would be awesome if you could get that started (although feel free to work on whichever features you want, of course)

DJMcNab · 2024-04-27T15:02:53Z

crates/xilem_masonry/examples/mason.rs

+                    data.count = -1_000_000;
+                }),
+            ))
+            .direction(Axis::Horizontal),


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

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.

crates/masonry/src/widget/flex.rs

…g on active state

Philipp-M · 2024-04-27T15:26:32Z

We do also need to add some xilem_masonry level tests. It would be awesome if you could get that started (although feel free to work on whichever features you want, of course)

I can look into that some time soon, but I think focus on "bling bling" right now seems more appropriate ?

DJMcNab · 2024-04-27T15:29:40Z

but I think focus on "bling bling" right now seems more appropriate ?

Probably 😆. It would be nice to have at least some tests we can point to, but I see your point

As I say, merging-as-is is fine, although I've not run this myself

Philipp-M · 2024-04-27T15:55:35Z

Are you willing to add a Masonry level test for it?

I think I would do that, but for me locally a lot of the tests are failing, because I suspect a different font is chosen for me (new test snapshots are generated, with slightly different looking font), not sure currently how to force one specific font (I haven't looked into it, but I think it makes sense to include a font in the repo, and force it in the tests?)
Tagging @PoignardAzur

Anyway, it should be easy to add a test for the direction, but I'd rather merge this now and do that in different PR.

PoignardAzur · 2024-04-27T15:59:17Z

Yeah, we can probably postpone tests until we figure out how to get a reliable font stack.

Overall LGTM.

xilem_masonry: Add Label widget

b43c331

Philipp-M commented Apr 27, 2024

View reviewed changes

DJMcNab reviewed Apr 27, 2024

View reviewed changes

A few fixes (support setting flex direction in masonry and `xilem_m…

785623c

…asonry`)

Philipp-M requested a review from DJMcNab April 27, 2024 14:50

DJMcNab approved these changes Apr 27, 2024

View reviewed changes

Philipp-M mentioned this pull request Apr 27, 2024

Remove Default in Color linebender/peniko#31

Closed

Small fixes and add dynamic flex direction for +n buttons, dependin…

00ee6df

…g on active state

Philipp-M added this pull request to the merge queue Apr 27, 2024

Merged via the queue into linebender:main with commit 235fbb7 Apr 27, 2024

Philipp-M deleted the xilem_masonry-label branch April 27, 2024 16:04

Conversation

Philipp-M commented Apr 27, 2024

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!

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!

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!

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!

Uh oh!

Philipp-M commented Apr 27, 2024

Uh oh!

DJMcNab commented Apr 27, 2024

Uh oh!

Philipp-M commented Apr 27, 2024

Uh oh!

PoignardAzur commented Apr 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants