Skip to content

Bind subplot_tool more closely to target figure. #21681

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 1 commit into from
Nov 20, 2021

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Nov 19, 2021

  • More the implementation to backend_bases and have pyplot read it,
    rather than having backend_bases fetch it from pyplot, which was a
    bit weird.
  • Attach the subplot_tool to the toolbar, which avoids having to keep
    a reference to it (when calling plt.subplot_tool), and prevents
    opening two subplot_tools for the same figure. On most backends,
    closing the subplot_tool deletes the reference, because reopening a
    closed figure seems not guaranteed to work; on Qt, which has its own
    subplot_tool, reopening the closed QDialog does work, so just reuse
    it.
  • Ensure that closing the main figure also closes the subplot_tool.
    ([MNT]: Close subplot_parameters window when main figure closes #21679, the main motivation for this PR.)

PR Summary

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • 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).
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).

Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

You sniped me on this PR!

Copy link
Contributor

@greglucas greglucas left a comment

Choose a reason for hiding this comment

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

When I click on the subplot tool using the qt5agg backend, it immediately looks like it clears the figure. Somehow the values for the left/right adjustment are getting set to near zero.
image

I combined this with #21663 and it worked like a charm on the macosx backend!

One other request would be to add a test for this. Something that adds the subplot adjust window, then closes the main figure and asserts there is nothing left in the manager so we can be sure we're keeping this behavior in the future.

- More the implementation to backend_bases and have pyplot read it,
  rather than having backend_bases fetch it from pyplot, which was a
  bit weird.
- Attach the subplot_tool to the toolbar, which avoids having to keep
  a reference to it (when calling plt.subplot_tool), and prevents
  opening two subplot_tools for the same figure.  On most backends,
  closing the subplot_tool deletes the reference, because reopening a
  closed figure seems not guaranteed to work; on Qt, which has its own
  subplot_tool, reopening the closed QDialog does work, so just reuse
  it.
- Ensure that closing the main figure also closes the subplot_tool.
@anntzer
Copy link
Contributor Author

anntzer commented Nov 20, 2021

Fixed the bug with qt.
Testing is not completely trivial, because the toolwindow is explicitly not registered with Gcf so that one cannot e.g. make it the current figure (that wouldn't make sense), so it's just hanging off the toolbar and there isn't anything left to test once it's gone. But if you can suggest a test implementation, please add it :-)

Copy link
Contributor

@greglucas greglucas left a comment

Choose a reason for hiding this comment

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

Yeah, there were no tests before, so this is fine if there is not a trivial way of doing it. I just tested with macosx and qt5agg and it works as expected for me now.

@greglucas greglucas merged commit 54aa92a into matplotlib:main Nov 20, 2021
@anntzer anntzer deleted the spt branch November 20, 2021 15:05
@QuLogic QuLogic added this to the v3.5.1 milestone Nov 23, 2021
@QuLogic
Copy link
Member

QuLogic commented Nov 23, 2021

@meeseeksdev backport to v3.5.x

meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Nov 23, 2021
jklymak added a commit that referenced this pull request Nov 23, 2021
…681-on-v3.5.x

Backport PR #21681 on branch v3.5.x (Bind subplot_tool more closely to target figure.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants