Dedupe implementation of axes grid switching in toolmanager. #16823
Conversation
lib/matplotlib/backend_tools.py
Outdated
|
|
||
| class ToolMinorGrid(_ToolGridBase): | ||
| def trigger(self, sender, event, data=None): | ||
| sentinel = str(uuid.uuid4()) # Prevent triggering the wrong handler. |
There was a problem hiding this comment.
I'd add a comment to explain on a higher level what we're doing here, e.g.
# Trigger grid switching by temporarily setting rcParams['keymap.grid']
# to a unique key and sending an appropriate event.
Also, is it safe to modify the event here? Now knowing about the implementation, I could imagine e.g. that events are divided in separate subclasses like MouseEvent, KeyPressEvent and that key_press_handler() will ignore MouseEvents. But for some reason this is triggered via a MouseEvent.
I may be overly cautious here. From the little I know about the event system, I have the impression that it's rather loosely defined. I'm afraid that this could break if we do refinements on the event system.
Speaking of which, is it possible to test this?
There was a problem hiding this comment.
Added your clarifying comment.
The event is completely restored immediately after the event is processed, so we should be fine.
There's no tests for now, but helping setting up testing is actually a good reason for this dedupe -- that means less stuff to test twice (between the classic toolbar and the toolmanager).
There was a problem hiding this comment.
Well, actually I wanted to test this injection logic, which means you'd need to write a test around toolmanager. Independently, one may want a test for the classic trigger as well. 👼
Anyway, I'm half-way approving without a test, but want to think about it for a bit.
... by making it forward to the "classic" toolbar implementation.
|
Actually once #16933 goes in it should be easy to test this. |
|
This is very clever. |
... by making it forward to the "classic" toolbar implementation.
(I have the vague feeling we need to decide what we really want to do with toolmanager, at some point...)
PR Summary
PR Checklist