<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/125208/">https://git.reviewboard.kde.org/r/125208/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 2nd, 2015, 8:46 p.m. UTC, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Please check if the unittests in frameworkintegration still pass for you.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p></pre>
</blockquote>
<p>On October 2nd, 2015, 10 p.m. UTC, <b>Xuetian Weng</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It works here localily. Maybe for some reason that QTest::qWaitForWindowExposed will not work on test machine?</p></pre>
</blockquote>
<p>On October 2nd, 2015, 10:20 p.m. UTC, <b>Xuetian Weng</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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</p></pre>
</blockquote>
<p>On October 2nd, 2015, 10:27 p.m. UTC, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">kinit is a requirement indeed. I see it's not explicitly listed in kde-build-metadata/dependency-data-kf5-qt5, I'll add it.</p></pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p></pre>
<br />
<p>- Xuetian</p>
<br />
<p>On September 30th, 2015, 6:04 p.m. UTC, Xuetian Weng wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Frameworks, David Faure and Jean-Baptiste Mardelle.</div>
<div>By Xuetian Weng.</div>
<p style="color: grey;"><i>Updated Sept. 30, 2015, 6:04 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=350758">350758</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
frameworkintegration
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><ol style="padding: 0;text-rendering: inherit;margin: 0 0 0 2em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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().</p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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().</p>
</li>
</ol>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">https://github.com/qtproject/qtbase/blob/5bfac9d653357c906946563b9494d7ae69cdad92/src/widgets/dialogs/qfiledialog.cpp#L868</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">and call QDialog::setVisible() here</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">https://github.com/qtproject/qtbase/blob/17d6b2d1f02e5f679008d97036befd713025a0f2/src/widgets/dialogs/qdialog.cpp#L713</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p>
<ol style="padding: 0;text-rendering: inherit;margin: 0 0 0 2em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">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.</li>
</ol>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Before this change, the calling sequence is </p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">m_dialog->show();
The dummy dialog setVisible(true);
m_dialog->hide();
m_dialog->exec();</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">After this change, it becomes
The dummy dialog setVisible(true);
m_dialog->exec();</p>
<ol style="padding: 0;text-rendering: inherit;margin: 0 0 0 2em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Required changes to autotest
QTest::qWaitForWindowExposed(fw->window()) is added to some necessary places.</li>
</ol></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">not able to reproduce the bug.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>autotests/kfiledialog_unittest.cpp <span style="color: grey">(0d4c816)</span></li>
<li>autotests/kfiledialogqml_unittest.cpp <span style="color: grey">(f805ef2)</span></li>
<li>src/platformtheme/kdeplatformfiledialogbase.cpp <span style="color: grey">(8e696bd)</span></li>
<li>src/platformtheme/kdeplatformfiledialogbase_p.h <span style="color: grey">(5936dfb)</span></li>
<li>src/platformtheme/kdeplatformfiledialoghelper.h <span style="color: grey">(dfbbed1)</span></li>
<li>src/platformtheme/kdeplatformfiledialoghelper.cpp <span style="color: grey">(94f2059)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/125208/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>