D23194: Making FileJob behave consistently.
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 https://phabricator.kde.org/D23194 AFFECTED FILES autotests/jobremotetest.cpp autotests/jobremotetest.h src/core/filejob.cpp src/core/filejob.h src/core/slavebase.h src/core/slaveinterface.h src/ioslaves/file/file.cpp src/ioslaves/file/file.h To: feverfew, dfaure, fvogt, chinmoyr, apol Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D23194: Making FileJob behave consistently.
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, michaelh, ngraham, bruns
D23194: Making FileJob behave consistently.
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 ugliness :-) ). Thanks for the added test. INLINE COMMENTS > feverfew wrote in filejob.h:48 > 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 forgets to change the other... Makes sense to me. 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
D23194: Making FileJob behave consistently.
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
D23194: Making FileJob behave consistently.
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 https://phabricator.kde.org/D23194 AFFECTED FILES autotests/jobremotetest.cpp autotests/jobremotetest.h src/core/filejob.cpp src/core/filejob.h src/core/slavebase.h src/core/slaveinterface.h src/ioslaves/file/file.cpp src/ioslaves/file/file.h To: feverfew, dfaure, fvogt, chinmoyr, apol Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D23194: Making FileJob behave consistently.
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
D23194: Making FileJob behave consistently.
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 BRANCH ConsistentRead (branched from master) REVISION DETAIL https://phabricator.kde.org/D23194 AFFECTED FILES autotests/jobremotetest.cpp autotests/jobremotetest.h src/core/filejob.cpp src/core/filejob.h src/core/slavebase.h src/core/slaveinterface.h src/ioslaves/file/file.cpp src/ioslaves/file/file.h To: feverfew, dfaure, fvogt, chinmoyr, apol Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns
D23194: Making FileJob behave consistently.
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 motivations for this patch was that for KIOFuse, we do read 0 bytes in case the user truncates to 0, otherwise we'd have to have a workaround if it didn't work (for example, before this patch, we'd get both an empty QByteArray() and the error signal...). 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
D23194: Making FileJob behave consistently.
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(). > + * emitted -> Emitted > filejob.h:112 > + * > + * Unless a read() request was sent for 0 bytes, End of data (EOD) has > been > + * reached if data.size() == 0 Why would someone request reading 0 bytes? That doesn't seem sensible to me. > filejob.h:144 > /** > - * Bytes written to the file. > + * \p written bytes were written to the file. Emitted after write(); > * @param job the job that emitted this signal s/;/./ > filejob.h:185 > + * On error the open() signal is not emitted. To catch errors please > + * connect to the result() signal. > + * (OK -- that's supposed to be a known fact from the base class KJob, but I'm not opposed to saying it again ;) 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
D23194: Making FileJob behave consistently.
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 https://phabricator.kde.org/D23194 AFFECTED FILES src/core/filejob.cpp src/core/filejob.h src/core/slavebase.h src/core/slaveinterface.h src/ioslaves/file/file.cpp src/ioslaves/file/file.h To: feverfew, dfaure, fvogt, chinmoyr, apol Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns
D23194: Making FileJob behave consistently.
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 forgets to change the other... REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D23194 To: feverfew, dfaure, fvogt, chinmoyr, apol Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns
D23194: Making FileJob behave consistently.
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 have silent data loss. Maybe we should implement a flush method in FileJob. It only really makes sense for the file slave, for sftp/smb it will be a noop... Thoughts? REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D23194 To: feverfew, dfaure, fvogt, chinmoyr, apol Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns
D23194: Making FileJob behave consistently.
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: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns
D23194: Making FileJob behave consistently.
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 read until size was reached (or EOD). I decided to just convert file's behaviour to what smb/sftp do and updated the docs accordingly, for ease of use (and so that if new slaves implement KIO::open() they'd know how to implement the functions. Of course, I could do the converse, and change smb/sftp to behave like the file slave when reading/writing, but I can leave that up to debate, if others want to chime in. REPOSITORY R241 KIO REVISION DETAIL https://phabricator.kde.org/D23194 To: feverfew, dfaure, fvogt, chinmoyr, apol Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns
D23194: Making FileJob behave consistently.
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 https://phabricator.kde.org/D23194 AFFECTED FILES src/core/filejob.cpp src/core/filejob.h src/core/slavebase.h src/core/slaveinterface.h src/ioslaves/file/file.cpp src/ioslaves/file/file.h To: feverfew, dfaure, fvogt, chinmoyr, apol Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns
D23194: Making FileJob behave consistently.
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
> /**
> - * Read block
> + * This function attempts to read up to \p size bytes from the URL
> passed to
> + * KIO::open() and returns the bytes received via the data() signal.
Document the methods in SlaveBase as well.
> slavebase.h:107
> + * @see close()
> + */
> +void closed();
@since 5.62
> slaveinterface.h:90
> +MSG_SLAVE_STATUS_V2,
> +MSG_CLOSED
> // add new ones here once a release is done, to avoid breaking binary
> compatibility
Nitpick; add a ,
Next time someone adds an enum it will change only one line.
> file.cpp:520
>
> -if (!res.isEmpty()) {
> -data(res);
> -bytes -= res.size();
> -} else {
> -// empty array designates eof
> -data(QByteArray());
> -if (!mFile->atEnd()) {
> -error(KIO::ERR_CANNOT_READ, mFile->fileName());
> -close();
> -}
> -break;
> -}
> -if (bytes <= 0) {
> -break;
> -}
> +qint64 bytesRead = mFile->read(buffer.data(), bytes);
> +
A loop is required here. The docs don't really specify anything about reading
data in one go.
> file.cpp:584
> +closeWithoutFinish();
> +closed();
> }
missing a finished() here
> file.h:117
> +// Close without calling finish(). Use this to close after error.
> +void closeWithoutFinish();
> +
Nitpick; why not just closeFile() ?
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D23194
To: feverfew, dfaure, fvogt, chinmoyr, apol
Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns
D23194: Making FileJob behave consistently.
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
D23194: Making FileJob behave consistently.
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
D23194: Making FileJob behave consistently.
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 close() signal is actually emitted when close() is called. 2. Documents the FileJob functions more accurately, and ensures the file slave acts similarly to the two other slaves that implement these functions (smb/sftp). 3. Fixes an issue when purposefully reading 0 bytes. TEST PLAN The application I am developing on that depends on FileJob now successfully receives the close() signal when required and does not experience the bug mentioned when reading 0 bytes. Existing read/write/seek functionality is not broken. Tests also pass. REPOSITORY R241 KIO BRANCH ConsistentRead (branched from master) REVISION DETAIL https://phabricator.kde.org/D23194 AFFECTED FILES src/core/filejob.cpp src/core/filejob.h src/core/slavebase.cpp src/core/slavebase.h src/core/slaveinterface.cpp src/core/slaveinterface.h src/ioslaves/file/file.cpp src/ioslaves/file/file.h To: feverfew, dfaure, fvogt, chinmoyr, apol Cc: kde-frameworks-devel, LeGast00n, michaelh, ngraham, bruns
