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

Add hot state updating via wheel events. #951

Merged
merged 1 commit into from
May 17, 2020
Merged

Conversation

xStrom
Copy link
Member

@xStrom xStrom commented May 17, 2020

This fixes an edge case where we receive a wheel event at a location different than the last move event.

@xStrom xStrom added the S-needs-review waits for review label May 17, 2020
Copy link
Collaborator

@luleyleo luleyleo left a comment

Choose a reason for hiding this comment

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

Looks good.
I was wondering why MouseMove is a special case for recursion.
It should probably get a comment about why that was done.
Should I open an issue for that?

@luleyleo luleyleo removed the S-needs-review waits for review label May 17, 2020
@xStrom
Copy link
Member Author

xStrom commented May 17, 2020

No need for an issue, I'll send a simple PR with that comment in a few minutes.

@xStrom xStrom merged commit 4eba61c into linebender:master May 17, 2020
@xStrom xStrom deleted the wheel-hot branch May 17, 2020 13:11
@cmyr
Copy link
Member

cmyr commented May 17, 2020

I have a thought related to this that I'm not sure I've ever spelled out:

I think it should be an invariant that changes in mouse position always generate a move event.

That is: a click or a wheel or any other mouse event should always occur at the same position as the last move.

If this isn't something the platform enforces, it's something that we could implement in druid-shell.

I think this offers a lot of clarity at the druid layer; it would simplify a lot of code, and would then be a very useful guarantee that we could pass along to users.

Is there any reason this doesn't make sense?

@xStrom
Copy link
Member Author

xStrom commented May 17, 2020

Yeah that makes sense and we can do it. I don't think any of the platforms we support guarantee a move event before click, but druid-shell could do that. It would indeed greatly simplify not just druid logic, but also the logic in apps built on druid.

@luleyleo
Copy link
Collaborator

Yes, this sounds like a good idea, after all widgets care about mouse movement relative to their own position, which would be changed by scrolling a Scroll for example. Not sure tho, if it should always generate a MouseMove, for example when nothing is moving despite scrolling. But then again I doubt there is any harm in having to many stationary move events 🤔

@cmyr
Copy link
Member

cmyr commented May 17, 2020

I wouldn't include scroll in this, I think that's an over-complication. Hot might change as scroll does, but this doesn't involve a mouse move?

@xStrom
Copy link
Member Author

xStrom commented May 17, 2020

Hmm, well this is actually a bit tricky. So druid-shell could guarantee that there's a move event before a click event if the position has changed - but this is only in relation to the window.

Druid would still suffer. Imagine the following scenario:

  • Mouse is moved to location X, druid widget gets the move event.
  • Druid widget moves to a new location (could be a timer, a database result, whatever)
  • Mouse is clicked, druid-shell doesn't send a move event because in relation to the window the mouse is in the same position as previously. However in relation to the widget the location has changed.
  • The widget gets a click event with a position different from the last move event.

I don't think this can be solved without keeping some sort of state per widget about the mouse location. This would have to be in druid, not druid-shell.

Thus I'm not sure if it's worth doing this move event guarantee. It would end up in more complex code than right now.

@cmyr
Copy link
Member

cmyr commented May 17, 2020

I guess the other thing to consider is do we want to make scrolling first-class, and have some sort of 'ScrollChanged' lifecycle event?

@xStrom
Copy link
Member Author

xStrom commented May 17, 2020

Not sure I understand. ScrollChanged sent to children of Scroll to inform of origin changes?`

(Also just to be clear, my example above about a widget moving is independent of scrolling and can happen with regular widgets and regular clicks.)

@luleyleo
Copy link
Collaborator

I think druid should generate MouseMove events when scrolling or layout in general changes 🤔
Because currently the list example visualizes the issue quite nicely, hovering a button marks it 'hot', after scrolling until the cursor is no longer over the button it still is hot and the button that should be hot did not get an update.

@xStrom
Copy link
Member Author

xStrom commented May 17, 2020

Hot state is already updated after layout changes. I think this is something specific to Scroll. I opened #956 to track this.

@luleyleo
Copy link
Collaborator

I see, as long as hot state is reliable, it could be ok not to generate MouseMoves when scrolling but I still think it would be necessary to get something like region select + scroll which most platforms allow, as it would probably require reliable MouseMoves relative to the widget position.

@cmyr
Copy link
Member

cmyr commented May 18, 2020

I think MouseMove should only be sent if the user has moved the mouse.

And yes, ScrollChanged would be a lifecycle event sent to children in a container that has been scrolled. I'm not saying we want this, but it's possible; there are a bunch of funny things about scrolling that suggest it might need special casing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants