From 699c3c0af3e1a8d35d7e7b398bb739953257268e Mon Sep 17 00:00:00 2001 From: Richard Sheridan Date: Mon, 29 Jun 2020 20:23:28 -0400 Subject: [PATCH 01/12] fix FigureManagerTk close behavior if embedded in Tk App Check if the Tk mainloop was dispatching (with an ugly thread hack) and don't quit it if that was the case. --- lib/matplotlib/backends/_backend_tk.py | 43 +++++++++++++++++++++++--- 1 file changed, 39 insertions(+), 4 deletions(-) diff --git a/lib/matplotlib/backends/_backend_tk.py b/lib/matplotlib/backends/_backend_tk.py index 7c1ff265aea3..b76aeaf54130 100644 --- a/lib/matplotlib/backends/_backend_tk.py +++ b/lib/matplotlib/backends/_backend_tk.py @@ -398,7 +398,7 @@ class FigureManagerTk(FigureManagerBase): The tk.Window """ - def __init__(self, canvas, num, window): + def __init__(self, canvas, num, window, mainloop_running): FigureManagerBase.__init__(self, canvas, num) self.window = window self.window.withdraw() @@ -414,6 +414,7 @@ def __init__(self, canvas, num, window): backend_tools.add_tools_to_container(self.toolbar) self._shown = False + self._tk_mainloop_already_started_before_creation = mainloop_running def _get_toolbar(self): if mpl.rcParams['toolbar'] == 'toolbar2': @@ -460,7 +461,7 @@ def destroy(self, *args): self.canvas._tkcanvas.after_cancel(self.canvas._idle_callback) self.window.destroy() if Gcf.get_num_fig_managers() == 0: - if self.window is not None: + if self.window is not None and not self._tk_mainloop_already_started_before_creation: self.window.quit() self.window = None @@ -869,7 +870,7 @@ def new_figure_manager_given_figure(cls, num, figure): _log.info('Could not load matplotlib icon: %s', exc) canvas = cls.FigureCanvas(figure, master=window) - manager = cls.FigureManager(canvas, num, window) + manager = cls.FigureManager(canvas, num, window, is_tk_mainloop_running()) if mpl.is_interactive(): manager.show() canvas.draw_idle() @@ -882,5 +883,39 @@ def trigger_manager_draw(manager): @staticmethod def mainloop(): managers = Gcf.get_all_fig_managers() - if managers: + if not managers: + return + if not is_tk_mainloop_running(): managers[0].window.mainloop() + + +def is_tk_mainloop_running(): + import threading + dispatching = sentinel = object() + + def target(): + nonlocal dispatching + try: + tk._default_root.call('while', '0', '{}') + except RuntimeError as e: + if str(e) == "main thread is not in main loop": + print('not dispatching') + dispatching = False + else: + raise + except BaseException as e: + print(e) + raise + else: + dispatching = True + print('dispatching') + + t = threading.Thread(target=target, daemon=True) + t.start() + tk._default_root.update() + t.join() + + if dispatching is sentinel: + raise RuntimeError('thread failed to determine dispatching value') + + return dispatching From 03dc5f8fbe224bc3b006745db6f33b2016a5f75a Mon Sep 17 00:00:00 2001 From: Richard Sheridan Date: Tue, 30 Jun 2020 08:50:38 -0400 Subject: [PATCH 02/12] use frame inspection instead with revised _get_running_interactive_framework --- lib/matplotlib/backends/_backend_tk.py | 39 +++----------------------- lib/matplotlib/cbook/__init__.py | 3 +- 2 files changed, 6 insertions(+), 36 deletions(-) diff --git a/lib/matplotlib/backends/_backend_tk.py b/lib/matplotlib/backends/_backend_tk.py index b76aeaf54130..8303942b850a 100644 --- a/lib/matplotlib/backends/_backend_tk.py +++ b/lib/matplotlib/backends/_backend_tk.py @@ -398,7 +398,7 @@ class FigureManagerTk(FigureManagerBase): The tk.Window """ - def __init__(self, canvas, num, window, mainloop_running): + def __init__(self, canvas, num, window): FigureManagerBase.__init__(self, canvas, num) self.window = window self.window.withdraw() @@ -414,7 +414,7 @@ def __init__(self, canvas, num, window, mainloop_running): backend_tools.add_tools_to_container(self.toolbar) self._shown = False - self._tk_mainloop_already_started_before_creation = mainloop_running + self._tk_mainloop_already_started_before_creation = 'tk' == cbook._get_running_interactive_framework() def _get_toolbar(self): if mpl.rcParams['toolbar'] == 'toolbar2': @@ -870,7 +870,7 @@ def new_figure_manager_given_figure(cls, num, figure): _log.info('Could not load matplotlib icon: %s', exc) canvas = cls.FigureCanvas(figure, master=window) - manager = cls.FigureManager(canvas, num, window, is_tk_mainloop_running()) + manager = cls.FigureManager(canvas, num, window) if mpl.is_interactive(): manager.show() canvas.draw_idle() @@ -885,37 +885,6 @@ def mainloop(): managers = Gcf.get_all_fig_managers() if not managers: return - if not is_tk_mainloop_running(): + if not 'tk' == cbook._get_running_interactive_framework(): managers[0].window.mainloop() - -def is_tk_mainloop_running(): - import threading - dispatching = sentinel = object() - - def target(): - nonlocal dispatching - try: - tk._default_root.call('while', '0', '{}') - except RuntimeError as e: - if str(e) == "main thread is not in main loop": - print('not dispatching') - dispatching = False - else: - raise - except BaseException as e: - print(e) - raise - else: - dispatching = True - print('dispatching') - - t = threading.Thread(target=target, daemon=True) - t.start() - tk._default_root.update() - t.join() - - if dispatching is sentinel: - raise RuntimeError('thread failed to determine dispatching value') - - return dispatching diff --git a/lib/matplotlib/cbook/__init__.py b/lib/matplotlib/cbook/__init__.py index 53b21f45acdb..e79e32d0dfc6 100644 --- a/lib/matplotlib/cbook/__init__.py +++ b/lib/matplotlib/cbook/__init__.py @@ -64,9 +64,10 @@ def _get_running_interactive_framework(): return "wx" tkinter = sys.modules.get("tkinter") if tkinter: + codes = {tkinter.mainloop.__code__, tkinter.Misc.mainloop.__code__} for frame in sys._current_frames().values(): while frame: - if frame.f_code == tkinter.mainloop.__code__: + if frame.f_code in codes: return "tk" frame = frame.f_back if 'matplotlib.backends._macosx' in sys.modules: From d7e67993ce4a9820120a8985c9896bd5b13ad2e5 Mon Sep 17 00:00:00 2001 From: Richard Sheridan Date: Wed, 1 Jul 2020 13:59:58 -0400 Subject: [PATCH 03/12] Track ownership of mainloop with class attribute Part I: RED --- lib/matplotlib/backends/_backend_tk.py | 16 ++++++++-------- lib/matplotlib/tests/test_backend_tk.py | 23 +++++++++++++++++++++++ 2 files changed, 31 insertions(+), 8 deletions(-) diff --git a/lib/matplotlib/backends/_backend_tk.py b/lib/matplotlib/backends/_backend_tk.py index 8303942b850a..e00fca4d14d5 100644 --- a/lib/matplotlib/backends/_backend_tk.py +++ b/lib/matplotlib/backends/_backend_tk.py @@ -398,6 +398,8 @@ class FigureManagerTk(FigureManagerBase): The tk.Window """ + #_owns_mainloop = False + def __init__(self, canvas, num, window): FigureManagerBase.__init__(self, canvas, num) self.window = window @@ -414,7 +416,6 @@ def __init__(self, canvas, num, window): backend_tools.add_tools_to_container(self.toolbar) self._shown = False - self._tk_mainloop_already_started_before_creation = 'tk' == cbook._get_running_interactive_framework() def _get_toolbar(self): if mpl.rcParams['toolbar'] == 'toolbar2': @@ -461,7 +462,7 @@ def destroy(self, *args): self.canvas._tkcanvas.after_cancel(self.canvas._idle_callback) self.window.destroy() if Gcf.get_num_fig_managers() == 0: - if self.window is not None and not self._tk_mainloop_already_started_before_creation: + if self.window is not None:# and self._owns_mainloop: self.window.quit() self.window = None @@ -880,11 +881,10 @@ def new_figure_manager_given_figure(cls, num, figure): def trigger_manager_draw(manager): manager.show() - @staticmethod - def mainloop(): + @classmethod + def mainloop(cls): managers = Gcf.get_all_fig_managers() - if not managers: - return - if not 'tk' == cbook._get_running_interactive_framework(): + if managers: + #cls._owns_mainloop = True managers[0].window.mainloop() - + #cls._owns_mainloop = False diff --git a/lib/matplotlib/tests/test_backend_tk.py b/lib/matplotlib/tests/test_backend_tk.py index 18ffcb40a0b1..d9d950408b4f 100644 --- a/lib/matplotlib/tests/test_backend_tk.py +++ b/lib/matplotlib/tests/test_backend_tk.py @@ -1,3 +1,5 @@ +import tkinter + import pytest import numpy as np from matplotlib import pyplot as plt @@ -26,3 +28,24 @@ def evil_blit(photoimage, aggimage, offsets, bboxptr): np.ones((4, 4, 4)), (0, 1, 2, 3), bad_boxes) + + +@pytest.mark.backend('TkAgg', skip_on_importerror=True) +def test_figuremanager_preserves_host_mainloop(): + success = False + def do_plot(): + plt.figure() + plt.plot([1,2],[3,5]) + plt.close() + root.after(0, legitmate_quit) + + def legitmate_quit(): + root.quit() + nonlocal success + success = True + + root = tkinter.Tk() + root.after(0, do_plot) + root.mainloop() + + assert success From 0bd9367d5d0f8029946606004bc9d80984f37ae4 Mon Sep 17 00:00:00 2001 From: Richard Sheridan Date: Wed, 1 Jul 2020 14:00:49 -0400 Subject: [PATCH 04/12] Track ownership of mainloop with class attribute Part II: GREEN --- lib/matplotlib/backends/_backend_tk.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/matplotlib/backends/_backend_tk.py b/lib/matplotlib/backends/_backend_tk.py index e00fca4d14d5..48d7556a57ea 100644 --- a/lib/matplotlib/backends/_backend_tk.py +++ b/lib/matplotlib/backends/_backend_tk.py @@ -398,7 +398,7 @@ class FigureManagerTk(FigureManagerBase): The tk.Window """ - #_owns_mainloop = False + _owns_mainloop = False def __init__(self, canvas, num, window): FigureManagerBase.__init__(self, canvas, num) @@ -462,7 +462,7 @@ def destroy(self, *args): self.canvas._tkcanvas.after_cancel(self.canvas._idle_callback) self.window.destroy() if Gcf.get_num_fig_managers() == 0: - if self.window is not None:# and self._owns_mainloop: + if self.window is not None and self._owns_mainloop: self.window.quit() self.window = None @@ -885,6 +885,6 @@ def trigger_manager_draw(manager): def mainloop(cls): managers = Gcf.get_all_fig_managers() if managers: - #cls._owns_mainloop = True + cls._owns_mainloop = True managers[0].window.mainloop() - #cls._owns_mainloop = False + cls._owns_mainloop = False From ebd24ec0d19f1805749a25535039dba661a92e48 Mon Sep 17 00:00:00 2001 From: Thomas A Caswell Date: Wed, 1 Jul 2020 15:04:19 -0400 Subject: [PATCH 05/12] STY: satisfy pep8 --- lib/matplotlib/tests/test_backend_tk.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/matplotlib/tests/test_backend_tk.py b/lib/matplotlib/tests/test_backend_tk.py index d9d950408b4f..e0dc88f66bfc 100644 --- a/lib/matplotlib/tests/test_backend_tk.py +++ b/lib/matplotlib/tests/test_backend_tk.py @@ -35,7 +35,7 @@ def test_figuremanager_preserves_host_mainloop(): success = False def do_plot(): plt.figure() - plt.plot([1,2],[3,5]) + plt.plot([1, 2], [3, 5]) plt.close() root.after(0, legitmate_quit) From ae96b4f4aa474f77c179a940fc1237cf479ae854 Mon Sep 17 00:00:00 2001 From: Richard Sheridan Date: Wed, 1 Jul 2020 15:26:20 -0400 Subject: [PATCH 06/12] fix class reference --- lib/matplotlib/backends/_backend_tk.py | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/lib/matplotlib/backends/_backend_tk.py b/lib/matplotlib/backends/_backend_tk.py index 48d7556a57ea..22f8e9f1486f 100644 --- a/lib/matplotlib/backends/_backend_tk.py +++ b/lib/matplotlib/backends/_backend_tk.py @@ -881,10 +881,12 @@ def new_figure_manager_given_figure(cls, num, figure): def trigger_manager_draw(manager): manager.show() - @classmethod - def mainloop(cls): + @staticmethod + def mainloop(): managers = Gcf.get_all_fig_managers() if managers: - cls._owns_mainloop = True - managers[0].window.mainloop() - cls._owns_mainloop = False + first_manager = managers[0] + manager_class = type(first_manager) + manager_class._owns_mainloop = True + first_manager.window.mainloop() + manager_class._owns_mainloop = False From 3860b21d58b74065c3e2d3d4876497220693bc5b Mon Sep 17 00:00:00 2001 From: Richard Sheridan Date: Wed, 1 Jul 2020 15:26:37 -0400 Subject: [PATCH 07/12] create test_figuremanager_cleans_own_mainloop --- lib/matplotlib/tests/test_backend_tk.py | 33 +++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/lib/matplotlib/tests/test_backend_tk.py b/lib/matplotlib/tests/test_backend_tk.py index d9d950408b4f..e982b83e006d 100644 --- a/lib/matplotlib/tests/test_backend_tk.py +++ b/lib/matplotlib/tests/test_backend_tk.py @@ -1,7 +1,10 @@ +import threading +import time import tkinter -import pytest import numpy as np +import pytest + from matplotlib import pyplot as plt @@ -33,9 +36,10 @@ def evil_blit(photoimage, aggimage, offsets, bboxptr): @pytest.mark.backend('TkAgg', skip_on_importerror=True) def test_figuremanager_preserves_host_mainloop(): success = False + def do_plot(): plt.figure() - plt.plot([1,2],[3,5]) + plt.plot([1, 2], [3, 5]) plt.close() root.after(0, legitmate_quit) @@ -49,3 +53,28 @@ def legitmate_quit(): root.mainloop() assert success + + +@pytest.mark.backend('TkAgg', skip_on_importerror=True) +def test_figuremanager_cleans_own_mainloop(): + root = tkinter.Tk() + plt.plot([1, 2, 3], [1, 2, 5]) + can_detect_mainloop = False + thread_died_before_quit = True + + def target(): + nonlocal can_detect_mainloop + nonlocal thread_died_before_quit + from matplotlib.cbook import _get_running_interactive_framework + + time.sleep(0.1) # should poll for mainloop being up + can_detect_mainloop = 'tk' == _get_running_interactive_framework() + plt.close() + time.sleep(0.1) # should poll for mainloop going down + root.quit() + thread_died_before_quit = False + + threading.Thread(target=target, daemon=True).start() + plt.show(block=True) + assert can_detect_mainloop + assert thread_died_before_quit From bea3a3f89b075307fdb14a10cf509de039fe1d14 Mon Sep 17 00:00:00 2001 From: Richard Sheridan Date: Wed, 1 Jul 2020 16:04:47 -0400 Subject: [PATCH 08/12] rewrite of FigureManagerTk destroy logic Likely due to misusing bind("",...) rather than using protocol("WM_DELETE_WINDOW",...), there was a lot of unnecessary logic for dealing with recursive entry into this method. --- lib/matplotlib/backends/_backend_tk.py | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/lib/matplotlib/backends/_backend_tk.py b/lib/matplotlib/backends/_backend_tk.py index 22f8e9f1486f..eea06ab18296 100644 --- a/lib/matplotlib/backends/_backend_tk.py +++ b/lib/matplotlib/backends/_backend_tk.py @@ -444,9 +444,8 @@ def show(self): with _restore_foreground_window_at_end(): if not self._shown: def destroy(*args): - self.window = None Gcf.destroy(self) - self.canvas._tkcanvas.bind("", destroy) + self.window.protocol("WM_DELETE_WINDOW", destroy) self.window.deiconify() else: self.canvas.draw_idle() @@ -456,15 +455,13 @@ def destroy(*args): self._shown = True def destroy(self, *args): - if self.window is not None: - #self.toolbar.destroy() - if self.canvas._idle_callback: - self.canvas._tkcanvas.after_cancel(self.canvas._idle_callback) - self.window.destroy() - if Gcf.get_num_fig_managers() == 0: - if self.window is not None and self._owns_mainloop: - self.window.quit() - self.window = None + if self.canvas._idle_callback: + self.canvas._tkcanvas.after_cancel(self.canvas._idle_callback) + + self.window.destroy() + + if not Gcf.get_num_fig_managers() and self._owns_mainloop: + self.window.quit() def get_window_title(self): return self.window.wm_title() From c578d8c4ba1da0e5b8c336f796a95c34f9649183 Mon Sep 17 00:00:00 2001 From: Richard Sheridan Date: Wed, 1 Jul 2020 19:29:18 -0400 Subject: [PATCH 09/12] Address timeout issues directly --- lib/matplotlib/tests/test_backend_tk.py | 66 ++++++++++++++++--------- 1 file changed, 43 insertions(+), 23 deletions(-) diff --git a/lib/matplotlib/tests/test_backend_tk.py b/lib/matplotlib/tests/test_backend_tk.py index e982b83e006d..31a87e866781 100644 --- a/lib/matplotlib/tests/test_backend_tk.py +++ b/lib/matplotlib/tests/test_backend_tk.py @@ -1,5 +1,6 @@ -import threading -import time +import os +import subprocess +import sys import tkinter import numpy as np @@ -56,25 +57,44 @@ def legitmate_quit(): @pytest.mark.backend('TkAgg', skip_on_importerror=True) +@pytest.mark.flaky(reruns=3) def test_figuremanager_cleans_own_mainloop(): - root = tkinter.Tk() - plt.plot([1, 2, 3], [1, 2, 5]) - can_detect_mainloop = False - thread_died_before_quit = True - - def target(): - nonlocal can_detect_mainloop - nonlocal thread_died_before_quit - from matplotlib.cbook import _get_running_interactive_framework - - time.sleep(0.1) # should poll for mainloop being up - can_detect_mainloop = 'tk' == _get_running_interactive_framework() - plt.close() - time.sleep(0.1) # should poll for mainloop going down - root.quit() - thread_died_before_quit = False - - threading.Thread(target=target, daemon=True).start() - plt.show(block=True) - assert can_detect_mainloop - assert thread_died_before_quit + script = ''' +import tkinter +import time +import matplotlib.pyplot as plt +import threading +from matplotlib.cbook import _get_running_interactive_framework + +root = tkinter.Tk() +plt.plot([1, 2, 3], [1, 2, 5]) + +def target(): + while not 'tk' == _get_running_interactive_framework(): + time.sleep(.01) + plt.close() + if show_finished_event.wait(): + print('success') + +show_finished_event = threading.Event() +thread = threading.Thread(target=target, daemon=True) +thread.start() +plt.show(block=True) # testing if this function hangs +show_finished_event.set() +thread.join() + +''' + try: + proc = subprocess.run( + [sys.executable, "-c", script], + env={**os.environ, "MPLBACKEND": "TkAgg", "SOURCE_DATE_EPOCH": "0"}, + timeout=10, + stdout=subprocess.PIPE, + universal_newlines=True, + check=True + ) + except subprocess.TimeoutExpired: + pytest.fail("Most likely plot.show(block=True) hung") + except subprocess.CalledProcessError: + pytest.fail("Subprocess failed to test intended behavior") + assert proc.stdout.count("success") == 1 From e7af5c9c68a385a04551df702eb5066a74ab2171 Mon Sep 17 00:00:00 2001 From: Richard Sheridan Date: Wed, 1 Jul 2020 19:33:06 -0400 Subject: [PATCH 10/12] flake8 --- lib/matplotlib/tests/test_backend_tk.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/matplotlib/tests/test_backend_tk.py b/lib/matplotlib/tests/test_backend_tk.py index 31a87e866781..7803e23b9ced 100644 --- a/lib/matplotlib/tests/test_backend_tk.py +++ b/lib/matplotlib/tests/test_backend_tk.py @@ -75,7 +75,7 @@ def target(): plt.close() if show_finished_event.wait(): print('success') - + show_finished_event = threading.Event() thread = threading.Thread(target=target, daemon=True) thread.start() @@ -87,7 +87,9 @@ def target(): try: proc = subprocess.run( [sys.executable, "-c", script], - env={**os.environ, "MPLBACKEND": "TkAgg", "SOURCE_DATE_EPOCH": "0"}, + env={**os.environ, + "MPLBACKEND": "TkAgg", + "SOURCE_DATE_EPOCH": "0"}, timeout=10, stdout=subprocess.PIPE, universal_newlines=True, From 118876e867f416c6d22752c3955c9427cefbc46c Mon Sep 17 00:00:00 2001 From: Richard Sheridan Date: Thu, 2 Jul 2020 08:12:35 -0400 Subject: [PATCH 11/12] Use finally to reset mainloop ownership skip running mainloop if we already own one do cheaper test first --- lib/matplotlib/backends/_backend_tk.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/lib/matplotlib/backends/_backend_tk.py b/lib/matplotlib/backends/_backend_tk.py index eea06ab18296..746ac2d59e7e 100644 --- a/lib/matplotlib/backends/_backend_tk.py +++ b/lib/matplotlib/backends/_backend_tk.py @@ -460,7 +460,7 @@ def destroy(self, *args): self.window.destroy() - if not Gcf.get_num_fig_managers() and self._owns_mainloop: + if self._owns_mainloop and not Gcf.get_num_fig_managers(): self.window.quit() def get_window_title(self): @@ -884,6 +884,10 @@ def mainloop(): if managers: first_manager = managers[0] manager_class = type(first_manager) + if manager_class._owns_mainloop: + return manager_class._owns_mainloop = True - first_manager.window.mainloop() - manager_class._owns_mainloop = False + try: + first_manager.window.mainloop() + finally: + manager_class._owns_mainloop = False From c64c6a7c0453f8db36c20e6471c73be69056cd3f Mon Sep 17 00:00:00 2001 From: richardsheridan Date: Thu, 6 Aug 2020 07:09:59 -0400 Subject: [PATCH 12/12] spelling correction Co-authored-by: Elliott Sales de Andrade --- lib/matplotlib/tests/test_backend_tk.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/matplotlib/tests/test_backend_tk.py b/lib/matplotlib/tests/test_backend_tk.py index 7803e23b9ced..b56b25a717fb 100644 --- a/lib/matplotlib/tests/test_backend_tk.py +++ b/lib/matplotlib/tests/test_backend_tk.py @@ -42,9 +42,9 @@ def do_plot(): plt.figure() plt.plot([1, 2], [3, 5]) plt.close() - root.after(0, legitmate_quit) + root.after(0, legitimate_quit) - def legitmate_quit(): + def legitimate_quit(): root.quit() nonlocal success success = True