D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[David Faure]

dfaure added a comment.


  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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent, meven, #dolphin
Cc: luco, nicopons, meven, chinmoyr, mlaurent, asensi, rkflx, dfaure, aacid, 
ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Luca Errani]

luco added a comment.


  @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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent, meven, #dolphin
Cc: luco, nicopons, meven, chinmoyr, mlaurent, asensi, rkflx, dfaure, aacid, 
ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli added a comment.


  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, no questions asked and without any manipulation of the file names.
  
  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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent, meven, #dolphin
Cc: luco, nicopons, meven, anthonyfieroni, chinmoyr, mlaurent, asensi, rkflx, 
dfaure, aacid, ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Luca Errani]

luco added a comment.


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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent, meven, #dolphin
Cc: luco, nicopons, meven, anthonyfieroni, chinmoyr, mlaurent, asensi, rkflx, 
dfaure, aacid, ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli added a comment.


  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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent, meven, #dolphin
Cc: luco, nicopons, meven, anthonyfieroni, chinmoyr, mlaurent, asensi, rkflx, 
dfaure, aacid, ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Luca Errani]

luco added a comment.


  @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?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent, meven, #dolphin
Cc: luco, nicopons, meven, anthonyfieroni, chinmoyr, mlaurent, asensi, rkflx, 
dfaure, aacid, ngraham, kde-frameworks-devel, LeGast00n, cblack, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli added a comment.


  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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent, meven, #dolphin
Cc: meven, anthonyfieroni, chinmoyr, mlaurent, asensi, rkflx, dfaure, aacid, 
ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Nathaniel Graham]

ngraham added a reviewer: meven.
ngraham added a comment.


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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent, meven
Cc: meven, anthonyfieroni, chinmoyr, mlaurent, asensi, rkflx, dfaure, aacid, 
ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Nathaniel Graham]

ngraham added a reviewer: Dolphin.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent, meven, #dolphin
Cc: meven, anthonyfieroni, chinmoyr, mlaurent, asensi, rkflx, dfaure, aacid, 
ngraham, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli added a comment.


  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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: meven, anthonyfieroni, chinmoyr, mlaurent, asensi, rkflx, dfaure, aacid, 
ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Méven Car]

meven added a comment.


  This could be an alternative to D17595 

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: meven, anthonyfieroni, chinmoyr, mlaurent, asensi, rkflx, dfaure, aacid, 
ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Nathaniel Graham]

ngraham added a comment.


  @emateli Ping :)

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: anthonyfieroni, chinmoyr, mlaurent, asensi, rkflx, dfaure, aacid, ngraham, 
kde-frameworks-devel, LeGast00n, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[David Faure]

dfaure added a comment.


  In D14631#499600 , @emateli wrote:
  
  > 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 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` 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)`
  
  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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: anthonyfieroni, chinmoyr, mlaurent, asensi, rkflx, dfaure, aacid, ngraham, 
kde-frameworks-devel, LeGast00n, sbergeron, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli added a comment.


  In D14631#499038 , @dfaure wrote:
  
  > 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 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` 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)` 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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: anthonyfieroni, chinmoyr, mlaurent, asensi, rkflx, dfaure, aacid, ngraham, 
kde-frameworks-devel, LeGast00n, sbergeron, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[David Faure]

dfaure added a comment.


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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: anthonyfieroni, chinmoyr, mlaurent, asensi, rkflx, dfaure, aacid, ngraham, 
kde-frameworks-devel, LeGast00n, sbergeron, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli added a comment.


  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?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: anthonyfieroni, chinmoyr, mlaurent, asensi, rkflx, dfaure, aacid, ngraham, 
kde-frameworks-devel, LeGast00n, sbergeron, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Nathaniel Graham]

ngraham added a comment.


  Ping. :)

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: anthonyfieroni, chinmoyr, mlaurent, asensi, rkflx, dfaure, aacid, ngraham, 
kde-frameworks-devel, LeGast00n, sbergeron, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Nathaniel Graham]

ngraham added a comment.


  No problem, thanks for checking in again!

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: anthonyfieroni, chinmoyr, mlaurent, asensi, rkflx, dfaure, aacid, ngraham, 
kde-frameworks-devel, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> batchrenametypes.cpp:98-106
> +QStringList firstCapturedGroups()
> +{
> +return capturedGroups;
> +}
> +
> +void clearCapturedGroups()
> +{

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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: anthonyfieroni, chinmoyr, mlaurent, asensi, rkflx, dfaure, aacid, ngraham, 
kde-frameworks-devel, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli added a comment.


  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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: chinmoyr, mlaurent, asensi, rkflx, dfaure, aacid, ngraham, 
kde-frameworks-devel, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Nathaniel Graham]

ngraham added a comment.


  Ping!

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: chinmoyr, mlaurent, asensi, rkflx, dfaure, aacid, ngraham, 
kde-frameworks-devel, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: chinmoyr, mlaurent, asensi, rkflx, dfaure, aacid, ngraham, 
kde-frameworks-devel, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli planned changes to this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: chinmoyr, mlaurent, asensi, rkflx, dfaure, aacid, ngraham, 
kde-frameworks-devel, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[David Faure]

dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.


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

INLINE COMMENTS

> batchrenamejob.cpp:45
> +QList m_items;
> +QList::const_iterator m_listIterator;
>  const JobFlags m_flags;

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

> batchrenamejob.cpp:115
>  {
> -return BatchRenameJobPrivate::newJob(src, newName, index, placeHolder, 
> flags);
> +return batchRenameFiles({}, flags);
> +}

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

> batchrenamejob.h:31
>  
> +struct KioBatchRenameItem {
> +QUrl src;

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

> batchrenamejob.h:94
>  
> +KIOCORE_EXPORT BatchRenameJob *batchRenameFiles(const 
> QList , JobFlags flags = DefaultFlags);
> +

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.

> batchrenamedialog.cpp:43
> +#include 
> +#include 
> +#include 

remove KI18n prefix, this isn't a namespaced header

> batchrenamedialog.cpp:53
> +
> +class BatchRenameDialogPrivate : public QDialog
> +{

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.

> batchrenamedialog.cpp:76
> +
> +QList> m_itemsToBeRenamed;
> +QList m_renamedItems;

QVector

> batchrenamedialog.cpp:339
> +
> +job->exec();
> +}

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

> batchrenamedialogmodel.cpp:31
> +{
> +itemData = new QList;
> +itemData->reserve(items.count());

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

> batchrenamedialogmodel_p.h:20
> +
> +#ifndef KIO_BATCHRENAMEDIALOGMODEL
> +#define KIO_BATCHRENAMEDIALOGMODEL

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

> batchrenamedialogmodel_p.h:43
> +private:
> +QList *itemData;
> +

QVector

> batchrenametypes.cpp:33
> +
> +QStringList capturedGroups;
> +

static

(and can this global variable be avoided?)

> batchrenamevar_p.h:21
> +
> +#ifndef KIO_BATCHRENAMEVAR_H
> +#define KIO_BATCHRENAMEVAR_H

_P_H

> batchrenamedialogtest_gui.cpp:32
> +
> +QList items = {};
> +

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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: chinmoyr, mlaurent, asensi, rkflx, dfaure, aacid, ngraham, 
kde-frameworks-devel, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Nathaniel Graham]

ngraham added a comment.


  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 
.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: chinmoyr, mlaurent, asensi, rkflx, dfaure, aacid, ngraham, 
kde-frameworks-devel, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli added a subscriber: chinmoyr.
emateli added a comment.


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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: chinmoyr, mlaurent, asensi, rkflx, dfaure, aacid, ngraham, 
kde-frameworks-devel, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli updated this revision to Diff 47694.
emateli added a comment.


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

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14631?vs=45438=47694

BRANCH
  batchrename2

REVISION DETAIL
  https://phabricator.kde.org/D14631

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/batchrenametypestest.cpp
  autotests/batchrenametypestest.h
  src/core/batchrenamejob.cpp
  src/core/batchrenamejob.h
  src/widgets/CMakeLists.txt
  src/widgets/rename/batchrenamedialog.cpp
  src/widgets/rename/batchrenamedialog.h
  src/widgets/rename/batchrenamedialogmodel.cpp
  src/widgets/rename/batchrenamedialogmodel_p.h
  src/widgets/rename/batchrenametypes.cpp
  src/widgets/rename/batchrenametypes_p.h
  src/widgets/rename/batchrenamevar.cpp
  src/widgets/rename/batchrenamevar_p.h
  src/widgets/rename/filenameutils.cpp
  src/widgets/rename/filenameutils_p.h
  tests/CMakeLists.txt
  tests/batchrenamedialogtest_gui.cpp
  tests/previewtest.cpp

To: emateli, #frameworks, dfaure, mlaurent
Cc: mlaurent, asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, 
michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli marked 3 inline comments as done.
emateli added a comment.


  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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: mlaurent, asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, 
michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Nathaniel Graham]

ngraham added a comment.


  @dfaure, how's this looking now?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: mlaurent, asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, 
michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli updated this revision to Diff 45438.
emateli marked 9 inline comments as done.
emateli added a comment.


  - 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

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14631?vs=44938=45438

BRANCH
  batchrename2

REVISION DETAIL
  https://phabricator.kde.org/D14631

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/batchrenametypestest.cpp
  autotests/batchrenametypestest.h
  src/widgets/CMakeLists.txt
  src/widgets/rename/batchrenamedialog.cpp
  src/widgets/rename/batchrenamedialog.h
  src/widgets/rename/batchrenamedialogmodel.cpp
  src/widgets/rename/batchrenamedialogmodel_p.h
  src/widgets/rename/batchrenametypes.cpp
  src/widgets/rename/batchrenametypes_p.h
  src/widgets/rename/batchrenamevar.cpp
  src/widgets/rename/batchrenamevar_p.h
  src/widgets/rename/filenameutils.cpp
  src/widgets/rename/filenameutils_p.h
  tests/CMakeLists.txt
  tests/batchrenamedialogtest_gui.cpp

To: emateli, #frameworks, dfaure, mlaurent
Cc: mlaurent, asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, 
michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> batchrenamedialog.cpp:38
> +#include 
> +#include 
> +#include 

Remove all the framework prefixes. E.g. this should be .

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.

> batchrenamedialog.cpp:259
> +
> +if (!job->error()) {
> +m_renamedItems << newUrl;

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`

> batchrenamedialog.h:27
> +
> +#include 
> +#include 

Forward declaration would be enough (`class QCheckBox`).

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

> batchrenamedialog.h:42
> +
> +class KIOWIDGETS_EXPORT BatchRenameDialog : public QDialog
> +{

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

> batchrenamedialog.h:66
> +
> +QList> m_itemsToBeRenamed;
> +QList m_renamedItems;

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

> batchrenamedialogmodel_p.cpp:30
> +{
> +itemData = new QList;
> +

itemData.reserve(items.count());

> batchrenamedialogmodel_p.cpp:40
> +{
> +Q_UNUSED(parent);
> +

remove this line, it's a lie :)

> batchrenametypes_p.h:20
> +
> +#ifndef KIO_RENAMETYPES
> +#define KIO_RENAMETYPES

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

> batchrenametypes_p.h:32
> +
> +class KIOWIDGETS_EXPORT BatchRenameTypes
> +{

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.

> batchrenametypes_p.h:35
> +private:
> +static QStringList capturedGroups;
> +public:

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

> batchrenamevar_p.h:25
> +
> +#include 
> +#include 

not used here, remove

> filenameutils_p.h:20
> +
> +#ifndef KIO_FILENAMEUTILS
> +#define KIO_FILENAMEUTILS

_P_H

> batchrenamedialogtest_gui.cpp:32
> +
> +auto dlg = new KIO::BatchRenameDialog(nullptr, KFileItemList({e1, e2}));
> +dlg->show();

create it on stack to avoid leaking it

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: mlaurent, asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, 
michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Laurent Montel]

mlaurent accepted this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: mlaurent, asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, 
michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Nathaniel Graham]

ngraham added a comment.


  In D14631#355791 , @mlaurent wrote:
  
  > 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!

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: mlaurent, asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, 
michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Laurent Montel]

mlaurent added a comment.


  For me seems good +1

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: mlaurent, asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, 
michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli updated this revision to Diff 44938.
emateli added a comment.


  - 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

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14631?vs=44737=44938

BRANCH
  batchrename2

REVISION DETAIL
  https://phabricator.kde.org/D14631

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/batchrenametypestest.cpp
  autotests/batchrenametypestest.h
  src/widgets/CMakeLists.txt
  src/widgets/rename/batchrenamedialog.cpp
  src/widgets/rename/batchrenamedialog.h
  src/widgets/rename/batchrenamedialogmodel_p.cpp
  src/widgets/rename/batchrenamedialogmodel_p.h
  src/widgets/rename/batchrenametypes_p.cpp
  src/widgets/rename/batchrenametypes_p.h
  src/widgets/rename/batchrenamevar_p.cpp
  src/widgets/rename/batchrenamevar_p.h
  src/widgets/rename/filenameutils_p.cpp
  src/widgets/rename/filenameutils_p.h
  tests/CMakeLists.txt
  tests/batchrenamedialogtest_gui.cpp

To: emateli, #frameworks, dfaure, mlaurent
Cc: mlaurent, asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, 
michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Laurent Montel]

mlaurent added inline comments.

INLINE COMMENTS

> emateli wrote in batchrenamedialog.h:69
> You mean to initialize them to nullptr on the header file or using a 
> constructor initialization list? Also see what problem?

in header file.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: mlaurent, asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, 
michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli added a comment.


  Also, thoughts on my previous comments regarding `BatchRenameJob`?

INLINE COMMENTS

> mlaurent wrote in batchrenamedialog.h:69
> it's a good idea to initialize to nullptr each pointer here.
> So we can see problem easily.

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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: mlaurent, asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, 
michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli updated this revision to Diff 44737.
emateli marked 4 inline comments as done.
emateli added a comment.


  - coding style fix

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14631?vs=44218=44737

BRANCH
  batchrename2

REVISION DETAIL
  https://phabricator.kde.org/D14631

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/batchrenametypestest.cpp
  autotests/batchrenametypestest.h
  src/widgets/CMakeLists.txt
  src/widgets/rename/batchrenamedialog.cpp
  src/widgets/rename/batchrenamedialog.h
  src/widgets/rename/batchrenamedialogmodel_p.cpp
  src/widgets/rename/batchrenamedialogmodel_p.h
  src/widgets/rename/batchrenametypes_p.cpp
  src/widgets/rename/batchrenametypes_p.h
  src/widgets/rename/batchrenamevar_p.cpp
  src/widgets/rename/batchrenamevar_p.h
  src/widgets/rename/filenameutils_p.cpp
  src/widgets/rename/filenameutils_p.h
  tests/CMakeLists.txt
  tests/batchrenamedialogtest_gui.cpp

To: emateli, #frameworks, dfaure, mlaurent
Cc: mlaurent, asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, 
michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Laurent Montel]

mlaurent requested changes to this revision.
mlaurent added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> batchrenamedialog.cpp:40
> +#include 
> +#include 
> +#include 

remove QtWidget

> batchrenamedialog.cpp:41
> +#include 
> +#include 
> +

remove QtGui prefix

> batchrenamedialog.cpp:46
> +
> +BatchRenameDialog::BatchRenameDialog(QWidget *parent, const KFileItemList 
> ) : QDialog(parent)
> +{

new line before ": QDialog"

> batchrenamedialog.h:69
> +
> +KMessageWidget *m_messageWidget;
> +QTimer *m_timer;

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

> batchrenamedialogmodel_p.cpp:88
> +
> +BatchRenameDialogModel::BatchRenameDialogModel(QObject *parent, const 
> KFileItemList ) : QAbstractTableModel(parent)
> +{

new line before : QAbstr...

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: mlaurent, asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, 
michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli marked 2 inline comments as done.
emateli added a comment.


  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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: mlaurent, asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, 
michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli updated this revision to Diff 44218.
emateli marked 3 inline comments as done.
emateli added a comment.


  - Remove QtCore/

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14631?vs=44217=44218

BRANCH
  batchrename2

REVISION DETAIL
  https://phabricator.kde.org/D14631

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/batchrenametypestest.cpp
  autotests/batchrenametypestest.h
  src/widgets/CMakeLists.txt
  src/widgets/rename/batchrenamedialog.cpp
  src/widgets/rename/batchrenamedialog.h
  src/widgets/rename/batchrenamedialogmodel_p.cpp
  src/widgets/rename/batchrenamedialogmodel_p.h
  src/widgets/rename/batchrenametypes_p.cpp
  src/widgets/rename/batchrenametypes_p.h
  src/widgets/rename/batchrenamevar_p.cpp
  src/widgets/rename/batchrenamevar_p.h
  src/widgets/rename/filenameutils_p.cpp
  src/widgets/rename/filenameutils_p.h
  tests/CMakeLists.txt
  tests/batchrenamedialogtest_gui.cpp

To: emateli, #frameworks, dfaure, mlaurent
Cc: mlaurent, asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, 
michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli updated this revision to Diff 44217.
emateli added a comment.


  - fix indent
  - remove setlayout call
  - Add missing parent parameters
  - Remove unnecessary QtCore/ prefix

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14631?vs=44189=44217

BRANCH
  batchrename2

REVISION DETAIL
  https://phabricator.kde.org/D14631

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/batchrenametypestest.cpp
  autotests/batchrenametypestest.h
  src/widgets/CMakeLists.txt
  src/widgets/rename/batchrenamedialog.cpp
  src/widgets/rename/batchrenamedialog.h
  src/widgets/rename/batchrenamedialogmodel_p.cpp
  src/widgets/rename/batchrenamedialogmodel_p.h
  src/widgets/rename/batchrenametypes_p.cpp
  src/widgets/rename/batchrenametypes_p.h
  src/widgets/rename/batchrenamevar_p.cpp
  src/widgets/rename/batchrenamevar_p.h
  src/widgets/rename/filenameutils_p.cpp
  src/widgets/rename/filenameutils_p.h
  tests/CMakeLists.txt
  tests/batchrenamedialogtest_gui.cpp

To: emateli, #frameworks, dfaure, mlaurent
Cc: mlaurent, asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, 
michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Laurent Montel]

mlaurent requested changes to this revision.
mlaurent added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> CMakeLists.txt:65
> +  rename/batchrenamedialog.cpp
> +rename/batchrenamedialogmodel_p.cpp
> +rename/batchrenametypes_p.cpp

fix indent

> batchrenamedialog.cpp:48
> +{
> +auto *mainLayout = new QVBoxLayout();
> +setWindowTitle(i18nc("@title:window", "Rename Items"));

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

> batchrenamedialog.cpp:60
> +
> +m_previewTable = new QTableView();
> +m_previewTable->setModel(model);

add parent ?

> batchrenamedialog.cpp:172
> +// Button Bar
> +auto *buttonBox = new QDialogButtonBox(QDialogButtonBox::Ok | 
> QDialogButtonBox::Cancel | QDialogButtonBox::Help);
> +m_btnOk = buttonBox->button(QDialogButtonBox::Ok);

add parent here too

> batchrenamevar_p.h:25
> +
> +#include 
> +#include 

not necessary to add QtCore

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure, mlaurent
Cc: mlaurent, asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, 
michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Nathaniel Graham]

ngraham added a comment.


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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure
Cc: asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli updated this revision to Diff 44189.
emateli added a comment.


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

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14631?vs=42324=44189

BRANCH
  batchrename2

REVISION DETAIL
  https://phabricator.kde.org/D14631

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/batchrenametypestest.cpp
  autotests/batchrenametypestest.h
  src/widgets/CMakeLists.txt
  src/widgets/rename/batchrenamedialog.cpp
  src/widgets/rename/batchrenamedialog.h
  src/widgets/rename/batchrenamedialogmodel_p.cpp
  src/widgets/rename/batchrenamedialogmodel_p.h
  src/widgets/rename/batchrenametypes_p.cpp
  src/widgets/rename/batchrenametypes_p.h
  src/widgets/rename/batchrenamevar_p.cpp
  src/widgets/rename/batchrenamevar_p.h
  src/widgets/rename/filenameutils_p.cpp
  src/widgets/rename/filenameutils_p.h
  tests/CMakeLists.txt
  tests/batchrenamedialogtest_gui.cpp

To: emateli, #frameworks, dfaure
Cc: asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Albert Astals Cid]

aacid added a comment.


  In D14631#331827 , @emateli wrote:
  
  > @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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure
Cc: asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli added a comment.


  In D14631#303886 , @aacid wrote:
  
  > As far as i can see none of these headers get installed so should they all 
be renamed to _p.h ?
  >
  > Also if the headers don't get installed how do you use the new classes?
  
  
  @aacid Would be great if you'd point me towards making them installable. Only 
the dialog itself should be exported.
  @dfaure Do functions for getting file suffix and name already exist in KIO or 
other KDE libraries? The whole filenameutils feels like duplicate work.
  
  I want to give KDevelop a spin for this, how do I import a new formatter so 
it can format my code according to the KDE guidelines.

INLINE COMMENTS

> dfaure wrote in batchrenametypes.h:32
> This looks more like a namespace than an actual class, given that everything 
> is static.
> 
> Alternatively (and even better), make capturedGroups and the two associated 
> methods non-static, meaning that one has to instanciate the class in order to 
> use it. This is only used in the dialog, right? So there's no need for this 
> "global" variable, it can just be a member of BatchRenameTypes which can be a 
> member of the dialog, AFAICS.

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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure
Cc: asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli updated this revision to Diff 42324.
emateli marked 15 inline comments as done.
emateli added a comment.


  - Code style fixes
  - use .insert instead of []
  - Indent Q_OBJECT
  - use isEmpty instead of len==0
  - use aggregate initialization
  - protected -> private
  - added parent check
  - use functions instead of QString fields
  - delete itemData
  - code format
  - FileNameUtils converted to namespace
  - remove unnecessary using

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D14631?vs=39137=42324

BRANCH
  batchrename2

REVISION DETAIL
  https://phabricator.kde.org/D14631

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/batchrenametypestest.cpp
  autotests/batchrenametypestest.h
  src/widgets/CMakeLists.txt
  src/widgets/rename/batchrenamedialog.cpp
  src/widgets/rename/batchrenamedialog.h
  src/widgets/rename/batchrenamedialogmodel.cpp
  src/widgets/rename/batchrenamedialogmodel.h
  src/widgets/rename/batchrenametypes.cpp
  src/widgets/rename/batchrenametypes.h
  src/widgets/rename/batchrenamevar.cpp
  src/widgets/rename/batchrenamevar.h
  src/widgets/rename/filenameutils.cpp
  src/widgets/rename/filenameutils.h
  tests/CMakeLists.txt
  tests/batchrenamedialogtest_gui.cpp

To: emateli, #frameworks, dfaure
Cc: asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Nathaniel Graham]

ngraham added a comment.


  Any progress on this, @emateli?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure
Cc: asensi, rkflx, dfaure, aacid, ngraham, kde-frameworks-devel, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> batchrenametypestest.h:20
> +
> +#ifndef KIO_RENAMETYPESTEST_H
> +#define KIO_RENAMETYPESTEST_H

Doesn't match the header filename, and anyway, feel free to move this to the 
.cpp file, for qtestlib unittests.

> batchrenamedialog.cpp:193
> +
> +params[QStringLiteral("timestamp")] = QDateTime::currentDateTime();
> +

params.insert(key, value) is faster than params[key] = value, see the book 
Effective C++ (or was it Effective STL?)

> batchrenamedialog.cpp:221
> +bool valid = (newName == itemName) || !(newName.isEmpty() || exists);
> +allItemsOk &= valid;
> +generatedNames.append(newName);

bitfield operation on booleans can lead to unexpected behaviour.
Use `allItemsOk = allItemsOk && valid` instead, or `if (!valid) allItemsOk = 
false;`

> batchrenamedialog.cpp:257
> +
> +if (!job->error()) {
> +m_renamedItems << newUrl;

This makes no sense, you can't test for a job error before the job is done.
Connect to the result signal and do that in the slot (e.g. lambda) if it should 
really only be done on success -- after the move happened.

> batchrenamedialog.cpp:264
> +{
> +for (const auto : m_itemsToBeRenamed) {
> +renameItem(pair.first, pair.second);

qAsConst(m_itemsToBeRenamed)

> batchrenamedialog.cpp:287
> +{
> +Q_UNUSED(checked);
> +

Just remove the argument from the slot then.

> batchrenamedialog.cpp:365
> +for (int i = 0; i < capturedGroups.length(); i++) {
> +if (capturedGroups[i].length() == 0) {
> +continue;

use .isEmpty()

> batchrenamedialog.cpp:377
> +}
> \ No newline at end of file


Please configure your text editor to add newlines at the end of files. 
Surprising that it doesn't do that, most do.

> batchrenamedialog.h:44
> +{
> +Q_OBJECT
> +

indent this line by 4 spaces please

> batchrenamedialogmodel.cpp:28
> +int BatchRenameDialogModel::rowCount(const QModelIndex ) const {
> +Q_UNUSED(parent);
> +return itemData->count();

Technically this should be `if (parent.isValid()) { return 0; }`
in case anyone plugs this model into a QTreeView one day, or plugs a proxy that 
supports trees.

> batchrenamedialogmodel.cpp:79
> +
> +BatchRenameDialogModel::BatchRenameDialogModel(QObject *parent, const 
> KFileItemList ) : QAbstractTableModel(parent) {
> +itemData = new QList;

the '{' that opens a method implementation should go on a separate line 
(repeats below)

> batchrenamedialogmodel.h:35
> +
> +BatchRenameDialogModelData(const KFileItem , QString newName, bool 
> valid)
> +{

This ctor isn't really needed, one could use aggregate initialization instead, 
no?
i.e. BatchRenameDialogModelData{item, newName, valid}

(Not 100% sure)

If you keep this ctor, then use member initialization syntax.

> batchrenamedialogmodel.h:47
> +Q_OBJECT
> +protected:
> +QList *itemData;

private (AFAICS nothing derives from this class), and at the end please.

> batchrenametypes.h:32
> +
> +class KIOWIDGETS_EXPORT BatchRenameTypes
> +{

This looks more like a namespace than an actual class, given that everything is 
static.

Alternatively (and even better), make capturedGroups and the two associated 
methods non-static, meaning that one has to instanciate the class in order to 
use it. This is only used in the dialog, right? So there's no need for this 
"global" variable, it can just be a member of BatchRenameTypes which can be a 
member of the dialog, AFAICS.

> batchrenamevar.h:39
> +
> +static const QString dateYear4Digit;
> +static const QString dateYear2Digit;

Make all these functions rather than variables. It will speed up starting time 
(right now all those QStrings have to be created upfront).

> filenameutils.h:29
> +
> +class KIOWIDGETS_EXPORT FileNameUtils
> +{

This looks more like a namespace than a class, given that it only contains 2 
static methods.

(We don't need a ctor / dtor to be generated)

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks, dfaure
Cc: dfaure, aacid, ngraham, kde-frameworks-devel, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Albert Astals Cid]

aacid added a comment.


  As far as i can see none of these headers get installed so should they all be 
renamed to _p.h ?
  
  Also if the headers don't get installed how do you use the new classes?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks
Cc: aacid, ngraham, kde-frameworks-devel, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Nathaniel Graham]

ngraham added a comment.


  Awesome! Does Dolphin need a patch to use this?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks
Cc: ngraham, kde-frameworks-devel, michaelh, bruns

D14631: Adds a new RenameDialog to KIO with more options for batch renaming

[Emirald Mateli]

emateli retitled this revision from "Migrate D10698 from Dolphin to KIO" to 
"Adds a new RenameDialog to KIO with more options for batch renaming".
emateli edited the summary of this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D14631

To: emateli, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns