Skip to content

Tkagg fixes #9275

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Jan 10, 2018
Merged

Tkagg fixes #9275

merged 4 commits into from
Jan 10, 2018

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Oct 3, 2017

PR Summary

Fix tkagg animation error at exit.

Apparently, with a small enough animation interval, tk does not have the
time to fully proceed through FigureManagerTkAgg.show before the animation
takes over. Removing the call to self.window.update (which had been added
back in 2004 (6c96389), but seems unnecessary (including on Windows)) fixes
the issue (#8107).

Also update the animation example so that it can serve as manual test for
the issue; changed the rate at which the data moves to keep it reasonable,
and switched to None (= count()) as index iterable to avoid a jump every
time we reach the end of the indices.

Deprecations in tkagg.

  • Deprecate passing event to FigureManagerTkAgg.resize (in favor of passing
    width and height separately). This has been "deprecated in comment" for a
    while and is consistent with other backends.
  • Deprecate FigureCanvasTkAgg.show in favor of .draw. This provides the same
    method name as in the other backends.

Cleanup embedding_in_tk, remove embedding_in_tk2.

embedding_in_tk2_sgskip is essentially a slightly simplified duplicate of
embedding_in_tk_sgskip.

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 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

@anntzer anntzer added the GUI: tk label Oct 3, 2017
@tacaswell tacaswell added this to the 2.1.1 (next bug fix release) milestone Oct 3, 2017
@dopplershift
Copy link
Contributor

I'm pretty sure that comment from 2004 about anim.py actually predates the current animation module.

@tacaswell tacaswell modified the milestones: 2.1.1 (next bug fix release), 2.2 (next feature release) Oct 5, 2017
@anntzer anntzer mentioned this pull request Dec 3, 2017
Closed
@anntzer
Fix tkagg animation error at exit.
77d9500 
Apparently, with a small enough animation interval, tk does not have
the time to fully proceed through FigureManagerTkAgg.show before the
animation takes over.  Removing the call to self.window.update (which
had been added back in 2004 (6c96389), but seems unnecessary (including
on Windows)) fixes the issue.

Also update the animation example so that it can serve as manual test
for the issue; changed the rate at which the data moves to keep it
reasonable, and switched to None (= count()) as index iterable to avoid
a jump every time we reach the end of the indices.
@anntzer
Deprecate passing event to FigureManagerTkAgg.resize.
87fde92
@anntzer
Deprecate FigureCanvasTkAgg.show in favor of .draw.
f56e001 
This provides the same method name as in the other backends.
@tacaswell tacaswell mentioned this pull request Dec 5, 2017
Closed
@mattip mattip mentioned this pull request Dec 8, 2017
Merged
6 tasks
@efiring
Copy link
Member

efiring commented Dec 11, 2017

On OS X, the following

(test) efiring@manini2:~/work/programs/py/mpl/matplotlib/examples/animation$ ipython
Python 3.5.2 |Anaconda custom (x86_64)| (default, Jul  2 2016, 17:52:12)
Type 'copyright', 'credits' or 'license' for more information
IPython 6.2.1 -- An enhanced Interactive Python. Type '?' for help.

In [1]: import matplotlib

In [2]: matplotlib.use('tkagg')

In [3]: run simple_anim.py

hangs with a spinning ball when I close the window with the window manager button. Control-C ends it.
With qt5agg backend, the window closes on command.
It also closes on command with tkagg backend on master, so it looks like this PR might be fixing one problem and introducing another.

@anntzer
Copy link
Contributor Author

anntzer commented Dec 11, 2017

I see that this patch also adds an error message in the same context as given by @efiring but on Linux:

invalid command name "140521287480648_on_timer"
    while executing
"140521287480648_on_timer"
    ("after" script)

which is not solved by #9956 either.

We probably need to look into clearing the timers at destroy time too. Registering the Timers in a (weak)set and stopping them the the figuremanager's destroy hook doesn't seem to help.

tacaswell
tacaswell reviewed Dec 11, 2017
View reviewed changes
examples/user_interfaces/embedding_in_tk_sgskip.py Outdated
key_press_handler(event, canvas, toolbar)

canvas.mpl_connect('key_press_event', on_key_event)
canvas.mpl_connect(
Copy link
Member

Choose a reason for hiding this comment

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

some debugging code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Either way works but I guess the previous one looks a bit better. Reverting.

@anntzer
Cleanup embedding_in_tk, remove embedding_in_tk2.
2597f3d 
embedding_in_tk2_sgskip is essentially a slightly simplified duplicate
of embedding_in_tk_sgskip.
@anntzer
Copy link
Contributor Author

anntzer commented Dec 13, 2017

Actually, this is the situation on Linux:

  • Original: Keeping the call to update() and the slow interval on the example: no error from command line or ipython
  • Keep the call to update() and switch to fast timer: I get the tk-level error ("invalid command name etc.") and the python traceback (both command line and IPython)
  • Remove the call to update() and keep a slow timer: no error
  • This PR: Remove the call to update and switch to a fast timer: I get the tk-level error only from ipython, no error at the command line.

So from this point of view the PR is strictly better, even though it doesn't fix all issues.

Perhaps I can just switch back the example to use an slow timer if the failure mode is more severe on OSX than on Linux (but the same as before the removal of the call to update()).

@anntzer
Copy link
Contributor Author

anntzer commented Dec 13, 2017

Some further research pointed to https://bugs.python.org/issue1252236 (by Matplotlib's own @mdehoon :-)) which indicates that tkinter only checks for events every 20ms (the line static int Tkinter_busywaitinterval = 20; is still present in the sources); and, indeed, it looks like the erroneous behavior starts to appear as the interval goes from above 20ms to below (I tried 25 and 15; at 15 you may need to try a few times before reproducing into the issue, probably because this presumably requires some whole interval to occur between two event checks, or something similar).

This doesn't mean that the issue is necessarily unsolvable, but may point out why certain attempts don't work...

@anntzer
Copy link
Contributor Author

anntzer commented Jan 10, 2018

Would be nice to get this in so that I can make a tkcairo PR that doesn't cause conflicts with this.

@efiring efiring merged commit 4f89ef2 into matplotlib:master Jan 10, 2018
@anntzer anntzer deleted the tkagg-anim branch January 10, 2018 02:38
@anntzer anntzer mentioned this pull request Jan 10, 2018
Merged
6 tasks
@QuLogic QuLogic modified the milestones: needs sorting, v2.2.0 Feb 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tacaswell tacaswell tacaswell approved these changes

@efiring efiring efiring approved these changes

Assignees
No one assigned
Labels
GUI: tk
Projects
None yet
Milestone
v2.2.0
Development

Successfully merging this pull request may close these issues.
5 participants
@anntzer @dopplershift @efiring @tacaswell @QuLogic