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

This revision was automatically updated to reflect the committed changes. Closed by commit R241:80d5f52b0675: [DeleteJob] Use a separate worker thread to run actual IO operation (authored by meven). REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE

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

meven added inline comments. INLINE COMMENTS > deletejob.cpp:130 > +/// Callback of worker rmfile > +void rmFileResult(bool result, bool isLink); > +/// Callback of worker rmdir Now we are at it, isLink could become m_curentURLIsLink, but isLink being a bool, it is much cheaper to

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

meven updated this revision to Diff 69332. meven marked 4 inline comments as done. meven added a comment. Use m_current instead of passing const QUrl around REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24962?vs=68946=69332 BRANCH arcpatch-D24962

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

meven edited the summary of this revision. REPOSITORY R241 KIO BRANCH arcpatch-D24962 REVISION DETAIL https://phabricator.kde.org/D24962 To: meven, dfaure, #frameworks Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

dfaure added inline comments. INLINE COMMENTS > deletejob.cpp:406 > +QMetaObject::invokeMethod(worker(), "rmfile", > Qt::QueuedConnection, > + Q_ARG(const QUrl&, m_currentURL), > + Q_ARG(bool, isLink)); Since

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

meven added inline comments. INLINE COMMENTS > dfaure wrote in deletejob.cpp:67 > Seems negligible to me. If you remove the argument, you'll have to > "reconstruct" the URL from files.first()/symlinks.first()? Or [ab]use > m_currentURL? > The "no premature optimization" rule says: don't change

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

dfaure added inline comments. INLINE COMMENTS > meven wrote in deletejob.cpp:67 > I am hesitant to remove `const QUrl& url` from this signal, it could save > some space in message passing. Seems negligible to me. If you remove the argument, you'll have to "reconstruct" the URL from

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

meven edited the summary of this revision. REPOSITORY R241 KIO BRANCH arcpatch-D24962 REVISION DETAIL https://phabricator.kde.org/D24962 To: meven, dfaure, #frameworks Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

meven added a task: T11627: Improve KIO asynchronicity. REPOSITORY R241 KIO BRANCH arcpatch-D24962 REVISION DETAIL https://phabricator.kde.org/D24962 To: meven, dfaure, #frameworks Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

meven added a reviewer: Frameworks. REPOSITORY R241 KIO BRANCH arcpatch-D24962 REVISION DETAIL https://phabricator.kde.org/D24962 To: meven, dfaure, #frameworks Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

meven added inline comments. INLINE COMMENTS > dfaure wrote in deletejob.cpp:67 > "const bool" doesn't do anything in a signal, I suggest removing the const. I am hesitant to remove `const QUrl& url` from this signal, it could save some space in message passing. REPOSITORY R241 KIO BRANCH

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

dfaure accepted this revision. dfaure added a comment. This revision is now accepted and ready to land. OK, let's get this in :-) REPOSITORY R241 KIO BRANCH arcpatch-D24962 REVISION DETAIL https://phabricator.kde.org/D24962 To: meven, dfaure Cc: kde-frameworks-devel, LeGast00n, GB_2,

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

meven added a comment. In D24962#555443 , @dfaure wrote: > Can't wait to get clang-format fully automated into the review workflow Such a time saver it would be. REPOSITORY R241 KIO REVISION DETAIL

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

meven updated this revision to Diff 68946. meven marked 5 inline comments as done. meven added a comment. Mostly formatting, change style to init m_ioworker and check it in worker() REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24962?vs=68872=68946 BRANCH

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

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

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

meven marked an inline comment as done. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D24962 To: meven, dfaure Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

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

meven added a comment. In D24962#554961 , @dfaure wrote: > Any reason why you didn't implement my suggestion of > >DeleteJobIOWorker *ioworker() { >if (!m_ioworker) { > ... >} >return

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

meven updated this revision to Diff 68872. meven added a comment. Refactor initWorkerThread() to *worker() accessor REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24962?vs=68854=68872 BRANCH arcpatch-D24962 REVISION DETAIL

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

dfaure added a comment. Any reason why you didn't implement my suggestion of DeleteJobIOWorker *ioworker() { if (!m_ioworker) { ... } return m_ioworker; } [...] QMetaObject::invokeMethod(ioworker(), "rmfile", [...]); ? A

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

meven updated this revision to Diff 68854. meven marked 7 inline comments as done. meven added a comment. Create worker thread only once needed, fix a removeLast missing, formatting REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24962?vs=68826=68854 BRANCH

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

dfaure requested changes to this revision. dfaure added a comment. This revision now requires changes to proceed. Almost there ;) INLINE COMMENTS > deletejob.cpp:73 > +/** > + * Deletes the file @p points to > + * The file must be a LocalFile Missing "url" after @p (which

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

meven updated this revision to Diff 68826. meven marked 4 inline comments as done. meven added a comment. Remove some const bool in function signature, use m_currentURL for consistency REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24962?vs=68825=68826 BRANCH

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

dfaure added inline comments. INLINE COMMENTS > deletejob.cpp:67 > +Q_SIGNALS: > +void rmfileResult(bool succeeded, const QUrl& url, const bool isLink); > +void rmddirResult(bool succeeded, const QUrl& url); "const bool" doesn't do anything in a signal, I suggest removing the const. >

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

meven updated this revision to Diff 68825. meven added a comment. Fix rmdir, we must use removeLast since we remove in reverse order REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24962?vs=68822=68825 BRANCH master REVISION DETAIL

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

meven updated this revision to Diff 68822. meven added a comment. Fix argument passing to invokeMethod REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24962?vs=68821=68822 BRANCH master REVISION DETAIL https://phabricator.kde.org/D24962 AFFECTED FILES

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

meven updated this revision to Diff 68821. meven added a comment. amend commit REPOSITORY R241 KIO CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D24962?vs=68819=68821 BRANCH master REVISION DETAIL https://phabricator.kde.org/D24962 AFFECTED FILES src/core/deletejob.cpp

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

meven retitled this revision from "WIP [DeleteJob] Use a separate worker thread to run actual IO operation" to "[DeleteJob] Use a separate worker thread to run actual IO operation". meven edited the summary of this revision. REPOSITORY R241 KIO REVISION DETAIL