Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Stop relying on dead-reckoning mouse buttons for motion_notify_event.#28453
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
03589b1 to96afd2cCompare| is_over, x, y, event_state = surface.get_device_position( | ||
| self.get_display().get_default_seat().get_pointer()) | ||
| # NOTE: alternatively we could use | ||
| # event_state = controller.get_current_event_state() |
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.
Given that MouseEvent rejects passingbuttons for anything but move events, should we use the non-spammy version?
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.
Both versions are spammy :/ Perhaps@QuLogic may be knowledgeable here?
I think this definitely needs to be fixed before merging; I would hope that gtk4 offers some non-broken way of checking mouse button state on macos... Fixing multiclicks+tk+macos, while desirable, seems to be less of a blocker for me; that issue won't prevent fixing the example in the initial message.
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.
(Also, this appears empirically to have possibly been fixed at some point between gtk4.14.2 and 4.14.5...)
Do you have some script to test this out? |
anntzer commentedOct 22, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
To check that # pick a backend and then...frompylabimport*;gcf().canvas.mpl_connect("motion_notify_event",lambdae:print(e.buttons));show() then move the mouse over the canvas while pressing various buttons. To check that this does not rely on dead-reckoning (i.e. the right-click context menu example above): the context menu is implemented for gtk3/qt/tk/wx (but not gtk4 or macos) athttps://github.com/anntzer/mplinorm; run # pick a backend and then...frompylabimport*;importmplinorm;imshow([[1,2]]);mplinorm.install()gcf().canvas.mpl_connect("motion_notify_event",lambdae:print(e.buttons));show() then right click to trigger the menu and check that after quitting the menu, motion_notify_event sees that no buttons is pressed. (Also rebased the PR.) |
96afd2c to4091945CompareOn TkAgg, middle and right buttons are swapped for me. |
Previously, for motion_notify_event, `event.attribute` was set bychecking the last button_press_event/button_release event. This isbrittle for the same reason as to why we introduced `event.modifiers`to improve over `event.key`, so introduce the more robust`event.buttons` (see detailed discussion in the attribute docstring).For a concrete example, consider e.g. from matplotlib import pyplot as plt from matplotlib.backends.qt_compat import QtWidgets def on_button_press(event): if event.button != 3: # Right-click. return menu = QtWidgets.QMenu() menu.addAction("Some menu action", lambda: None) menu.exec(event.guiEvent.globalPosition().toPoint()) fig = plt.figure() fig.canvas.mpl_connect("button_press_event", on_button_press) fig.add_subplot() plt.show()(connecting a contextual menu on right button click) where a right clickwhile having selected zoom mode on the toolbar starts a zoom mode thatstays on even after the mouse release (because the mouse release eventis received by the menu widget, not by the main canvas). This PR doesnot fix the issue, but will allow a followup fix (where themotion_notify_event associated with zoom mode will be able to firstcheck whether the button is indeed still pressed when the motionoccurs).Limitations, on macOS only (everything works on Linux and WindowsAFAICT):- tk only reports a single pressed button even if multiple buttons are pressed.- gtk4 spams the terminal with Gtk-WARNINGs: "Broken accounting of active state for widget ..." on right-clicks only; similar issues appear to have been reported a while ago to Gtk (https://gitlab.gnome.org/GNOME/gtk/-/issues/3356 and linked issues) but it's unclear whether any action was taken on their side.(Alternatively, some GUI toolkits have a "permissive" notion ofdrag events defined as mouse moves with a button pressed, which we coulduse as well to define event.button{,s} for motion_notify_event, bute.g. Qt attaches quite heavy semantics to drags which we probably don'twant to bother with.)4091945 tocd37b73CompareAfter some testing the values appear to be swapped on macosx compared to linux & windows, I adjusted the table accordingly. (The relevant chunk of code ishttps://github.com/tcltk/tk/blob/c9f915990df0d05cd6cc0ede5575a7ed32bb9978/macosx/tkMacOSXMouseEvent.c#L131-L133) Perhaps if we want to be extra safe we should toggle based on |
Works with 1, 2, 3 on tk + qt5 in Mac and 1, 2, 3 on tk in linux (x11) and 1, 2, 3, 8, 9 with qt6 on linux for me. |
I was able to test with GTK3Agg, GTK4Agg, QtAgg, TkAgg, and wxAgg on Linux. GTK3Agg, TkAgg, and wxAgg couldn't see the back/forward buttons, but they're possibly just too old to do that right on Wayland. |
Looking at it again I note that I don't try to report back/forward for macos just because I don't have such a mouse to test, but perhaps mpl_buttons() in _macosx.m just needs some additional entries for |
I am happy to merge this as-is and follow up with the back/forward buttons in another PR. |
Mypy passes locally, CI was fixed in#29014 |
6a8a80d intomatplotlib:mainUh oh!
There was an error while loading.Please reload this page.
Previously, for motion_notify_event,
event.attributewas set by checking the last button_press_event/button_release event. This is brittle for the same reason as to why we introducedevent.modifiersto improve overevent.key(#23473), so introduce the more robustevent.buttons(see detailed discussion in the attribute docstring).For a concrete example, consider e.g.
(connecting a contextual menu on right button click) where a right click while having selected zoom mode on the toolbar starts a zoom mode that stays on even after the mouse release (because the mouse release event is received by the menu widget, not by the main canvas). This PR does not fix the issue, but will allow a followup fix (where the motion_notify_event associated with zoom mode will be able to first check whether the button is indeed still pressed when the motion occurs).
Limitations, on macOS only (everything works on Linux and Windows AFAICT):
(Alternatively, some GUI toolkits have a "permissive" notion of drag events defined as mouse moves with a button pressed, which we could use as well to define event.button{,s} for motion_notify_event, but e.g. Qt attaches quite heavy semantics to drags which we probably don't want to bother with.)
PR summary
PR checklist