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
