Implement event and update_pointer passes from pass spec RFC #488

PoignardAzur · 2024-08-05T13:37:30Z

This is a first step in implementing the "Pass Specification" RFC:
linebender/rfcs#7

Create a passes module.
Create event passes.
Create the update_pointer pass.

Remove WidgetPod::update_hot_state method.
Move mouse-cursor-handling code to update_pointer pass.
Implement pointer capture.
Refactor the TreeArena code.

PoignardAzur · 2024-08-09T13:30:50Z

Unit tests pass, examples seem to work fine, and the last commit fixed a performance-cliff-in-waiting. If we want to implement the RFC step by step, now would be a good point to merge.

Philipp-M

Just started looking into this. And quickly tested the examples in xilem. The Textbox widget seems to lead to a panic when hovering over it. Should be easy to reproduce by running the mason example.

Philipp-M · 2024-08-09T15:22:37Z

masonry/src/contexts.rs

@@ -333,6 +326,7 @@ impl_context_method!(
 // --- MARK: CURSOR ---
 // Cursor-related impls.
 impl_context_method!(EventCtx<'_>, {
+    // TODO - Rewrite doc


Not sure if there's something missing yet in the code, but it would help reviewing this IMO.

I'm leaving this for a later PR.

Philipp-M · 2024-08-09T15:22:43Z

masonry/src/contexts.rs

+    // TODO - Document
+    // TODO - Figure out cases where widget should be notified of pointer capture
+    // loss
+    // TODO - Panic when event isn't PointerDown


I wonder whether this should probably be done in this PR? It would make reviewing easier at least. (Same for the other methods)

I've added a quick-and-dirty assertion. Documentation will be for a later PR.

PoignardAzur · 2024-08-09T16:02:03Z

Will look into the crash.

masonry/src/tree_arena.rs

masonry/src/passes/event.rs

masonry/src/event.rs

waywardmonkeys · 2024-08-09T17:36:16Z

masonry/src/passes/event.rs

@@ -0,0 +1,344 @@
+// Copyright 2024 the Xilem Authors
+// SPDX-License-Identifier: Apache-2.0
+


Module level comments, if no other comments were added, would help to understand this PR. Same is true of other files.

Maybe add a lot of that RFC (that seems to document a lot) as module comments?

I'll move a lot of the RFC into documentation once the port is complete. Right now I'm focusing on implementation.

masonry/src/passes/event.rs

waywardmonkeys · 2024-08-09T17:39:45Z

masonry/src/widget/align.rs

-    fn on_pointer_event(&mut self, ctx: &mut EventCtx, event: &PointerEvent) {
-        self.child.on_pointer_event(ctx, event);
-    }
+    fn on_pointer_event(&mut self, _ctx: &mut EventCtx, _event: &PointerEvent) {}


Should these end up having a default implementation on Widget and not have blank ones everywhere?

I'm split. On the one hand, some widgets won't need to implement them. On the other hand, filling in default implementations take like two minutes, and it's good to be mindful about what methods you implement. I lean towards "no unless >95% of widgets are expected not to implement the method".

On the other hand, it is a bunch of boilerplate and it means you can't search for meaningful implementations of the method rather than "all implementations, even the empty ones".

Hmm I also think default implementations may reduce unnecessary clutter and I think are one of the main advantages of avoiding the hierarchical child-calling we had previously.
Instead I think good documentation should help the implementer of that trait what they actually need to implement for their widget.

Reasons for default impls get even more vaild, when we add more of these methods to the trait (which I'm pretty sure will happen). So we could extend the trait more deliberately without having to think about all the boilerplate or breaking changes that it causes.

I'm not so sure, but I guess it doesn't hurt to add a default impl.

Empty implementations can be removed in a follow-up PR.

masonry/src/widget/checkbox.rs

PoignardAzur · 2024-08-09T19:18:39Z

I've fixed that crash with the mason example. I'm getting some flickering from the cursor for some reason, will look into that.

EDIT: It's fixed.

masonry/src/render_root.rs

waywardmonkeys · 2024-08-10T15:55:33Z

I'm essentially okay with this in the current state as an incremental step towards a better place. I haven't run it myself, just reviewed code and RFCs. I think it would benefit strongly from additional documentation and comments within the code, even if it were largely adapted from some of the RFC text.

I'll defer to @DJMcNab or @Philipp-M for an actual approval as they're more up to speed on some of this than I am.

Philipp-M

Looks mostly good (Daniel should probably look over this as well, I likely have missed stuff (so far)).

I'm all for more incremental PRs of that RFC, though it would be nice, when they're more atomic/single isolated changes like the arena changes (and ideally including already doc-comments, and a little bit more polished).

It would make reviewing easier (and likely faster) and more effective, like splitting single changes that seem to be mostly consolidated out of a working branch. I know that may increase a little bit the workload, but I think also results in higher code-quality merged in main (and less numerous half-finished building-blocks).
(I'll review more tomorrow, way too late here...)

Philipp-M · 2024-08-11T18:02:43Z

masonry/src/passes/event.rs

@@ -0,0 +1,344 @@
+// Copyright 2024 the Xilem Authors
+// SPDX-License-Identifier: Apache-2.0
+


Maybe add a lot of that RFC (that seems to document a lot) as module comments?

Philipp-M · 2024-08-12T00:48:27Z

masonry/src/passes/event.rs

+    let mut pass_fn = pass_fn;
+
+    let mut target_widget_id = target;


Nit: you could just use target/pass_fn directly, by declaring it mut in the function signature (or is this intentional?)

Done for pass_fn. Renamed target_widget_id for something clearer instead.

Philipp-M · 2024-08-12T00:48:32Z

masonry/src/passes/event.rs

+        }
+
+        merge_state_up(&mut root.widget_arena, widget_id, root_state);
+        target_widget_id = parent_id;


So unlike browser behavior, this doesn't do a capturing phase (yet?), only bubbling.

(Just noticing)

We decided against a capturing phase, in the hope that it won't be necessary.

masonry/src/passes/update.rs

Philipp-M · 2024-08-12T00:48:48Z

masonry/src/passes/update.rs

+    if let Some(capture_target) = root.state.pointer_capture_target {
+        if next_hovered_widget != Some(capture_target) {
+            next_hovered_widget = None;
+        }
+    }


Suggested change

if let Some(capture_target) = root.state.pointer_capture_target {

if next_hovered_widget != Some(capture_target) {

next_hovered_widget = None;

}

}

let pointer_capture_target = root.state.pointer_capture_target;

if pointer_capture_target.is_some() && pointer_capture_target != next_hovered_widget {

next_hovered_widget = None;

}

(could reuse that pointer_capture_target below)

I'm not sure why you're resolving/ignoring my comments without any action on it (not the first time I notice this)?

That's not really encouraging for reviewing TBH.

In that case it's because I don't want to merge that change. I don't think it improves readability.

It was kind of a dick move to mark it as resolved without explaining the reason, sorry.

I think every other comment you'd made I've either addressed in a follow-up commit or explained why I wouldn't?

(Note that sometimes I mark a comment as resolved after I wrote the change that addresses it, but before I've pushed it. It helps me keep track of what to do, but maybe it's not worth the confusion it creates for reviewers.)

Yeah, I'm fine with a short comment, explaining that (although I disagree), generally I'd prefer, when the author of a review-comment resolves it, unless it's obviously resolved, as (and don't take this personal) it looks like silencing concerns.

I think every other comment you'd made I've either addressed

No, the nits for example not (although being nits, it's not being really important), but a short answer to e.g. "or is this intentional?" would've been nice.

(I also don't think that this suggestion is better)

Philipp-M · 2024-08-12T00:48:52Z

masonry/src/passes/update.rs

+    // This algorithm is written to be resilient to future changes like reparenting and multiple
+    // cursors. In theory it's O(Depth² * CursorCount) in the worst case, which isn't too bad
+    // (cursor count is usually 1 or 2, depth is usually small), but in practice it's almost
+    // always O(Depth * CursorCount) because we only need to update the hovered status of the


Isn't that then more like the average case?

It's not average or amortized, because by default after you visit a widget you immediately visit the next one in next_hovered_path (its parent).

To be specific, worst case for the algorithm below that comment is O(Depth * CursorCount) unless a widget has been reparented (which currently isn't possible but might be in the future) in which case it's O(Depth² * CursorCount).

(The above assumes that accessing a widget is O(1), which isn't the case but will be in the future.)

Ok, so the factor of $$\text{Depth}^{2}$$ occurs if at every item in the tree, the parent has changed. I.e. the tree used to be:

graph TB; A; B; A-->C; A-->D; C-->E;

Loading

where E was hovered over, and is now something like:

graph TB; A; A-->D; B-->C; D-->E;

Loading

even if E is still hovered over?

masonry/src/tree_arena.rs

Philipp-M · 2024-08-12T00:49:24Z

masonry/src/widget/align.rs

-    fn on_pointer_event(&mut self, ctx: &mut EventCtx, event: &PointerEvent) {
-        self.child.on_pointer_event(ctx, event);
-    }
+    fn on_pointer_event(&mut self, _ctx: &mut EventCtx, _event: &PointerEvent) {}


Hmm I also think default implementations may reduce unnecessary clutter and I think are one of the main advantages of avoiding the hierarchical child-calling we had previously.
Instead I think good documentation should help the implementer of that trait what they actually need to implement for their widget.

Reasons for default impls get even more vaild, when we add more of these methods to the trait (which I'm pretty sure will happen). So we could extend the trait more deliberately without having to think about all the boilerplate or breaking changes that it causes.

masonry/src/widget/checkbox.rs

DJMcNab · 2024-08-12T10:30:28Z

I think the PR description needs some details, and a link to the RFC.

DJMcNab

I've not reasoned about this from the top down, but I really like the core of the new code!

Not approving to allow time for review feedback to be handled.

DJMcNab · 2024-08-12T15:29:25Z

masonry/src/widget/checkbox.rs

@@ -30,15 +30,15 @@ impl Checkbox {
    pub fn new(checked: bool, text: impl Into<ArcStr>) -> Checkbox {
        Checkbox {
            checked,
-            label: WidgetPod::new(Label::new(text)),
+            label: WidgetPod::new(Label::new(text).with_skip_pointer(true)),


When would a label not skip pointer handling?

I guess maybe if we get link support within labels?

Links and selecting text (though I think maybe we have the Prose widget for that now?).

I guess we could go ahead and say they're always skipped and only Prose gets to have links. I haven't really thought this through and I don't think it's a priority.

DJMcNab · 2024-08-12T15:30:22Z

masonry/src/testing/harness.rs

@@ -570,7 +582,7 @@ impl TestHarness {
            // Remove '<test_name>.new.png' file if it exists
            let _ = std::fs::remove_file(&new_path);
            new_image.save(&new_path).unwrap();
-            panic!("No reference file");
+            panic!("Snapshot test '{test_name}' failed: No reference file");


Great. I am planning on making these snapshot tests much more user-friendly, but this is a good start.

masonry/src/passes/update.rs

DJMcNab · 2024-08-12T15:40:20Z

masonry/src/passes/update.rs

+    // This algorithm is written to be resilient to future changes like reparenting and multiple
+    // cursors. In theory it's O(Depth² * CursorCount) in the worst case, which isn't too bad
+    // (cursor count is usually 1 or 2, depth is usually small), but in practice it's almost
+    // always O(Depth * CursorCount) because we only need to update the hovered status of the


Ok, so the factor of $$\text{Depth}^{2}$$ occurs if at every item in the tree, the parent has changed. I.e. the tree used to be:

graph TB; A; B; A-->C; A-->D; C-->E;

Loading

where E was hovered over, and is now something like:

graph TB; A; A-->D; B-->C; D-->E;

Loading

even if E is still hovered over?

DJMcNab · 2024-08-12T15:48:17Z

masonry/src/passes/update.rs

+        });
+    }
+
+    // TODO - Make sure widgets are iterated from the bottom up


Suggested change

// TODO - Make sure widgets are iterated from the bottom up

// TODO - Make sure widgets are iterated from the bottom up for efficiency.

It's not just for efficiency, we want iteration order to be predictable. We'll probably want to document it too.

DJMcNab · 2024-08-12T15:48:42Z

masonry/src/passes/update.rs

+    root.state.hovered_path = next_hovered_path;
+
+    // -- UPDATE CURSOR --
+    // TODO - Rewrite more cleanly


I don't really know, but I do want to leave that comment for a future cleanup.

I did end up rewriting it. I think it's a lot more readable now.

DJMcNab · 2024-08-12T15:50:07Z

masonry/src/passes/update.rs

+    // -- UPDATE CURSOR --
+    // TODO - Rewrite more cleanly
+    let cursor_changed = next_hovered_widget
+        .is_some_and(|id| root.widget_arena.get_state_mut(id).item.cursor_changed);


Can this be a mem::take of the cursor_changed?

I don't think so?

Oh, you meant to skip the later part that set this to false? Yeah, in retrospect that was a good idea. It's moot now.

DJMcNab · 2024-08-12T15:50:49Z

masonry/src/passes/update.rs

+        .is_some_and(|id| root.widget_arena.get_state_mut(id).item.cursor_changed);
+    if hovered_widget != next_hovered_widget || cursor_changed {
+        let cursor;
+        if let Some(capture_target) = root.state.pointer_capture_target {


I'm presuming the case where the capture target's cursor_changed is set is handled elsewhere.

... Crap, I don't think it is. Good catch.

Re-reading this, I think I was assuming capture_target would always be the same as hovered_widget, but that's not actually the case (hovered_widget could be None it we'd still want to check the capture target).

In the end I just removed the cursor_changed part for expediency.

DJMcNab · 2024-08-12T15:52:36Z

masonry/src/passes/event.rs

I really like this code! It's reasonably clear what's happening, and why.

I think there could be a few more comments to make understanding a bit easier.

DJMcNab · 2024-08-12T15:55:52Z

Seems like that was poor timing - just did a review pass.

DJMcNab

Looking at it again, nothing in these review comments is especially controversial, and so we should be fine to land once these are addressed.

(I would however like to echo the concern about review comments not being appropriately actioned)

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

DJMcNab · 2024-08-13T08:35:19Z

masonry/src/passes/update.rs

+    if let Some(capture_target) = root.state.pointer_capture_target {
+        if next_hovered_widget != Some(capture_target) {
+            next_hovered_widget = None;
+        }
+    }


(I also don't think that this suggestion is better)

waywardmonkeys · 2024-08-13T10:04:45Z

Yay!

…mprove docs Continuation of 59ee615 (linebender#488). Makes `has_pointer_capture` available on all context types except `LayoutCtx`, like `is_active` used to be.

Improve documentation of pointer capture. Continuation of 59ee615 (linebender#488). Makes `has_pointer_capture` available on all context types except `LayoutCtx`, like `is_active` used to be.

Fixes scrolling `Portal` on scrollbar drag by recomposing instead of relayouting. Regression probably caused in 59ee615 (#488) or ff7635e (#522).

Improve documentation of pointer capture. Continuation of 59ee615 (linebender#488). Makes `has_pointer_capture` available on all context types except `LayoutCtx`, like `is_active` used to be.

) Also improve documentation of pointer capture. Continuation of 59ee615 (#488). Makes `has_pointer_capture` available on all context types except `LayoutCtx`, like `is_active` used to be.

`WidgetState::merge_up` no longer actually mutates `child_state` since linebender#488 and linebender#599, but it may do so again in the future. See also this Zulip thread: https://xi.zulipchat.com/#narrow/stream/317477-masonry/topic/WidgetState.3A.3Amerge_up.20no.20longer.20needs.20mutable.20child.20state

`WidgetState::merge_up` no longer actually mutates `child_state` since #488 and #599, but it may do so again in the future. See also this Zulip thread: https://xi.zulipchat.com/#narrow/stream/317477-masonry/topic/WidgetState.3A.3Amerge_up.20no.20longer.20needs.20mutable.20child.20state

PoignardAzur force-pushed the implement_pass_spec branch from dc44cd8 to 213b11d Compare August 6, 2024 08:43

PoignardAzur changed the title ~~Implement and UPDATE_POINTER passes from pass spec RFC~~ Implement event and UPDATE_POINTER passes from pass spec RFC Aug 8, 2024

PoignardAzur requested review from DJMcNab and waywardmonkeys August 9, 2024 13:29

Philipp-M reviewed Aug 9, 2024

View reviewed changes

waywardmonkeys reviewed Aug 9, 2024

View reviewed changes

waywardmonkeys reviewed Aug 10, 2024

View reviewed changes

masonry/src/render_root.rs Outdated Show resolved Hide resolved

Philipp-M reviewed Aug 12, 2024

View reviewed changes

PoignardAzur added 17 commits August 12, 2024 16:17

Implement and UPDATE_POINTER passes from pass spec RFC

30285cf

Improve snapshot test messages

771e78f

Remove WidgetPod::on_xxx_event methods

515b5a9

Remove WidgetPod event method calls in Xilem widget impls

ad33704

Simplify cursor-handling code and fix pointer capture bug

6c8a2dd

Add missing copyright headers

a327d18

Add other missing copyright header

18eff2c

Add missing Box<dyn Widget> methods

ab3eeca

Refactor TreeArena types

c97e7b5

Clean up arena-related code

6680eb2

Add parents map to TreeArena for efficient lookup

d430eaf

Fix cursor-related crash

8b4606b

Fix clippy lint

1fedd39

Fix cursor flickering bug

e6b3465

Run cargo fmt

220d2f0

Remove dead code

9d232e6

Misc doc fixes

7a8a57e

PoignardAzur added 2 commits August 12, 2024 17:49

Factor out WidgetArena into separate mod.

b5bac88

Add default implementations for Widget event methods.

df80e47

DJMcNab reviewed Aug 12, 2024

View reviewed changes

DJMcNab approved these changes Aug 12, 2024

View reviewed changes

PoignardAzur and others added 5 commits August 12, 2024 17:04

Update masonry/src/passes/update.rs

f10d72e

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

Add debug assertion for pointer capture

565f1a6

Rewrote mouse cursor handling in cleaner way

71809f6

Tweak update pass for clarity

5163f14

Fix clippy lint

ff664c3

DJMcNab approved these changes Aug 13, 2024

View reviewed changes

PoignardAzur added this pull request to the merge queue Aug 13, 2024

Merged via the queue into main with commit 59ee615 Aug 13, 2024
17 checks passed

PoignardAzur deleted the implement_pass_spec branch August 13, 2024 08:56

PoignardAzur mentioned this pull request Aug 13, 2024

Implement mutate pass from pass spec RFC #510

Merged

tomcur mentioned this pull request Aug 29, 2024

masonry: fix updating Portal on scrollbar drag #563

Merged

tomcur mentioned this pull request Aug 29, 2024

masonry: replace set_active and is_active with pointer capture #564

Merged

tomcur mentioned this pull request Sep 27, 2024

Add note about mutable child state in WidgetState::merge_up #618

Merged

		@@ -0,0 +1,344 @@
		// Copyright 2024 the Xilem Authors
		// SPDX-License-Identifier: Apache-2.0

		let mut pass_fn = pass_fn;

		let mut target_widget_id = target;

	// TODO - Make sure widgets are iterated from the bottom up
	// TODO - Make sure widgets are iterated from the bottom up for efficiency.

Implement event and update_pointer passes from pass spec RFC #488

Implement event and update_pointer passes from pass spec RFC #488

Conversation

PoignardAzur commented Aug 5, 2024 • edited Loading

PoignardAzur commented Aug 9, 2024

Philipp-M left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PoignardAzur commented Aug 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PoignardAzur commented Aug 9, 2024 • edited Loading

waywardmonkeys commented Aug 10, 2024

Philipp-M left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DJMcNab commented Aug 12, 2024

DJMcNab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PoignardAzur Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DJMcNab commented Aug 12, 2024

DJMcNab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waywardmonkeys commented Aug 13, 2024

PoignardAzur commented Aug 5, 2024 •

edited

Loading

PoignardAzur commented Aug 9, 2024 •

edited

Loading

PoignardAzur Aug 13, 2024 •

edited

Loading