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
Group Reviewers
Frameworks
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 6090
Build 6108: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
mlaurent added inline comments.Oct 25 2018, 5:18 AM
src/widgets/CMakeLists.txt
65

fix indent

src/widgets/rename/batchrenamedialog.cpp
49

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

61

add parent ?

173

add parent here too

src/widgets/rename/batchrenamevar_p.h
26

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

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
30 ↗(On Diff #44938)

itemData.reserve(items.count());

40 ↗(On Diff #44938)

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

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

115

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

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

94

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
43

remove KI18n prefix, this isn't a namespaced header

53

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.

76

QVector

339

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

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

src/widgets/rename/batchrenamedialogmodel_p.h
20

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

43

QVector

src/widgets/rename/batchrenametypes.cpp
33

static

(and can this global variable be avoided?)

src/widgets/rename/batchrenamevar_p.h
21

_P_H

tests/batchrenamedialogtest_gui.cpp
32

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

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.Wed, Aug 28, 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.