There are many ways for things to end up in odd states, the goal here is to detect if the the canvas has changed under us when trying to manage the state of widgets.

PR Checklist

Documentation and Tests

Has pytest style unit tests (andpytest passes)
[N/A] Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).
[N/A] New plotting related features are documented with examples.

Release Notes

[N/A] New features are marked with a.. versionadded:: directive in the docstring and documented indoc/users/next_whats_new/
[N/A] API changes are marked with a.. versionchanged:: directive in the docstring and documented indoc/api/next_api_changes/
[N/A] Release notes conform with instructions innext_whats_new/README.rst ornext_api_changes/README.rst

tacaswell added this to thev3.7.2 milestone

Mar 29, 2023

timhoffm approved these changes

Mar 29, 2023

View reviewed changes

Copy link

Contributor

anntzer commentedMar 29, 2023

Perhaps the simpler check would beself.canvas is self.canvas.figure.canvas? Note that while figure.canvas can change (hence the introduction of _changed_canvas), I think canvas.figure does not ever change (at least in practice... it may even be possible to spec that), so the proposed check is effectively "does the figure I initially got still use the same canvas as initially?" Also none of these attributes should ever be None, AFAICT.

Alternatively, we could additionally recordself._figure = ax.figure in the constructor and checkself._figure.canvas is self.canvas; again this should sidestep the None problems?

Copy link

MemberAuthor

tacaswell commentedMar 31, 2023

Alternatively, we could additionally record self._figure = ax.figure in the constructor and check self._figure.canvas is self.canvas; again this should sidestep the None problems?

I think that would still miss the "middle removed" issue as we have references that skip generations and we should avoid adding more hard-references than we strictly need....

But I will change toself.canvas is self.canvas.figure.canvas

FIX: Use more reliable way of checking the current figure's canvas

7100b57

closesmatplotlib#25572

tacaswell force-pushed thefix_removed_widget_axes branch from54db976 to7100b57Compare

March 31, 2023 20:08

anntzer reviewed

Mar 31, 2023

View reviewed changes

lib/matplotlib/widgets.py Outdated

		# no canvas, something is very fishy
		if self.canvas is None:
		return True
		return self.canvas is not self.canvas.figure.canvas

Copy link

Contributor

anntzerMar 31, 2023

Choose a reason for hiding this comment

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

Can be made one-liner asreturn self.canvas is not None and self.canvas is not self.canvas.figure.canvas.

anntzer reviewed

Mar 31, 2023

View reviewed changes

Copy link

Contributor

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 CI, and with an optional additional comment.

anntzer approved these changes

Mar 31, 2023

View reviewed changes

MNT: simplify check if canvas has changed

a9f4c6c

Co-authored-by: Antony Lee <anntzer.lee@gmail.com>

Copy link

MemberAuthor

tacaswell commentedApr 3, 2023

Hmm, we are back to not being careful enough...but only an azure?!

Copy link

Contributor

anntzer commentedApr 3, 2023

That's a bit fishy, but perhaps just change all theself.ax.draw_artist(artist) byartist.draw(self.canvas.get_renderer())? (which should bypass the problem of whetherself.ax.figure is None or not -- anyways, having draw_artist be an Axes method always seemed a bit weird to me (I do realize that historically we cached the renderer on the axes, but that's not really the logical place to have it)).