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

[Martin Klapetek]

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


Looks good as far as I can tell

- Martin Klapetek


On Sept. 13, 2015, 2:18 a.m., Xuetian Weng wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125208/
> -----------------------------------------------------------
> 
> (Updated Sept. 13, 2015, 2:18 a.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/20150925/8bbfbc11/attachment.html>