> 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 > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
