D9103: Add BatchRenameJob to KIO

[Chinmoy Ranjan Pradhan]

chinmoyr added inline comments.

INLINE COMMENTS

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

Because there might not be a first or last sequence present. In case 3 we don't need a replacement and in case 4 we just append the integer irrespective of placeholder's location.

> dfaure wrote in batchrenamejob.cpp:55
> 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.

Here if there is exactly one sequence of placeholders then it is valid otherwise invalid.

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

Here it is searching for exactly one sequence. If more than one sequence are present like $$file$$name$$ then it is considered invalid.

> dfaure wrote in batchrenamejob.cpp:75
> not done. You added an if(), but you didn't implement my suggestion.
> 
> In a hurry? ;)

Not really in a hurry :P I missed two comments and I also hoped arc would submit my comments but it didn't.

> dfaure wrote in batchrenamejob.cpp:109
> nice docu, wrong location :-)

I just copied whatever there was in Dolphin. Where should I move it to?

> dfaure wrote in batchrenamejob.cpp:158
> what if that's -1 because not found? can this happen?

This cannot happen. m_useIndex and m_appendIndex are there for that purpose only.

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

In dolphin the overwrite dialog pops up. Otherwise the fileRenamed is still emitted and the job continues. Maybe I should make that its emission conditional?

> dfaure wrote in batchrenamejob.h:52
> not done

I hope it is for BatchRenameJob.

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

It is not necessary for newName to contain the placeholder. If its not there  index will be appended to newName.

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/cc42de0d/attachment-0001.html>