----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/125208/#review86148 -----------------------------------------------------------
I can confirm that I cannot reproduce the problem in Kdenlive with this patch. I would really like to have it included in KDE Frameworks 5.15 because having our apps randomly freeze is a really big problem. - Jean-Baptiste Mardelle On Sept. 13, 2015, 12: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, 12: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 > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
