D9103: Add BatchRenameJob to KIO

[David Faure]

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

INLINE COMMENTS

> batchrenamejob.cpp:56
> +        // In this case nothing is substituted and all files have the same $newName.
> +        // 4. Atleast two files have same extension and $newName contains an invalid placeholder.
> +        // In this case $index is appended to $newName.

At least is two words in this sentence too ;)

> batchrenamejob.cpp:86
> +            }
> +            else {
> +               m_useIndex = false;

coding style: join with previous line

> dfaure wrote in batchrenamejob.cpp:55
> Why special case "not a sequence", rather than just replacing the first (last?) sequence?

Not done, the comment here still talks about the "invalid because not connected sequence" case. Instead, point 3 could be simplified to "no placeholder", i.e. no longer "invalid" that needs to be explained with "(This means...)".

But of course the code also needs to do this: stop at the first sequence of placeholders, use that.

> dfaure wrote in batchrenamejob.cpp:75
> this code iterates over newName's 3 times... I wonder why this isn't just indexOf + a small loop until (current char != placeholder). Simpler, faster, and easier to specify (as mentioned above)

not done. You added an if(), but you didn't implement my suggestion.

In a hurry? ;)

> dfaure wrote in batchrenamejob.cpp:196
> what happens if we get "file already exists"? It seems the error is ignored?

?

> dfaure wrote in batchrenamejob.h:46
> add a one-line docu

not done

> dfaure wrote in batchrenamejob.h:52
> /// @internal

not done

> dfaure wrote in batchrenamejob.h:65
> Can you add an example here, to make it clearer?
> 
> Also mention that newName *must* contain the indexPlaceHolder character (right?) (what happens if it doesn't?)
> 
> Also I find the name "indexPlaceHolder" misleading, it reads like "index of the placeholder". Why not just call it placeHolder? (or replacementCharacter?)

not done

REPOSITORY
  R241 KIO

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

To: chinmoyr, #frameworks, dfaure
Cc: apol
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20171202/08ad7a30/attachment.html>