New configure_subplots behaves badly on TkAgg backend

[richardsheridan]

Bug report

Bug summary

After #16818, with the TkAgg backend, configure subplots window can't be closed and the sliders do not respond to input. The window close issue is straightforward to understand: the button response calls Gcf.destroy but the figure is intentionally not registered, so the FigureManagerTk.destory is never called. I suspect (but have not confirmed) that the slider events are being lost due to the new figure manager making a new tkinter.Tk instance here. [EDIT: NOPE, see comment]

CC @anntzer

Code for reproduction

import matplotlib
import matplotlib.pyplot as plt

matplotlib.use("TkAgg")
plt.plot([1,2,3],[1,2,5])
#  if not interactive
plt.show()
# click Configure Subplots

Actual outcome

interactive Tk window but sliders don't work and close window button is not responsive

Expected outcome

previous behavior

Matplotlib version

• Operating system: Win10
• Matplotlib version: master
• Matplotlib backend (print(matplotlib.get_backend())): TkAgg
• Python version: 3.8.5
• Jupyter version (if applicable):
• Other libraries:

[richardsheridan]

Testing updates:

1. Modifying Gcf.destroy to allow figure managers that don't have a _cidgcf attribute and are not in the Gcf.figs dict is simple enough, however that risks regression of Fix RecursionError when closing nbAgg figures. #17025. It seems to me that figure managers should have the responsibility of destroying themselves, and then inform Gcf after the fact. This would possibly prevent some of the weird recursion bugs that keep cropping up? Does anyone know why this indirect strategy was chosen in the first place?
2. The slider callbacks weren't working because the FigureCanvasBase.callbacks CallbackRegistry dropped them. We have to keep a reference to the output of plt.subplot_tool to keep them alive. (This took way too long to find :( ) Historically, this reference was dangled off the new canvas but in using plt.subplot_tool that becomes very awkward.

Any suggestions on the design of (1) or the way to keep a reference for (2) would be appreciated, as these are design questions rather than hack-till-it-works issues.

[tacaswell]

For (2) that is an intentional design choice of how the callback system works motivated by preventing circular references.

For (1), I'm not sure why we have this inverted logic, it has been than way as long as I have been working on the project (8yrs), I suspect the original motivation may be lost to time.

[anntzer]

Thank you for the careful analysis, and apologies for the disruption.

For 1 it looks like the easy fix is

diff --git i/lib/matplotlib/_pylab_helpers.py w/lib/matplotlib/_pylab_helpers.py
index 26ba7a171..27904dd84 100644
--- i/lib/matplotlib/_pylab_helpers.py
+++ w/lib/matplotlib/_pylab_helpers.py
@@ -53,18 +53,17 @@ class Gcf:
         It is recommended to pass a manager instance, to avoid confusion when
         two managers share the same number.
         """
-        if all(hasattr(num, attr) for attr in ["num", "_cidgcf", "destroy"]):
+        if all(hasattr(num, attr) for attr in ["num", "destroy"]):
             manager = num
             if cls.figs.get(manager.num) is manager:
                 cls.figs.pop(manager.num)
-            else:
-                return
         else:
             try:
                 manager = cls.figs.pop(num)
             except KeyError:
                 return
-        manager.canvas.mpl_disconnect(manager._cidgcf)
+        if hasattr(manager, "_cidgcf"):
+            manager.canvas.mpl_disconnect(manager._cidgcf)
         manager.destroy()
         gc.collect(1)

i.e. when passed an instance, always destroy it regardless of whether it's pyplot-managed or not (but I agree that long-term we should just let the figures destroy themselves and notify Gcf).

For 2 it looks like restoring the attachment of the tool on the canvas is easy enough per

diff --git i/lib/matplotlib/pyplot.py w/lib/matplotlib/pyplot.py
index f6f9ff4c2..1befccebd 100644
--- i/lib/matplotlib/pyplot.py
+++ w/lib/matplotlib/pyplot.py
@@ -1561 +1561,3 @@ def subplot_tool(targetfig=None):
-    return SubplotTool(targetfig, tool_fig)
+    tool = SubplotTool(targetfig, tool_fig)
+    tool_fig.canvas.tool = tool
+    return tool

but I guess I'll take advantage of this to once again question whether weak references really make sense here; we are basically creating an artificial (strong) circular reference between the canvas and the tool (which, like any other cycle, needs the GC to be broken) to work around the fact that the more natural circular reference between the canvas and the callbacks doesn't work, because supposedly we don't want to rely on the GC to break these cycles... (The latter is more natural because they literally need to know each other to work.)