Created attachment 238200 [details] [review]
downloader: Rename download method to download_remote_file

The following patch will add variables and parameters of the same name
and I couldn't think of a better name for those so I decided to rename
this method instead.

Created attachment 238201 [details] [review]
downloader: Add a utility Download class

A new private class that keeps info about each download, including its
progress. In following patch we'll make use of this to report progress
of already running downloads (which we currently don't).

Created attachment 238202 [details] [review]
downloader: Report progress of already running downloads

This is actually more like reporting progress of duplicate downloads.

Review of attachment 238202 [details] [review]:

Would be nice to add concrete descriptions of when this happens. Apparently this hits because we now download drivers at boxes startup in the background, so if someone starts boxes, and then starts gnome-boxes ./win.iso, then the latter will not show download progress

Review of attachment 238200 [details] [review]:

Why is it needed? As long as you are not adding a method with the same name, this shouldn't clash. I've tried that reverting this patch on top of the series does not break the build

Review of attachment 238202 [details] [review]:

Looks good with a better commitlog

Review of attachment 238201 [details] [review]:

Looks good otherwise

::: src/downloader.vala
@@ +59,3 @@
                                             ActivityProgress progress = new ActivityProgress ()) throws GLib.Error {
         var uri = remote_file.get_uri ();
+        var download = downloads.get (uri);

Slight preference for Download download there, this makes things easier if for example we need to pass 'download' to a helper function, no need to guess the type, it's already right there.

Review of attachment 238200 [details] [review]:

The issue isn't exactly clashing in the following patch itself but more of confusion/readability.

Review of attachment 238201 [details] [review]:

::: src/downloader.vala
@@ +59,3 @@
                                             ActivityProgress progress = new ActivityProgress ()) throws GLib.Error {
         var uri = remote_file.get_uri ();
+        var download = downloads.get (uri);

Well my preference is the opposite here and as you can see from the rest of the code and CodingStyle.txt, this is the prefered way for the project. Since its not really a big deal, we don't force it on your patches. :)

Created attachment 238294 [details] [review]
downloader: Report progress of already running downloads

v2: Better commit log.

Review of attachment 238201 [details] [review]:

::: src/downloader.vala
@@ +59,3 @@
                                             ActivityProgress progress = new ActivityProgress ()) throws GLib.Error {
         var uri = remote_file.get_uri ();
+        var download = downloads.get (uri);

Whereas the arguments are concerned, instead of looking at declaration of the variable, you look at the declaration/definition of the function (in this case, the declaration of 'downloads' hashtable). So only slightly inconveniently to find out the type but not enough to varant to have to type the Type for every variable. IMO, this is a very good balance between extreme verbosity of strong-typed languages and extreme flexibility of dynamic languages.

Not trying to convince you, just putting it on the record that I have reasons for my preference. :)

Review of attachment 238200 [details] [review]:

I'd rather we keep the best name for the public method, and change the internal names if you don't want to use download everywhere (I don't particularly mind). 'dload' instead of 'download' for the local variable maybe?

Review of attachment 238201 [details] [review]:

::: src/downloader.vala
@@ +59,3 @@
                                             ActivityProgress progress = new ActivityProgress ()) throws GLib.Error {
         var uri = remote_file.get_uri ();
+        var download = downloads.get (uri);

Well, in this case it's not too bad, it can get quite convoluted with stuff like var foo = get_foo_singleton().get_foo() where you have to hunt down get_foo_singleton() type in some external lib, and then the type returned by get_foo() once you have that type.

Using var everywhere is not a 'very good' balance, it's heavily skewed toward the convenience of the code writer, and detrimental to future code maintainance, the very good balance is when you use var only when using a constructor, and explicit types everywhere else.

Given that what a 'very good balance' is is highly subjective, let's keep it at that.

Review of attachment 238294 [details] [review]:

After the discussion on IRC, it seems we'll soon no longer have parallel downloads going on, but these changes seem worthwhile to have anyway, make the Download code more flexible.

Attachment 238201 [details] pushed as fc9a17a - downloader: Add a utility Download class
Attachment 238294 [details] pushed as 48e2bb5 - downloader: Report progress of already running downloads