Remove custom backend_nbagg.show(), putting logic in manager show. #23100
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The _Backend class machinery can autogenerate a backend_module.show() function, which (in short) calls manager.show() on all managers and then calls the optionally-provided mainloop() function. backend_nbagg currently completely bypasses this autogeneration because it wants to additionally fiddle with callbacks and Gcf (it doesn't need any mainloop()), but that logic can just as well go to FigureManagerNbAgg.show() instead. This way it benefits from the default-generated show; moreover, it seems a bit strange that figures shown with manager.show() (instead of plt.show()) do indeed get displayed, but just don't have their callbacks/Gcf-ness adjusted. This is also related to the move towards putting show()-generating information on the canvas class for inheritability.
timhoffm
approved these changes
May 23, 2022
jklymak
reviewed
Jun 23, 2022
| # one. Disable this behaviour, as it results in figures being put as | ||
| # the active figure after they have been shown, even in non-interactive | ||
| # mode. | ||
| if hasattr(self, '_cidgcf'): |
There was a problem hiding this comment.
Is there any way to add tests to argue that this is working as expected?
There was a problem hiding this comment.
Actually, there was previously no test coverage for the corresponding old code paths (as can be checked e.g. via pytest --cov --pyargs matplotlib.tests.test_backend_nbagg && coverage html: the entire show() function was untested), so I'll ask for an out there and just point out that I simply moved some code around (also, my understanding of nbagg is on the weaker side...).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The _Backend class machinery can autogenerate a backend_module.show()
function, which (in short) calls manager.show() on all managers and then
calls the optionally-provided mainloop() function.
backend_nbagg currently completely bypasses this autogeneration because
it wants to additionally fiddle with callbacks and Gcf (it doesn't need
any mainloop()), but that logic can just as well go to
FigureManagerNbAgg.show() instead. This way it benefits from the
default-generated show; moreover, it seems a bit strange that figures
shown with manager.show() (instead of plt.show()) do indeed get
displayed, but just don't have their callbacks/Gcf-ness adjusted.
This is also related to the move towards putting show()-generating
information on the canvas class for inheritability (#23090).
PR Summary
PR Checklist
Tests and Styling
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).