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

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kfilecustomdialogtest.h:30
> +public:
> +    explicit KFileCustomDialogTest(QObject *parent = nullptr);
> +    ~KFileCustomDialogTest() = default;

not really useful, in unittests (there's initTestcase which is better for initializations anyway)

> kfilecustomdialog.h:32
> +  * This class implement a custom file dialog.
> +  * It uses a KFileWidget and we can add some custom widget
> +  * @since 5.42

API documentation rarely says "we".
Suggested rephrasing:

It uses a KFileWidget and allows the application to provide a custom widget.

... which will be shown where, BTW? Below the directory view and above the buttons? Or on the side? I see the implementation is KFileWidget::setCustomWidget so I could be less lazy and look that up, but the user of KFileCustomDialog wouldn't even know this anyway (unless the docu for setCustomWidget is improved to point there).

> kfilecustomdialog.h:43
> +    /**
> +     * @brief setUrl assign base url
> +     * @param url

Please reuse the documentation from KFileWidget, it's better.

> kfilecustomdialog.h:68
> +    void accept() override;
> +    void slotOk();
> +    void slotCancel();

Is this needed? Does it need to be public? Why not connect to KFileWidget's slotOk directly?

  (or using a lambda)

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/064992fb/attachment.html>