Make the cursor API stateful and widget-local. #1433

jneem · 2020-11-27T21:44:31Z

This is an attempt at #1368. The point is that instead of calling set_cursor on every mouse move, we call it once and then it will take effect whenever the calling widget is hot or active.

Since this is a more "structured" API than the previous one, it's also a bit less powerful. This is because druid now takes care of bubbling and deciding between the various cursor requests, whereas in the previous API it was all under the caller's control. Having said that, I don't actually know of any realistic use-cases that the new API misses out on.

JAicewizard · 2020-11-28T12:42:09Z

If I am not mistaken, this only allows setting cursors for WidgetPod boundaries? Since that's where the WidgetState is located. I think that is fine since most users should be using WidgetPod anyways. I don't think that should be a problem, but might be worth documenting somewhere.

I guess this is sort of what I also imagined at first, except I hit a wall at having some widget state managed by druid. I guess this is exactly that except it only works for setting cursors within WidgetPod boundaries.

I took a quick look at the code and it seems very straight-forward to me, looks good.

raphlinus · 2020-11-28T14:52:13Z

We definitely want to be able to set the cursor in Runebender to represent hovering over, eg, control points. Does this preclude that?

JAicewizard · 2020-11-28T15:05:36Z

druid/src/window.rs

            self.handle.set_cursor(&cursor);
+        } else if matches!(event, Event::MouseMove(..)) {
+            self.handle.set_cursor(&Cursor::Arrow);


It would be nice if it would only set the cursor when it actually changed. I think it was part of the nice to haves.
You could probably just compare the Rc since that should stay the same if the cursor doesn't change.

I think there are cases where you have to set the cursor because it was invalidated some other way, for example when the window loses focus. My personal feeling is that it is possible to track those other state changes but simpler not to, so set it every time.

Yeah, there's an issue open about this (#112) but I didn't feel like taking it on.

jneem · 2020-11-28T17:53:14Z

If I am not mistaken, this only allows setting cursors for WidgetPod boundaries? Since that's where the WidgetState is located. I think that is fine since most users should be using WidgetPod anyways. I don't think that should be a problem, but might be worth documenting somewhere.

If you call set_cursor once and forget about it, then yes, it will go according to the WidgetPod boundaries. But you can always track the location of the mouse and call set_cursor again if you want to do something more sophisticated.

jneem · 2020-11-28T17:54:35Z

We definitely want to be able to set the cursor in Runebender to represent hovering over, eg, control points. Does this preclude that?

I don't think so, no. You'll call set_cursor when the mouse goes over a control point, and then clear_cursor (or set_cursor again) when it leaves the control point. The main difference between this and the previous state of things is that you used to have to call set_cursor on every mouse move; now you do it when you actually want to change the cursor.

JAicewizard · 2020-11-28T18:34:12Z

I don't think so, no. You'll call set_cursor when the mouse goes over a control point, and then clear_cursor (or set_cursor again) when it leaves the control point. The main difference between this and the previous state of things is that you used to have to call set_cursor on every mouse move; now you do it when you actually want to change the cursor.

I think it does? but maybe I'm interpreting the question wrong. Because if you set the cursor within a widget(pod) it will automatically set the cursor to that when it hovers over that widget(pod) right?

jneem · 2020-11-28T18:59:26Z

Right, but if you want to do something fancier (like only show a cursor while the mouse is over some more complicated subset of the WidgetPod), you can just listen to mouse moves and call set_cursor again (or clear_cursor) when appropriate.

cmyr

Thanks for taking this on! have left my comments inline, nothing really major. I think this is definitely a more intuitive API.

cmyr · 2020-12-02T21:25:11Z

druid/examples/cursor.rs

+        data: &AppState,
+        env: &Env,
+    ) {
+        if !Rc::ptr_eq(&data.cursor, &old_data.cursor) {


I'd prefer to just write this out as !data.cursor.same(&old_data.cursor), that should be more familiar to folks.

This brings up an interesting question: which is better

first.same(&second)

Data::same(&first, &second)

I prefer the second one, the reason being that I can immediately grok that we are using the fact that both types impl Data. I also find it easier to read. This is a subjective opinion though and I'd be interested in others' opinions.

I think the first is more idiomatic, but I see your point. We have definitely been using the first version everywhere, though, so I'm inclined to be consistent; understanding that same is the method on Data is a pretty fundamental druid concept.

I make Cursor implement PartialEq, so now this is just ==.

cmyr · 2020-12-02T21:26:40Z

druid/examples/cursor.rs

 }

 fn ui_builder() -> impl Widget<AppState> {
    Button::new("Change cursor")
-        .on_click(|ctx, data: &mut AppState, _env| {
+        .on_click(|_ctx, data: &mut AppState, _env| {
            data.cursor = next_cursor(&data.cursor, data.custom.clone());


totally unrelated to this but just noticing that it might make sense for this to just be a method on AppState? then we wouldn't need to pass in anything.

cmyr · 2020-12-02T21:36:14Z

druid/src/contexts.rs

    pub fn set_cursor(&mut self, cursor: &Cursor) {
-        *self.cursor = Some(cursor.clone());
+        self.widget_state.cursor_setting = CursorSetting::Set(Rc::new(cursor.clone()));


What's the motivation for using the Rc? is this so that we play well with Data? Or to avoid having cloning be expensive?

In either case I think it might be more ergonomic to solve this at the druid-shell level; for instance Cursor::Custom could have an Rc<platform::window::Cursor>, and we could add a Data impl for Cursor in druid, if we wanted.

I was worried about the cost. Which is silly, because I only did the druid-shell cursor thing recently, and I already forgot that it does the shared pointer thing already (within the platform-specific code). I've added the Data impl anyway.

cmyr · 2020-12-02T21:36:57Z

druid/src/core.rs

@@ -140,6 +145,18 @@ pub(crate) enum FocusChange {
    Previous,
 }

+/// The possible cursor states for a widget.
+#[derive(Clone)]
+pub(crate) enum CursorSetting {


Feels weird to have use a gerund for the name here, although it's not a big deal. I might match the naming of the FocusChange and go with CursorChange? or SetCursor? I don't think it's a big deal for an internal type but at least want to consider what some alternatives could be.

I went with CursorChange.

cmyr · 2020-12-02T22:19:04Z

druid/src/core.rs

+                self.cursor = child_cursor;
+            } else if let CursorSetting::Set(cursor) = &child_state.cursor_setting {
+                self.cursor = Some(Rc::clone(cursor));
+            }


this logic is hurting my head a bit.

Is there a situation where the child_cursor.is_some() ends up unintentionally overriding the child's cursor_setting?

or... what if both a parent and a child have called set_cursor, and the mouse moves over to the child, and then back to the parent, does the parent end up correctly resetting the cursor? This happens at the grandparent, in its own merge_up?

I think I'm satisfied, but want to at least write that concern down.

I've re-written the logic so that it's only merging self.cursor_change and child.cursor to make self.cursor. (i.e. child.cursor_change is no longer involved, so there's less multi-generational thinking involved)

I've also tried to make it clearer that cursor gets reset on every pass through the tree, whereas cursor_change is persistent.

jneem · 2020-12-03T16:54:48Z

Thanks for the comments! I think I've addressed them all, and I also realized that I never updated Split to clear the cursor...

cmyr

Great, thanks again for taking this on!

druid-shell/src/mouse.rs

Co-authored-by: Colin Rofls <colin@cmyr.net>

jneem mentioned this pull request Nov 27, 2020

Improve cursor api #1368

Closed

JAicewizard reviewed Nov 28, 2020

View reviewed changes

cmyr reviewed Dec 2, 2020

View reviewed changes

jneem added 5 commits December 3, 2020 10:55

Make the cursor API stateful and widget-local.

1498218

Address review comments.

fcee3be

Add changelog.

5b44293

Fix Split to work the the new API.

500f7d9

Add a missing PartialEq.

72b9d33

cmyr approved these changes Dec 3, 2020

View reviewed changes

druid-shell/src/mouse.rs Outdated Show resolved Hide resolved

Update druid-shell/src/mouse.rs

9ab2806

Co-authored-by: Colin Rofls <colin@cmyr.net>

jneem merged commit 3eb1a84 into linebender:master Dec 4, 2020

jneem mentioned this pull request Dec 21, 2020

[feature request] A widget that sets the cursor on hover. #1478

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the cursor API stateful and widget-local. #1433

Make the cursor API stateful and widget-local. #1433

jneem commented Nov 27, 2020

JAicewizard commented Nov 28, 2020

raphlinus commented Nov 28, 2020

JAicewizard Nov 28, 2020

raphlinus Nov 28, 2020

jneem Nov 28, 2020

jneem commented Nov 28, 2020

jneem commented Nov 28, 2020

JAicewizard commented Nov 28, 2020

jneem commented Nov 28, 2020

cmyr left a comment

cmyr Dec 2, 2020

derekdreery Dec 3, 2020 •

edited

Loading

cmyr Dec 3, 2020

jneem Dec 3, 2020

cmyr Dec 2, 2020

cmyr Dec 2, 2020

jneem Dec 3, 2020

cmyr Dec 2, 2020

jneem Dec 3, 2020

cmyr Dec 2, 2020

jneem Dec 3, 2020

jneem commented Dec 3, 2020

cmyr left a comment

Make the cursor API stateful and widget-local. #1433

Make the cursor API stateful and widget-local. #1433

Conversation

jneem commented Nov 27, 2020

JAicewizard commented Nov 28, 2020

raphlinus commented Nov 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jneem commented Nov 28, 2020

jneem commented Nov 28, 2020

JAicewizard commented Nov 28, 2020

jneem commented Nov 28, 2020

cmyr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekdreery Dec 3, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jneem commented Dec 3, 2020

cmyr left a comment

Choose a reason for hiding this comment

derekdreery Dec 3, 2020 •

edited

Loading