Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use QMainWindow.closeEvent for close events #1498

Merged
merged 1 commit into from Jan 16, 2013

Conversation

tecki
Copy link
Contributor

@tecki tecki commented Nov 14, 2012

The current code uses the QWidget.destroyed signal
to emit a close event into matplotlib. This is a very
bad idea, as it is a heavy abuse of Qt functionality.

When a main window is closed in Qt, it and its children
are deleted if the DeleteOnClose flag is set (which we
did set). This causes the QWidget.destroyed signal to be
called. Setting the DeleteOnClose flag is a very bad
idea, since objects should only be deleted by the
garbage collector. Soon after, PyQt4 will complain
that "the underlying C++ object has been deleted", since
the object is alive for python but dead for C++.

This patch changes the machinery to use the
QMainWindow.closeEvent to emit a close_event, which is
what one actually wants.

The current code uses the QWidget.destroyed signal
to emit a close event into matplotlib. This is a very
bad idea, as it is a heavy abuse of Qt functionality.

When a main window is closed in Qt, it and its children
are deleted if the DeleteOnClose flag is set (which we
did set). This causes the QWidget.destroyed signal to be
called. Setting the DeleteOnClose flag is a very bad
idea, since objects should only be deleted by the
garbage collector. Soon after, PyQt4 will complain
that "the underlying C++ object has been deleted", since
the object is alive for python but dead for C++.

This patch changes the machinery to use the
QMainWindow.closeEvent to emit a close_event, which is
what one actually wants.
@mdboom
Copy link
Member

mdboom commented Nov 14, 2012

Who's our resident Qt4 expert these days? @ddale? git blame shows a lot of people involved. I don't feel terribly qualified to assess whether this change makes sense.

@WeatherGod
Copy link
Member

This might actually resolve a race condition I have seen when closing an animation with the Qt4 backend. I am not qualified to assess the correctness here, but @dopplershift might be.

@dopplershift
Copy link
Contributor

I'll claim expertise with C++ Qt4, but not so much expertise in either Qt4 with Python or, more importantly, our (ab)use of it (though I hope to rectify that in the somewhat near future)

I will say that this commit seems to clean things up. However, I'm not using the GUI backends on a common enough basis right now to give it a good shakeout.

mdboom added a commit that referenced this pull request Jan 16, 2013
use QMainWindow.closeEvent for close events
@mdboom mdboom merged commit a203d02 into matplotlib:master Jan 16, 2013
mdboom added a commit that referenced this pull request Jan 16, 2013
use QMainWindow.closeEvent for close events
@tacaswell
Copy link
Member

One (possibly too late) concern is that this completely shadows the closeEvent behavior of QMainWindow. I think the sub-class's closeEvent should include the line QMainWindow.closeEvent(self,event) to make sure that things are properly cascaded up the inheritance chain.

@mdboom
Copy link
Member

mdboom commented Jan 17, 2013

@tacaswell: That's a good point. As I'm not a Qt expert, can you propose a fix -- ideally with a standalone test -- in a new PR? I'll go ahead and file a new issue for it so it doesn't get lost, in any event.

tacaswell pushed a commit to tacaswell/matplotlib that referenced this pull request Jan 18, 2013
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()'.
tacaswell pushed a commit to tacaswell/matplotlib that referenced this pull request Jan 26, 2013
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()'.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants