> 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 > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
