D17595: Upstream Dolphin's file rename dialog

[David Faure]

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

INLINE COMMENTS

> renamefiledialog.cpp:67
> +{
> +    const QSize minSize = minimumSize();
> +    setMinimumSize(QSize(320, minSize.height()));

This returns an invalid size (because nobody called setMinimumSize at this point yet, and this is not to be confused with minimumSizeHint), i.e. this code uses a convoluted way to pass -1 as minimum height.

Just use setMinimumWidth(320) instead.

> renamefiledialog.cpp:119
> +        if (!items.first().isDir()) {
> +            const QString fileName = items.first().url().toDisplayString();
> +            QMimeDatabase db;

items.first().path() would be simpler and faster.
(that doesn't give the same result, but it's sufficient for calling suffixForFileName with it)

> renamefiledialog.cpp:121
> +            QMimeDatabase db;
> +            const QString extension = db.suffixForFileName(fileName.toLower());
> +

why toLower()? Should work without.

> renamefiledialog.cpp:143
> +        for (const KFileItem& item : qAsConst(d->items)) {
> +            const QString extension = db.suffixForFileName(item.url().toDisplayString().toLower());
> +

Same here, suffixForFileName(item.url().path()) --- and this should be done the same way in both places, e.g. no intermediate variable in either location.

> renamefiledialog.cpp:208
> +
> +    job->uiDelegate()->setAutoErrorHandlingEnabled(true);
> +

Does Dolphin really want that? I thought a GUI design principle in Dolphin was to avoid messageboxes and show errors in an embedded widget instead.

So I was expecting this dialog to emit an error signal so that the app can decide on how to handle errors, instead.

> renamefiledialog.cpp:222
> +        } else {
> +            // Assure that the new name contains exactly one # (or a connected sequence of #'s)
> +            const int first = newName.indexOf(QLatin1Char('#'));

s/Assure/Ensure/ ?

> renamefiledialog.cpp:246
> +{
> +    d->lineEdit->setFocus();
> +

Isn't it enough to focus the lineEdit in the constructor?

Doing it here in showEvent (without a test for event->spontaneous()) means that if you focus another widget, switch virtual desktops, and switch back, the focus will unexpectedly go to the lineedit again.

> meven wrote in renamefiledialog.cpp:60
> I am very happy to learn about this one.
> 
> This script cannot work with a single file it seems and most of the files in this directory are modified by the script.
> So I won't use it here.

You can always revert the changes to the other files ;)
(commit any other changes, run uncrustify, git commit --amend renamefiledialog.*, git checkout .)

Or do you mean it would really make the coding style inconsistent with the other files? I would be surprised that the difference would be major.

> renamefiledialog.h:47
> + * The dialog deletes itself when accepted or rejected.
> + */
> +class KIOWIDGETS_EXPORT RenameFileDialog : public QDialog

@since 5.62

> renamefiledialog.h:59
> +     */
> +    explicit RenameFileDialog(QWidget* parent, const KFileItemList& items);
> +    ~RenameFileDialog() override;

Generally the parent widget argument goes last.

> meven wrote in renamefiledialog.h:76
> I tried it but it did not work.

Yeah, it's more complicated inside namespaces.

REPOSITORY
  R241 KIO

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

To: meven, #frameworks, #dolphin, broulik, ngraham, dfaure
Cc: dfaure, sitter, mitchell, emmanuelp, ltoscano, bruns, meven, dhaumann, pino, kde-frameworks-devel, LeGast00n, michaelh, ngraham
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20190816/09e50103/attachment-0001.html>