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
emateli added inline comments.Sep 25 2018, 8:22 PM
src/widgets/rename/batchrenametypes.h
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.

install(FILES

${KIOWidgets_HEADERS}
${CMAKE_CURRENT_BINARY_DIR}/kiowidgets_export.h
DESTINATION ${KDE_INSTALL_INCLUDEDIR_KF5}/KIOWidgets COMPONENT Devel)

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