BUG: 395988
In certain cases KMainWindow::saveMainWindowSettings() could have been
called after mainwindow started destroying itself. Window settings would
be saved with incorrect child widget states. e.g. some widgets would be
saved as hidden even if they were visible before destroying
Details
- Reviewers
dfaure elvisangelaccio broulik cfeck - Group Reviewers
KDE Applications
Diff Detail
- Repository
- R263 KXmlGui
- Branch
- fix-window-state-save
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 588 Build 600: arc lint + arc unit
The bug report indicates that this patch is for adapting to a change in Qt 5.11. Is this patch fully backwards-compatible with older Qt versions? If not, we'll need to guard this behind a Qt version check.
Also, is the new behavior in Qt is a bug rather than an intended behavioral change? If so, we'll need for a Qt bug report to be mentioned in the comment.
Hard to tell if the qt behavior change is a bug or a feature, but the fix makes sense to me.
(BTW I've been experiencing this regression in zanshin)
I just like to note that VirtualBox has the same problem with Qt 5.11.1, that the toolbar and the status bar disappear on next start if you close the window:
http://bugzilla.opensuse.org/show_bug.cgi?id=1099589
So it does not only affect applications using KMainWindow...
The change in qt affects when the window close event is handled in queue, it didn't change when it is fired. With qt 5.11.1 child widgets get destroyed a bit sooner, but it's still gets triggered by same close event - i think that stayed like it always was.
Also KMainWindow code is made to autosave settings (if not disabled) every time some relevant widget changes, so am pretty sure that this patch won't ever cause it to save incorrect settings.
Window/widget visibility and size is unlikely to change by user after they click to close window.
However to be 100% sure will submit the test.
Unit test is there... and it passes... always... with or without the kmainwindow.cpp patch :-/
Is there some simpler way to initialize/kill QApplication for each mainwindow creation/deletion, other than adding another unit test that uses QTEST_APPLESS_MAIN instead of QTEST_MAIN?
It does, but not between your
NativeMainWindow mw; mw.close();
and as far as i understood the problem was with the close request comming from a loop (maybe i'm wrong) so i'm suggesting you either manually create an event loop or post the close event instead of directly calling close(). Of course ignore my comment if the close event being queued is not part of the problem/regression
Have tried doing QApplication::processEvents() and QApplication::sendPostedEvents(), between main window instances... didnt change
Have removed mw.close() and clicked close buttons manually... didnt change
Then have started all three in separate process using QProcess and hack to main, and just now have figured that mw is supposed to be deleted during close... not sooner like it's happening now.
Will change them to be created with new, make it not work and update.
Thank you! QEventLoop and creating window with new operator did it. Now the only problem is that no matter how i close the window from code it's not reproducing behavior. Only closing the window manually through window manager works.
Have tried:
QApplication::postEvent(mw, new QCloseEvent); // this one does absolutely nothing
QApplication::postEvent(mw, new QDeferredDeleteEvent);
QTimer::singleShot(1000, [&](){ QApplication::sendEvent(mw, new QDeferredDeleteEvent); });
mv.close();
QTimer::singleShot(1000, [&](){ mv.close(); });
The close event from window manager is QDeferredDeleteEvent and has spontaneous flag, i didn't notice any other differences in debugger. Any ideas?
Relative Qt Bug is here: https://bugreports.qt.io/browse/QTBUG-69277
Seems it's not their bug. And Qt guy commented there with:
... You should, however, not save the window layout anymore then, because after closeEvent() any other widgets or child windows could also be gone already...
Hm.
Interestingly, Qt Creator has similar problems with Qt 5.11.1 that are also fixed by reverting that commit in Qt:
http://bugzilla.opensuse.org/show_bug.cgi?id=1099752
(and Qt Creator also doesn't use kxmlgui AFAIK... ;-) )
And the second last comment in https://codereview.qt-project.org/#/c/227398/ says:
This change somehow breaks QMainWindow::saveState() when it's called from the destructor: toolbars are not shown on the next start.
Anyway, that's not really related to this review, or whether the proposed change makes sense...
autotests/kmainwindow_unittest.cpp | ||
---|---|---|
278 | Or rather QVERIFY(mw->m_dock->isVisible()), that's what QVERIFY is for ;-) |
autotests/kmainwindow_unittest.cpp | ||
---|---|---|
278 | :D Right, I got distracted by the == |
Clened up test code. Replaced isVisible() with isHidden() test to match KMainWindow's condition.
autotests/kmainwindow_unittest.cpp | ||
---|---|---|
267 | Retried with all of these... none of them causes the failure, only closing the window manually cause it QApplication::postEvent(mw, new QDeferredDeleteEvent); // qpa/window manager sends this one.. with spontaneous flag tho QApplication::postEvent(mw, new QCloseEvent); // this one does absolutely nothing QTimer::singleShot(1000, [&](){ QApplication::sendEvent(mw, new QDeferredDeleteEvent); }); mw->close(); QTimer::singleShot(1000, [&](){ mw->close(); }); mw->deleteLater(); |
This has been approved for two weeks and fixes a major issue... could the fix be pushed and then keep working on the auto test later? It already missed 5.48
Shall we commit this? maxrd2 , can you do it, or does one of us need to commit it for you?
@ngraham Don't think I can. I have no commit rights.
Should i revert/remove test part, as currently it doesn't test anything i believe.. it always passes.
Yeah, I say let's remove the test for now so we can commit this, and then you can add the test back in another patch please (in working form, of course!). :)
autotests/kmainwindow_unittest.cpp | ||
---|---|---|
267 | mw->setAttribute(Qt::WA_DeleteOnClose); |
I just pushed the fix https://commits.kde.org/kxmlgui/d35a88289513c0420863b80aa6c1cb7d2c6e978f
Please continue working on the unittest here ;)
Events from the window system seem to be handled after normal events.
http://code.qt.io/cgit/qt/qtbase.git/tree/src/platformsupport/eventdispatchers/qeventdispatcher_glib.cpp#n70
Is there a way to send the event so qt processes it as window system event? Have tried looking through code and documentation, but everything that deals with window system events is private.
I don't know much about window system events, but some of the private headers are still accessible if you use the -private includes see like for example plasma-integration uses QtPlatformSupport/private/qdbusplatformmenu_p.h so that may help you (it's not great using private headers, but sometimes you have to do what you have to do)
Hmm, for me the bug is still there. Or maybe it means it's a different bug, but I kind of doubt it.
Steps:
- I start kmail (with a cleaned up ~/.config/kmail2rc to make sure the StatusBar and the Toolbar are shown).
- I close it (Alt+F4).
- I start it again, it has no statusbar and no toolbar.
Steps 1 and 2 alone lead to this in kmail2rc, section [Main Window]:
MenuBar=Disabled
StatusBar=Disabled
ToolBarsMovable=Disabled
Any idea if this is related to this bug?
I do have kxmlgui 5.48, with the presumed fix from this review request, of course.
I think it means the fix isn't good enough?
In the Bugzilla ticket, it was mentioned that KMail (and only KMail) still suffers from the issue even with this patch, so it might be a KMail-specific issue.
OK I see, this is because KMail doesn't enable autosaving, but instead saves in the KMMainWin destructor, after the window (and all children) have been hidden.
I'll try whether porting KMail to setAutoSaveSettings solves it.
I don't understand your question.
By "this", do you mean the KMainWindow fix that was posted in a previous iteration here and that was pushed? That one is very much needed, yes, to make setAutoSaveSettings work.
The change that you link to, is about porting KMail to setAutoSaveSettings, so that it benefits from that fix.
If by "this" you mean what's left in this review request, which is to write a proper unittest for the KMainWindow fix, yes, that would still be very nice to have. But apparently it's pretty hard to write (if emulating native events is the only way then indeed it seems really difficult)... so maybe we have to give up on the idea.