-
-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Qt wheelEvent: use x coordinate if ALT is press #25895
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
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.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
|
Thanks! I think that this makes sense, but will leave it to others to determine if it is a feature that we want. Will also chip in that it is more or less impossible to test wheelEvent in Qt (and we do not even use pytest-qt). |
|
Thank you for working on this, however I am currently 👎 on taking this change. That we do not currently deal with horizontal scroll is arguably a bug (or at least a missing feature) so I have concerns that this would pin us in the future. My other concern is a bit philosophical. I think of our event system as being "the last in line" of a long chain of software between the user pressing a physical key and the mpl-side callback being notified and as such we should trust the layers between us and the users to have done their jobs correctly / as intended and we should no second-guess those layers. For example, there are some keys the user hits that get intercepted by the window manager (for example "change my virtual desktop") that never even make it to the GUI application we are wrapped in. In general we have no visibility into what (or why) those layers are doing and while it may seem "obvious" in some cases, what we should (un)do, it is impossible to be sure we got it right so we should not do anything. If the issue is behavior at a level closer to the user, the user should fix that layer, rather than sometimes maybe fixing it for some users and leaving users who did want that behavior stuck. |
tacaswell
left a comment
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.
See my long comment.
TL;DR
- we may want to support horizontal scroll events in the future
- we should not second-guess Qt
If consensus goes the other way anyone can dismiss this review.
|
Thank you for your work on this @GerardDorado ! Even if we do not take this patch, your research on this being expected Qt behavior is extremely valuable. |
|
I agree with @tacaswell's assessment. |
|
Thanks everyone for the feedback!, It was my first time contributing to open source so I'm glad I could help a little |
|
@GerardDorado Please do not be discouraged and I hope we hear from you again. Even though we did not merge your PR, your work did result in closing #25671 which is a contribution to Matplotlib (even if it won't show up in the git history or GH's green-square metrics (which one of the reasons those metrics should be treated with a bit of skepticism but I digress))! |
PR summary
Based on what I found here and here it looks like Qt default behaviour is to reverse the axis coordinates for the
WheelEventwhen the'alt'key is pressed, so I made a small modification to thewheelEventmethod inside theFigureCanvasQTclass to check if the key is pressed, if so, instead of usingangleDelta().y()we useangleDelta().x()to get the correct vertical movement.I couldn't find a reference to the x axis anywhere in the code, and since the
wheelEventmethod only used the vertical movement, i don't think this would rise an issue in the future.I don't know if this happens for
pixelDelta(), since I can't test it.PR checklist