D9103: Add BatchRenameJob to KIO

[David Faure]

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


  I'm OK with this being in KIO. One day we might want the same from the file dialog, or krusader, etc.

INLINE COMMENTS

> batchrenamejobtest.cpp:20
> +
> +#include <QtTest>
> +

this is all of QtTest + all of QtCore, please use <QTest> instead

> batchrenamejobtest.cpp:51
> +    {
> +        QList<QUrl> srcList;
> +        foreach (const QString &filename, fileList) {

srcList.reserve(fileList.count());

> batchrenamejob.cpp:48
> +    {
> +        // Here Dolphin's logic for renaming multiple files is being followed.
> +        // There occur four cases when renaming multiple files this way,

Well soon this won't be dolphin's logic anymore, right? Anyone reading this in 2 years won't find such logic in dolphin but only here? Then don't point to Dolphin ;)

> batchrenamejob.cpp:51
> +        // 1. All files have different extension and $newName contains a valid placeholder.
> +        // 2. Atleast two files have same extension and $newName contains a valid placeholder.
> +        // In these two cases the placeholder character will be replaced by an integer($index).

"At least" is two words.

> batchrenamejob.cpp:55
> +        //    (this means either $newName doesn't contain the placeholder or the placeholders
> +        //     are not in a connected sequence).
> +        // In this case nothing is substituted and all files have the same $newName.

Why special case "not a sequence", rather than just replacing the first (last?) sequence?

> batchrenamejob.cpp:64
> +        foreach (const QUrl &url, m_srcList) {
> +            QMimeDatabase db;
> +            const QString extension = db.suffixForFileName(url.toDisplayString().toLower());

cheap to create, but still, move outside foreach

> batchrenamejob.cpp:75
> +        // Check for exactly one placeholder character or a sequence of placeholders.
> +        const int count = newName.count(indexPlaceHolder);
> +        const int first = newName.indexOf(indexPlaceHolder);

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)

> batchrenamejob.cpp:109
> +     *         A connected sequence of placeholders results in leading zeros:
> +     *         indexedName("Test ####.jpg", 12, '#') returns "Test 0012.jpg".
> +     */

nice docu, wrong location :-)

> batchrenamejob.cpp:153
> +    const int minIndexLength = name.count(indexPlaceHolder);
> +    while (indexString.length() < minIndexLength) {
> +        indexString.prepend(QLatin1Char('0'));

Faster: indexString.prepend(QString(minIndexLength - indexString.length(), QLatin1Char('0'));
Well, please test that it's correct, but you see the point, QString(int, char) creates the string with the repeated char in one go.

> batchrenamejob.cpp:158
> +    // Replace the index placeholders by the indexString
> +    const int placeHolderStart = newName.indexOf(indexPlaceHolder);
> +    newName.replace(placeHolderStart, minIndexLength, indexString);

what if that's -1 because not found? can this happen?

> batchrenamejob.cpp:196
> +    Q_D(BatchRenameJob);
> +    if (job->error() && job->error() != KIO::ERR_FILE_ALREADY_EXIST) {
> +        KIO::Job::slotResult(job);

what happens if we get "file already exists"? It seems the error is ignored?

> batchrenamejob.h:46
> +Q_SIGNALS:
> +    void fileRenamed(const QUrl &oldUrl, const QUrl &newUrl);
> +

add a one-line docu

> batchrenamejob.h:52
> +protected:
> +    BatchRenameJob(BatchRenameJobPrivate &dd);
> +

/// @internal

> batchrenamejob.h:65
> + * @param index The integer(incremented after renaming an item) to add to the base name.
> + * @param indexPlaceHolder The character(s) which @p index will replace.
> + *

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

> batchrenamejob.h:71
> +KIOCORE_EXPORT BatchRenameJob *batchRename(const QList<QUrl> &src, const QString &newName,
> +                                           int index, const QChar& indexPlaceHolder,
> +                                           JobFlags flags = DefaultFlags);

QChar is basically a quint16, pass it by value, not by const ref.

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