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

Provide additional modifiers as part of the mouse wheel event #895

Merged
merged 11 commits into from
May 9, 2020

Conversation

teddemunnik
Copy link
Contributor

@teddemunnik teddemunnik commented Apr 30, 2020

As part of the prototype I'm working on I wanted to be able to access the current mouse position as part of mouse wheel events, in order to implement zoom to mouse cursor. Since we already supply shared modifiers between other mouse events, and from following the discussion in #843, it seemed somewhat reasonable to do this by changing WinHandler::wheel to take a MouseEvent like the other mouse events, and adding wheel_delta to MouseEvent.

I've implemented the changes I am suggesting above as a draft on the windows and GTK backends, but work on the other backends is remaining.

Copy link
Member

@xStrom xStrom left a comment

Choose a reason for hiding this comment

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

Generally looks good. A few inline comments.

druid-shell/src/platform/windows/window.rs Outdated Show resolved Hide resolved
druid-shell/src/platform/windows/window.rs Outdated Show resolved Hide resolved
druid-shell/src/platform/windows/window.rs Show resolved Hide resolved
druid-shell/src/platform/windows/window.rs Outdated Show resolved Hide resolved
druid-shell/src/platform/windows/window.rs Outdated Show resolved Hide resolved
@xStrom xStrom added architecture changes the architecture, usually breaking breaking change pull request breaks user code labels May 1, 2020
@teddemunnik
Copy link
Contributor Author

Thanks for the review @xStrom! I implemented all the changes you requested in the windows shell, let me know if I understood correctly the changes with LOWORD extracting.

@xStrom
Copy link
Member

xStrom commented May 1, 2020

Yeah the LOWORD stuff is mostly fine, but I think let's do it only once.

let system_delta = HIWORD(wparam as u32) as i16 as f64;
let down_state = LOWORD(wparam as u32) as usize;

Then the subsequent use can just be on down_state.

@teddemunnik teddemunnik force-pushed the wheel_event_data branch 2 times, most recently from e4f9266 to 4db45bd Compare May 2, 2020 15:36
@cmyr
Copy link
Member

cmyr commented May 7, 2020

I think this is reasonable. The only alternative I can think of is to have some general mechanism that exposes the mouse position, and plumb that through EventCtx.

CHANGELOG.md Show resolved Hide resolved
@teddemunnik teddemunnik marked this pull request as ready for review May 8, 2020 18:27
@teddemunnik
Copy link
Contributor Author

@cmyr getting the mouse position through EventCtx, sounds pretty neat too, and I could imagine that being useful for getting the mouse position at other points in the application, even if this PR would be merged. Looking at Web API WheelEvent inherits from MouseEvent, but adding the wheel event info to the MouseEvent seemed like a simpler approach for rust.

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

@xStrom xStrom 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, thanks!

@xStrom xStrom merged commit 1e90783 into linebender:master May 9, 2020
@xStrom xStrom removed the S-needs-review waits for review label May 9, 2020
@teddemunnik teddemunnik deleted the wheel_event_data branch May 9, 2020 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
architecture changes the architecture, usually breaking breaking change pull request breaks user code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants