Code review comment for lp:~mandel/ubuntu-download-manager/network-errors-management

> This MP and its description is confusing me:
>
> I see the change from 'emit q->error(errorMsg)' to 'q->emitError(errorMsg)',
> which will correctly set the state to ERROR in onError, but there are a bunch
> of other changes that you haven't described and I can't tell from the linked
> bugs what's going on:
>
> 1. What was causing the segfault bug, and which changes in here fixed it?
> 1a. Is it possible to add a test that triggers that bug? The tests added seem
> to just be about the ERROR state.
>

The segafault was caused because the DownloadQueue calls deleteLater when a Download is completed or canceled, once that is done the pointer in the _downloads array in the GroupDownload is pointer to unknown memory space and we go banannas due to that. I really cannot think of a good way to test that error specially now that the q is not longer present.

> 2. Is the change in cancelDownload() and pauseDownload() from a reverse loop
> to a regular foreach significant?
>

At the very beginning (with the use of the DownloadQueue) it was a good idea to do it in reverse order because it minimized the number of operations to be performed in the q, mainlye because the downloads being paused/canceled are not the 'current download'. With the q implementation if you paused from first to last the download q will attempt to recalculate the next download to be performed something that really is not required in a group. Once the Download Queue was out there was no need to be careful with the order.

>
> On an API style note - It took a while to understand why you had pairs of
> methods like cancel() and cancelDownload() - I guess it was because the
> DownloadQueue listens for the state change caused by cancel(), and then calls
> cancelDownload() itself. That seems like cancel() should be called something
> like setState(CANCELLED) or something like that, because
> 'download->cancelDownload()' is awkward - that should be 'download->cancel()',
> because it's actually doing the cancelling.

Yes, I agree that it is a funny way to do it, the main issue is that the cancel, start, etc.. methods are exposed by the DownloadAdapetor that does something of the following:

QMetaObject::invokeMethod(parent(), "cancel");

As you can see, if you use the adaptors generated by the qt tool you need to call the methods the same way as those found in the interface... I might move away from the xml-generated classes but I need to consider if that is a source of problems later in the future if we change the xml.

>
> It was also strange to me that the object can be in the "cancelled" (or
> "paused" or whatever) state, while not actually having been cancelled yet. So
> for each of these states, there's two states - "about to be <state>" and
> "really <state>". This seems like it might be a source of problems, and was
> definitely a source of confusion while reading. Anyway, probably not worth
> messing with here, but I thought I'd mention it.

That is not perse true atm, the Download state is correct, if you set it to be canceled is not about to be, is canceled, the main reason is that the DownloadQueue atm ensures that there is a singled download at a time (being a SingleDownload or a GroupDownload). When the state is changed the download check if there current download 'slot' is empty and does the correct action. I understand s a little confusing because the DownloadQueue takes the ownership of the actual method but that is to allow later (desktop/tablet) to have q implementations that have more than one download at a time.