D11649: Fix crash in the file slave

[David Faure]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20180329/e082f415/attachment.html>