D9206: Implement a kfile dialog where we can add custom widget

[David Faure]

dfaure added a comment.


  Hmm. I can see the usefulness, it's certainly much better than the way people have historically inserted custom widgets into QFileDialog (https://stackoverflow.com/questions/16987916/add-widgets-to-qfiledialog, URGH).
  
  On the other hand, I'm wondering if this is going to be abused as a KFileDialog replacement, losing the "native" look-n-feel for the file dialog on Windows / macOS.
  Well, that's certainly a pre-requisite for any custom widget to work, it should just be made extra clear to app developers that what they lose in return is the ability to get native dialogs on Windows and macOS (and Gnome, I suppose).
  
  So, I'm OK with this, provided that this documentation is added:
  
  The downside of using this class is that your application will not be able to get a native file dialog on non-Plasma environments (Windows, macOS, ...). For this reason you should really think twice before deciding to use it, as a custom widget might improve user experience within the Plasma workspace, but might make it worse for users who are used to the native file dialog. In 99% of the cases, you want to use QFileDialog instead.

INLINE COMMENTS

> kfilecustomdialogtest.cpp:42
> +
> +    QVBoxLayout *mainLayout = dlg.findChild<QVBoxLayout *>(QStringLiteral("mainlayout"));
> +    QVERIFY(mainLayout);

This isn't really testing anything useful. Rename the layout and the test breaks. Maybe better verify that dlg itself has a layout, rather than "there is a child layout somwhere called mainlayout".

> kfilecustomdialogtest.cpp:47
> +    QVERIFY(mFileWidget);
> +    QVERIFY(dlg.fileWidget());
> +}

QCOMPARE(dlg.fileWidget(), mFileWidget) ? More useful ;)

> kfilecustomdialogtest_gui.cpp:26
> +
> +KFileCustomDialogTest_gui::KFileCustomDialogTest_gui()
> +{

remove

> kfilecustomdialogtest_gui.h:26
> +
> +class KFileCustomDialogTest_gui
> +{

unused, remove (remove the whole header...)

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D9206

To: mlaurent, mwolff, dfaure
Cc: #frameworks
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20171212/c24628b6/attachment-0001.html>