Review Request 125208: Fixes file dialog randomly not showing up with frameworkintegration

[Xuetian Weng]

> On Oct. 2, 2015, 8:46 p.m., David Faure wrote:
> > Please check if the unittests in frameworkintegration still pass for you.
> > 
> > https://build.kde.org/job/frameworkintegration%20master%20stable-kf5-qt5/PLATFORM=Linux,compiler=gcc/20/testReport/junit/(root)/TestSuite/frameworkintegration_kfiledialog_unittest/ seems to indicate that your commit broke the tests, now the test program gets stuck and never terminates.
> 
> Xuetian Weng wrote:
>     It works here localily. Maybe for some reason that QTest::qWaitForWindowExposed will not work on test machine?
> 
> Xuetian Weng wrote:
>     Ok now I get it. I uninstall kinit on my machine to make sure my test can't launch a kdeinit5 just like the server, and in Xephyr I saw that a dialog with content "Could not start process. Cannot talk to klauncher: The name org.kde.klauncher5 was not provided by and .service files." waiting for user to click on it. http://imgur.com/vCRMfkm
> 
> David Faure wrote:
>     kinit is a requirement indeed. I see it's not explicitly listed in kde-build-metadata/dependency-data-kf5-qt5, I'll add it.

Just FYI, kdeinit5 is launched automatically but doesn't end automatically on my test case, which also stuck the cmake test (I guess it just waits for all processes created in test end). I have to manually kill the kdeinit5 to make the test pass.  Not sure if this is handled automatically on test server.


- Xuetian


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125208/#review86251
-----------------------------------------------------------


On Sept. 30, 2015, 6:04 p.m., Xuetian Weng wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125208/
> -----------------------------------------------------------
> 
> (Updated Sept. 30, 2015, 6:04 p.m.)
> 
> 
> Review request for KDE Frameworks, David Faure and Jean-Baptiste Mardelle.
> 
> 
> Bugs: 350758
>     https://bugs.kde.org/show_bug.cgi?id=350758
> 
> 
> Repository: frameworkintegration
> 
> 
> Description
> -------
> 
> 1. The directly reason of 350758 is https://bugreports.qt.io/browse/QTBUG-48248, which seems to be a Qt problem. See the code example. One possible to avoid this is to remove the m_dialog->hide().
> 
> 2. Why hide() is needed in the first place?
> If one runs dialog.show() and dialog.exec(), it should works. So there is something happened in between the calls to KDEPlatformFileDialogHelper::show() and KDEPlatformFileDialogHelper::exec().
> 
> The real reason is the platform dialog helper doesn't expect that user may use "Qt" dialog to implement it.
> https://github.com/qtproject/qtbase/blob/5bfac9d653357c906946563b9494d7ae69cdad92/src/widgets/dialogs/qfiledialog.cpp#L845
> here it sets Qt::WA_DontShowOnScreen on the actual dialog.
> 
> https://github.com/qtproject/qtbase/blob/5bfac9d653357c906946563b9494d7ae69cdad92/src/widgets/dialogs/qfiledialog.cpp#L868
> 
> and call QDialog::setVisible() here
> 
> https://github.com/qtproject/qtbase/blob/17d6b2d1f02e5f679008d97036befd713025a0f2/src/widgets/dialogs/qdialog.cpp#L713
> 
> Thus it will by pass the check, and QDialog will actually show an invisible qdialog. I assume https://git.reviewboard.kde.org/r/123335/ tries to solve this issue by "steal" the focus from that invisible dialog by hide our dialog and show it again.
> 
> It makes some sense for Qt with "real" native dialog which is not controlled by Qt, because this invisble dialog helps Qt to aware that there's a modal dialog, but doesn't work for our case since our dialog is also a QDialog.
> 
> 3. To avoid calling hide() in exec()
> I'd rather to see that KDEPlatformFileDialogHelper::show() is not called in QDialog::exec(), but that's not gonna happen anytime soon. So here I used a trick with QTimer to delegate the call to show() to next event.
> 
> I uses QTimer instead of QMetaObject::invoke here, because we can discard the "m_dialog->show()" if hide() or exec() is called immediately. (Actually we don't need to discard the one in exec(), but we can save a call to m_dialog->show() if we discard it.)
> 
> Before this change, the calling sequence is 
> 
> m_dialog->show();
> The dummy dialog setVisible(true);
> m_dialog->hide();
> m_dialog->exec();
> 
> After this change, it becomes
> The dummy dialog setVisible(true);
> m_dialog->exec();
> 
> 4. Required changes to autotest
> QTest::qWaitForWindowExposed(fw->window()) is added to some necessary places.
> 
> 
> Diffs
> -----
> 
>   autotests/kfiledialog_unittest.cpp 0d4c816 
>   autotests/kfiledialogqml_unittest.cpp f805ef2 
>   src/platformtheme/kdeplatformfiledialogbase.cpp 8e696bd 
>   src/platformtheme/kdeplatformfiledialogbase_p.h 5936dfb 
>   src/platformtheme/kdeplatformfiledialoghelper.h dfbbed1 
>   src/platformtheme/kdeplatformfiledialoghelper.cpp 94f2059 
> 
> Diff: https://git.reviewboard.kde.org/r/125208/diff/
> 
> 
> Testing
> -------
> 
> not able to reproduce the bug.
> 
> 
> Thanks,
> 
> Xuetian Weng
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20151002/bff82852/attachment-0001.html>