D9206: Implement a kfile dialog where we can add custom widget
This revision was automatically updated to reflect the committed changes. Closed by commit R241:41fbb247d97c: Implement a kfile dialog where we can add custom widget (authored by mlaurent). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D9206?vs=23916=23917 REVISION DETAIL https://phabricator.kde.org/D9206 AFFECTED FILES autotests/CMakeLists.txt autotests/kfilecustomdialogtest.cpp autotests/kfilecustomdialogtest.h src/filewidgets/CMakeLists.txt src/filewidgets/kfilecustomdialog.cpp src/filewidgets/kfilecustomdialog.h tests/CMakeLists.txt tests/kfilecustomdialogtest_gui.cpp To: mlaurent, mwolff, dfaure Cc: #frameworks
D9206: Implement a kfile dialog where we can add custom widget
mlaurent updated this revision to Diff 23916. mlaurent added a comment. - Remove indirection methods as requested by David. Fix doc too REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D9206?vs=23908=23916 BRANCH add_kfile_dialog REVISION DETAIL https://phabricator.kde.org/D9206 AFFECTED FILES autotests/CMakeLists.txt autotests/kfilecustomdialogtest.cpp autotests/kfilecustomdialogtest.h src/filewidgets/CMakeLists.txt src/filewidgets/kfilecustomdialog.cpp src/filewidgets/kfilecustomdialog.h tests/CMakeLists.txt tests/kfilecustomdialogtest_gui.cpp To: mlaurent, mwolff, dfaure Cc: #frameworks
D9206: Implement a kfile dialog where we can add custom widget
mlaurent marked 2 inline comments as done. REPOSITORY R241 KIO BRANCH add_kfile_dialog REVISION DETAIL https://phabricator.kde.org/D9206 To: mlaurent, mwolff, dfaure Cc: #frameworks
D9206: Implement a kfile dialog where we can add custom widget
dfaure accepted this revision.
dfaure added inline comments.
This revision is now accepted and ready to land.
INLINE COMMENTS
> CMakeLists.txt:41
>
> +
> )
nothing to see here
> kfilecustomdialog.cpp:65
> +
> +void KFileCustomDialogPrivate::setUrl(const QUrl )
> +{
I wonder about the usefulness of these indirection methods, the only caller
could do d->mFileWidget->setUrl(url), but well. Your choice :)
(same for the 3 that follow, I don't see much benefit, but OK I don't see much
harm either)
> kfilecustomdialog.h:61
> + * @brief fileWidget
> + * @return the filewidget element
> + */
"element" seems weird here. Maybe something like this?
@return the filewidget used inside this dialog
REPOSITORY
R241 KIO
BRANCH
add_kfile_dialog
REVISION DETAIL
https://phabricator.kde.org/D9206
To: mlaurent, mwolff, dfaure
Cc: #frameworks
D9206: Implement a kfile dialog where we can add custom widget
mlaurent updated this revision to Diff 23908. mlaurent added a comment. - Fix doc, remove unused code, etc. REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D9206?vs=23514=23908 BRANCH add_kfile_dialog REVISION DETAIL https://phabricator.kde.org/D9206 AFFECTED FILES autotests/CMakeLists.txt autotests/kfilecustomdialogtest.cpp autotests/kfilecustomdialogtest.h src/filewidgets/CMakeLists.txt src/filewidgets/kfilecustomdialog.cpp src/filewidgets/kfilecustomdialog.h tests/CMakeLists.txt tests/kfilecustomdialogtest_gui.cpp To: mlaurent, mwolff, dfaure Cc: #frameworks
D9206: Implement a kfile dialog where we can add custom widget
mlaurent marked 7 inline comments as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D9206 To: mlaurent, mwolff, dfaure Cc: #frameworks
D9206: Implement a kfile dialog where we can add custom widget
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
D9206: Implement a kfile dialog where we can add custom widget
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 *>(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
D9206: Implement a kfile dialog where we can add custom widget
mlaurent added a comment. It's the problem that if class doesn't exist nobody will think to use it (or he will reimplement it). After that it's not a problem for me to put this code only in LO :) As you want :) REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D9206 To: mlaurent, mwolff, dfaure Cc: #frameworks
D9206: Implement a kfile dialog where we can add custom widget
mwolff added a comment. Most of this is just forwarding code from KFileWidget, so we could just use that directly? I mean if our LO integration is going to be the only user of this class, then maybe we should start by adding this code there and only upstream it if we think more people are going to use it? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D9206 To: mlaurent, mwolff, dfaure Cc: #frameworks
D9206: Implement a kfile dialog where we can add custom widget
mlaurent edited the test plan for this revision. mlaurent added reviewers: mwolff, dfaure. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D9206 To: mlaurent, mwolff, dfaure Cc: #frameworks
D9206: Implement a kfile dialog where we can add custom widget
mlaurent created this revision. Restricted Application added a project: Frameworks. Restricted Application added a subscriber: Frameworks. REPOSITORY R241 KIO BRANCH add_kfile_dialog REVISION DETAIL https://phabricator.kde.org/D9206 AFFECTED FILES autotests/CMakeLists.txt autotests/kfilecustomdialogtest.cpp autotests/kfilecustomdialogtest.h src/filewidgets/CMakeLists.txt src/filewidgets/kfilecustomdialog.cpp src/filewidgets/kfilecustomdialog.h tests/CMakeLists.txt tests/kfilecustomdialogtest_gui.cpp tests/kfilecustomdialogtest_gui.h To: mlaurent Cc: #frameworks
