masonry: reimplement Widget::get_child_at_pos

[tomcur]

The Widget::get_child_at_pos has a default linear search implementation, upholding some conditions (e.g., picking the last child in "z-order" in case of overlap). Widgets can override this with a custom implementation (e.g., a widget with many children that maintains a quadtree to search through). Custom implementations should uphold the same conditions, we can add some debug assertions to check this in a later PR.

This introduces a QueryCtx, allowing widget methods access to the global state, widget state, and widget subtree. QueryCtx is similar to the other (mutable) context types, but can be shared.

This also changes WidgetRef to consist of a Widget and a QueryCtx, similar to WidgetMut consisting of a Widget and MutateCtx. This required changing where WidgetRef can be constructed.

This resolves code comment

xilem/masonry/src/widget/widget_ref.rs

Lines 192 to 199 in ac95f25

	// TODO - Use Widget::get_child_at_pos method
	if let Some(child) = innermost_widget.children().into_iter().find(|child| {
	!child.widget.skip_pointer() && child.state().window_layout_rect().contains(pos)
	}) {
	innermost_widget = child;
	} else {
	break;
	}

[waywardmonkeys]

I don't know about the overall strategy that is desired for this, but I'm definitely interested in seeing this feature be underway.

Would be good to have a couple of tests as well though, including checking skip_pointer and some other edge cases.

[PoignardAzur]

The default Widget::get_child_at_pos implementation assumes children are in increasing "z-order", picking the last child in case of overlapping children. If that assumption is kept, it should be documented, but perhaps a better solution is needed.

That assumption is likely to remain, since it's how children are painted right now, and the cursor should general pick the widget whose pixels are "visible" under the mouse.

I'm not crazy about the approach of passing an array of WidgetRef, though. I would lean towards passing a nont-mutable LifecycleCtx (soon to be renamed to UpdateCtx) instead. In the cases where that method would be useful (eg a very large container), you want to avoid any kind of O(N) work.

[tomcur]

I'm not crazy about the approach of passing an array of WidgetRef, though. I would lean towards passing a nont-mutable LifecycleCtx (soon to be renamed to UpdateCtx) instead. In the cases where that method would be useful (eg a very large container), you want to avoid any kind of O(N) work.

That's a good point.

Currently LifeCycleCtx (UpdateCtx) requires mutable-everything:

xilem/masonry/src/contexts.rs

Lines 65 to 73 in 6c49516

	/// A context provided to the [`lifecycle`] method on widgets.
	///
	/// [`lifecycle`]: Widget::lifecycle
	pub struct LifeCycleCtx<'a> {
	pub(crate) global_state: &'a mut RenderRootState,
	pub(crate) widget_state: &'a mut WidgetState,
	pub(crate) widget_state_children: ArenaMutChildren<'a, WidgetState>,
	pub(crate) widget_children: ArenaMutChildren<'a, Box<dyn Widget>>,
	}

That would mean WidgetRef::find_widget_at_pos would need mutable references to construct LifeCycleCtx, but it by design holds shared references. Perhaps a new context type with shared references is required? Alternatively, WidgetMut could get the find_widget_at_pos method. Though querying for a child seems like it should be possible through a shared ref.

[waywardmonkeys]

It would be nice if this supported the sort of use case where one might want an acceleration structure (such as a quad tree) for a widget-canvas with a lot of widgets.

[tomcur]

It would be nice if this supported the sort of use case where one might want an acceleration structure (such as a quad tree) for a widget-canvas with a lot of widgets.

Indeed, that's a good use-case. If, as @PoignardAzur suggested, a cheaply-constructable context type is passed to the widget, the widget can choose to either do a linear search through its children (for example, sticking to the default behavior), or it can use whatever domain knowledge it has (for example, your use-case of a canvas with a quad tree for searching).

[PoignardAzur]

That would mean WidgetRef::find_widget_at_pos would need mutable references to construct LifeCycleCtx, but it by design holds shared references.

Oh, that's right. That makes things more awkward.

Perhaps a new context type with shared references is required?

Probably, yes, but I'm not happy about adding yet another context type.

[tomcur]

Instead of creating a new context type, WidgetRef can be provided to the widget. The signature then becomes:

fn get_child_at_pos<'w>(
    &self,
    widget_ref: WidgetRef<'w, dyn Widget>,
    pos: Point,
) -> Option<WidgetRef<'w, dyn Widget>>

I've pushed a commit with this change to illustrate. It is a bit circular, though: WidgetRef carries a reference to the widget it is now calling with a reference to itself.

Going the context route, the context could look as follows. Some of the methods on WidgetRef (like WidgetRef::find_widget_by_id) could be moved/duplicated to QueryCtx.

/// A context provided to widget methods that need read-only (shared) access.
pub struct QueryCtx<'a> {
    pub(crate) global_state: &'a RenderRootState,
    pub(crate) widget_state: &'a WidgetState,
    pub(crate) widget_state_children: ArenaRefChildren<'a, WidgetState>,
    pub(crate) widget_children: ArenaRefChildren<'a, Box<dyn Widget>>,
}

Note QueryCtx itself would not be sufficient to iterate over child WidgetRef, as a reference to the widget is needed to call Widget::children_ids. This is no problem inside Widget::get_child_at_pos, of course.

xilem/masonry/src/widget/widget_ref.rs

Lines 109 to 112 in 6c49516

	pub fn children(&self) -> SmallVec<[WidgetRef<'w, dyn Widget>; 16]> {
	let parent_id = self.widget_state.id.to_raw();
	self.widget
	.children_ids()

[PoignardAzur]

Sounds good. Fair warning, I won't have time to look at this over the weekend, but I'll have a look on Monday.

[tomcur]

A context type plus a Widget is similar to a WidgetRef (a WidgetRef can be constructed from those two), with the option of having additional fields. In the example above, QueryCtx has a global_state: &RenderRootState field like the other context types (though shared, not mutable), so it can have methods like QueryCtx::has_pointer_capture.

Unlike WidgetRef, context types don't hold references to the Widget itself, so calling a Widget with a QueryCtx doesn't have that somewhat inelegant circular referencing.

Existing Widget methods requiring state receive it through mut ref context types, using a shared ref QueryCtx is perhaps less surprising than using a WidgetRef. It also prevents the Widget API from needing to know about WidgetRef at all.

[DJMcNab]

Some "drive-by" thoughts, pending Olivier's review.

[PoignardAzur]

Something that occurs to me reading this is we probably want to rename find_widget_at_pos to find_widget_under_cursor across the crate. It's of scope for now though.

I think the WidgetRef based API is too awkward. I'd rather have one based on a new QueryCtx type.

[tomcur]

Rebased onto main, and added commits. One introduces QueryCtx, replacing WidgetRef in the method call, but otherwise introduces minimal changes.

The commit after explores a more substantial change. WidgetRef and WidgetMut are conceptually similar. WidgetMut is a struct of a Widget and a MutateCtx. For symmetry, in this second commit WidgetRef becomes a struct of a Widget and a QueryCtx.

Some thoughts:

1. MutateCtx::get_mut was already used to get a WidgetMut, I've added QueryCtx::get to get a WidgetRef.
2. QueryCtx::get should perhaps take a &WidgetPod, like how MutateCtx::get_mut takes a &mut WidgetPod. It currently takes a WidgetId, because Widget::children_ids returns just that: ids, not pods. Maybe we can construct WidgetPod in the default Widget::get_child_at_pos method implementation by assuming WidgetPodInner::Inserted.
3. QueryCtx having access to global state, like all other context types, has some ramifications. For example, to construct a WidgetRef containing a QueryCtx, having access to a widget's state is no longer enough, and means WidgetArena cannot construct a WidgetRef by itself anymore. In that commit I've moved the method for getting widget refs to RenderRoot::get_widget, which already had a method for getting a WidgetMut through RenderRoot::edit_widget.

(Zulip thread: https://xi.zulipchat.com/#narrow/stream/317477-masonry/topic/.60WidgetRef.60.20and.20.60WidgetMut.60)

[tomcur]

There are lifetime issues here: child widgets fetched from Widget::get_child_at_pos can't be returned from the function because they don't live long enough. To get around this, the render root is passed in to refetch widgets with a longer lifetime from the global widget arena, but maybe there's a more elegant solution.

[PoignardAzur]

Yeah, this exact kind of lifetime issue (when you want to return a borrow to something pointed to by a local, but rustc thinks you're returning a borrow to the local) has bitten me before.

I'm not sure there's an elegant solution, but finding one is a mid-high priority.

[PoignardAzur]

LGTM at a skim. Will look more in-depth later.

[PoignardAzur]

Yeah, this exact kind of lifetime issue (when you want to return a borrow to something pointed to by a local, but rustc thinks you're returning a borrow to the local) has bitten me before.

I'm not sure there's an elegant solution, but finding one is a mid-high priority.

Didn't mean to approve.

[PoignardAzur]

This is roughly what a idiomatic version looks like. The key thing is that since QueryCtx is Copy, you can take it by value in QueryCtx::get, and that means you can request "smaller" lifetimes.

[tomcur]

QueryCtx::widget_state is pub(crate), we can access it here but user code currently can't.

[PoignardAzur]

Right, same with the clip field access.

[PoignardAzur]

LGTM.

You could probably add a few methods to QueryCtx so that the default implementation doesn't need to use private fields, but either way, it's a good first implementation.

[tomcur]

You could probably add a few methods to QueryCtx so that the default implementation doesn't need to use private fields, but either way, it's a good first implementation.

I'll do a follow-up PR for that, if that's fine with you? Then we can think about the signatures of QueryCtx methods (and whether other context types might want some of those methods, too).

[PoignardAzur]

Sure.