This revision was automatically updated to reflect the committed changes.
Closed by commit R241:3154fc3e096e: Making FileJob behave consistently.
(authored by feverfew).
REPOSITORY
R241 KIO
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D23194?vs=64673=64976
REVISION DETAIL
chinmoyr accepted this revision.
This revision is now accepted and ready to land.
REPOSITORY
R241 KIO
BRANCH
ConsistentRead (branched from master)
REVISION DETAIL
https://phabricator.kde.org/D23194
To: feverfew, dfaure, fvogt, chinmoyr, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2,
dfaure accepted this revision.
dfaure added a comment.
I didn't know the code of that unittest. It could really use some modernizing
(using job->exec() instead of enterLoop, using lambdas instead of numbered
slots...), but that's for a separate commit anyway, this one keeps consistency
(in
feverfew marked an inline comment as done.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D23194
To: feverfew, dfaure, fvogt, chinmoyr, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
feverfew updated this revision to Diff 64673.
feverfew added a comment.
- Removing flush on write
REPOSITORY
R241 KIO
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D23194?vs=64672=64673
BRANCH
ConsistentRead (branched from master)
REVISION DETAIL
feverfew marked 3 inline comments as done.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D23194
To: feverfew, dfaure, fvogt, chinmoyr, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
feverfew updated this revision to Diff 64672.
feverfew added a comment.
- Fixing typos
- Adding test for reading of 0 bytes and asserting that the close signal is
actually emitted.
REPOSITORY
R241 KIO
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D23194?vs=64051=64672
feverfew added inline comments.
INLINE COMMENTS
> dfaure wrote in filejob.h:112
> Why would someone request reading 0 bytes? That doesn't seem sensible to me.
Well it can happen and this is technically more correct than saying that an
empty `QByteArray()` is always EOD. In fact, the one of the
dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.
Unittests are missing.
INLINE COMMENTS
> filejob.h:110
> /**
> - * Data from the slave has arrived.
> + * Data from the slave has arrived. emitted after read().
> +
feverfew updated this revision to Diff 64051.
feverfew added a comment.
Making comments clearer
REPOSITORY
R241 KIO
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D23194?vs=63861=64051
BRANCH
ConsistentRead (branched from master)
REVISION DETAIL
feverfew marked 4 inline comments as done.
feverfew added inline comments.
INLINE COMMENTS
> chinmoyr wrote in filejob.h:48
> Document the methods in SlaveBase as well.
I've used @see to help me out. Is that enough? I don't want to duplicate the
docs, especially if someone changes it and
feverfew added inline comments.
INLINE COMMENTS
> file.cpp:550
> +// Make sure data gets to disk.
> +if (mFile->flush())
> +written(bytesWritten);
Flushing on every write IMO is a bit expensive, but I don't like the fact that
close() can't report errors and so we
feverfew added inline comments.
INLINE COMMENTS
> chinmoyr wrote in file.h:117
> Nitpick; why not just closeFile() ?
That's how sftp does it, so for consistency.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D23194
To: feverfew, dfaure, fvogt, chinmoyr, apol
Cc:
feverfew added inline comments.
INLINE COMMENTS
> chinmoyr wrote in file.cpp:520
> A loop is required here. The docs don't really specify anything about reading
> data in one go.
Exactly. The docs weren't specific at all. smb/sftp would read once, and return
the result. the file slave would
feverfew updated this revision to Diff 63861.
feverfew added a comment.
Addressing comments
REPOSITORY
R241 KIO
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D23194?vs=63841=63861
BRANCH
ConsistentRead (branched from master)
REVISION DETAIL
chinmoyr requested changes to this revision.
chinmoyr added inline comments.
This revision now requires changes to proceed.
INLINE COMMENTS
> filejob.cpp:181
> +slaveDone();
> +// Scheduler::doJob(this);
> +q->emitResult();
Nitpick; spacing gotcha
> filejob.h:48
> /**
> -
feverfew edited the summary of this revision.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D23194
To: feverfew, dfaure, fvogt, chinmoyr, apol
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns
feverfew edited the summary of this revision.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D23194
To: feverfew, dfaure, fvogt, chinmoyr, apol
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns
feverfew created this revision.
feverfew added reviewers: dfaure, fvogt, chinmoyr, apol.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
feverfew requested review of this revision.
REVISION SUMMARY
This patch does the following:
1. Makes sure the
19 matches
Mail list logo
