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