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
