WIP | Added BatchMoveJob
Needs ReviewPublic
Actions

Authored by emateli on Mar 1 2020, 12:02 PM.

Details

Reviewers

dfaure
ngraham

Summary

Adds a BatchMoveJob class which enables moving multiple files in a single operation.

Looking for comments/thoughts. Regarding implementation, I took a look at batchrenamejob so I did something similar with multiple hidden sub-jobs. Some of the methods such as slotstart/slotresult are adapted from batchrenamejob. Should I include the original copyright or is that not necessary.

Also looking for some thoughts on the test suite with respect to the undo. The last test I wanted to do is to undo the move so that I can verify all files have been moved back to their original locations by calling a single undo.

Issues that depend on this: D14631

Diff Detail

Repository

R241 KIO

Branch

batchmove

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 23118
Build 23136: arc lint + arc unit

emateli created this revision.Mar 1 2020, 12:02 PM

Restricted Application added a project: Frameworks. · View Herald TranscriptMar 1 2020, 12:02 PM

Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript

emateli requested review of this revision.Mar 1 2020, 12:02 PM

Harbormaster completed remote builds in B23118: Diff 76723.Mar 1 2020, 12:02 PM

emateli retitled this revision from Added BatchMoveJob to WIP | Added BatchMoveJob.Mar 1 2020, 12:08 PM

emateli edited the summary of this revision. (Show Details)

emateli added reviewers: dfaure, ngraham.

What's the difference to the job you get with KIO::move with a list of urls? https://api.kde.org/frameworks/kio/html/namespaceKIO.html#a20da0b04660f834de30f9440673f6d5e

In that it supports moving them to different directories but as well as moving them with a different name. This operation is to allow also batch renaming of items. See this patch for more details: D14631

eg: BatchMove {a.txt -> Sample.txt, b.md -> new/Readme.md}. In the context of the mentioned patch the directory wouldn't change of course, but this is also a more generic move job. Move these files from here to somewhere else in a single job (also so that a single undo reverts everything back to the original place).

The general concept seems sane to me, but let's make sure @dfaure agrees.

Looks good, just a few things.

autotests/batchmovejobtest.cpp
91	Use newer syntax &KIO::BatchMoveJob::fileMoved?
104	This reads really scary! :-) Can you rename the var to what it is, like m_homeTmpDir? And this can be re-enabled, no? Alternatively, use QTemporaryDir, it cleans up automatically.
src/core/batchmovejob.cpp
104	prefer prefix increment (++ in front)
src/core/batchmovejob.h
37	Where is this ctor needed? Both could just be removed, then you can use aggregate initialization with {}
50	typo: simultaneously (bulk purchase of the 'o' letter?) :)
52	5.69
src/widgets/fileundomanager.h
140	5.69

emateli added inline comments.Apr 3 2020, 8:06 AM

autotests/batchmovejobtest.cpp
89	@dfaure How do I correctly register this job so I can undo it in one operation (In the undo call a few lines below)

emateli mentioned this in D14631: Adds a new RenameDialog to KIO with more options for batch renaming.Jul 5 2020, 6:01 PM

luco added a subscriber: luco.Jul 9 2020, 2:38 PM

Revision Contents
Changeset List

		Path
M		autotests/CMakeLists.txt (1 line)
A	M	autotests/batchmovejobtest.cpp (111 lines)
M		src/core/CMakeLists.txt (1 line)
A	M	src/core/batchmovejob.h (80 lines)
A	M	src/core/batchmovejob.cpp (116 lines)
M		src/widgets/fileundomanager.h (4 lines)