<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/127944/">https://git.reviewboard.kde.org/r/127944/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 17th, 2016, 3:29 p.m. BST, <b>Martin Gräßlin</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;">The test does not verify the problem. I just pulled the patch, undid your change, but the test passed nevertheless.</p></pre>
</blockquote>
<p>On May 17th, 2016, 3:30 p.m. BST, <b>Martin Gräßlin</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;">ah now I see. You adjusted the test application, but not the autotest.</p></pre>
</blockquote>
<p>On May 17th, 2016, 3:41 p.m. BST, <b>Jonathan Marten</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;">Now I see that there are autotests after all. How do I run them?</pre>
</blockquote>
<p>On May 17th, 2016, 3:46 p.m. BST, <b>Martin Gräßlin</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;">you can go to the build directory and just do:
make test</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">or just run the individial test binary created in build/autotests. The relevant one would be kfiledialog_unittest and kfiledialogqml_unittest</p></pre>
</blockquote>
<p>On May 17th, 2016, 4:35 p.m. BST, <b>Jonathan Marten</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;">Ok, found them and how - thanks.
It doesn't appear to be possible to just check that the QFileDialog option is passed correctly through to the file widget, because KFileWidget has no way to read back the option set by setConfirmOverwrite. So it won't be a simple test like the setFileMode tests - it will have to look for the message box being shown. Is that worth doing (and reliable enough)?</pre>
</blockquote>
<p>On May 17th, 2016, 5:19 p.m. BST, <b>Martin Gräßlin</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;">hmm yeah, tricky. I think it would be ok to check for the messagebox being shown as that's kind of also how the autotest for the dialog works in general. But I also don't have an idea on how to check whether the messagebox got opened. So maybe just push without it.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Btw. please push for the Plasma/5.6 branch.</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;">Ok, have worked out an autotest. It's not pretty, but it does work - passing with the modified platform theme and failing with the original. It puts up a save box for a file which already exists, clicks the OK button and checks that a message box appears or not according to the QFileDialog option. The message box has to be checked in a QTimer slot because, if it does appear, it runs its own event loop which halts the test until the user clicks one of its buttons.
Go easy with it - it's my first ever Frameworks autotest :-)</pre>
<br />
<p>- Jonathan</p>
<br />
<p>On May 18th, 2016, 11:07 a.m. BST, Jonathan Marten 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-workspace and Plasma.</div>
<div>By Jonathan Marten.</div>
<p style="color: grey;"><i>Updated May 18, 2016, 11:07 a.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=360666">360666</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
plasma-integration
</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;">The referenced bug says that, by default, there is no file overwrite check when using QFileDialog to save a file. Indeed, on closer investigating it appears that there is no way to even explictly force an overwrite check when using the KDE platform theme, because of this code in plasma-integration/src/platformtheme/kdeplatformfiledialoghelper.cpp:
// overwrite option
if (options()->testOption(QFileDialogOptions::FileDialogOption::DontConfirmOverwrite)) {
dialog->m_fileWidget->setConfirmOverwrite(false);
}
The default for KFileWidget is already for no overwrite check (as set in kio/src/filewidgets/kfilewidget.cpp which initialises KFileWidgetPrivate::confirmOverwrite to false). There is no way to override this from the calling application through the platform plugin.
Suggest that the default option should be the same as that defined by Qt for QFileDialog: always perform an ovewrwrite check on saving, unless the caller has set the QFileDialog::DontConfirmOverwrite option. This is also a sensible default to have from the user's point of view. This change does that in the platform theme plugin, for all saving operations.</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;">Built plasma-intergration with this change, confirmed correct operation of file dialogues and that confirmation is requested when overwriting an existing file, unless the QFileDialog::DontConfirmOverwrite option is specified.</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">(59915da)</span></li>
<li>src/platformtheme/kdeplatformfiledialoghelper.cpp <span style="color: grey">(139c35d)</span></li>
<li>tests/qfiledialogtest.cpp <span style="color: grey">(1d69ea1)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/127944/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>