Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

fix FigureManagerTk close behavior if embedded in Tk App#17802

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

richardsheridan
Copy link
Contributor

@richardsheridanrichardsheridan commentedJun 30, 2020
edited
Loading

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 thewindow.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 myis_tk_mainloop_running function should be provided as a method on_tkinter.tkapp AKATkappObject to access thedispatching 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

  • Has Pytest style unit tests
  • Code isFlake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

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():
Copy link
Contributor

Choose a reason for hiding this comment

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

how does this compare withhttps://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/cbook/__init__.py#L65 (possibly combined with checking that the thread matchesthreading.main_thread())?

Copy link
ContributorAuthor

@richardsheridanrichardsheridanJun 30, 2020
edited
Loading

Choose a reason for hiding this comment

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

In the contexts that I callis_tk_mainloop_running,_get_running_interactive_framework returnsNone, 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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

with revised _get_running_interactive_framework
@tacaswell
Copy link
Member

Thanks for working on this@richardsheridan !

I am not sure that this will work, consider the sequence of:

  • user creates a window
  • callsplt.show(block=True) which starts the tk event loop
  • a callback triggered from inside the first figure creates a second
  • the user closes the first figure (which does not try to quit the tk event loop because there are still other of our figures open)
  • the user then closes the second figure (which does not quit the event loop because it detected the loop was running when it was created)
  • the user now has a hung python session

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 inmainloop orshow 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 thatwe 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....

@tacaswelltacaswell added this to thev3.4.0 milestoneJul 1, 2020
@richardsheridan
Copy link
ContributorAuthor

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 thenshow(block=True) and close a figure:

import tkinterimport matplotlib.pyplot as pltroot= tkinter.Tk()plt.plot([1,2,3],[1,2,5])plt.show(block=True)

However, it is not necessary to callplt.show(block=True) from the REPL due to theEventHook functionality of tkinter and the behavior ofplt.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 armingquit uponmainloop on my next pass at this PR.

@tacaswell
Copy link
Member

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().

While true, we definitely have users that want to work in a terminal and do not useplt.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).

@richardsheridan
Copy link
ContributorAuthor

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 toEventHook. This can be fixed with an additionaldestroy in thelegitimate_quit but I'm not sure how to automatically test REPL behavior??

This does not fix theplt.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.

@richardsheridanrichardsheridan marked this pull request as ready for reviewJuly 1, 2020 18:33
@tacaswell
Copy link
Member

@richardsheridan I took the liberty of fixing the style issues in the test, I hope you don't mind.

@tacaswell
Copy link
Member

This can be fixed with an additional destroy in the legitimate_quit but I'm not sure how to automatically test REPL behavior??

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.

This does not fix the plt.show(block=True) hang but IMO that is a separate issue.

I'm not sure how to get the hang any more though, the following:

importmatplotlib.pyplotaspltplt.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 callplt.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.

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.
@richardsheridan
Copy link
ContributorAuthor

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 correctsFigureManager destruction behavior to fix that.

This appears to also fix theplt.show(block=True) hang in REPL issue.

@tacaswell
Copy link
Member

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?

@richardsheridan
Copy link
ContributorAuthor

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...

@richardsheridan
Copy link
ContributorAuthor

How were you getting the repl to hang?

See my commentabove, run those on the REPL and simply close the figure window but not the Tk root window. On3860b21 this hangs but onbea3a3f you can enter commands again.

if self.canvas._idle_callback:
self.canvas._tkcanvas.after_cancel(self.canvas._idle_callback)

self.window.destroy()
Copy link
Member

Choose a reason for hiding this comment

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

We used to drop the window, presumably to avoid callingdestroy() on it twice (?). Is that a concern we should have?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Calling destroy twice was an artifact of mistakenly binding instead of using protocol. It should be fine now.

Copy link
Member

@tacaswelltacaswell left a comment

Choose a reason for hiding this comment

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

One small question, but still approve!

@tacaswell
Copy link
Member

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.

skip running mainloop if we already own onedo cheaper test first
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
@richardsheridanrichardsheridan mentioned this pull requestAug 6, 2020
6 tasks
@QuLogicQuLogic merged commit97fb361 intomatplotlib:masterAug 7, 2020
@QuLogic
Copy link
Member

Thanks@richardsheridan! Congratulations on your first PR to Matplotlib 🎉 We hope to hear from you again.

richardsheridan reacted with hooray emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@anntzeranntzeranntzer left review comments

@tacaswelltacaswelltacaswell approved these changes

@QuLogicQuLogicQuLogic approved these changes

Assignees
No one assigned
Labels
Projects
None yet
Milestone
v3.4.0
Development

Successfully merging this pull request may close these issues.

Matplotlib.pyplot.close() interacts with tk.Tk() and closes the tk.Tk() object.
4 participants
@richardsheridan@tacaswell@QuLogic@anntzer

[8]ページ先頭

©2009-2025 Movatter.jp