-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Move set_cursor from the toolbar to FigureCanvas. #20620
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
(Not directly related, but I guess you could also do the same with draw_rubberband/remove_rubberband, which is already tightly coupled with the draw loop?) |
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.
Side note:
We should move away from cursors
and switch to directly using the enum Cursors
everywhere. Possbibly also deprecate cursors
, Not sure if we can already issue warnings on module variables. If not, still announce the deprecation, and extend the duration by 1 or 2 extra releases. When people have to touch set_cursor
anyway, they should switch to the enum while they are at it. This need not (possibly should not) be part of this PR for simplicity, but it should be a follow-up
lib/matplotlib/backend_managers.py
Outdated
_api.warn_deprecated("3.5", | ||
message="Overriding ToolSetCursor with " | ||
f"{tool_cls.__qualname__} was only " | ||
"necessary to provide the .set_cursor() " | ||
"method, which is deprecated since " | ||
"%(since)s and will be removed " | ||
"%(removal)s") | ||
|
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 don't quite follow this. From a user perspective, under which conditions is the warning emitted, and what action should they take if they see this 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.
None, in Matplotlib. It's for third-party backends, of which I'm not sure any even implement tool manager. I'll tweak the message to say so.
There's module-level |
I think #10735 is what you are looking for :-) (or some variants thereof, I can open a new PR if there's interest) |
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.
modulo rebase
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.
Modulo one comment on a deprecation.
While the toolbar does need to track what it wants to set the cursor to based on the active tool, actually setting it is an act on the canvas (or sometimes the window, but this is toolkit dependent, so we won't worry about that.)
Which is now replaced by `FigureCanvasBase.set_cursor`.
@QuLogic, I just tested the Somehow it has to do with the plotted mappable on the axes... import matplotlib.pyplot as plt
fig, ax = plt.subplots()
ax.contourf([[0, 1], [2, 3]]) # cursor changes to the grab/zoom image
# ax.imshow([[0, 1], [2, 3]]) # cursor does not change
plt.show() It works properly for contourf, but not for imshow/pcolormesh. The cursor works as expected for all mappables with |
PR Summary
While the toolbar does need to track what it wants to set the cursor to based on the active tool, actually setting it is an act on the canvas (or sometimes the window, but this is toolkit dependent, so we won't worry about that.)
It also means we don't need redundant
set_cursor
definitions on both toolbar/toolmanager(not yet committed.)PR Checklist
pytest
passes).flake8
on changed files to check).flake8-docstrings
and runflake8 --docstring-convention=all
).doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).