| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
Check if the Tk mainloop was dispatching (with an ugly thread hack) and don't quit it if that was the case.
| managers[0].window.mainloop() | ||
|
|
||
|
|
||
| def is_tk_mainloop_running(): |
There was a problem hiding this comment.
how does this compare with https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/cbook/__init__.py#L65 (possibly combined with checking that the thread matches threading.main_thread())?
Sorry, something went wrong.
There was a problem hiding this comment.
In the contexts that I call is_tk_mainloop_running, _get_running_interactive_framework returns None, not "tk". This is because _get_running_interactive_framework is incorrectly implemented. There are several mainloop functions in various classes in tkinter and it only checks for one of them. Importantly one of them is a built-in method that has no __code__ attribute to check ("built-in method mainloop of _tkinter.tkapp object") so i'm not sure how to check for it. It would be a rare case to call that method directly, however, so maybe it would be ok to overlook?
Sorry, something went wrong.
There was a problem hiding this comment.
I see your point. Adding a check for Misc.mainloop as you did certainly seems better. I don't have a good solution for the general case, though.
Sorry, something went wrong.
with revised _get_running_interactive_framework
|
Thanks for working on this @richardsheridan ! I am not sure that this will work, consider the sequence of:
I think this is going the right direction, but instead of detecting on a per-figure-manager basis if it should quit the tk event loop we should do the check in mainloop or show and then "arm" the Figure managers with some global state (probably stash it on FigureManagerTk as a class level attribute?) that they should quit the tkmain loop when the last of them exits (and then flip the state back to "not armed") thus we will only try to stop the tk event loop if we are sure that we started it. That is more global state (which I'm not wild about), but GUI frameworks seem to be an endless source of global state so one more bit won't do any harm.... |
Sorry, something went wrong.
|
For what it's worth, I'm not sure your sequence demonstrates an actual issue. Yes, the REPL hangs if you open a tkinter window and then show(block=True) and close a figure: import tkinter import matplotlib.pyplot as plt root= tkinter.Tk() plt.plot([1,2,3],[1,2,5]) plt.show(block=True) However, it is not necessary to call plt.show(block=True) from the REPL due to the EventHook functionality of tkinter and the behavior of plt.ion(). The expected behavior occurs when run as a script (you have to press the close button of all the windows to exit the script) I'll try arming quit upon mainloop on my next pass at this PR. |
Sorry, something went wrong.
While true, we definitely have users that want to work in a terminal and do not use plt.ion(). That aside, the same scenario in a script is a much better story (user wants to stop the script, open a plot (that spawns more figures) and if the windows get closed in the wrong order the script hangs forever). |
Sorry, something went wrong.
|
This idea came out very elegantly, as long as there are no unexpected wider behavior changes. I added a test that is more minimal than the MVE shown in #13470. For the record, this test also passes when performed on the REPL as well, except the root window hangs around due to EventHook. This can be fixed with an additional destroy in the legitimate_quit but I'm not sure how to automatically test REPL behavior?? This does not fix the plt.show(block=True) hang but IMO that is a separate issue. Should we make it literally a separate issue on github or fully fix it on this PR? The fix to _get_running_interactive_framework remains even though now totally unrelated. |
Sorry, something went wrong.
|
@richardsheridan I took the liberty of fixing the style issues in the test, I hope you don't mind. |
Sorry, something went wrong.
While testing is good, there is a lot of code in the GUI / interactive / repl space that we do not have tests on because the trade off of the effort to write the test vs the benefit is not worth it. I think that this is in that category.
I'm not sure how to get the hang any more though, the following: import matplotlib.pyplot as plt
plt.ion()
fig, ax = plt.subplots()
fig.canvas.mpl_connect('button_press_event', lambda *args, **kwargs: plt.figure())
plt.show(block=True)sets it up so if you click anywhere on the first figure it spawns more. You can close them in any order and it seems to work (which is the case I was worried about). If you call plt.show(block=True) from of a running tk mainloop than you are trying to start a tk mainloop inside of a tk mainloop which I am not convinced has well defined behavior (I know Qt has the ability to run a second event loop inside of the first, but that is not what we are doing here?) If that is the only problem we have left, I am happy to defer it to a later issue. |
Sorry, something went wrong.
Likely due to misusing bind("<Destroy>",...) rather than using protocol("WM_DELETE_WINDOW",...), there was a lot of unnecessary logic for dealing with recursive entry into this method.
…nto tk_backend_figuremanager_quit
|
I discovered two bugs in the PR, which I believe the additional commits address. Managers were not quitting loops that they had responsibility for. I cherry picked a commit from my other PR #17789 that corrects FigureManager destruction behavior to fix that. This appears to also fix the plt.show(block=True) hang in REPL issue. |
Sorry, something went wrong.
|
One thing we have found with CI is that you need to use rather extreme timeouts, things take longer than "reasonable" because they are running on very over-subscribed machines at low-priority (the service is free so we can not complain too much). How were you getting the repl to hang? |
Sorry, something went wrong.
|
It seems there is some kind of deeper race condition going on. Before the fix, there a spurious green zone around 0.1 seconds of waiting that will make the test pass and more or less than that everything hangs unless Tcl recieves some user events (i.e. mouseover the remaining window). I tried a dozen different things to iron this out and nothing was working. Now I finally understand why that test in test_backends_interactive.py was running in a subprocess... |
Sorry, something went wrong.
| if self.canvas._idle_callback: | ||
| self.canvas._tkcanvas.after_cancel(self.canvas._idle_callback) | ||
|
|
||
| self.window.destroy() |
There was a problem hiding this comment.
We used to drop the window, presumably to avoid calling destroy() on it twice (?). Is that a concern we should have?
Sorry, something went wrong.
There was a problem hiding this comment.
Calling destroy twice was an artifact of mistakenly binding instead of using protocol. It should be fine now.
Sorry, something went wrong.
There was a problem hiding this comment.
One small question, but still approve!
Sorry, something went wrong.
|
Ah, thanks! Sorry I was being dense. Thank you for working on this @richardsheridan ! I will try to take a look at your other PR tomorrow. |
Sorry, something went wrong.
skip running mainloop if we already own one do cheaper test first
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
|
Thanks @richardsheridan! Congratulations on your first PR to Matplotlib 🎉 We hope to hear from you again. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
PR Summary
Fixes #13470. I test to see if the Tk mainloop was running before FigureManagerTk was created and if that was the case, then I don't call the window.quit method.
The solution relies on implementation details of the tkinter c module and a disgusting hack using threads, so I have made this PR a draft. Really, the functionality of my is_tk_mainloop_running function should be provided as a method on _tkinter.tkapp AKA TkappObject to access the dispatching struct member from Python, but that's a job for cPython upstream.
I suppose there could be an alternative method involving traversing the children of the _default_root, or parents of the figure window but that feels just as much like leaning on implementation details and more error prone.
If there is any interest in maintaining the "fix" within matplotlib, I'll work on this PR further, otherwise this and #13470 should be closed pending upstream changes.
PR Checklist