Review Request 125208: Workaround bug 350758

[Xuetian Weng]

> On Sept. 12, 2015, 10:12 p.m., David Rosca wrote:
> > I am the author of the mentioned patch. I had the same idea - don't call show() before calling exec(), but I didn't want to use a timer for that, so I found a hack with hide() that worked ... at least it seemed to work.
> > So +1 from me.

Yeah, I know why you want a hide() there, but unfortunately it hits a Qt bug and seems to be slower. My feeling is that this patch makes open a dialog much faster..


- Xuetian


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


On Sept. 12, 2015, 9:40 p.m., Xuetian Weng wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125208/
> -----------------------------------------------------------
> 
> (Updated Sept. 12, 2015, 9:40 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 reason of 350758: https://bugreports.qt.io/browse/QTBUG-48248 , I just tried to reproduce it with pure Qt code. If a dialog called against show, hide, and exec one immediately after another, the dialog may not show up.
> 
> 2. Why hide() is needed in the first place?
> If one runs dialog.show() and dialog.exec(), it usually works. So there is something happened in between 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 first check, and QDialog will actually show an invisible qdialog with it. I assume https://git.reviewboard.kde.org/r/123335/ tries to solve this issue by "steal" the focus from that invisible dialog by hide it first.
> 
> It makes some sense for Qt with "real" native dialog, because this invisble dialog helps Qt to aware that there's a modal dialog, but doesn't work for our case. 
> 
> 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. So here I used a trick with QTimer to delegate the show to next event when calling to KDEPlatformFileDialogHelper::show().
> 
> 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 on m_dialog 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/20150912/fa9405fd/attachment.html>