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

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

Details

Reviewers
dfaure
mlaurent
meven
Group Reviewers
Frameworks
Dolphin
Summary

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

Repository
R241 KIO
Branch
batchrename2
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 4193
Build 4211: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
mlaurent requested changes to this revision.Oct 26 2018, 5:14 AM
mlaurent added inline comments.
src/widgets/rename/batchrenamedialog.cpp
41

remove QtWidget

42

remove QtGui prefix

47

new line before ": QDialog"

src/widgets/rename/batchrenamedialog.h
70

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

src/widgets/rename/batchrenamedialogmodel_p.cpp
89

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?

src/widgets/rename/batchrenamedialog.h
70

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
src/widgets/rename/batchrenamedialog.h
70

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.
src/widgets/rename/batchrenamedialog.cpp
39

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.

260

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

src/widgets/rename/batchrenamedialog.h
28

Forward declaration would be enough (class QCheckBox).

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

43

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

67

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...

src/widgets/rename/batchrenamedialogmodel_p.cpp
31

itemData.reserve(items.count());

41

remove this line, it's a lie :)

src/widgets/rename/batchrenametypes_p.h
21

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

33

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.

36

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

src/widgets/rename/batchrenamevar_p.h
26

not used here, remove

src/widgets/rename/filenameutils_p.h
21

_P_H

tests/batchrenamedialogtest_gui.cpp
33

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 https://lxr.kde.org/search?_filestring=&_string=BatchRenameJob 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 lxr.kde.org 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.

src/core/batchrenamejob.cpp
45 ↗(On Diff #47694)

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

115 ↗(On Diff #47694)

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).

src/core/batchrenamejob.h
31 ↗(On Diff #47694)

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

94 ↗(On Diff #47694)

Needs to be documented + @since 5.54

QList of something bigger than the size of a pointer is a bad idea (see e.g. https://marcmutz.wordpress.com/effective-qt/containers/). Please make it a QVector.

src/widgets/rename/batchrenamedialog.cpp
44

remove KI18n prefix, this isn't a namespaced header

54

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.

77

QVector

340

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).

src/widgets/rename/batchrenamedialogmodel.cpp
31 ↗(On Diff #47694)

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

src/widgets/rename/batchrenamedialogmodel_p.h
21

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

44

QVector

src/widgets/rename/batchrenametypes.cpp
33 ↗(On Diff #47694)

static

(and can this global variable be avoided?)

src/widgets/rename/batchrenamevar_p.h
22

_P_H

tests/batchrenamedialogtest_gui.cpp
33

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.
src/widgets/rename/batchrenametypes.cpp
98–106 ↗(On Diff #47694)

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!

Before continuing work I'd like to hear some opinions on this.

  1. From @dfaure or other people at frameworks: How do we handle batch rejame jobs. Should I create a new patch to submit a basic batch move job that simply moves files around without any additional processing, that way it can also serve as a generic batch move job(I have written in previous comments more on this how I think it should look like). As I don't think going back to doing many single move jobs is the best of ideas. Also this patch is getting way too big and difficult to review.
  2. From everyone else (also pinging @ngraham) since we're doing this and the code sans the KIO batch stuff is almost done, are there any additional features we might want to take into consideration? E.g: File metadata?

KIO::move (implemented by CopyJob) can move N files to a single destination directory, but they get the same name at that dest.
KIO::moveAs (implemented by CopyJob too) can move/rename a single file to a specific filename at destination.

What you're suggesting is a new job that can move N files to a destination directory (to make this generic it doesn't have to be in-place renames, right?), but you're providing the filename at destination for each file, right?
This sounds good to me. I could imagine this being handled by CopyJob too.
The struct CopyInfo already stores source and destination URLs for every file, it's just that the destination URL is filled in CopyJobPrivate::slotResultStating by appending the filename to the dest dir, except in moveAs (m_asMethod==true) where the destination URL *is* the final filename.
I could imagine a KIO::moveAs that takes two QList<QUrl> and then this information is fetched from there rather than using m_dest.
In fact, if the existing moveAs() method is ported to call the two-QLists one, that will mean less special casing in the code (which wouldn't use m_dest anymore in slotResultStating, when m_asMethod).

KIO::move (implemented by CopyJob) can move N files to a single destination directory, but they get the same name at that dest.
KIO::moveAs (implemented by CopyJob too) can move/rename a single file to a specific filename at destination.

What you're suggesting is a new job that can move N files to a destination directory (to make this generic it doesn't have to be in-place renames, right?), but you're providing the filename at destination for each file, right?

More or less but they don't have to be in the same directory. Think of it as a sequence of KIO::moveAs operations. Any N files can be moved anywhere.

I could imagine a KIO::moveAs that takes two QList<QUrl> and then this information is fetched from there rather than using m_dest.
In fact, if the existing moveAs() method is ported to call the two-QLists one, that will mean less special casing in the code (which wouldn't use m_dest anymore in slotResultStating, when m_asMethod).

The two list version could work, but I was thinking of one QList<QSomeStruct> that contains src and dest names. Looks less error prone IMO. Also yes, moveAs can be implemented as a special case of this with just one item to move. The option of adapting move to accept a list of dest also involves modifying CopyJobPrivate's dest which will lead to a larger refactoring needed than the proposal below, right?


I was thinking of implementing this as a new subclass of Job where it will create the new Job and add a subjob for each of the files to be moved. In the same fashion that the current BatchRenameJob works. Otherwise if overloading will not break binary compatibility then this can be an overloaded KIO::moveAs(QList<SomeStruct>) which again does the same thing, a series of the current moveAs. It also has to remain as a single job so that it can be undone in one go instead of undoing for each item that was renamed.

e.g:

auto items = {
        KioRenameItem{QUrl("~/a.doc"), QUrl("~/Documents/a.doc")},
        KioRenameItem{QUrl("~/dir/file"), QUrl("~/Documents/file-2019")},
};

If this looks OK then I'll just get started with this and continue this part of the work in another patch.

More or less but they don't have to be in the same directory. Think of it as a sequence of KIO::moveAs operations. Any N files can be moved anywhere.

Right.

I could imagine a KIO::moveAs that takes two QList<QUrl> and then this information is fetched from there rather than using m_dest.
In fact, if the existing moveAs() method is ported to call the two-QLists one, that will mean less special casing in the code (which wouldn't use m_dest anymore in slotResultStating, when m_asMethod).

The two list version could work, but I was thinking of one QList<QSomeStruct> that contains src and dest names. Looks less error prone IMO.

Yes, or that. Might be more tedious to fill in, I don't know. It's ok with me in any case.

I was thinking of implementing this as a new subclass of Job where it will create the new Job and add a subjob for each of the files to be moved.

Like all composite jobs, yes.

This is less efficient, though, in the case where the dest dir is the same for 500 files because EACH moveAs will :

  • stat() the dest dir
  • check for enough free space at destination

whereas this could be done only if the dest dir is different from the previous one.

Also, user interaction like "skip all" or "overwrite all" won't be possible, because that's within a single CopyJob. If you start 500 copyjobs, this information won't be shared.

overloading will not break binary compatibility

That's correct, it doesn't.
It just has to be non-ambiguous for existing code, but that's fine for the current proposals.

then this can be an overloaded KIO::moveAs(QList<SomeStruct>)

If that doesn't return a CopyJob like the other moveAs method, it'll be a bit confusing in itself IMHO.

It also has to remain as a single job so that it can be undone in one go instead of undoing for each item that was renamed.

Yes, that part is clear and agreed upon.

auto items = {
        KioRenameItem{QUrl("~/a.doc"), QUrl("~/Documents/a.doc")},
        KioRenameItem{QUrl("~/dir/file"), QUrl("~/Documents/file-2019")},
};

QUrl doesn't support ~ and needs fromLocalFile() when built from local paths, but I get the idea :-)

Maybe the struct could be named KIO::MoveItem ?
Generally we call it renaming when it's in the same directory and moving for the general case (same or different directory).

The option of adapting move to accept a list of dest also involves modifying CopyJobPrivate's dest which will lead to a larger refactoring needed than the proposal below, right?

CopyJobPrivate's m_dest already changes over time (when copying dirs it has to recurse into subdirs at dest). Only m_globalDest is currently fixed, and would have to change over time now.
And when it changes, we need to go back to state DEST_NOT_STATED and the "Stat the dest" code, to stat the new dest (to check that it exists and is a dir, check for free space).

IMHO it's not a question of which is the bigger amount of work, but which one will work better. Support for "Overwrite all" requires that this is done within CopyJob.

meven added a subscriber: meven.Aug 28 2019, 7:19 PM

This could be an alternative to D17595

I spent some time with modifying CopyJob but didn't make much progress and it's too deep into KIO internals - a lot one has to familiarize himself with. Maybe it's best someone who already did work on KIO to add the batch stuff. Once that is done and merged we can resume over here.

@emateli are you going to be able to finish this? Should someone take over? It would be a shame for such great work to get stalled so close to the finish line.

emateli added a comment.EditedFeb 28 2020, 9:25 AM

Well, this patch is more or less complete but it doesn't make sense to be deployed without batch operations from KIO as it would spawn a job for each file to be renamed.

Like I stated in my last comment, I tried to give adding batch renaming a go but it felt like I needed some time to familiarize myself with kio internals first. So to save everyone time, I was hoping someone who has worked KIO more extensively is willing to implement the batch rename stuff since he'll be a lot faster than I. Once that is merged we can resume work here.

That said I'll be giving the batch stuff another try.

Edit: Will soon send a PR to KIO for the batch jobs.

nicopons added a subscriber: nicopons.
luco added a subscriber: luco.Jul 5 2020, 5:46 PM

@emateli Are you still working on this? I'd love to see this feature implemented, I wanted to start by myself then on the Matrix Dolphin channel they suggested seeing this patch and it's awesome! Could you use some help?

I honestly think the GUI is mostly ready, at least what I wanted to achieve with it: Something that is advanced enough but not over the top with options as there's already tools for that. Will check at some point to finish the items left from the review here. What this needs to be fully implemented is support from KIO for a "proper" batch move operation. I've submitted a patch for that at D27760 but am still waiting for a review.

I'm completely open to new ideas though.

luco added a comment.Jul 5 2020, 6:13 PM

@emateli Thank you for your response, I saw that patch and I sincerely don't understand the reason why you're writing a completely different job (BatchMoveJob) when the required operation is exactly a BatchRenameJob.
I'm asking this because I've never used a "batch rename" feature to move files to a different folder, I always used that to rename multiple files with a regex pattern or a numbering sequence instead, but maybe it's just me.

Do you think an initial implementation of your GUI with the standard BatchRenameJob could work? What problems could appear? I see the usefulness of the BatchMoveJob, but I can't see how is that related to this patch!

emateli added a comment.EditedJul 5 2020, 6:27 PM

You're completely right in the sense that for a batch rename files will be in the same folder. However, rename really is just a move operation. The current BatchRenameJob has a hardcoded logic about placeholders and extensions which make it unusable outside of its current scope.

The proposed BatchMoveJob does exactly that: Move these files from here to there.

In the context of this job it Wil be used to rename files, but as a patch it is completely independent of this. Applications such as Dolphin or Krusader for example, can also use it to do their batch moves instead of implementing it themselves with multiple subjobs etc.

luco added a comment.Jul 6 2020, 8:34 AM

@emateli Thank you, I do understand that BatchMoveJob could be more versatile for a various kind of operations, but couldn't it be easier to just edit the BatchRenameJob's hardcoded logic and make it more parametric?
If I understand correctly you're referring to the fact that the ctor of BatchRenameJob is accepting a QChar and it only substitutes it with numbers, wouldn't be easier to edit that logic instead? I'm asking because I think you already have considered this option and concluded that it was not worth it

dfaure added a comment.Jul 6 2020, 9:02 AM

Only if you can find a way to change BatchRenameJob in a binary and behaviour compatible way. And then it will be a dual-headed thing with two modes of operations, awful. All this sounds to me like much more trouble than writing a different job.