matplotlib icon indicating copy to clipboard operation
matplotlib copied to clipboard

Separately track modifier keys for mouse events.

Open anntzer opened this issue 2 years ago • 2 comments

PR Summary

Ensure that modifier keys are correctly reported for mouse events even if the last key press occurred out of the canvas.

Whether the event modifiers are directly available on enter/leave events depends on the backend, but all are handled here (except possibly for macos, which I haven't checked).

There's a first separate commit for the macosx backend for some preliminary work with ObjC.

Followup to #16931 (it's the reason for that big PR...). Supersedes #6159. See also the side note at https://github.com/matplotlib/matplotlib/issues/23436#issuecomment-1186302186.

PR Checklist

Tests and Styling

  • [ ] Has pytest style unit tests (and pytest passes).
  • [ ] Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • [ ] New features are documented, with examples if plot related.
  • [ ] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • [ ] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • [ ] Documentation is sphinx and numpydoc compliant (the docs should build without error).

anntzer avatar Jul 22 '22 16:07 anntzer

I haven't had a chance to look at the details here, but I just pulled and tested quick on macosx and hit some errors that you can probably fix pretty quick:

Clicking anywhere:

TypeError: MouseEvent.__init__() got an unexpected keyword argument 'num'

ctrl + clicking anywhere:

SystemError: NULL object passed to Py_BuildValue

greglucas avatar Jul 29 '22 13:07 greglucas

Oops, thanks for trying. Hopefully fixed now?

anntzer avatar Jul 29 '22 15:07 anntzer

Looks like the failures with macosx are real and a PROCESS_EVENT snuck in after your initial PR.

greglucas avatar Dec 12 '22 23:12 greglucas

Oops, probably a careless rebase, thanks for the ping.

anntzer avatar Dec 13 '22 10:12 anntzer

Ah, I had forgotten to implement the exclude kwarg to _mpl_modifiers for some backends, thanks for catching that, the doubling of modifiers should now be fixed. Can I ask for a joker and defer adding modifiers to key events to another issue?

anntzer avatar Dec 13 '22 21:12 anntzer

I suspect it's just a matter of updating (if needed) the table in _mpl_modifiers (previously _get_key) (perhaps by manually inspecting event.state in various cases); as you have access to a mac, can you give it a try (perhaps both with a laptop keyboard and with an independent one just to be sure)?

anntzer avatar Dec 14 '22 07:12 anntzer

I tested qt + tk on linux and osx on osx.

Also saw something funny going on with 'meta' on tk. I saw it in the out put from the snippet Greg posted, but I could not sort out which keys I hit to get it.

tacaswell avatar Dec 15 '22 23:12 tacaswell

Anyone can merge on green (again, excluding the patch coverage).

tacaswell avatar Dec 15 '22 23:12 tacaswell

Thank you @anntzer !

tacaswell avatar Dec 16 '22 17:12 tacaswell

@greglucas I got my hands on a mac :) and had a look at https://github.com/matplotlib/matplotlib/pull/23473#pullrequestreview-1216540516. I think the relevant patch to fix the super+meta issue on tk is basically

diff --git i/lib/matplotlib/backends/_backend_tk.py w/lib/matplotlib/backends/_backend_tk.py
index 7aa24e06ff..6893120ace 100644
--- i/lib/matplotlib/backends/_backend_tk.py
+++ w/lib/matplotlib/backends/_backend_tk.py
@@ -354,7 +354,7 @@ class FigureCanvasTk(FigureCanvasBase):
             ("ctrl", 1 << 2, "control"),
             ("alt", 1 << 4, "alt"),
             ("shift", 1 << 0, "shift"),
-            ("super", 1 << 3, "super"),
+            ("foobar", 1 << 3, "meta"),
         ] if sys.platform == "darwin" else [
             ("ctrl", 1 << 2, "control"),
             ("alt", 1 << 3, "alt"),

where the last tuple item must be "meta" (to match tk reporting the cmd key with event.keysym = "Meta_L" (or "Meta_R")) and the first entry is how you think the modifier should actually be called (I guess "cmd", or perhaps "meta", I don't really have a good intuition here)? Can you decide on that last point and pick up the patch?

anntzer avatar Jan 31 '23 23:01 anntzer