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 introduces the name of an argument). Not that
we're going to run doxygen here, but well ;)
> deletejob.cpp:132
> + void rmdirResult(bool result, const QUrl& url);
> + void deleteFileUsingJob (const QUrl& url, bool isLink);
> + void deleteDirUsingJob (const QUrl& url);
no space after method name (same on next line)
> deletejob.cpp:188
> +
> + m_ioworker = new DeleteJobIOWorker;
> + m_ioworker->moveToThread(&m_thread);
One problem left: creating the worker and its thread is done even if we're
deleting only remote URLs. This is a bit wasteful. Suggestion: create a
worker() method which does all this (everything that you added to this method)
on demand, if m_ioworker is null.
> deletejob.cpp:340
> +
> + if (isLink) { // not a link
> + symlinks.removeFirst();
That is a really weird comment, for a bool called isLink :-)
> deletejob.cpp:348
> + } else {
> + // fallback if unlink() failed (we'll use the job's error handling
> in that case)
> + deleteFileUsingJob(url, isLink);
(pre-existing) s/unlink/QFile::remove/ in this comment would be less confusing
> deletejob.cpp:353
> +
> +void DeleteJobPrivate::deleteFileUsingJob (const QUrl &url, bool isLink)
> {
please remove spaces after method names everywhere
> dfaure wrote in deletejob.cpp:412
> removeLast here too?
marked as done but I still see removeFirst, I'm confused.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D24962
To: meven, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
