Move show() to somewhere naturally inheritable / document what pyplot expects from a backend. #23101
Conversation
ee82d35 to
da12a3e
Compare
da12a3e to
6670d0d
Compare
|
Failures looks real, and has picked up rebase issues... This looks like a reasonable simplification, thought I'm not an expert |
1aebeb8 to
86e5d5b
Compare
|
Thanks for pointing this out, this should be hopefully fixed now. I would also like to request the reviewer's thoughts (perhaps @timhoffm?) as to whether pyplot_show should be a canvas or a manager method (as explained in the commit message/top post). |
86e5d5b to
340d579
Compare
5118228 to
e181216
Compare
4a99249 to
7e5f7db
Compare
|
Discussed on the call: I'll move pyplot_show to the manager class instead, keeping that name; I'll also document explicitly that's it's only public for the purpose of being overridden by third-party backends, not for being called directly by end users. |
7e5f7db to
4c299b6
Compare
|
The discussed modifications have been implemented. |
0986e93 to
4500f83
Compare
4500f83 to
d30b372
Compare
d30b372 to
d6ebf93
Compare
d6ebf93 to
5f1d623
Compare
|
I have addressed your comments (thanks for the review) but moved the docs part of the PR to #24218, to make the pure implementation part easier to merge. |
| manager.show() # Emits a warning for non-interactive backend. | ||
| except NonGuiException as exc: | ||
| _api.warn_external(str(exc)) | ||
| if block is None: |
There was a problem hiding this comment.
Do we also want to check if pypplot is already imported?
5f1d623 to
2fca527
Compare
30dd65f to
e32ae61
Compare
| # Hack: Are we in IPython's %pylab mode? In pylab mode, IPython | ||
| # (>= 0.10) tacks a _needmain attribute onto pyplot.show (always | ||
| # set to False). | ||
| from matplotlib import pyplot | ||
| ipython_pylab = hasattr(pyplot.show, "_needmain") | ||
| ipython_pylab = hasattr( | ||
| getattr(sys.modules.get("pyplot"), "show", None), "_needmain") |
There was a problem hiding this comment.
Move this to a helper function in cbook?
There was a problem hiding this comment.
The duplication is only temporary, because the version in _Backend.show will go away as I plan to get rid of the _Backend class, which was an implementation detail anyways. (I'm not doing it immediately because the ipympl backend has started relying on it, but I plan to help them to get rid of that use, either going back to the old-style fully manual API if they want a long backcompat, or going to the new class-based API.)
It's actually not clear whether to move it to FigureCanvas or FigureManager. FigureCanvas already has start_event_loop (cf. start_main_loop), but pyplot_show/start_main_loop is more a global/pyplot-only concept, so perhaps it belongs to FigureManager instead? OTOH, being on canvas_class makes it easier for switch_backend to access it (it doesn't need to go through `canvas_class.manager_class.pyplot_show`, which is relevant considering that some designs of inheritable backends didn't even have a manager_class attribute).
e32ae61 to
86f26a0
Compare
PR Summary
Goes on top of #23090 (see #23090 (comment)).Move show() to somewhere naturally inheritable.
It's actually not clear whether to move it to FigureCanvas or
FigureManager. FigureCanvas already has start_event_loop (cf.
start_main_loop), but pyplot_show/start_main_loop is more a
global/pyplot-only concept, so perhaps it belongs to FigureManager
instead? OTOH, being on canvas_class makes it easier for switch_backend
to access it (it doesn't need to go through
canvas_class.manager_class.pyplot_show, which is relevant consideringthat some designs of inheritable backends didn't even have a
manager_class attribute).
Document what pyplot expects from a backend.
Finally, a document stating what's needed in a backend!
(only the pyplot interaction part, not the rendering part.)
Edit: now moved to #24218.
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).