-
Notifications
You must be signed in to change notification settings - Fork 8.8k
Add support for horizontal mouse wheel events #19248
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
Conversation
@microsoft-github-policy-service agree |
e326ed1
to
90a82d4
Compare
90a82d4
to
b9cfb5a
Compare
b9cfb5a
to
cc95e48
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd refactor the functions to take X and Y simultaneously, but I'm also okay with merging it as-is now. I can try to do this.
return _sendMouseEventHelper(terminalPosition, | ||
WM_MOUSEWHEEL, | ||
delta.X != 0 ? WM_MOUSEHWHEEL : WM_MOUSEWHEEL, | ||
modifiers, | ||
::base::saturated_cast<short>(delta), | ||
::base::saturated_cast<short>(delta.X != 0 ? delta.X : delta.Y), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh interesting... I wonder if we should refactor _sendMouseEventHelper
to allow for both X and Y to be passed? You don't have to do this in this PR of course - I can do that as a follow up (or I push into your PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left it as is because _sendMouseEventHelper
is used for other buttons with a delta
value of zero. Instead, we should perhaps refactor _core.SendMouseEvent
to make it take a Core::Point delta
instead of short
. However, that feels like a larger endeavor with little to no gain 😕
I will leave the decision to you
This adds support for horizontal mouse wheel events (WM_MOUSEHWHEEL). With this change, applications running in the terminal can now receive and respond to horizontal scroll inputs from the mouse/trackpad. Fixes: microsoft#19245
Co-authored-by: Dustin L. Howett <[email protected]>
f28d525
to
f6b7631
Compare
f6b7631
to
e53f03e
Compare
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/ap run |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@DHowett Could you please rerun the pipeline? |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is excellent work. Thank you so much for being comprehensive and for being patient with our slow code reviews. 🙂
This should be out in tonight's Canary build! I'll also mark it up as considered for the next update to the 1.24 preview release.
This adds support for horizontal mouse wheel events (`WM_MOUSEHWHEEL`). With this change, applications running in the terminal can now receive and respond to horizontal scroll inputs from the mouse/trackpad. Closes #19245 Closes #10329 (cherry picked from commit 814f78e) Service-Card-Id: PVTI_lADOAF3p4s4BBcTlzgep1sM Service-Version: 1.24
Summary of the Pull Request
This adds support for horizontal mouse wheel events (WM_MOUSEHWHEEL). With this change, applications running in the terminal can now receive and respond to horizontal scroll inputs from the mouse/trackpad.
References and Relevant Issues
Closes #19245
Closes #10329
Validation Steps Performed
Tested terminal applications that receive horizontal mouse wheel events.
PR Checklist