Adds a new RenameDialog to KIO with more options for batch renaming
Changes PlannedPublic

Authored by emateli on Aug 5 2018, 4:59 PM.


Group Reviewers

This is a contiunation of D10698.

This patch adds some new tools to the rename functionality in KIO. The idea behind this is to add some basic batch renaming capabilities to KIO for use in KDE Applications.

BUG: 381483
FEATURE: 371383
FEATURE: 395405

Diff Detail

R241 KIO
No Linters Available
No Unit Test Coverage
Build Status
Buildable 6090
Build 6108: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
emateli added inline comments.Sep 25 2018, 8:22 PM
32 ↗(On Diff #39137)

I tried the instance route but then I wouldn't be able to pass the method as a callback. Current way is not ideal either. Will figure something out.

@aacid Would be great if you'd point me towards making them installable. Only the dialog itself should be exported.



in src/widgets/CMakeLists.txt

emateli updated this revision to Diff 44189.Oct 24 2018, 10:32 PM
  • use _p notation. Add BatchRenameDialog to installable headers

Besides any current review, the last step for this patch would be to actually use the BatchRenameJob already in KIO. Regarding that: Should I put it into a separate patch and make this depend on it or continue working here?

It makes sense to me to have that in this same patch. Pretty awesome stuff here.

mlaurent requested changes to this revision.Oct 25 2018, 5:18 AM
mlaurent added a subscriber: mlaurent.
mlaurent added inline comments.

fix indent


new QVBoxLayout(this);
and remove setLayout(mainLayout);


add parent ?


add parent here too


not necessary to add QtCore

This revision now requires changes to proceed.Oct 25 2018, 5:18 AM
emateli updated this revision to Diff 44217.Oct 25 2018, 4:28 PM
  • fix indent
  • remove setlayout call
  • Add missing parent parameters
  • Remove unnecessary QtCore/ prefix
emateli updated this revision to Diff 44218.Oct 25 2018, 4:31 PM
emateli marked 3 inline comments as done.
  • Remove QtCore/
emateli marked 2 inline comments as done.Oct 25 2018, 4:42 PM

Something that might need a bit of input is BatchRenameJob.

  • This was added a while back so that Dolphin doesn't do a batch rename as a series of single file renames.
  • This job takes care of replacing the # placeholder as well.
  • What this patch really wants is an API to rename a list of files, into some new names without doing any additional handling except moving the files (as the placeholder replacement is done elsewhere).
  • By consulting we can see that Dolphin is the only application to make use of this BatchRenameJob API.
  • Leaving credit to the original author, since the # placeholder will not be used anymore. Should we re-purpose this class to accept two lists, one with the current names, the other with the new ones and remove the original placeholder implementation (as it would be mostly dead code at this point) while keeping everything else intact.
mlaurent requested changes to this revision.Oct 26 2018, 5:14 AM
mlaurent added inline comments.

remove QtWidget


remove QtGui prefix


new line before ": QDialog"


it's a good idea to initialize to nullptr each pointer here.
So we can see problem easily.

88 ↗(On Diff #44218)

new line before : QAbstr...

This revision now requires changes to proceed.Oct 26 2018, 5:14 AM
emateli updated this revision to Diff 44737.Nov 2 2018, 11:05 PM
emateli marked 4 inline comments as done.
  • coding style fix

Also, thoughts on my previous comments regarding BatchRenameJob?


You mean to initialize them to nullptr on the header file or using a constructor initialization list? Also see what problem?

mlaurent added inline comments.Nov 3 2018, 8:32 AM

in header file.

emateli updated this revision to Diff 44938.Nov 5 2018, 10:19 PM
  • Initialize pointers to nullptr
  • Code style fix

Thoughts on proceeding with the proposed changes to batchrenamejob? Also, the filenameutils namespace feels like duplicated work. Any existing solution available to use? It uses QMimeDatabase to look up the extension and if its not found, then it uses the suffix from QFileInfo

For me seems good +1

For me seems good +1

You still have a "Request changes" status for this patch. If you're happy with it now, can you change that to "Accepted? Thanks!

mlaurent accepted this revision.Nov 8 2018, 9:41 AM
dfaure requested changes to this revision.Nov 10 2018, 10:16 PM
dfaure added inline comments.

Remove all the framework prefixes. E.g. this should be <KIO/Job>.

This way, it fails to find the include if the include path (which comes from linking to an imported target, in cmake) for the dependency is missing.


My comment still stands: it makes no sense to test for error() before the job's result signal is emitted.

The comment was marked as done, but there's still no connect to result


Forward declaration would be enough (class QCheckBox).

Same for anything used by pointer (or ref) in this header.


If you export it in an installed header, you need to document it.


If you make this class public, you need to move all members to a Private class, and keep only a "d" pointer here. See all other public classes...

30 ↗(On Diff #44938)


40 ↗(On Diff #44938)

remove this line, it's a lie :)


doesn't match the name of the header, please adjust


why exported? for unit tests? if so, add a comment. If not, remove.

But anyhow this isn't a class, everything is static. To avoid generating a constructor and a destructor, make it a namespace (like you did for BatchRenameVar) and (if needed) export the individual functions.


move to .cpp file, it's not needed here at all


not used here, remove




create it on stack to avoid leaking it

This revision now requires changes to proceed.Nov 10 2018, 10:16 PM
emateli updated this revision to Diff 45438.Nov 13 2018, 11:28 PM
emateli marked 9 inline comments as done.
  • create on stack
  • upd define
  • remove unused import
  • use qstringliteral
  • remove unused export
  • match file name
  • remove import prefixes
  • remove q_unused on used parameter
  • use fwd class declaration
  • reserve space for items
  • Do not use _p for cpp files
  • simplify expressions
  • add cpp files w/o _p
  • optimize imports

@dfaure, how's this looking now?

emateli marked 3 inline comments as done.Dec 14 2018, 7:29 PM

There's still some work to do with the actual BatchRenameClass now and the d-pointer pattern. I'll update this in a few days.

emateli updated this revision to Diff 47694.Dec 16 2018, 8:40 PM
  • convert to namespace from class
  • do not show first captured group
  • remove help button
  • Use d-ptr pattern
  • remove show/hide methods and autoshow dialog
  • update doc
  • KFileItemList -> QList<QUrl>; KFileItem usage was unnecessary
  • update doc
  • WIP Change BatchRenameJobPrivate class

This is still a WIP mostly with the use of d-ptr pattern for the exported header and the conversion of BatchRenameJob from a class with a very specific purpose to a general-purpose batch rename operation.

Looking for comments especially for the repurposing of the BatchRenameJob class(currently used only by Doplhin) and other thoughts in general as well.

Pinging @chinmoyr as the original author of the BatchRenameJob class.

It might not be the worst thing in the world if you made this a general-purpose rename dialog, so that it handles the simple common case of renaming one items as well as the complicated case where multiple items are renamed. If you do that, I'll abandon D17595.

dfaure requested changes to this revision.Dec 16 2018, 10:54 PM
"By consulting we can see that Dolphin is the only application to make use of this BatchRenameJob API."

This isn't a valid reason to break BatchRenameJob. KF5 makes the promise to not break API/ABI for third-party applications, which wouldn't know about.
Sorry, if I had realized that this meant "break existing API" I would have objected earlier to this line of thought.


I suggest m_itemsIterator so it's independent from the type (which will be QVector instead of a QList)


So this breaks the existing functionality completely !?!?!

The new stuff should rather be a whole different job class, leaving old API untouched (deprecated, and to be removed in KF6).


Needs to be documented since it's public API, including @since 5.54


Needs to be documented + @since 5.54

QList of something bigger than the size of a pointer is a bad idea (see e.g. Please make it a QVector.


remove KI18n prefix, this isn't a namespaced header


This is very unusual. Any reason why the public class isn't the one that inherits from QDialog?

This would allow the caller to call show(), instead of show() happening automatically. Just for consistency with all other dialogs.




Better connect to the result() signal of the job and do the rest of btnOkClicked in the slot (or lambda) connected to that signal.
exec() opens many potential issues (re-entrancy).


Why is this a pointer instead of just a value member?


append _P_H, to avoid a clash with a public header of the same name





(and can this global variable be avoided?)




You can remove the = {} which serves no purpose.

This revision now requires changes to proceed.Dec 16 2018, 10:54 PM
emateli planned changes to this revision.Jan 15 2019, 6:33 PM

Any news on this @emateli? I'd really love to see it in Dolphin!

I'll need to rewrite the BatchRenameJob part which does the batch processing without breaking binary compatibility. Been unusually busy lately between job and uni but I'll start working on this sooner than later.

anthonyfieroni added inline comments.

Make a class instead, free functions with global variable in, is no-go. It's totally miss of encapsulation.

No problem, thanks for checking in again!