Skip to content

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

Merged
merged 3 commits into from
Jul 24, 2021

Conversation

QuLogic
Copy link
Member

@QuLogic QuLogic commented Jul 9, 2021

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

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • 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).

@QuLogic QuLogic marked this pull request as ready for review July 10, 2021 02:30
@QuLogic QuLogic added this to the v3.5.0 milestone Jul 10, 2021
@anntzer
Copy link
Contributor

anntzer commented Jul 10, 2021

(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?)

Copy link
Member

@timhoffm timhoffm left a 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

Comment on lines 259 to 266
_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")

Copy link
Member

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?

Copy link
Member Author

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.

@QuLogic
Copy link
Member Author

QuLogic commented Jul 12, 2021

Possbibly also deprecate cursors, Not sure if we can already issue warnings on module variables.

There's module-level __getattr__ in 3.7, but I was hoping @anntzer would have written some magic to wrap it all up.

@anntzer
Copy link
Contributor

anntzer commented Jul 12, 2021

I think #10735 is what you are looking for :-) (or some variants thereof, I can open a new PR if there's interest)

Copy link
Contributor

@anntzer anntzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

modulo rebase

Copy link
Member

@timhoffm timhoffm left a 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.

QuLogic added 3 commits July 23, 2021 20:13
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`.
@timhoffm timhoffm merged commit 01a1c49 into matplotlib:master Jul 24, 2021
@QuLogic QuLogic deleted the canvas-cursor branch July 24, 2021 08:15
@greglucas
Copy link
Contributor

@QuLogic, I just tested the macosx cursors a bit. It didn't have anything to do with this PR. Master is the same behavior as 3.4.2.

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 qt5agg.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants