Skip to content
This repository has been archived by the owner on Jul 10, 2023. It is now read-only.

return coordinates in MouseWheel events #109

Merged

Conversation

gterzian
Copy link
Member

@gterzian gterzian commented Dec 31, 2016

Glutin side of servo/servo#14808 to fix servo/servo#14290, it's a follow up from #97


This change is Reviewable

@@ -31,7 +31,7 @@ pub enum Event {
MouseMoved(i32, i32),

/// A mouse wheel movement or touchpad scroll occurred.
MouseWheel(MouseScrollDelta, TouchPhase),
MouseWheel(MouseScrollDelta, TouchPhase, Option<(i32, i32)>),
Copy link
Member Author

Choose a reason for hiding this comment

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

Should I bump up a version, since this does change the public API? (used in Servo at https://github.com/servo/servo/blob/master/ports/glutin/window.rs#L19)

Choose a reason for hiding this comment

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

Yes.

@@ -31,7 +31,7 @@ pub enum Event {
MouseMoved(i32, i32),

/// A mouse wheel movement or touchpad scroll occurred.
MouseWheel(MouseScrollDelta, TouchPhase),
MouseWheel(MouseScrollDelta, TouchPhase, Option<(i32, i32)>),

/// An event from the mouse has been received.
MouseInput(ElementState, MouseButton, Option<(i32, i32)>),
Copy link
Member Author

@gterzian gterzian Dec 31, 2016

Choose a reason for hiding this comment

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

Would we want to create a MousePosition struct, to try to be more descriptive than the current Option<(i32, i32)> ?

That way we could have:

  • MouseInput(ElementState, MouseButton, Option<MousePosition>)
  • MouseWheel(MouseScrollDelta, TouchPhase, Option<MousePosition>)

@jdm
Copy link
Member

jdm commented Dec 31, 2016

This should be submitted upstream as well.

@gterzian
Copy link
Member Author

gterzian commented Jan 1, 2017

@jdm thanks for pointing that out. Once it's reviewed and ready to go, I'll make usre to also submit it upstream...

@gterzian
Copy link
Member Author

gterzian commented Jan 1, 2017

@jdm actually I just saw this repo is 111 commits ahead of the upstream... Perhaps we should submit a PR all in one go to propose the sync? Or should all those other changes not be submitted, while this one should?

@jdm
Copy link
Member

jdm commented Jan 1, 2017

It's more complicated than that, unfortunately. Those commits being counted are merges with upstream, I'm pretty sure. The actual number of changes we have not upstreamed is quite small.

@paulrouget
Copy link

I don't know the cost of get_mouse_position. Maybe we should only add the coordinate when the phase is Started?

@gterzian
Copy link
Member Author

@paulrouget good idea, made the change, all still seems to work...

@paulrouget
Copy link

paulrouget commented Jan 16, 2017

Looks good to me (with version bump), but I can only review Tomaka's glutin.

Anyone want to review this?

@gterzian gterzian force-pushed the return_mouse_coords_in_wheel_events branch 3 times, most recently from e477fec to a45969b Compare January 17, 2017 15:26
@nox
Copy link

nox commented Jan 17, 2017

The bump should be 0.7 given it's a breaking change.

r? @pcwalton

@bors-servo
Copy link

☔ The latest upstream changes (presumably #111) made this pull request unmergeable. Please resolve the merge conflicts.

@pcwalton
Copy link

This looks good to me. Squash and I'll r+

@jdm jdm assigned pcwalton and unassigned nox Jan 20, 2017
@gterzian gterzian force-pushed the return_mouse_coords_in_wheel_events branch 6 times, most recently from b238948 to 1dfd538 Compare January 22, 2017 14:32
@gterzian gterzian force-pushed the return_mouse_coords_in_wheel_events branch from 1dfd538 to ace7e62 Compare January 22, 2017 14:34
@gterzian
Copy link
Member Author

gterzian commented Jan 22, 2017

@pcwalton thanks for the review, it has been squashed...

@jdm
Copy link
Member

jdm commented Jan 22, 2017

@bors-servo: r=pcwalton

@bors-servo
Copy link

📌 Commit ace7e62 has been approved by pcwalton

@bors-servo
Copy link

⚡ Test exempted - status

@bors-servo bors-servo merged commit ace7e62 into servo:servo Jan 22, 2017
bors-servo pushed a commit that referenced this pull request Jan 22, 2017
…pcwalton

return coordinates in MouseWheel events

Glutin side of servo/servo#14808 to fix servo/servo#14290, it's a follow up from #97

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/glutin/109)
<!-- Reviewable:end -->
@paulrouget paulrouget mentioned this pull request Nov 6, 2017
3 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When Servo isn't the window directly in front, scroll is stuck in previously scrolled layer
6 participants