dfaure added a comment.
canResume is an "internal" signal, I don't think any apps can do anything
meaningful with it, so I don't mind you connecting to it, except that I think
this is the wrong layer...
BTW did you notice the section about resuming in docs/design.txt? It might
help when thinking about all this.
INLINE COMMENTS
> jobtest.cpp:421
>
> +void JobTest::fileSlaveCrash()
> +{
That doesn't describe the behaviour anymore, after the fix ;)
Can you find a more descriptive name for which case this is testing?
E.g. I'm not sure why there are two storedPut jobs on the same data -- to test
data available after start and data available before start?
But then why not fill in putDataBuffer2 upfront, rather than after 200ms? For
readability/clarity, I mean.
> jobtest.cpp:440
> +
> + connect(job, &KJob::finished, [&job1Finished] {
> + job1Finished = true;
Apps (and unittests) are supposed to connect to `result`, not `finished` (which
is only for progress information to go away)
> jobtest.cpp:462
> + // Simulate the transfer is done
> + QTimer::singleShot(400, this, [&putDataBuffer, &putDataBuffer2](){
> + putDataBuffer.readChannelFinished();
Maybe do this inside the 200ms lambda, to 1) make sure it happens afterwards
(200<400 but QTimer can decide to do both together), and 2) shorten the delay
to make the test run faster?
> jobtest.cpp:467
> +
> + QTRY_VERIFY(job1Finished);
> + QTRY_VERIFY(job2Finished);
QVERIFY(job->exec()); would be shorter, removing the need for the first lambda,
no?
Or if you want to avoid an infinite hang if the job doesn't finish, QSignalSpy
on result + QVERIFY(spy.wait()).
I'm nitpicking, it's just that this form is a bit unusual and a bit more
verbose.
> storedtransferjob.cpp:98
> SLOT(slotStoredDataReq(KIO::Job*,QByteArray&)));
> + connect(this, &TransferJob::canResume, [&dd](KIO::Job*, KIO::filesize_t
> offset) {
> + if (offset != 0) {
The "Stored" in StoredTransferJob is just convenience (it stores the data for
you), that should have no relation to the handling of resume. So I think this
should be moved up to the base class TransferJob.
REPOSITORY
R241 KIO
REVISION DETAIL
https://phabricator.kde.org/D11649
To: aacid, #frameworks, dfaure
Cc: #kde_connect, michaelh, ngraham
