Qt5 reset signals after non-interactive plotting #13027
Conversation
|
I'm not sure how to test this without actually opening a window. I need to fire a non-interactive plot in Qt5. I don't need it to actually draw, but I need to run |
| @pytest.mark.backend('Qt5Agg') | ||
| def test_fig_close_ioff(qt_module): | ||
| # Turn off interactive mode | ||
| plt.ioff() |
There was a problem hiding this comment.
do you need this line for the tests to pass?
There was a problem hiding this comment.
I think so, yes. Otherwise plt.show() isn't a blocking call, which I think means that an entirely different branch of code is executed. The test would pass, but it wouldn't be testing the fix I made. But I'm working off of memory, I can double-check later.
There was a problem hiding this comment.
Not sure this really matters, but OTOH it probably doesn't hurt, so heh.
There was a problem hiding this comment.
Oh that's true, it might be a no-op...
| signal.signal(signal.SIGINT, CustomHandler) | ||
|
|
||
| # Show window- fires callback and exits | ||
| plt.show() |
There was a problem hiding this comment.
Instead show it may be more reliable to start the qt event loop.
There was a problem hiding this comment.
Did this in the new commit
|
Okay, now I'm done! Sorry @tacaswell, @anntzer for the multiple commits. This should be pretty thorough now, testing the signals during and after the event loop. I don't actually fire signals because Python doesn't let you fire SIGINT on Windows (see https://docs.python.org/3/library/os.html#os.kill). You can do it with win32api, but I don't want to create a testing dependency on pywin32. |
| @@ -114,6 +114,53 @@ def test_fig_close(backend): | |||
| assert init_figs == Gcf.figs | |||
|
|
|||
|
|
|||
| @pytest.mark.backend('Qt5Agg') | |||
| def test_fig_close_ioff(qt_module): | |||
There was a problem hiding this comment.
Rename the test perhaps? Right now the name seems unrelated.
Except for that, can be merged.
There was a problem hiding this comment.
Good call, renamed.
|
Do you want to squash the commits? up to you. |
|
Um... sure. It's cleaner, why not. Edit: also rebased to master. |
|
Thanks @joelfrederico ! |
PR Summary
Non-interactive plots change the signal handler after they have been closed. They should reset it instead
PR Checklist