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

INLINE COMMENTS

> renamefiledialog.cpp:215
> +    connect(job, &KJob::result, this, &QObject::deleteLater);
> +    connect(job, &KJob::error, this, &RenameFileDialog::slotError);
> +

This can't possibly work, KJob::error is a getter, not a signal.

Remove slotError, and emit error() in slotResult.

> meven wrote in renamefiledialog.cpp:119
> KFileItem has no path()  method, but here items.first().url().fileName() 
> should be equivalent.

Oops yes I meant items.first().url().path(). OK for fileName(), path() is 
faster but this code path isn't speed critical.

> meven wrote in renamefiledialog.cpp:121
> I guess the database doesn't work for JPEG file extension for instance.

Wrong, QMimeDatabase handles this correctly, extensions are case insensitive.

Proof:
$ kmimetypefinder5 -f foo.JPEG 
image/jpeg

> meven wrote in renamefiledialog.cpp:208
> Apparently here it did use the default error dialog.
> I have changed this to expose an error signal to let the class user handle it.
> I wonder if exposing a setAutoErrorHandling setter for this dialog would be a 
> good idea.

Good question, I don't know. I'd say, let the first person who needs it, add it 
:)

> dfaure wrote in renamefiledialog.cpp:246
> 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.

I see you used the more complicated solution of keeping showEvent and testing 
for spontaneous. Any reason? It didn't work from the constructor?

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

Reply via email to