Review Request 123514: Make it possible to treat non-sequential QIODevice asynchronously

[Aleix Pol Gonzalez]

> On April 30, 2015, 1:53 a.m., Mark Gaiser wrote:
> > autotests/accessmanagertest.cpp, line 82
> > <https://git.reviewboard.kde.org/r/123514/diff/2/?file=364611#file364611line82>
> >
> >     The / should be a QDir::separator, no?

Nope. Qt decodes the / accordingly.
http://doc.qt.io/qt-5.4/qdir.html

Qt uses "/" as a universal directory separator in the same way that "/" is used as a path separator in URLs. If you always use "/" as a directory separator, Qt will translate your paths to conform to the underlying operating system.


> On April 30, 2015, 1:53 a.m., Mark Gaiser wrote:
> > autotests/accessmanagertest.cpp, line 87
> > <https://git.reviewboard.kde.org/r/123514/diff/2/?file=364611#file364611line87>
> >
> >     QVERIFY this one?

No need, it's just cleaning up, in case another test was run before, so it doesn't give false positives.


> On April 30, 2015, 1:53 a.m., Mark Gaiser wrote:
> > src/core/transferjob.cpp, line 400
> > <https://git.reviewboard.kde.org/r/123514/diff/2/?file=364616#file364616line400>
> >
> >     This name is misleading. I read it like: "this is a bool that indicates if a read succeeded". While that is _also_ what it does, it basically returns the number of bytes read. Perhaps name it "bytesRead" or something alike?

Ok, yes, in fact I had bytesRead. But then I decided to get inspired by Qt code and adopted their variable name. But it's beter like this.


> On April 30, 2015, 1:53 a.m., Mark Gaiser wrote:
> > src/core/transferjob.cpp, line 438
> > <https://git.reviewboard.kde.org/r/123514/diff/2/?file=364616#file364616line438>
> >
> >     I might be missing the reason, but why do you always send an empty bytearray? You have an if just a couple lines higher that sends data if there is remaining data.
> >     
> >     Should this be needed at all?

It's how it works the internal communication with the slave. When it receives an empty QByteArray, it understands that it's over.

I'll add a comment.


- Aleix


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123514/#review79704
-----------------------------------------------------------


On April 29, 2015, 6:28 p.m., Aleix Pol Gonzalez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123514/
> -----------------------------------------------------------
> 
> (Updated April 29, 2015, 6:28 p.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Repository: kio
> 
> 
> Description
> -------
> 
> So far, we used to just read whenever some data was required. This works on sequential devices because the data is already available. This is not the case when we have a sequential device, such as a socket, where data arrives when it arrives. This will also prove useful on non-sequential devices as well when we want to keep reading in case new data appears.
> 
> This patch takes the AsyncDataEnabled setting on accordinly by:
> 
> * only reading from the device when readyRead is available.
> * finishes the transfer whenever the device is closed.
> 
> 
> Diffs
> -----
> 
>   src/core/transferjob.h e2fd2e7 
>   src/core/job_p.h 7ec1a69 
>   autotests/jobtest.cpp 327470a 
>   autotests/jobtest.h 5ccd492 
>   autotests/accessmanagertest.cpp 5d52553 
>   autotests/CMakeLists.txt 7bba3ea 
>   src/core/transferjob.cpp 97a724e 
>   src/widgets/accessmanager.cpp b4ec811 
> 
> Diff: https://git.reviewboard.kde.org/r/123514/diff/
> 
> 
> Testing
> -------
> 
> Tests still pass, new test also passes.
> 
> The test is using lambdas to delay write. I don't think it's available.
> Can I add some kind of #if HAS_LAMBDA and make the test depend on it?
> I don't think adding slots and make the buffer an attribute would be very nice... I can also sub-class the buffer.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20150430/ef9b70ed/attachment.html>