Remove figure from Gcf when it is closed #1683

tacaswell · 2013-01-18T16:45:58Z

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

Gcf when it is closed. In PR matplotlib#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()'.

dmcdougall · 2013-01-18T16:51:21Z

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 · 2013-01-18T17:10:29Z

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()' ),


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

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

tacaswell · 2013-01-18T17:16:47Z

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

dmcdougall · 2013-01-18T18:55:22Z

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

Add figure close test for qt4 backend

dmcdougall · 2013-01-18T19:57:36Z

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

tacaswell · 2013-01-19T21:26:37Z

I think all of the test failures are false-negatives

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

dmcdougall · 2013-01-19T21:31:43Z

lib/matplotlib/tests/test_backend_qt4.py

@@ -0,0 +1,25 @@
+from matplotlib import rcParams


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

dmcdougall · 2013-01-19T21:36:49Z

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.

tacaswell · 2013-01-19T22:15:55Z

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

dmcdougall · 2013-01-19T22:27:42Z

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 · 2013-01-23T17:03:19Z

lib/matplotlib/tests/test_backend_qt4.py

+@cleanup
+def test_fig_close():
+    # force switch to the Qt4 backend
+    plt.switch_backend('Qt4Agg')


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

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

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

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)

tacaswell · 2013-01-24T00:32:25Z

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.

dmcdougall · 2013-01-24T01:21:25Z

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

mdboom · 2013-01-24T16:33:13Z

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

dmcdougall · 2013-01-24T16:40:31Z

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.

dmcdougall · 2013-01-24T16:43:00Z

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

tacaswell · 2013-01-24T17:45:17Z

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

tacaswell · 2013-01-25T05:52:02Z

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

dmcdougall · 2013-01-25T06:03:14Z

+1

tacaswell · 2013-01-26T02:28:37Z

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

dmcdougall · 2013-01-26T03:08:26Z

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?

mdboom · 2013-01-28T15:42:40Z

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

mdboom · 2013-01-28T15:46:45Z

Superceded by #1705. Closing.

pelson reviewed Jan 18, 2013
View reviewed changes

PEP8 removed trailing whitespace

560f60a

Thomas A Caswell and others added 2 commits January 18, 2013 12:29

more white space corrections

a564b95

Add figure close test for qt4 backend

b87bffd

Merge pull request #1 from dmcdougall/qt4_closeevent_Gcf

3426a78

Add figure close test for qt4 backend

tacaswell added 3 commits January 19, 2013 15:01

PEP8 fixes to FigureManagerQT class

e56eec2

test for issue matplotlib#1683

7a929ca

fixed link rot

c613e71

dmcdougall reviewed Jan 19, 2013
View reviewed changes

removed unused import

582c72d

mdboom reviewed Jan 23, 2013
View reviewed changes

added knownfailureif to test based on if qt4_compat could be imported,

def3327

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)

added test_backend_qt4 to test list

d636acb

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

test for issue matplotlib#1683

75a92df

tacaswell mentioned this pull request Jan 26, 2013

Qt closeevent fixes for v1.2.x #1705

Merged

mdboom closed this Jan 28, 2013

tacaswell deleted the qt4_closeevent_Gcf branch January 28, 2013 20:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove figure from Gcf when it is closed #1683

Remove figure from Gcf when it is closed #1683

tacaswell commented Jan 18, 2013

dmcdougall commented Jan 18, 2013

pelson Jan 18, 2013

tacaswell Jan 18, 2013

tacaswell commented Jan 18, 2013

dmcdougall commented Jan 18, 2013

dmcdougall commented Jan 18, 2013

tacaswell commented Jan 19, 2013

dmcdougall Jan 19, 2013

dmcdougall commented Jan 19, 2013

tacaswell commented Jan 19, 2013

dmcdougall commented Jan 19, 2013

mdboom Jan 23, 2013

tacaswell Jan 23, 2013

mdboom Jan 23, 2013

tacaswell commented Jan 24, 2013

dmcdougall commented Jan 24, 2013

mdboom commented Jan 24, 2013

dmcdougall commented Jan 24, 2013

dmcdougall commented Jan 24, 2013

tacaswell commented Jan 24, 2013

tacaswell commented Jan 25, 2013

dmcdougall commented Jan 25, 2013

tacaswell commented Jan 26, 2013

dmcdougall commented Jan 26, 2013

mdboom commented Jan 28, 2013

mdboom commented Jan 28, 2013

Remove figure from Gcf when it is closed #1683

Remove figure from Gcf when it is closed #1683

Conversation

tacaswell commented Jan 18, 2013

dmcdougall commented Jan 18, 2013

pelson Jan 18, 2013

Choose a reason for hiding this comment

tacaswell Jan 18, 2013

Choose a reason for hiding this comment

tacaswell commented Jan 18, 2013

dmcdougall commented Jan 18, 2013

dmcdougall commented Jan 18, 2013

tacaswell commented Jan 19, 2013

dmcdougall Jan 19, 2013

Choose a reason for hiding this comment

dmcdougall commented Jan 19, 2013

tacaswell commented Jan 19, 2013

dmcdougall commented Jan 19, 2013

mdboom Jan 23, 2013

Choose a reason for hiding this comment

tacaswell Jan 23, 2013

Choose a reason for hiding this comment

mdboom Jan 23, 2013

Choose a reason for hiding this comment

tacaswell commented Jan 24, 2013

dmcdougall commented Jan 24, 2013

mdboom commented Jan 24, 2013

dmcdougall commented Jan 24, 2013

dmcdougall commented Jan 24, 2013

tacaswell commented Jan 24, 2013

tacaswell commented Jan 25, 2013

dmcdougall commented Jan 25, 2013

tacaswell commented Jan 26, 2013

dmcdougall commented Jan 26, 2013

mdboom commented Jan 28, 2013

mdboom commented Jan 28, 2013