D27760: WIP | Added BatchMoveJob

[David Faure]

dfaure added a comment.


  Looks good, just a few things.

INLINE COMMENTS

> batchmovejobtest.cpp:91
> +        job->setUiDelegate(nullptr);
> +        QSignalSpy spy(job, SIGNAL(fileMoved(QUrl, QUrl)));
> +        

Use newer syntax &KIO::BatchMoveJob::fileMoved?

> batchmovejobtest.cpp:104
> +    {
> +        //m_homeDir.removeRecursively();
> +    }

This reads really scary! :-)

Can you rename the var to what it is, like m_homeTmpDir?

And this can be re-enabled, no?

Alternatively, use QTemporaryDir, it cleans up automatically.

> batchmovejob.cpp:104
> +    emit fileMoved(d->m_listIterator->from, d->m_listIterator->to);
> +    d->m_listIterator++;
> +    emitPercent(d->m_listIterator - d->m_items.constBegin(), d->m_items.count());

prefer prefix increment (++ in front)

> batchmovejob.h:37
> +    
> +    BatchMoveJobItem()
> +    {

Where is this ctor needed?

Both could just be removed, then you can use aggregate initialization with {}

> batchmovejob.h:50
> + *
> + * A KIO job to move multiple files simoultaneousoly
> + *

typo: simultaneously

(bulk purchase of the 'o' letter?) :)

> batchmovejob.h:52
> + *
> + * @since 5.67
> + */

5.69

> fileundomanager.h:140
> +     * 
> +     * BatchMove: @since 5.67 KIO::batchMoveJob. Used when moving multiple files in a single job
>       */

5.69

REPOSITORY
  R241 KIO

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

To: emateli, dfaure, ngraham
Cc: nicolasfella, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20200321/5e92297a/attachment-0001.html>