Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.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 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
(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?) |
timhoffm left a comment• 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.
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 fromcursors and switch to directly using the enumCursors everywhere. Possbibly also deprecatecursors, 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 touchset_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
Uh oh!
There was an error while loading.Please reload this page.
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 |
anntzer commentedJul 12, 2021 • 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.
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.
Uh oh!
There was an error while loading.Please reload this page.
While the toolbar does need to track what it wants to set the cursor tobased 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'tworry 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... importmatplotlib.pyplotaspltfig,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 changeplt.show() It works properly for contourf, but not for imshow/pcolormesh. The cursor works as expected for all mappables with |
Uh oh!
There was an error while loading.Please reload this page.
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_cursordefinitions on both toolbar/toolmanager(not yet committed.)PR Checklist
pytestpasses).flake8on changed files to check).flake8-docstringsand 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).