D24962: [DeleteJob] Use a separate worker thread to run actual IO operation

[David Faure]

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


  Can't wait to get clang-format fully automated into the review workflow....

INLINE COMMENTS

> deletejob.cpp:76
> +     */
> +    void rmfile(const QUrl& url, bool isLink){
> +        emit rmfileResult(QFile::remove(url.toLocalFile()), url, isLink);

missing space before '{', sorry I missed it until now.

> deletejob.cpp:100
>          , m_reportTimer(nullptr)
> +        , m_ioworker(nullptr)
>      {

funny that you use the ctor init list for this one, and not a default value at declaration time, like you did for m_thread :-)

> deletejob.cpp:197
> +
> +    if (m_thread == nullptr) {
> +        m_thread = new QThread();

nitpick: it would be more expected to do `if (!m_ioworker) {` here, since m_ioworker is what we're creating on demand. m_thread is just an implementation detail.

> deletejob.cpp:404
> +        // If local file, try do it directly
> +        if (m_currentURL.isLocalFile()){
> +            // separate thread will do the work

space before `{` here as well

> deletejob.cpp:432
> +
> +void DeleteJobPrivate::deleteDirUsingJob (const QUrl &url)
> +{

I still see a space between method name and `(` here :-)

REPOSITORY
  R241 KIO

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

To: meven, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20191028/cd0c21ff/attachment.html>