Skip to content

Move draw_if_interactive logic to new_figure_manager_given_figure. #15

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
Jun 15, 2022

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented May 27, 2022

Currently, Matplotlib only ever calls draw_if_interactive at the end of
plt.figure() (and upon figure unpickling), i.e. this is a mechanism to
further customize handling of newly generated figures. (Prior to
Matplotlib 1.5, draw_if_interactive was also called at the end of all
pyplot functions to trigger a figure redraw, but this is now handled by
the stale attribute.)

In order to simplify the backend API ("what is the API that a backend
module must/can provide"), I am planning to deprecate (on Matplotlib's
side) the ability for backends to provide a draw_if_interactive function
(forcing them to always do if interactive(): draw_idle()) (matplotlib/matplotlib#23105). Instead,
any relevant new-figure-customization logic can instead go into
new_figure_manager_given_figure. This PR implements this change for
matplotlib-inline, and should be fully back-compatible all the way back
to Matplotlib 1.5. I would like to make these changes first on the side
of the clients (i.e., the third-party backends), to catch any possible
problems with the intended change on Matplotlib's side.

@fperez
Copy link
Member

fperez commented Jun 4, 2022

Other than the minor comments above, this looks clean enough, and I trust your judgment far more than mine on where the API is going. Thanks!!

Once we finalize/merge this, I'd like to cut a release to get the fix for mpl #23007 out so that can be closed.

@anntzer
Copy link
Contributor Author

anntzer commented Jun 4, 2022

Did you forget to put in the comments?

@fperez
Copy link
Member

fperez commented Jun 4, 2022

No, I mean the inline comments in the file review about the missing docstrings, that's all :)

Currently, Matplotlib only ever calls draw_if_interactive at the end of
`plt.figure()` (and upon figure unpickling), i.e. this is a mechanism to
further customize handling of newly generated figures.  (Prior to
Matplotlib 1.5, draw_if_interactive was also called at the end of all
pyplot functions to trigger a figure redraw, but this is now handled by
the stale attribute.)

In order to simplify the backend API ("what is the API that a backend
module must/can provide"), I am planning to deprecate (on Matplotlib's
side) the ability for backends to provide a draw_if_interactive function
(forcing them to always do `if interactive(): draw_idle()`).  Instead,
any relevant new-figure-customization logic can instead go into
`new_figure_manager_given_figure`.  This PR implements this change for
matplotlib-inline, and should be fully back-compatible all the way back
to Matplotlib 1.5.  I would like to make these changes first on the side
of the clients (i.e., the third-party backends), to catch any possible
problems with the intended change on Matplotlib's side.
@anntzer
Copy link
Contributor Author

anntzer commented Jun 10, 2022

I still don't see the comments, but I assume you meant that new_figure_manager{,_given_figure} needed docstrings; I have now added them.

@fperez
Copy link
Member

fperez commented Jun 15, 2022

Yup, that was it @anntzer, thx! BTW, that's weird - this is what I meant by my comments, I see them right in this page above:

image

Do you not see the above? Odd.

@fperez
Copy link
Member

fperez commented Jun 15, 2022

I'll give this a day or two in case anyone else can comments, and if not we should merge it (if by chance I forget, @anntzer please don't be shy to ping me - thx for the contribution!!)

@anntzer
Copy link
Contributor Author

anntzer commented Jun 15, 2022

The docstrings have been added now (your previous comments did not show up for me because they are still marked as pending on your side).

@fperez
Copy link
Member

fperez commented Jun 15, 2022

Ah, thanks for that note, @anntzer! I didn't realize that the "pending" label on my comments meant they hadn't been pushed for view of others.

But yes, I'd seen your docstring updates, thx for those.

I think this extra time is enough for a minor change like this, merging now (once tests complete, just realized I had to approve the workflow to run).

@fperez fperez self-requested a review June 15, 2022 18:14
@fperez fperez merged commit f764b43 into ipython:master Jun 15, 2022
@anntzer anntzer deleted the udif branch June 16, 2022 22:12
# should be a no-op (otherwise we'll generate duplicate plots, since a user
# who set ioff() manually expects to make separate draw/show calls).
if not matplotlib.is_interactive():
return
Copy link
Member

Choose a reason for hiding this comment

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

This should have been return manager, I missed that on review, thus introducing #18. Sorry about that, merging now #19 to correct.

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

Successfully merging this pull request may close these issues.

2 participants