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
