> On May 17, 2016, 3:29 p.m., Martin Gräßlin wrote:
> > The test does not verify the problem. I just pulled the patch, undid your
> > change, but the test passed nevertheless.
>
> Martin Gräßlin wrote:
> ah now I see. You adjusted the test application, but not the autotest.
>
> Jonathan Marten wrote:
> Now I see that there are autotests after all. How do I run them?
>
> Martin Gräßlin wrote:
> you can go to the build directory and just do:
> make test
>
> or just run the individial test binary created in build/autotests. The
> relevant one would be kfiledialog_unittest and kfiledialogqml_unittest
>
> Jonathan Marten wrote:
> 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)?
>
> Martin Gräßlin wrote:
> 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.
>
> Btw. please push for the Plasma/5.6 branch.
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 :-)
- Jonathan
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127944/#review95535
---
On May 18, 2016, 11:07 a.m., Jonathan Marten wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127944/
> ---
>
> (Updated May 18, 2016, 11:07 a.m.)
>
>
> Review request for kde-workspace and Plasma.
>
>
> Bugs: 360666
> https://bugs.kde.org/show_bug.cgi?id=360666
>
>
> Repository: plasma-integration
>
>
> Description
> ---
>
> 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.
>
>
> Diffs
> -
>
> autotests/kfiledialog_unittest.cpp 59915da
> src/platformtheme/kdeplatformfiledialoghelper.cpp 139c35d
> tests/qfiledialogtest.cpp 1d69ea1
>
> Diff: https://git.reviewboard.kde.org/r/127944/diff/
>
>
> Testing
> ---
>
> 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.
>
>
> Thanks,
>
> Jonathan Marten
>
>