Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Dedupe implementation of axes grid switching in toolmanager.#16823
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
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.
Overall, this looks like a great simplification. 👍
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 likeMouseEvent,KeyPressEvent and thatkey_press_handler() will ignoreMouseEvents. But for some reason this is triggered via aMouseEvent.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
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.
Even though a test would be a strong plus, in a way not testing the new code is not worse than not testing the old code.
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