Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Remove figure from Gcf when it is closed #1683

Closed
wants to merge 11 commits into
from

Conversation

Projects
None yet
4 participants
Owner

tacaswell commented Jan 18, 2013

Re-wired signal/slot connections so that the figure in removed from Gcf when it is closed.

In PR #1498 the attribute WA_DeleteOnClose was no longer set on the
QtMainWindow object in QtFigureManager. The signal connection that
was being used to remove the figure from Gcf when the window was closed
was tied to the destroyed() signal of QtMainWindow, which is no
longer being destroyed. Thus, gca and gcf would return references
to no-longer visible figures/axes. _widgetclosed is now called when
MainWindow emits 'closing()'.

Re-wired signal/slot connections so that the figure in removed from
Gcf when it is closed.

In PR #1498 the attribute WA_DeleteOnClose was no longer set on the
QtMainWindow object in QtFigureManager.  The signal connection that
was being used to remove the figure from Gcf when the window was closed
was tied to the `destroyed()` signal of QtMainWindow, which is no
longer being destroyed.  Thus, gca and gcf would return references
to no-longer visible figures/axes.  _widgetclosed is now called when
MainWindow emits 'closing()'.
Member

dmcdougall commented Jan 18, 2013

How do we test these Qt4-based issues? I merged #1678 because there appeared to be no succinct way to test it, but perhaps that was an oversight on my part.

@pelson pelson and 1 other commented on an outdated diff Jan 18, 2013

lib/matplotlib/backends/backend_qt4.py
@@ -389,7 +389,9 @@ def __init__( self, canvas, num ):
self.window = MainWindow()
self.window.connect(self.window, QtCore.SIGNAL('closing()'),
canvas.close_event)
-
+ self.window.connect( self.window, QtCore.SIGNAL( 'closing()' ),
@pelson

pelson Jan 18, 2013

Member

Extra whitespace has found its way in here. would you mind removing it? (Is it an automatic editor thing?)

@tacaswell

tacaswell Jan 18, 2013

Owner

sorry, on my to-do list this weekend is to get my editor set up to highlight pep8 issue.

Owner

tacaswell commented Jan 18, 2013

This is not directly related to #1678 , it's fixing a bug from #1498

I don't know how to test this stuff in general. As near as I can tell, there is no UI testing at all.

This particular bug should be straight forward to test (make a figure, call close() on it, look at the state of Gcf before and after), but I don't know enough about nose to write that test from scratch (if you can point me at a similar test, I can take a crack at it).

Member

dmcdougall commented Jan 18, 2013

@tacaswell I made a pull request against your branch to add a skeleton test for you. See tacaswell/matplotlib#1.

Merge pull request #1 from dmcdougall/qt4_closeevent_Gcf
Add figure close test for qt4 backend
Member

dmcdougall commented Jan 18, 2013

@tacaswell Now you need to add the Gcf check in that test file.

Owner

tacaswell commented Jan 19, 2013

I think all of the test failures are false-negatives

5201======================================================================
5202FAIL: matplotlib.tests.test_bbox_tight.test_bbox_inches_tight.test
5203----------------------------------------------------------------------

@dmcdougall dmcdougall commented on an outdated diff Jan 19, 2013

lib/matplotlib/tests/test_backend_qt4.py
@@ -0,0 +1,25 @@
+from matplotlib import rcParams
@dmcdougall

dmcdougall Jan 19, 2013

Member

Looks like you moved from rcParams to switch_backend, so you can remove this line.

Member

dmcdougall commented Jan 19, 2013

I think all of the test failures are false-negatives

5201======================================================================
5202FAIL: matplotlib.tests.test_bbox_tight.test_bbox_inches_tight.test
5203----------------------------------------------------------------------

I think you are right. The test_bbox_tight test passes locally for me.

Owner

tacaswell commented Jan 19, 2013

It is the same failure as this one which you pointed out in this thread:
#1671 (comment)

Member

dmcdougall commented Jan 19, 2013

Yep. I no longer get that failure locally, but it doesn't seem to sit well with Travis. You clearly didn't cause the error.

@mdboom mdboom and 1 other commented on an outdated diff Jan 23, 2013

lib/matplotlib/tests/test_backend_qt4.py
@@ -0,0 +1,24 @@
+from matplotlib import pyplot as plt
+from matplotlib.testing.decorators import cleanup
+from matplotlib._pylab_helpers import Gcf
+import copy
+
+
+@cleanup
+def test_fig_close():
+ # force switch to the Qt4 backend
+ plt.switch_backend('Qt4Agg')
@mdboom

mdboom Jan 23, 2013

Owner

We should probably mark this test as knownfail or skip if Qt4/PySide are not installed.

@tacaswell

tacaswell Jan 23, 2013

Owner

What is the correct way to test if Qt4/PySide is available?

@mdboom

mdboom Jan 23, 2013

Owner

Something like:

try:
    import Qt
    HAS_PYQT = True
except ImportError:
    HAS_PYQT = False

(And obviously extend to also check for PySide -- it should be good enough to have one or the other).

added knownfailureif to test based on if qt4_compat could be imported,
as this should hit either PySide or PyQt4, depending on which is
installed.  (I have faith that the setup code that will make sure that
if only one of them is installed, it defaults to using that one)
Owner

tacaswell commented Jan 24, 2013

It looks like travis does not have Qt, but yet this test seems to pass. I am deeply confused why this test did not fail on travis before adding the guard.

Member

dmcdougall commented Jan 24, 2013

@tacaswell To investigate, you can simulate a missing dependency locally:

sys.modules['Qt'] = None
try:
    import Qt  # will fail
    HAS_PYQT = True
except ImportError:
    HAS_PYQT = False

P.S.: I thought it was PyQt4, not Qt? My knowledge of Qt4 is limited.

Owner

mdboom commented Jan 24, 2013

Ah -- I think Travis is using matplotlib.test() to run the tests, which uses the default test categories listed in lib/matplotlib/__init__.py... test_backend_qt4.py is not among them, so it simply doesn't get run. I think the right thing to do there is add it to the list and then put the import check as described here.

And, yes, I misremembered the name of the module. Indeed it is PyQt4 (as I said, I hadn't tested my code... 😄)

Member

dmcdougall commented Jan 24, 2013

And, yes, I misremembered the name of the module. Indeed it is PyQt4 (as I said, I hadn't tested my code... 😄)

Not tested; merely proven to be correct.

Member

dmcdougall commented Jan 24, 2013

@tacaswell That's my fault. I should have added that when I forked your branch. Apologies for the wild goose chase.

Owner

tacaswell commented Jan 24, 2013

@dmcdougall no worries. I will deal with this tonight.

Owner

tacaswell commented Jan 25, 2013

I also checked that it does indeed fail (gracefully) if you hide both PyQt4 and PySide, but didn't include that in the commits.

Member

dmcdougall commented Jan 25, 2013

+1

Owner

tacaswell commented Jan 26, 2013

can this get tagged to 1.2.x? I think it should have the same milestone as the commit that introduced the bug.

Sorry if I am mis-understanding the meaning of milestones/the release process (as none of the commits related to this are on the v1.2.x branch).

Member

dmcdougall commented Jan 26, 2013

can this get tagged to 1.2.x? I think it should have the same milestone as the commit that introduced the bug.

I agree.

Sorry if I am mis-understanding the meaning of milestones/the release process (as none of the commits related to this are on the v1.2.x branch).

I just noticed this wasn't targeted to the v1.2.x. Would you be able to transfer your commits over to the v1.2.x branch as I think this change is big enough to be worried about git cherry-picking after the fact.

@mdboom Was #1498 cherry-picked to 1.2?

tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Jan 26, 2013

Owner

mdboom commented Jan 28, 2013

Yes: #1498 was cherry-picked to v1.2.x. This should be as well.

Owner

mdboom commented Jan 28, 2013

Superceded by #1705. Closing.

@mdboom mdboom closed this Jan 28, 2013

@tacaswell tacaswell deleted the tacaswell:qt4_closeevent_Gcf branch Jan 28, 2013

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