We read every piece of feedback, and take your input very seriously.
To see all available qualifiers, see our documentation.
There was an error while loading. Please reload this page.
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
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 bei 8000 ng 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()'.
destroyed()
Sorry, something went wrong.
Re-wired signal/slot connections so that the figure in removed from
121acdc
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()'.
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.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
PEP8 removed trailing whitespace
560f60a
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).
more white space corrections
a564b95
Add figure close test for qt4 backend
b87bffd
@tacaswell I made a pull request against your branch to add a skeleton test for you. See tacaswell#1.
Merge pull request #1 from dmcdougall/qt4_closeevent_Gcf
3426a78
@tacaswell Now you need to add the Gcf check in that test file.
Gcf
PEP8 fixes to FigureManagerQT class
e56eec2
test for issue matplotlib#1683
7a929ca
fixed link rot
c613e71
I think all of the test failures are false-negatives
5201====================================================================== 5202FAIL: matplotlib.tests.test_bbox_tight.test_bbox_inches_tight.test 5203----------------------------------------------------------------------
Looks like you moved from rcParams to switch_backend, so you can remove this line.
rcParams
switch_backend
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.
test_bbox_tight
removed unused import
582c72d
It is the same failure as this one which you pointed out in this thread: #1671 (comment)
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.
We should probably mark this test as knownfail or skip if Qt4/PySide are not installed.
knownfail
skip
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).
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)
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.
@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.
PyQt4
Qt
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.
matplotlib.test()
lib/matplotlib/__init__.py
test_backend_qt4.py
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.
@tacaswell That's my fault. I should have added that when I forked your branch. Apologies for the wild goose chase.
@dmcdougall no worries. I will deal with this tonight.
added test_backend_qt4 to test list
d636acb
I also checked that it does indeed fail (gracefully) if you hide both PyQt4 and PySide, but didn't include that in the commits.
PySide
+1
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).
I agree.
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.
v1.2.x
git cherry-pick
@mdboom Was #1498 cherry-picked to 1.2?
75a92df
Yes: #1498 was cherry-picked to v1.2.x. This should be as well.
Superceded by #1705. Closing.
Successfully merging this pull request may close these issues.