Fix KMainWindow saving incorrect widget settings
Needs ReviewPublic

Authored by maxrd2 on Jun 30 2018, 9:38 AM.

Details

Summary

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

Diff Detail

Repository
R263 KXmlGui
Branch
fix-window-state-save
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 948
Build 961: arc lint + arc unit
maxrd2 created this revision.Jun 30 2018, 9:38 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptJun 30 2018, 9:38 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
maxrd2 requested review of this revision.Jun 30 2018, 9:38 AM
maxrd2 updated this revision to Diff 36938.Jun 30 2018, 9:44 AM

Cleaned up indentation.

ngraham added a subscriber: ngraham.

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.

dfaure accepted this revision.Jun 30 2018, 2:11 PM

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)

This revision is now accepted and ready to land.Jun 30 2018, 2:11 PM
aacid added a subscriber: aacid.Jun 30 2018, 4:49 PM

Can we get an auto test?

wbauer added a subscriber: wbauer.Jun 30 2018, 4:59 PM

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.

maxrd2 updated this revision to Diff 36973.Jul 1 2018, 2:24 AM

Added unit test

maxrd2 added a comment.EditedJul 1 2018, 2:34 AM

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?

aacid added a comment.Jul 2 2018, 9:03 PM

Maybe you need an event loop in your tests?

maxrd2 added a comment.EditedJul 3 2018, 12:17 PM

QTEST_MAIN already provides QApplication and event loop

aacid added a comment.Jul 3 2018, 10:05 PM

QTEST_MAIN already provides QApplication and event loop

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

maxrd2 added a comment.Jul 4 2018, 2:39 AM

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.

maxrd2 updated this revision to Diff 37131.Jul 4 2018, 4:56 AM

Added event loop to test and replicated wanted KMainWindow behavior.

maxrd2 added a comment.Jul 4 2018, 4:56 AM

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?

maxrd2 added a comment.Jul 4 2018, 4:48 PM

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

wbauer added a comment.Jul 5 2018, 9:17 AM

Relative Qt Bug is here: https://bugreports.qt.io/browse/QTBUG-69277
Seems it's not their bug.

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

broulik added inline comments.Jul 5 2018, 9:20 AM
autotests/kmainwindow_unittest.cpp
265
connect(mw, &QObject::destroyed, &el, &QEventLoop::quit);
267

mw->deleteLater()?

278

QCOMPARE(mw->m-dock->isVisible(), true)

dfaure added inline comments.Jul 5 2018, 9:25 AM
autotests/kmainwindow_unittest.cpp
278

Or rather QVERIFY(mw->m_dock->isVisible()), that's what QVERIFY is for ;-)

broulik added inline comments.Jul 5 2018, 9:25 AM
autotests/kmainwindow_unittest.cpp
278

:D Right, I got distracted by the ==

maxrd2 added inline comments.Jul 5 2018, 3:51 PM
autotests/kmainwindow_unittest.cpp
267

tried it too... will retry all of them again with isHidden()

278

mmm.. sorry i was supposed to use isHidden() test here, isVisible() is not the opposite
will change to QCOMPARE(mw->m-dock->isHidden(), false)

maxrd2 updated this revision to Diff 37211.Jul 5 2018, 9:38 PM

Clened up test code. Replaced isVisible() with isHidden() test to match KMainWindow's condition.

maxrd2 added inline comments.Jul 5 2018, 9:51 PM
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();
marten added a subscriber: marten.Jul 8 2018, 2:40 PM
arojas added a subscriber: arojas.Jul 14 2018, 10:53 AM

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

Hmm, good point, I thought this was in already :(

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

anthonyfieroni added inline comments.
autotests/kmainwindow_unittest.cpp
267

mw->setAttribute(Qt::WA_DeleteOnClose);
Then post event close or deleteLater should work

dfaure requested changes to this revision.Jul 17 2018, 7:46 AM

I just pushed the fix https://commits.kde.org/kxmlgui/d35a88289513c0420863b80aa6c1cb7d2c6e978f
Please continue working on the unittest here ;)

This revision now requires changes to proceed.Jul 17 2018, 7:46 AM
maxrd2 updated this revision to Diff 37950.Jul 17 2018, 1:57 PM

Rebased to master

arojas removed a subscriber: arojas.Jul 17 2018, 2:00 PM

@dfaure great.. thanks

autotests/kmainwindow_unittest.cpp
267

KMainWindowPrivate::init() which is called from constructor already does setAttribute(Qt::WA_DeleteOnClose), so that doesnt change anything :-/

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.

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:

  1. I start kmail (with a cleaned up ~/.config/kmail2rc to make sure the StatusBar and the Toolbar are shown).
  2. I close it (Alt+F4).
  3. 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.

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.

For reference: https://bugs.kde.org/show_bug.cgi?id=396339

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.

cfeck added a comment.Aug 15 2018, 6:45 PM

Ah, unit tests are still open. Thanks for the clarification.