After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 729026 - Allow entering remote (HTTP & HTTPS) ISOs
Allow entering remote (HTTP & HTTPS) ISOs
Status: RESOLVED FIXED
Product: gnome-boxes
Classification: Applications
Component: wizard
unspecified
Other All
: Normal minor
: 3.22
Assigned To: Lasse Schuirmann
GNOME Boxes maintainer(s)
Depends on: 733170
Blocks: 732936
 
 
Reported: 2014-04-26 16:57 UTC by Lasse Schuirmann
Modified: 2016-03-31 13:22 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
First draft of a possible workflow (82.64 KB, application/pdf)
2014-04-26 16:57 UTC, Lasse Schuirmann
  Details
downloader: Make download cancellable (2.65 KB, patch)
2014-06-19 18:37 UTC, Lasse Schuirmann
needs-work Details | Review
util: Generalize cache getting, introduce iso cache (2.30 KB, patch)
2014-06-19 18:37 UTC, Lasse Schuirmann
needs-work Details | Review
wizard-source: Introduce has_to_download member (919 bytes, patch)
2014-06-19 18:37 UTC, Lasse Schuirmann
needs-work Details | Review
wizard: Add ISO downloading support (5.07 KB, patch)
2014-06-19 18:37 UTC, Lasse Schuirmann
needs-work Details | Review
wizard: Change indentation for better readability (1.73 KB, patch)
2014-06-19 18:37 UTC, Lasse Schuirmann
needs-work Details | Review
wizard: Show negative progress when analyzing (1.00 KB, patch)
2014-06-19 18:37 UTC, Lasse Schuirmann
reviewed Details | Review
downloader: Make download cancellable (3.78 KB, patch)
2014-06-23 15:09 UTC, Lasse Schuirmann
none Details | Review
util: Remove get_cache () (unneeded) (976 bytes, patch)
2014-06-23 15:09 UTC, Lasse Schuirmann
none Details | Review
util: Generalize cache getting (2.03 KB, patch)
2014-06-23 15:09 UTC, Lasse Schuirmann
none Details | Review
util: Introduce iso cache (925 bytes, patch)
2014-06-23 15:09 UTC, Lasse Schuirmann
none Details | Review
wizard-source: Introduce has_to_download prop (1.13 KB, patch)
2014-06-23 15:09 UTC, Lasse Schuirmann
none Details | Review
wizard: Add ISO downloading support (6.76 KB, patch)
2014-06-23 15:09 UTC, Lasse Schuirmann
none Details | Review
wizard: Reduce indentation for better readability (3.01 KB, patch)
2014-06-23 15:10 UTC, Lasse Schuirmann
none Details | Review
wizard: Show negative progress when analyzing (1.06 KB, patch)
2014-06-23 15:10 UTC, Lasse Schuirmann
none Details | Review
wizard-toolbar: Only click buttons when appropriate (1005 bytes, patch)
2014-06-23 15:10 UTC, Lasse Schuirmann
needs-work Details | Review
wizard: Codestyle changes (1.65 KB, patch)
2014-06-24 09:18 UTC, Lasse Schuirmann
none Details | Review
downloader: Make download cancellable (3.78 KB, patch)
2014-06-24 09:58 UTC, Lasse Schuirmann
needs-work Details | Review
util: Remove get_cache () (unneeded) (976 bytes, patch)
2014-06-24 09:58 UTC, Lasse Schuirmann
reviewed Details | Review
util: Generalize cache getting (2.03 KB, patch)
2014-06-24 09:58 UTC, Lasse Schuirmann
accepted-commit_now Details | Review
util: Introduce iso cache (925 bytes, patch)
2014-06-24 09:59 UTC, Lasse Schuirmann
rejected Details | Review
wizard-toolbar: Only click buttons when appropriate (1005 bytes, patch)
2014-06-24 09:59 UTC, Lasse Schuirmann
accepted-commit_now Details | Review
wizard-source: Introduce has_to_download prop (1.13 KB, patch)
2014-06-24 09:59 UTC, Lasse Schuirmann
needs-work Details | Review
wizard: Add ISO downloading support (6.76 KB, patch)
2014-06-24 09:59 UTC, Lasse Schuirmann
needs-work Details | Review
wizard: Codestyle changes (4.21 KB, patch)
2014-06-24 09:59 UTC, Lasse Schuirmann
accepted-commit_now Details | Review
wizard: Show negative progress when analyzing (1.06 KB, patch)
2014-06-24 09:59 UTC, Lasse Schuirmann
rejected Details | Review
downloader: Make download cancellable (3.59 KB, patch)
2014-06-26 16:47 UTC, Lasse Schuirmann
accepted-commit_now Details | Review
util: Generalize cache getting (2.48 KB, patch)
2014-06-26 16:47 UTC, Lasse Schuirmann
accepted-commit_now Details | Review
wizard-toolbar: Only click buttons when appropriate (1005 bytes, patch)
2014-06-26 16:47 UTC, Lasse Schuirmann
accepted-commit_now Details | Review
wizard-source: Introduce has_to_download prop (1.13 KB, patch)
2014-06-26 16:47 UTC, Lasse Schuirmann
needs-work Details | Review
downloader: Add fetch_iso () (1.85 KB, patch)
2014-06-26 16:47 UTC, Lasse Schuirmann
needs-work Details | Review
wizard: Make ActivityProgress for prepare_media() a member (2.57 KB, patch)
2014-06-26 16:47 UTC, Lasse Schuirmann
needs-work Details | Review
wizard: Add ISO downloading support (6.98 KB, patch)
2014-06-26 16:47 UTC, Lasse Schuirmann
needs-work Details | Review
wizard: Codestyle changes (4.17 KB, patch)
2014-06-26 16:47 UTC, Lasse Schuirmann
accepted-commit_now Details | Review
wizard-source: Introduce download_required prop (1.15 KB, patch)
2014-07-09 10:59 UTC, Lasse Schuirmann
accepted-commit_now Details | Review
downloader: Add fetch_iso() (1.90 KB, patch)
2014-07-09 10:59 UTC, Lasse Schuirmann
needs-work Details | Review
wizard: Create prep ActivityProgress on prepare() invokation (6.98 KB, patch)
2014-07-09 11:00 UTC, Lasse Schuirmann
needs-work Details | Review
wizard: Codestyle changes (6.09 KB, patch)
2014-07-09 11:00 UTC, Lasse Schuirmann
accepted-commit_now Details | Review
wizard: Add HTTP(S) ISO downloading support (7.11 KB, patch)
2014-07-09 11:00 UTC, Lasse Schuirmann
needs-work Details | Review
downloader: Add fetch_media() (1.94 KB, patch)
2014-07-14 15:57 UTC, Lasse Schuirmann
reviewed Details | Review
wizard: prepare() now report progress (6.61 KB, patch)
2014-07-14 15:57 UTC, Lasse Schuirmann
reviewed Details | Review
wizard: Codestyle changes (6.05 KB, patch)
2014-07-14 15:57 UTC, Lasse Schuirmann
reviewed Details | Review
wizard: Check basename for URIs (1.43 KB, patch)
2014-07-14 15:57 UTC, Lasse Schuirmann
needs-work Details | Review
wizard: Introduce media_download_cancellable (1.46 KB, patch)
2014-07-14 15:57 UTC, Lasse Schuirmann
reviewed Details | Review
wizard: Introduce download_media () (2.35 KB, patch)
2014-07-14 15:57 UTC, Lasse Schuirmann
needs-work Details | Review
wizard: Introduce ISO downloading feature (2.54 KB, patch)
2014-07-14 15:57 UTC, Lasse Schuirmann
none Details | Review
wizard: Introduce ISO downloading feature (2.81 KB, patch)
2014-07-14 16:05 UTC, Lasse Schuirmann
needs-work Details | Review
downloader: Add fetch_media() (1.94 KB, patch)
2014-07-15 11:15 UTC, Lasse Schuirmann
none Details | Review
wizard: prepare() now reports progress (6.61 KB, patch)
2014-07-15 11:15 UTC, Lasse Schuirmann
none Details | Review
wizard: Codestyle changes (6.05 KB, patch)
2014-07-15 11:15 UTC, Lasse Schuirmann
none Details | Review
wizard: Check basename for URIs (1.51 KB, patch)
2014-07-15 11:15 UTC, Lasse Schuirmann
none Details | Review
wizard: Introduce prepare_cancellable (1.52 KB, patch)
2014-07-15 11:16 UTC, Lasse Schuirmann
none Details | Review
wizard: Introduce download_media () (2.39 KB, patch)
2014-07-15 11:16 UTC, Lasse Schuirmann
none Details | Review
wizard: Introduce media downloading feature (2.65 KB, patch)
2014-07-15 11:16 UTC, Lasse Schuirmann
none Details | Review
wizard: Introduce media downloading feature (2.66 KB, patch)
2014-07-15 11:21 UTC, Lasse Schuirmann
needs-work Details | Review
wizard: Introduce media downloading feature (2.56 KB, patch)
2014-07-15 12:19 UTC, Lasse Schuirmann
none Details | Review
downloader: Make download cancellable (3.41 KB, patch)
2014-07-30 12:32 UTC, Lasse Schuirmann
none Details | Review
util: Generalize cache getting (2.48 KB, patch)
2014-07-30 12:32 UTC, Lasse Schuirmann
none Details | Review
wizard-toolbar: Only click buttons when appropriate (1005 bytes, patch)
2014-07-30 12:32 UTC, Lasse Schuirmann
none Details | Review
wizard-source: Introduce download_required prop (1.15 KB, patch)
2014-07-30 12:32 UTC, Lasse Schuirmann
none Details | Review
downloader: Add fetch_media() (1.94 KB, patch)
2014-07-30 12:32 UTC, Lasse Schuirmann
none Details | Review
wizard: prepare() now reports progress (6.61 KB, patch)
2014-07-30 12:32 UTC, Lasse Schuirmann
none Details | Review
wizard: Codestyle changes (6.05 KB, patch)
2014-07-30 12:32 UTC, Lasse Schuirmann
none Details | Review
wizard: Check basename for URIs (1.51 KB, patch)
2014-07-30 12:32 UTC, Lasse Schuirmann
none Details | Review
wizard: Introduce prepare_cancellable (1.52 KB, patch)
2014-07-30 12:33 UTC, Lasse Schuirmann
none Details | Review
wizard: Introduce download_media () (2.39 KB, patch)
2014-07-30 12:33 UTC, Lasse Schuirmann
none Details | Review
wizard: Introduce media downloading feature (2.56 KB, patch)
2014-07-30 12:33 UTC, Lasse Schuirmann
none Details | Review
downloader: Make download cancellable (3.80 KB, patch)
2014-08-01 13:44 UTC, Zeeshan Ali
committed Details | Review
util: Generalize cache getting (2.48 KB, patch)
2014-08-01 13:44 UTC, Zeeshan Ali
committed Details | Review
wizard-toolbar: Only click buttons when appropriate (1005 bytes, patch)
2014-08-01 13:45 UTC, Zeeshan Ali
committed Details | Review
wizard-source: Introduce download_required prop (1.15 KB, patch)
2014-08-01 13:45 UTC, Zeeshan Ali
committed Details | Review
downloader: Add fetch_media() (1.94 KB, patch)
2014-08-01 13:45 UTC, Zeeshan Ali
committed Details | Review
wizard: prepare() now reports progress (6.01 KB, patch)
2014-08-01 13:46 UTC, Zeeshan Ali
committed Details | Review
wizard: Codestyle changes (6.06 KB, patch)
2014-08-01 13:46 UTC, Zeeshan Ali
committed Details | Review
wizard: Allow entering of HTTP(S) URIs (1.66 KB, patch)
2014-08-01 13:46 UTC, Zeeshan Ali
committed Details | Review
wizard: Introduce prepare_cancellable (1.52 KB, patch)
2014-08-01 13:46 UTC, Zeeshan Ali
committed Details | Review
wizard: Introduce download_media () (2.34 KB, patch)
2014-08-01 13:47 UTC, Zeeshan Ali
none Details | Review
wizard: Introduce media downloading feature (2.86 KB, patch)
2014-08-01 13:47 UTC, Zeeshan Ali
none Details | Review
wizard: Introduce media downloading feature (2.54 KB, patch)
2014-08-01 13:54 UTC, Zeeshan Ali
none Details | Review
wizard: Introduce download_media () (2.39 KB, patch)
2014-08-01 13:56 UTC, Zeeshan Ali
committed Details | Review
wizard: Introduce media downloading feature (2.20 KB, patch)
2014-08-01 13:57 UTC, Zeeshan Ali
none Details | Review
wizard: Introduce media downloading feature (2.21 KB, patch)
2014-08-01 14:02 UTC, Zeeshan Ali
committed Details | Review

Description Lasse Schuirmann 2014-04-26 16:57:48 UTC
Created attachment 275219 [details]
First draft of a possible workflow

Hello everyone,

as part of the Google Summer of Code I am planning to implement the ISO downloading for Boxes.

The appended document contains a first draft for a possible workflow for ISO downloading process. I'd be glad if some of you take a short look at it and I'd love to read some comments on this.
Comment 1 Zeeshan Ali 2014-04-28 13:19:46 UTC
Thanks for taking the time to draw and flowchart but I'm not sure I follow.

Its simple actually:

* We allow users to enter HTTP(s) URLs.
* On remote URL:
  * We download first in preparation step
  * Once downloaded, we continue like we do for local files:
    * try to detect the OS
    * If its a known OS, we update the page with OS info and download logos/drivers

Once this is in place, we should also allow the same mechanism be used by us and distros to provide ready links to installers/images on the source page menu, through a simple ini file (GKeyFile). In that case, the info about the OS and media could be provided by the ini file and detection could then be skipped.
Comment 2 Christophe Fergeau 2014-04-28 13:39:16 UTC
Fwiw, the libosinfo database already has some URLs to installers ISOs.
Comment 3 Zeeshan Ali 2014-04-28 13:42:52 UTC
(In reply to comment #2)
> Fwiw, the libosinfo database already has some URLs to installers ISOs.

True but we can't show them all in the menu so there needs to be a way to filter them. This ini file could just be a list of libosinfo media IDs. While we are at it, we should allow people to add/override some info, e.g title, vendor etc too.
Comment 4 Lasse Schuirmann 2014-04-28 13:54:35 UTC
As I understood this we agreed on the following:

1. There will be an initial implementation of the ISO downloading feature that allows only textual input of the URL, download it and then recognize the OS.
2. If this is working we can recognize the URL via the URLs in libosinfo. If the URL is recognized as express installable we can go to setup page directly.
3. If 2. works we can think about filtering. I thought that we make a kind of search field out of the URL input field. You can
  a) input a spice URL or some other stuff like this
  b) input an ISO URL
  c) input a distribution name
In the last case we just look up in the libosinfo database so that we can "auto-complete" the URL.

(Be aware that the 3rd goal is an optional goal for this project and might not be implemented by the GSOC deadline.)
Comment 5 Zeeshan Ali 2014-04-28 15:00:44 UTC
(In reply to comment #4)
> As I understood this we agreed on the following:
> 
> 1. There will be an initial implementation of the ISO downloading feature that
> allows only textual input of the URL, download it and then recognize the OS.
> 2. If this is working we can recognize the URL via the URLs in libosinfo.

Actually I didn't think of detection through URL searching in osinfo database at all but yeah that feature could be added after 1 is in the place.

> If
> the URL is recognized as express installable we can go to setup page directly.

That is already implemented so we shouldn't need to do anything special here.

> 3. If 2. works we can think about filtering. I thought that we make a kind of
> search field out of the URL input field. You can
>   a) input a spice URL or some other stuff like this
>   b) input an ISO URL
>   c) input a distribution name
> In the last case we just look up in the libosinfo database so that we can
> "auto-complete" the URL.

Sounds awesome!
 
> (Be aware that the 3rd goal is an optional goal for this project and might not
> be implemented by the GSOC deadline.)

Sure thing! I can help too. :)
Comment 6 Lasse Schuirmann 2014-05-02 18:41:24 UTC
Thank you for the input.

Here is the new workflow we developed in todays discussion. The different versions will be implemented after each other:
(<this> indicates a click to a ui element labeled with "this")

First version:
1. <New>
2. <Continue>
3. <Enter URL>
4. "http://mirror.com/my_cool.iso"
5. Show download progress on preparation page
6. Show setup page if applicable, <Continue>
7. Show review page, <Create>

Second version:
1. - 4. like 1st version
4.1. URL is recognized as express installable by libosinfo:
  5. Show setup page, <Continue>
  6. Show review page, <Create>
  7. Show a download progress (like epiphany) on the bottom of a dummy thumbnail
4.2. URL not recognized as express installable
  -> Go to first version, step 5

Third version:
1. - 4. like 1st version
4.1. URL is recognized as express installable by libosinfo:
  -> Go to second version, 4.1.
4.2. URL not recognized as express installable
  5. Begin download
  6. As soon as enought data is available to perform detection detect (non-)express installable media
  7. Go to second version 4.1. if express installable, 4.2 if not

The following functionalities are on the whishlist for the downloading:
- automatic pausing/resuming of downloading when closing/reopening Boxes
- manual pausing/resuming of the download
- if download fails, go back to wizard on click on the VM and tell the user about this
- if download succeeds and express installation fails, go back to wizard and let the user know about the error; allow him to change settings
Comment 7 Zeeshan Ali 2014-05-04 17:44:35 UTC
(In reply to comment #6)
> Thank you for the input.
> 
> Here is the new workflow we developed in todays discussion. The different
> versions will be implemented after each other:
> (<this> indicates a click to a ui element labeled with "this")

Nice work with describing all the steps and iterations. Some notes below.
 
> First version:
> 1. <New>
> 2. <Continue>
> 3. <Enter URL>
> 4. "http://mirror.com/my_cool.iso"
> 5. Show download progress on preparation page
> 6. Show setup page if applicable, <Continue>
> 7. Show review page, <Create>

That would actually already fix this bug so I think after we are done with this iteration, we should put rest into separate bugs.

> Second version:
> 1. - 4. like 1st version
> 4.1. URL is recognized as express installable by libosinfo:

I think this description is mixing up 'autocompletion of ISO entry using recognition of URL from libosinfo' and 'express installation'. i-e just cause ISO is recognised by libosinfo, doesn't mean express installation is supported.

I think this second iteration needs further division:

1. Autocomplete ISOs in the 'Enter URL entry' by searching through ISOs.
2. Move downloading to outside of wizard (as part of installation).

>   5. Show setup page, <Continue>
>   6. Show review page, <Create>
>   7. Show a download progress (like epiphany) on the bottom of a dummy
> thumbnail
>
> 4.2. URL not recognized as express installable
>   -> Go to first version, step 5

I don't see why for unknown OS (I think you meant that instead of non-express installable) we can't show progress in collection view as well.
Comment 8 Lasse Schuirmann 2014-06-19 18:37:04 UTC
The following patches represent the first iteration.
Comment 9 Lasse Schuirmann 2014-06-19 18:37:15 UTC
Created attachment 278792 [details] [review]
downloader: Make download cancellable

This will be used in the following patches to introduce ISO
downloading.
Comment 10 Lasse Schuirmann 2014-06-19 18:37:19 UTC
Created attachment 278793 [details] [review]
util: Generalize cache getting, introduce iso cache

ISO cache will be needed by the following patches to introduce ISO
downloading.
Comment 11 Lasse Schuirmann 2014-06-19 18:37:23 UTC
Created attachment 278794 [details] [review]
wizard-source: Introduce has_to_download member

This function will be used by the following patches to determine wether
a remote source has to be downloaded or not.
Comment 12 Lasse Schuirmann 2014-06-19 18:37:26 UTC
Created attachment 278795 [details] [review]
wizard: Add ISO downloading support
Comment 13 Lasse Schuirmann 2014-06-19 18:37:30 UTC
Created attachment 278796 [details] [review]
wizard: Change indentation for better readability
Comment 14 Lasse Schuirmann 2014-06-19 18:37:34 UTC
Created attachment 278797 [details] [review]
wizard: Show negative progress when analyzing
Comment 15 Zeeshan Ali 2014-06-20 12:00:57 UTC
Review of attachment 278793 [details] [review]:

Short log mentions two changes and hence it should be two separate patches.

::: src/util.vala
@@ +115,3 @@
+    }
+
+    public string get_universal_cache (string cache_name, string? file_name = null) {

universal?
Comment 16 Zeeshan Ali 2014-06-20 12:07:34 UTC
Review of attachment 278792 [details] [review]:

Looks good.

Its not exactly required for ISO downloading itself so commit log details actually doesn't say anything. Either mention the exact requirement of following patch or just omit description as async calls should ideally always be cancellable anyway (you can write this as rationale too).

::: src/downloader.vala
@@ +122,3 @@
+                session.cancel_message (msg, Soup.Status.CANCELLED);
+            });
+

You also want to not throw error on msg.status_code != Soup.Status.CANCELLED below.
Comment 17 Zeeshan Ali 2014-06-20 12:16:38 UTC
Review of attachment 278794 [details] [review]:

Nitckpick on shortlog: 'member' is redundant and inconsistent. Usually you put '()' after the name to denote a method/function. In case of property, ' prop'. Same with signals.

::: src/wizard-source.vala
@@ +133,3 @@
     public MediaManager media_manager;
 
+    public bool has_to_download () {

This should be a property.

@@ +134,3 @@
 
+    public bool has_to_download () {
+        return uri.contains ("http://");

* It doesn't have to be HTTP specific. It could be an gvfs URI for NFS or SAMBA share as well and Downloader.download can handle those cases. Instead you could check if it has ':' in it and doesn't have prefix 'file:'.
* there is has_prefix method on string.
Comment 18 Zeeshan Ali 2014-06-20 12:36:06 UTC
Review of attachment 278795 [details] [review]:

Good stuff. :) If its kept HTTP(S)-specific, I think log should reflect that.

::: src/wizard.vala
@@ +192,2 @@
     public void cleanup () {
+        if (download_iso_cancellable != null)

download_iso_cancellable -> iso_download_cancellable sounds better in English. :)

@@ +276,3 @@
         if (uri.scheme == "spice") {
             spice_validate_uri (uri_as_text);
+        } else if (uri.scheme == "vnc" || uri.scheme == "http") {

As I said in another comment, I don't think we need to keep it specific to HTTP. Maybe we should just check the '.iso' or '.ISO' suffix instead. Later we can see if we can simply add '.img' and other formats too in the same way and re-use the same code.

@@ +541,3 @@
     }
 
+    private Cancellable? download_iso_cancellable;

This should be move up with other props/fields.

@@ +552,3 @@
+        var progress = new ActivityProgress ();
+        progress.notify["progress"].connect (() => {
+            if (Math.fabs (progress.progress - prep_progress.fraction) >= 0.01)

* Don't think there is a need for Math.fabs here.
* So do I understand correctly that progress bar goes from 0-100% for download first and then it goes back from 0-100 for rest? Hmm.. I'm not actually sure wether its better this way or having it as one progress for everything.

@@ +566,3 @@
+        prep_status_label.label = "Downloading ISO...";
+
+        new GLib.Thread<void*> (null, () => {

Why do you need a thread here? download() is async already and this method being async can just yield on that.

@@ +572,3 @@
+                                       progress,
+                                       download_iso_cancellable,
+                                       (obj, res) => {

The level of nesting in here is hurting my eye. :(
Comment 19 Zeeshan Ali 2014-06-20 12:43:27 UTC
Review of attachment 278796 [details] [review]:

::: src/wizard.vala
@@ +335,3 @@
             download_iso.begin (wizard_source.uri);
             return true;
+        }

Could you add a newline above all the return statements here as per coding style to further improve readability?
Comment 20 Zeeshan Ali 2014-06-20 12:44:58 UTC
Review of attachment 278797 [details] [review]:

Missing "Don't" in commit log?

::: src/wizard.vala
@@ +317,3 @@
         var progress = new ActivityProgress ();
         progress.notify["progress"].connect (() => {
+            if (Math.fabs (progress.progress - prep_progress.fraction) >= 0.01) // Only entertain >= 1% change

In which case there is a negative progress? Seem to me like its trying to hide the actual problem.
Comment 21 Christophe Fergeau 2014-06-23 09:17:33 UTC
Review of attachment 278794 [details] [review]:

::: src/wizard-source.vala
@@ +134,3 @@
 
+    public bool has_to_download () {
+        return uri.contains ("http://");

maybe use g_vfs_get_supported_uri_schemes () and g_uri_parse_scheme () ?
Comment 22 Lasse Schuirmann 2014-06-23 11:38:55 UTC
Review of attachment 278797 [details] [review]:

The commit log is as it should.

::: src/wizard.vala
@@ +317,3 @@
         var progress = new ActivityProgress ();
         progress.notify["progress"].connect (() => {
+            if (Math.fabs (progress.progress - prep_progress.fraction) >= 0.01) // Only entertain >= 1% change

You'd have negative progress when you want to reset the progress bar to zero.
Comment 23 Lasse Schuirmann 2014-06-23 12:07:24 UTC
Review of attachment 278792 [details] [review]:

::: src/downloader.vala
@@ +122,3 @@
+                session.cancel_message (msg, Soup.Status.CANCELLED);
+            });
+

ok.
Comment 24 Lasse Schuirmann 2014-06-23 12:09:09 UTC
Review of attachment 278795 [details] [review]:

::: src/wizard.vala
@@ +192,2 @@
     public void cleanup () {
+        if (download_iso_cancellable != null)

Ok with me.

@@ +276,3 @@
         if (uri.scheme == "spice") {
             spice_validate_uri (uri_as_text);
+        } else if (uri.scheme == "vnc" || uri.scheme == "http") {

Or we could just ask has_to_download and handle the issue there.

@@ +541,3 @@
     }
 
+    private Cancellable? download_iso_cancellable;

I did this because the review_cancellable in line 379 is also declared before review (). Thought it might be easier to read. For consistency I'd move both up then.

@@ +552,3 @@
+        var progress = new ActivityProgress ();
+        progress.notify["progress"].connect (() => {
+            if (Math.fabs (progress.progress - prep_progress.fraction) >= 0.01)

* Don't think there is a need for Math.fabs here.
If it isnt there the progress bar will never reset to 0.

* So do I understand correctly that progress bar goes from 0-100% for download first and then it goes back from 0-100 for rest? Hmm.. I'm not actually sure wether its better this way or having it as one progress for everything.

We don't know what other preparation steps are needed. To be clear to the user it would be best in my mind to have two progress bars for the whole process. (I remember firefox installer does the same on windows; I quite liked that, its honest.)

@@ +566,3 @@
+        prep_status_label.label = "Downloading ISO...";
+
+        new GLib.Thread<void*> (null, () => {

True.

@@ +572,3 @@
+                                       progress,
+                                       download_iso_cancellable,
+                                       (obj, res) => {

Yes. And its a lambda in a lambda. Very ugly. This is solved since I remove the thread anyway.
Comment 25 Lasse Schuirmann 2014-06-23 12:09:11 UTC
Review of attachment 278795 [details] [review]:

::: src/wizard.vala
@@ +192,2 @@
     public void cleanup () {
+        if (download_iso_cancellable != null)

Ok with me.

@@ +276,3 @@
         if (uri.scheme == "spice") {
             spice_validate_uri (uri_as_text);
+        } else if (uri.scheme == "vnc" || uri.scheme == "http") {

Or we could just ask has_to_download and handle the issue there.

@@ +541,3 @@
     }
 
+    private Cancellable? download_iso_cancellable;

I did this because the review_cancellable in line 379 is also declared before review (). Thought it might be easier to read. For consistency I'd move both up then.

@@ +552,3 @@
+        var progress = new ActivityProgress ();
+        progress.notify["progress"].connect (() => {
+            if (Math.fabs (progress.progress - prep_progress.fraction) >= 0.01)

* Don't think there is a need for Math.fabs here.
If it isnt there the progress bar will never reset to 0.

* So do I understand correctly that progress bar goes from 0-100% for download first and then it goes back from 0-100 for rest? Hmm.. I'm not actually sure wether its better this way or having it as one progress for everything.

We don't know what other preparation steps are needed. To be clear to the user it would be best in my mind to have two progress bars for the whole process. (I remember firefox installer does the same on windows; I quite liked that, its honest.)

@@ +566,3 @@
+        prep_status_label.label = "Downloading ISO...";
+
+        new GLib.Thread<void*> (null, () => {

True.

@@ +572,3 @@
+                                       progress,
+                                       download_iso_cancellable,
+                                       (obj, res) => {

Yes. And its a lambda in a lambda. Very ugly. This is solved since I remove the thread anyway.
Comment 26 Lasse Schuirmann 2014-06-23 12:11:45 UTC
Review of attachment 278794 [details] [review]:

::: src/wizard-source.vala
@@ +133,3 @@
     public MediaManager media_manager;
 
+    public bool has_to_download () {

Ok.

@@ +134,3 @@
 
+    public bool has_to_download () {
+        return uri.contains ("http://");

I'll look into it. Thanks for the suggestions!
Comment 27 Lasse Schuirmann 2014-06-23 12:16:21 UTC
Review of attachment 278793 [details] [review]:

I'll split it up, thats sounds better indeed.

::: src/util.vala
@@ +115,3 @@
+    }
+
+    public string get_universal_cache (string cache_name, string? file_name = null) {

Yeah, thats a bit bad indeed. I've just noticed that the get_cache method is used nowhere (which is good and modular!) so I can remove it and name this method get_cache. Sorry for not doing this earlier.
Comment 28 Lasse Schuirmann 2014-06-23 14:33:23 UTC
Review of attachment 278796 [details] [review]:

I am rebasing this to the new parents. Also adding few more stylefixes.
Comment 29 Lasse Schuirmann 2014-06-23 15:09:34 UTC
Created attachment 279033 [details] [review]
downloader: Make download cancellable
Comment 30 Lasse Schuirmann 2014-06-23 15:09:39 UTC
Created attachment 279034 [details] [review]
util: Remove get_cache () (unneeded)

This function was introduced when solving bug#698144 but never used.
Comment 31 Lasse Schuirmann 2014-06-23 15:09:43 UTC
Created attachment 279035 [details] [review]
util: Generalize cache getting
Comment 32 Lasse Schuirmann 2014-06-23 15:09:47 UTC
Created attachment 279036 [details] [review]
util: Introduce iso cache

ISO cache will be needed by the following patches to introduce ISO
downloading.
Comment 33 Lasse Schuirmann 2014-06-23 15:09:51 UTC
Created attachment 279037 [details] [review]
wizard-source: Introduce has_to_download prop

This function will be used by the following patches to determine
whether a remote source has to be downloaded or not.
Comment 34 Lasse Schuirmann 2014-06-23 15:09:56 UTC
Created attachment 279038 [details] [review]
wizard: Add ISO downloading support
Comment 35 Lasse Schuirmann 2014-06-23 15:10:00 UTC
Created attachment 279039 [details] [review]
wizard: Reduce indentation for better readability
Comment 36 Lasse Schuirmann 2014-06-23 15:10:04 UTC
Created attachment 279040 [details] [review]
wizard: Show negative progress when analyzing

This is necessary for e.g. resetting the progress bar.
Comment 37 Lasse Schuirmann 2014-06-23 15:10:08 UTC
Created attachment 279041 [details] [review]
wizard-toolbar: Only click buttons when appropriate

Buttons should only be clicked if they are sensitive.
Comment 38 Lasse Schuirmann 2014-06-23 16:08:01 UTC
Review of attachment 279041 [details] [review]:

Wrong order, sorry. This patch has to go before the introduction of the ISO download functionality since this breaks the keyboard navigation in its current implementation. This does not change the patch.
Comment 39 Lasse Schuirmann 2014-06-24 09:18:38 UTC
Created attachment 279089 [details] [review]
wizard: Codestyle changes

This moves some properties up and corrects an indentation error.

This fixes some inconsistency introduced by the "Add ISO downloading support" patch.
Comment 40 Lasse Schuirmann 2014-06-24 09:58:48 UTC
Created attachment 279093 [details] [review]
downloader: Make download cancellable
Comment 41 Lasse Schuirmann 2014-06-24 09:58:53 UTC
Created attachment 279094 [details] [review]
util: Remove get_cache () (unneeded)

This function was introduced when solving bug#698144 but never used.
Comment 42 Lasse Schuirmann 2014-06-24 09:58:58 UTC
Created attachment 279095 [details] [review]
util: Generalize cache getting
Comment 43 Lasse Schuirmann 2014-06-24 09:59:03 UTC
Created attachment 279096 [details] [review]
util: Introduce iso cache

ISO cache will be needed by the following patches to introduce ISO
downloading.
Comment 44 Lasse Schuirmann 2014-06-24 09:59:07 UTC
Created attachment 279097 [details] [review]
wizard-toolbar: Only click buttons when appropriate

Buttons should only be clicked if they are sensitive.
Comment 45 Lasse Schuirmann 2014-06-24 09:59:12 UTC
Created attachment 279098 [details] [review]
wizard-source: Introduce has_to_download prop

This function will be used by the following patches to determine
whether a remote source has to be downloaded or not.
Comment 46 Lasse Schuirmann 2014-06-24 09:59:16 UTC
Created attachment 279099 [details] [review]
wizard: Add ISO downloading support
Comment 47 Lasse Schuirmann 2014-06-24 09:59:21 UTC
Created attachment 279100 [details] [review]
wizard: Codestyle changes

- Reduce indentation for better readability
- Correct indentation error
- Move properties up
Comment 48 Lasse Schuirmann 2014-06-24 09:59:25 UTC
Created attachment 279101 [details] [review]
wizard: Show negative progress when analyzing

This is necessary for e.g. resetting the progress bar.
Comment 49 Lasse Schuirmann 2014-06-24 10:05:49 UTC
I just reordered the patches and merged the codestyle patches. No need to look at any old ones.
Comment 50 Lasse Schuirmann 2014-06-24 10:08:48 UTC
Review of attachment 279093 [details] [review]:

::: src/downloader.vala
@@ +62,2 @@
      *                     first element points to.
      * @param progress The ActivityProgress object to report progress to.

I didn't add the trivial @param tag here. If you want it there for consistency just say it.
Comment 51 Lasse Schuirmann 2014-06-24 10:16:23 UTC
Review of attachment 279098 [details] [review]:

::: src/wizard-source.vala
@@ +135,3 @@
+    public bool has_to_download {
+        get {
+            const string[] supported_schemes = { "http", "https" };

I think its better to hardcode the supported formats.

The suggested g_vfs_get_supported_uri_schemes returns also formats that are handled locally (see wizard.vala, prepare_for_location and prepare_for_uri). Checking for the : would mean we had to have an exclude list (file, smb, spice, vnc are handled elsewhere), check in addition if the protocol has a "qemu" prefix and so on.

This is the easiest and cleanest possibility and to have this list of supported schemes. If you're missing a scheme, just name it.
Comment 52 Zeeshan Ali 2014-06-25 13:31:08 UTC
Review of attachment 278797 [details] [review]:

::: src/wizard.vala
@@ +317,3 @@
         var progress = new ActivityProgress ();
         progress.notify["progress"].connect (() => {
+            if (Math.fabs (progress.progress - prep_progress.fraction) >= 0.01) // Only entertain >= 1% change

That would be another reason we shouldn't reset it to zero while at preparation step. Also we should ensure that this notify is no longer in place after we are done with preparation step.
Comment 53 Zeeshan Ali 2014-06-25 13:43:19 UTC
Review of attachment 278795 [details] [review]:

::: src/wizard.vala
@@ +552,3 @@
+        var progress = new ActivityProgress ();
+        progress.notify["progress"].connect (() => {
+            if (Math.fabs (progress.progress - prep_progress.fraction) >= 0.01)

We don't need to know the exact amount of time/steps beforehand. Just divide the progress into two sub-progresses, one for download and the other for the rest. We should assign most of it to download since other operations are local and will typically be much faster than download. If you see closely, I'm already doing this division for all the steps.

And no, having two progressbars sounds like a bad UX as user is given the illusion that preparation step is 99% complete while it isn't. If we gave them a clue that there is more to follow after download, that it would be fine.
Comment 54 Zeeshan Ali 2014-06-25 17:07:13 UTC
Review of attachment 279093 [details] [review]:

almost there. :)

::: src/downloader.vala
@@ +62,2 @@
      *                     first element points to.
      * @param progress The ActivityProgress object to report progress to.

Yes please.

@@ +95,3 @@
+                yield download_from_http (download, cancellable);
+                if (cancellable != null && cancellable.is_cancelled ()) {
+                    debug ("Download has been cancelled.");

This probably won't be very useful w/o the URI
Comment 55 Zeeshan Ali 2014-06-25 17:09:33 UTC
Review of attachment 279094 [details] [review]:

Why not 'Remove redunant get_cache()' ? Space before '(' in commit logs is optional btw. :)
Comment 56 Zeeshan Ali 2014-06-25 17:11:32 UTC
Review of attachment 279094 [details] [review]:

There is no point of this patch if you are going to re-introduce the same function in the next patch. :) You want to squash them.
Comment 57 Zeeshan Ali 2014-06-25 17:13:20 UTC
Review of attachment 279095 [details] [review]:

Looks good.
Comment 58 Zeeshan Ali 2014-06-25 17:14:47 UTC
Review of attachment 279096 [details] [review]:

I don't think ISO is something that should be hidden from user. You want to download it in ~/Downloads.
Comment 59 Zeeshan Ali 2014-06-25 17:16:13 UTC
Review of attachment 279096 [details] [review]:

g_get_user_special_dir (G_USER_DIRECTORY_DOWNLOAD) in C.
Comment 60 Zeeshan Ali 2014-06-25 17:17:32 UTC
Review of attachment 279097 [details] [review]:

ACK
Comment 61 Zeeshan Ali 2014-06-25 17:21:23 UTC
Review of attachment 279098 [details] [review]:

::: src/wizard-source.vala
@@ +135,3 @@
+    public bool has_to_download {
+        get {
+            const string[] supported_schemes = { "http", "https" };

The exclude list is fine (SMB isn't in this list, is it?) as its not big and there is no reason to not support non-HTTP(s) just to avoid this list.
Comment 62 Zeeshan Ali 2014-06-25 17:40:08 UTC
Review of attachment 279099 [details] [review]:

::: src/wizard.vala
@@ +157,3 @@
             try {
+                if (wizard_source.has_to_download) {
+                    text = _("Will download an ISO and add a single box.");

I think it would be very trivial to add support for downloading of images once all this is in place and we should add that soon after. So better have a generic message here.

However. I don't think we need a special message here. If user provides/types URL, pretty sure they know downloading is involved. If its from media menu, user doesn't see this screen anyway.

@@ +555,3 @@
+    }
+
+    private async void download_iso (string uri) {

Pretty big function. Pretty sure you noticed too and are annoyed by it. :) How about dividing it? Feel free to move most/some of it to Downloader btw. We already have a download_avatar in there and its more to do with downloading than the fact that its an ISO.

@@ +558,3 @@
+        var progress = new ActivityProgress ();
+        progress.notify["progress"].connect (() => {
+            if (Math.fabs (progress.progress - prep_progress.fraction) >= 0.01)

As I pointed in last review on previous version of this patch, we want a single progress bar.

@@ +563,3 @@
+        progress.bind_property ("info", prep_status_label, "label");
+
+        var downloader = Downloader.get_instance ();

I'd move this down before its needed cause you could be exitting early and this object is then useless.

@@ +585,3 @@
+            debug ("Downloading ISO from '%s' to '%s'.", uri, cache_path);
+            yield downloader.download (file, {cache_path}, progress, iso_download_cancellable);
+            return_to_source = iso_download_cancellable.is_cancelled ();

You don't want to check for that but rather catch IOError.CANCELLED. If you are not getting that error thrown on cancellation, its a bug in Downloader.download(). Also that way you don't need to complicate the code with return_to_source variable.
Comment 63 Zeeshan Ali 2014-06-25 17:41:28 UTC
Review of attachment 279100 [details] [review]:

ack
Comment 64 Zeeshan Ali 2014-06-25 17:42:45 UTC
Review of attachment 279101 [details] [review]:

Show -> Don't show? :) I don't think you want to use this for resetting but rather reset it explicitly when needed.
Comment 65 Lasse Schuirmann 2014-06-25 17:44:22 UTC
Review of attachment 279098 [details] [review]:

::: src/wizard-source.vala
@@ +135,3 @@
+    public bool has_to_download {
+        get {
+            const string[] supported_schemes = { "http", "https" };

smb is handled in wizard.vala, prepare_for_location. An exclude list will not do since prepare_for_uri apparently accepts any scheme with an qemu _prefix_ and then there are these brokers which add support for some other schemes too.
Comment 66 Zeeshan Ali 2014-06-26 16:46:09 UTC
Review of attachment 279098 [details] [review]:

::: src/wizard-source.vala
@@ +135,3 @@
+    public bool has_to_download {
+        get {
+            const string[] supported_schemes = { "http", "https" };

hmm.. yeah you might have a point. Lets go with whitelist then and HTTP-specific for now.
Comment 67 Lasse Schuirmann 2014-06-26 16:47:20 UTC
Created attachment 279322 [details] [review]
downloader: Make download cancellable
Comment 68 Lasse Schuirmann 2014-06-26 16:47:24 UTC
Created attachment 279323 [details] [review]
util: Generalize cache getting
Comment 69 Lasse Schuirmann 2014-06-26 16:47:29 UTC
Created attachment 279324 [details] [review]
wizard-toolbar: Only click buttons when appropriate

Buttons should only be clicked if they are sensitive.
Comment 70 Lasse Schuirmann 2014-06-26 16:47:34 UTC
Created attachment 279325 [details] [review]
wizard-source: Introduce has_to_download prop

This function will be used by the following patches to determine
whether a remote source has to be downloaded or not.
Comment 71 Lasse Schuirmann 2014-06-26 16:47:38 UTC
Created attachment 279326 [details] [review]
downloader: Add fetch_iso ()
Comment 72 Lasse Schuirmann 2014-06-26 16:47:43 UTC
Created attachment 279327 [details] [review]
wizard: Make ActivityProgress for prepare_media() a member

This allows introducing subprocesses for the preparation step.
Comment 73 Zeeshan Ali 2014-06-26 16:47:46 UTC
Review of attachment 279098 [details] [review]:

::: src/wizard-source.vala
@@ +133,3 @@
     public MediaManager media_manager;
 
+    public bool has_to_download {

nitpick: download_required would be better IMHO.

@@ +140,3 @@
+                return true;
+
+            return false;

I'd do return ((scheme != null && scheme in supported_schemes);
Comment 74 Lasse Schuirmann 2014-06-26 16:47:54 UTC
Created attachment 279328 [details] [review]
wizard: Add ISO downloading support
Comment 75 Lasse Schuirmann 2014-06-26 16:47:59 UTC
Created attachment 279329 [details] [review]
wizard: Codestyle changes

- Reduce indentation for better readability
- Correct indentation error
- Move properties up
Comment 76 Lasse Schuirmann 2014-06-26 18:06:29 UTC
Review of attachment 279325 [details] [review]:

Just copying this comment from zeenix which got lost in the obsoleted patch.


::: src/wizard-source.vala
@@ +133,3 @@
     public MediaManager media_manager;

+    public bool has_to_download {

nitpick: download_required would be better IMHO.

@@ +140,3 @@
+                return true;
+
+            return false;

I'd do return ((scheme != null && scheme in supported_schemes);
Comment 77 Zeeshan Ali 2014-06-27 12:55:54 UTC
Review of attachment 279322 [details] [review]:

ack
Comment 78 Zeeshan Ali 2014-06-27 12:59:47 UTC
Review of attachment 279323 [details] [review]:

ack, still
Comment 79 Zeeshan Ali 2014-06-27 13:01:01 UTC
Review of attachment 279324 [details] [review]:

ACK
Comment 80 Zeeshan Ali 2014-06-27 13:20:19 UTC
Review of attachment 279326 [details] [review]:

Some more details in the log would be nice.

::: src/downloader.vala
@@ +182,3 @@
+        var basename = file.get_basename ();
+        if (basename == null || basename == "") {
+            warning ("ISO '%s' cannot be downloaded. No basename available.", uri);

* We don't want warning here if we are throwing an error.
* We should ensure a malformed URI is not passed instead. Perhaps in the calling code that receives the URI from user and here we can just do a return_if_fail as passing invalid URI to this function is then a programmer error.
Comment 81 Zeeshan Ali 2014-06-30 11:09:58 UTC
Review of attachment 279327 [details] [review]:

Not sure I like adding more state to this class which already has quite a lot. Is there a particular reason why you can't implement sub processes the way I've done already? Also the progressbar is already keeping the progress for us in the object so you can make use of that for tracking overall progress.
Comment 82 Zeeshan Ali 2014-06-30 15:14:30 UTC
Review of attachment 279328 [details] [review]:

From previous review: "Good stuff. :) If its kept HTTP(S)-specific, I think log should reflect that."

Also since the code is a bit hard to follow (I know it already is a messy), explanation in the log explaining a bit how patch works, would be nice.

::: src/wizard.vala
@@ +48,3 @@
     private ActivityProgress? prepare_progress;
     private ActivityProgress? prepare_media_progress;
+    private ActivityProgress? download_progress;

Same comment as about adding the ActivityProgress just above this one in a previous patch.

@@ +161,3 @@
             try {
+                if (!wizard_source.has_to_download)
+                    prepare_for_location (wizard_source.uri, true);

with 'probing' passed as 'true', this mainly serves as URL verifier AFAIK so I don't think you want this change.

@@ +349,3 @@
     }
 
+    private bool prepare (bool just_downloaded = false) {

I don't see anyone passing a different value of 'just_downloading' so why do we need this argument?

@@ +354,3 @@
+        if (!just_downloaded) {
+            if (wizard_source.has_to_download) {
+                setup_progress (true);

setup_progress being a non-static method can directly access wizard_source.has_to_download so no need to pass it this boolean.

@@ +585,3 @@
+            prepare_downloaded_iso (cache_path);
+        } catch (GLib.IOError.INVALID_FILENAME e) {
+            App.window.notificationbar.display_error (_("Please provide a valid URI."));

I think this should be handled earlier as part of prepare_for_location (.., probing=true)
Comment 83 Zeeshan Ali 2014-06-30 15:17:16 UTC
Review of attachment 279329 [details] [review]:

ack
Comment 84 Lasse Schuirmann 2014-07-08 16:05:31 UTC
Review of attachment 279327 [details] [review]:

You're right. I'll take a local var and pass it down to the functions that need it.
Comment 85 Lasse Schuirmann 2014-07-08 16:38:43 UTC
Review of attachment 279328 [details] [review]:

::: src/wizard.vala
@@ +349,3 @@
     }
 
+    private bool prepare (bool just_downloaded = false) {

its down in line 576
Comment 86 Lasse Schuirmann 2014-07-09 10:59:51 UTC
Created attachment 280221 [details] [review]
wizard-source: Introduce download_required prop

This function will be used by the following patches to determine
whether a remote source has to be downloaded or not. A source is
considered downloadable if it uses the http or https URI scheme.
Comment 87 Lasse Schuirmann 2014-07-09 10:59:57 UTC
Created attachment 280222 [details] [review]
downloader: Add fetch_iso()

This allows easy downloading of ISOs to the users Download folder. If
the user already downloaded the ISO (via Boxes or externally) and left
it with the same name as the initial one in the Download folder it will
be taken from there.
Comment 88 Lasse Schuirmann 2014-07-09 11:00:03 UTC
Created attachment 280223 [details] [review]
wizard: Create prep ActivityProgress on prepare() invokation

The ActivityProgress for the preparation was handled by prepare_media
before. This implies that no other functions could add subprogresses.

This patch changes this which makes it possible to introduce a
subprocess for ISO downloading.
Comment 89 Lasse Schuirmann 2014-07-09 11:00:09 UTC
Created attachment 280224 [details] [review]
wizard: Codestyle changes

- Reduce indentation for better readability
- Correct indentation error
- Move properties up
- Remove unneeded this.
Comment 90 Lasse Schuirmann 2014-07-09 11:00:15 UTC
Created attachment 280225 [details] [review]
wizard: Add HTTP(S) ISO downloading support

This adds the possibility to download ISOs via the HTTP or HTTPS
protocol from the internet. Functions of other objects used here were
gradually introduced with the parent commits of this.

The download process is invoked in download_iso (). All other changes
are only to
- recognize (in)valid HTTP(S) URLs while the user enters it
- cancel the operation appropriately if user clicks back/cancel
- set up the progress bar correctly
Comment 91 Zeeshan Ali 2014-07-09 13:29:57 UTC
Review of attachment 280221 [details] [review]:

Log calls it function while its a property. Good otherwise. Just fix the log when pushing.
Comment 92 Zeeshan Ali 2014-07-09 13:30:44 UTC
Review of attachment 280221 [details] [review]:

Oh and while changing the log, you might want to put http and https to caps as they are abbreviation.
Comment 93 Zeeshan Ali 2014-07-09 13:39:50 UTC
Review of attachment 280222 [details] [review]:

Looks good but since we'll want to use the same code for downloading ready medias (and hopefully appliances), would be nice to s/iso/media/' and 's/ISO/media' everywhere in this patch.

::: src/downloader.vala
@@ +181,3 @@
+        var file = File.new_for_uri (uri);
+        var basename = file.get_basename ();
+        return_if_fail (basename != null && basename != "");

doesn't this give you error? Method has a return type.
Comment 94 Lasse Schuirmann 2014-07-09 13:42:59 UTC
Review of attachment 280222 [details] [review]:

::: src/downloader.vala
@@ +181,3 @@
+        var file = File.new_for_uri (uri);
+        var basename = file.get_basename ();
+        return_if_fail (basename != null && basename != "");

compiles fine on my machine.
Comment 95 Zeeshan Ali 2014-07-09 14:31:41 UTC
Review of attachment 280223 [details] [review]:

* invokation -> invocation. :)
* 'prep ' in shortlog doesn't help much. I'd write 'wizard: prepare() now report progress' and in description its more clear to say that

"Currently, only prepare_media reports progress. This patch adds progress reporting to the whole 'prepare' call chain.

This patch makes it possible to introduce a subprogress for ISO downloading in a later patch."

::: src/wizard.vala
@@ +136,3 @@
     }
 
+    private ActivityProgress get_prepare_progress () {

get -> create.

@@ +137,3 @@
 
+    private ActivityProgress get_prepare_progress () {
+        var prepare_progress = new ActivityProgress ();

its obvious from contexts everywhere what the progress is about so I'll loose 'prepare_' prefix from variable and arguments everywhere. Also it can be easily confused with 'prep_progress' that is a different thing. We probably should also rename that to 'prep_progressbar' to avoid confusion.

@@ +308,3 @@
+        media_manager.create_installer_media_for_path.begin (path, null, (obj, res) => {
+            on_installer_media_instantiated (res, prepare_progress);
+            });

indented a bit too much.

@@ +330,3 @@
         }
 
+        var prepare_media_progress = prepare_progress.add_child_activity (1);

I don't think you need child activity if its going to be consuming the whole activity. If you'll change that in a following patch, I think this child activity addition belong in there then.
Comment 96 Zeeshan Ali 2014-07-09 14:39:29 UTC
Review of attachment 280224 [details] [review]:

ack

::: src/wizard.vala
@@ +52,3 @@
+    private Cancellable? review_cancellable;
+
+    private bool skip_review_for_live;

since they are both related to review, there is no need for empty line in between.
Comment 97 Zeeshan Ali 2014-07-09 18:01:24 UTC
Review of attachment 280225 [details] [review]:

Listing of "other changes" is an indication of likely need to divide the patch further. :)

"recognize (in)valid HTTP(S) URLs while the user enters it" -> "verify URLs while user enters them"

I think at least the verification of URL deserves to be in a separate patch.

::: src/wizard.vala
@@ +13,3 @@
 [GtkTemplate (ui = "/org/gnome/Boxes/ui/wizard.ui")]
 private class Boxes.Wizard: Gtk.Stack, Boxes.UI {
+    private const double DOWNLOAD_PROGRESS_SCALE = 0.9125;

I'd go for 95% at least since downloading would typically take most of the time. I don't know why 91.25 specifically? We are just making a blind guess here so shouldn't be so specific.

@@ +269,3 @@
+        if (wizard_source.download_required) {
+            var basename = file.get_basename ();
+            if (basename == null || basename == "") {

This should be checked for all URIs not just the one that get downloaded and probably checked when they are entered.

@@ +582,3 @@
     }
 
+    private void prepare_downloaded_iso (string cache_path, ActivityProgress prepare_progress) {

The usual convention I've followed is to define method after its usage.

@@ +588,3 @@
+    }
+
+    private async void download_iso (string uri, ActivityProgress prepare_progress) {

Same comment here as for previous commit. I don't see anything very specific to ISOs involved so better keep the names etc generic from start.

@@ +612,3 @@
+        } catch (GLib.Error e) {
+            debug ("Failed downloading ISO '%s'! %s", uri, e.message);
+            App.window.notificationbar.display_error (_("Failed downloading ISO. Is the URI correct?"));

That sounds like we failed it on purpose. "Download of '%s' failed." is fine, where %s could just be the basename. URI correction is checked already (or at least it should be) so we we shouldn't need to ask user to check URI here.

Maybe we should instead provide a 'Retry' button in the error message but we could add that later.
Comment 98 Lasse Schuirmann 2014-07-09 18:10:38 UTC
Review of attachment 280225 [details] [review]:

::: src/wizard.vala
@@ +13,3 @@
 [GtkTemplate (ui = "/org/gnome/Boxes/ui/wizard.ui")]
 private class Boxes.Wizard: Gtk.Stack, Boxes.UI {
+    private const double DOWNLOAD_PROGRESS_SCALE = 0.9125;

Yeah, the intention was to take something that fits into the register exactly. I think I miscalculated. I can take 0.95 ;)

@@ +582,3 @@
     }
 
+    private void prepare_downloaded_iso (string cache_path, ActivityProgress prepare_progress) {

Didn't know that yet!

@@ +612,3 @@
+        } catch (GLib.Error e) {
+            debug ("Failed downloading ISO '%s'! %s", uri, e.message);
+            App.window.notificationbar.display_error (_("Failed downloading ISO. Is the URI correct?"));

If this occurs the user has likely put in a syntactically right URL that points to nowhere.
Comment 99 Zeeshan Ali 2014-07-09 21:12:49 UTC
Review of attachment 280222 [details] [review]:

::: src/downloader.vala
@@ +181,3 @@
+        var file = File.new_for_uri (uri);
+        var basename = file.get_basename ();
+        return_if_fail (basename != null && basename != "");

werid, it should be return_val_if_fail. Could be a bug in vala's async syntax.
Comment 100 Zeeshan Ali 2014-07-09 21:15:56 UTC
Review of attachment 280225 [details] [review]:

::: src/wizard.vala
@@ +612,3 @@
+        } catch (GLib.Error e) {
+            debug ("Failed downloading ISO '%s'! %s", uri, e.message);
+            App.window.notificationbar.display_error (_("Failed downloading ISO. Is the URI correct?"));

Yes but there could be other reasons too (especially since its catch block for all kinds of errors.
Comment 101 Lasse Schuirmann 2014-07-11 15:17:47 UTC
Review of attachment 280222 [details] [review]:

::: src/downloader.vala
@@ +181,3 @@
+        var file = File.new_for_uri (uri);
+        var basename = file.get_basename ();
+        return_if_fail (basename != null && basename != "");

Here is it:
https://bugzilla.gnome.org/show_bug.cgi?id=630153

Considering the comments there and my bad feeling about this whole return_if_fail thing in general I'd like to go for a solution without this. Anything agains a plain and simple assert?
Comment 102 Zeeshan Ali 2014-07-11 18:29:12 UTC
Review of attachment 280222 [details] [review]:

::: src/downloader.vala
@@ +181,3 @@
+        var file = File.new_for_uri (uri);
+        var basename = file.get_basename ();
+        return_if_fail (basename != null && basename != "");

Seems pretty old and unrelated to me. Also I have never heard of the Jiří Zárevúcky so I won't put way too much weight into his comment. :)

I don't know about your feelings but assert means that whole app will exit. Do we really want that to happen here? Is this a very crucial code path?
Comment 103 Lasse Schuirmann 2014-07-14 15:57:05 UTC
Created attachment 280652 [details] [review]
downloader: Add fetch_media()

This allows easy downloading of media to the users Download folder. If
the user already downloaded the media (via Boxes or externally) and left
it with the same name as the initial one in the Download folder it will
be taken from there.
Comment 104 Lasse Schuirmann 2014-07-14 15:57:12 UTC
Created attachment 280653 [details] [review]
wizard: prepare() now report progress

Currently, only prepare_media reports progress. This patch adds
progress reporting to the whole 'prepare' call chain.

This patch makes it possible to introduce a subprogress for ISO
downloading in a later patch.
Comment 105 Lasse Schuirmann 2014-07-14 15:57:19 UTC
Created attachment 280654 [details] [review]
wizard: Codestyle changes

- Reduce indentation for better readability
- Correct indentation error
- Move properties up
- Remove unneeded this.
Comment 106 Lasse Schuirmann 2014-07-14 15:57:25 UTC
Created attachment 280655 [details] [review]
wizard: Check basename for URIs
Comment 107 Lasse Schuirmann 2014-07-14 15:57:32 UTC
Created attachment 280656 [details] [review]
wizard: Introduce media_download_cancellable

This cancellable will be used for media downloading in the following
patches.
Comment 108 Lasse Schuirmann 2014-07-14 15:57:39 UTC
Created attachment 280657 [details] [review]
wizard: Introduce download_media ()

This adds a function to download ISOs via HTTP or HTTPS protocol from
the internet. Functions of other objects used here were gradually
introduced with the parent commits of this.
Comment 109 Lasse Schuirmann 2014-07-14 15:57:45 UTC
Created attachment 280658 [details] [review]
wizard: Introduce ISO downloading feature

This adds the possibility to download ISOs via the HTTP or HTTPS
protocol from the internet. Functions of other objects used here were
gradually introduced with the parent commits of this.
Comment 110 Lasse Schuirmann 2014-07-14 16:05:20 UTC
Created attachment 280659 [details] [review]
wizard: Introduce ISO downloading feature

This adds the possibility to download ISOs via the HTTP or HTTPS
protocol from the internet. Functions of other objects used here were
gradually introduced with the parent commits of this.
Comment 111 Zeeshan Ali 2014-07-14 17:22:49 UTC
Review of attachment 280652 [details] [review]:

Good otherwise.

::: src/downloader.vala
@@ +181,3 @@
+        var file = File.new_for_uri (uri);
+        var basename = file.get_basename ();
+        return_val_if_fail (basename != null && basename != "" && basename != "/", "");

should return 'null' even though return value is not nullable but this is going to almost never happen.
Comment 112 Zeeshan Ali 2014-07-14 17:27:46 UTC
Review of attachment 280653 [details] [review]:

* 'now report' -> 'now reports'
* ISO -> media

Good otherwise
Comment 113 Zeeshan Ali 2014-07-14 17:29:08 UTC
Review of attachment 280654 [details] [review]:

almost there

::: src/wizard.vala
@@ +51,3 @@
 
+    private Cancellable? review_cancellable;
+

I think I said in previous review that we want no empty line here.
Comment 114 Lasse Schuirmann 2014-07-14 17:30:05 UTC
Review of attachment 280654 [details] [review]:

::: src/wizard.vala
@@ +51,3 @@
 
+    private Cancellable? review_cancellable;
+

ah, yes sorry. I missed that :s
Comment 115 Zeeshan Ali 2014-07-14 17:38:07 UTC
Review of attachment 280655 [details] [review]:

::: src/wizard.vala
@@ +293,1 @@
+        if (basename == null || basename == "" || basename == "/" || uri == null || uri.scheme == null)

* with so many conditions, we better divide it a bit in two lines at least.
* I'd think uri being null is the first then we check and if uri is null, i don't think you should create the file or take basename even. So maybe last two checks should be separate 'if'.
Comment 116 Zeeshan Ali 2014-07-14 17:40:28 UTC
Review of attachment 280656 [details] [review]:

Shouldn't we have same cancellable for all of 'prepare' operations?
Comment 117 Zeeshan Ali 2014-07-14 17:40:31 UTC
Review of attachment 280656 [details] [review]:

Shouldn't we have same cancellable for all of 'prepare' operations?
Comment 118 Zeeshan Ali 2014-07-14 17:43:27 UTC
Review of attachment 280657 [details] [review]:

* description still says 'ISO'.
* 'Functions of other objects used here were gradually introduced with the parent commits of this.' is redundant IMO.

Good otherwise.

::: src/wizard.vala
@@ +568,3 @@
+            prepare_downloaded_media (cache_path, progress);
+        } catch (GLib.IOError.CANCELLED e) {
+            page = WizardPage.SOURCE;

we'd want a debug here.

@@ +570,3 @@
+            page = WizardPage.SOURCE;
+        } catch (GLib.Error e) {
+            debug ("Failed downloading media '%s'! %s", uri, e.message);

This should be warning
Comment 119 Zeeshan Ali 2014-07-14 17:46:25 UTC
Review of attachment 280659 [details] [review]:

You forgot s/ISO/media/ in here completely. :) You can use this opportunity to call it 'installation/live media' in the description to be clear.

::: src/wizard.vala
@@ +366,3 @@
+        if (wizard_source.download_required) {
+            try {
+                // validate URI

As I pointed out in the previous reviews, we'd want to validate URI whether download is required or not.
Comment 120 Lasse Schuirmann 2014-07-15 10:13:46 UTC
Review of attachment 280652 [details] [review]:

::: src/downloader.vala
@@ +181,3 @@
+        var file = File.new_for_uri (uri);
+        var basename = file.get_basename ();
+        return_val_if_fail (basename != null && basename != "" && basename != "/", "");

Strange. Since this is obviously a macro thing vala lets me return null for this string value without even a warning.
Comment 121 Lasse Schuirmann 2014-07-15 11:08:43 UTC
Review of attachment 280657 [details] [review]:

::: src/wizard.vala
@@ +570,3 @@
+            page = WizardPage.SOURCE;
+        } catch (GLib.Error e) {
+            debug ("Failed downloading media '%s'! %s", uri, e.message);

for me its rather debug information since the error is shown to the user directly.
Comment 122 Lasse Schuirmann 2014-07-15 11:15:33 UTC
Created attachment 280702 [details] [review]
downloader: Add fetch_media()

This allows easy downloading of media to the users Download folder. If
the user already downloaded the media (via Boxes or externally) and left
it with the same name as the initial one in the Download folder it will
be taken from there.
Comment 123 Lasse Schuirmann 2014-07-15 11:15:41 UTC
Created attachment 280704 [details] [review]
wizard: prepare() now reports progress

Currently, only prepare_media reports progress. This patch adds
progress reporting to the whole 'prepare' call chain.

This patch makes it possible to introduce a subprogress for media
downloading in a later patch.
Comment 124 Lasse Schuirmann 2014-07-15 11:15:47 UTC
Created attachment 280705 [details] [review]
wizard: Codestyle changes

- Reduce indentation for better readability
- Correct indentation error
- Move properties up
- Remove unneeded this.
Comment 125 Lasse Schuirmann 2014-07-15 11:15:54 UTC
Created attachment 280706 [details] [review]
wizard: Check basename for URIs
Comment 126 Lasse Schuirmann 2014-07-15 11:16:00 UTC
Created attachment 280707 [details] [review]
wizard: Introduce prepare_cancellable

This cancellable will be used for media downloading in the following
patches. It may also be used later for cancelling other preparation
actions.
Comment 127 Lasse Schuirmann 2014-07-15 11:16:07 UTC
Created attachment 280708 [details] [review]
wizard: Introduce download_media ()

This adds a function to download media via HTTP or HTTPS protocol from
the internet. Functions of other objects used here were gradually
introduced with the parent commits of this.
Comment 128 Lasse Schuirmann 2014-07-15 11:16:13 UTC
Created attachment 280709 [details] [review]
wizard: Introduce media downloading feature

This adds the possibility to download installation/live media via the
HTTP or HTTPS protocol from the internet. Functions of other objects
used here were gradually introduced with the parent commits of this.
Comment 129 Lasse Schuirmann 2014-07-15 11:21:08 UTC
Created attachment 280710 [details] [review]
wizard: Introduce media downloading feature

This adds the possibility to download installation/live media via the
HTTP or HTTPS protocol from the internet. Functions of other objects
used here were gradually introduced with the parent commits of this.
Comment 130 Zeeshan Ali 2014-07-15 11:38:22 UTC
Review of attachment 280652 [details] [review]:

::: src/downloader.vala
@@ +181,3 @@
+        var file = File.new_for_uri (uri);
+        var basename = file.get_basename ();
+        return_val_if_fail (basename != null && basename != "" && basename != "/", "");

yes and as I said its fine in this case at least.
Comment 131 Zeeshan Ali 2014-07-15 11:40:28 UTC
Review of attachment 280657 [details] [review]:

::: src/wizard.vala
@@ +570,3 @@
+            page = WizardPage.SOURCE;
+        } catch (GLib.Error e) {
+            debug ("Failed downloading media '%s'! %s", uri, e.message);

no, its a warning because something has went wrong. Nothing to do with whether or not we show error to user.
Comment 132 Zeeshan Ali 2014-07-15 12:16:03 UTC
Review of attachment 280710 [details] [review]:

You missed the second point about commit log:

* 'Functions of other objects used here were gradually introduced with the
parent commits of this.' is redundant IMO.
Comment 133 Lasse Schuirmann 2014-07-15 12:18:46 UTC
Review of attachment 280710 [details] [review]:

Can't find it in your previous review, don't remember it either. Am I too blind? Anyway. Reupload in a minute.
Comment 134 Lasse Schuirmann 2014-07-15 12:19:24 UTC
Created attachment 280712 [details] [review]
wizard: Introduce media downloading feature

This adds the possibility to download installation/live media via the
HTTP or HTTPS protocol from the internet.
Comment 135 Lasse Schuirmann 2014-07-30 12:32:05 UTC
Created attachment 282035 [details] [review]
downloader: Make download cancellable
Comment 136 Lasse Schuirmann 2014-07-30 12:32:13 UTC
Created attachment 282036 [details] [review]
util: Generalize cache getting
Comment 137 Lasse Schuirmann 2014-07-30 12:32:20 UTC
Created attachment 282037 [details] [review]
wizard-toolbar: Only click buttons when appropriate

Buttons should only be clicked if they are sensitive.
Comment 138 Lasse Schuirmann 2014-07-30 12:32:28 UTC
Created attachment 282038 [details] [review]
wizard-source: Introduce download_required prop

This function will be used by the following patches to determine
whether a remote source has to be downloaded or not. A source is
considered downloadable if it uses the http or https URI scheme.
Comment 139 Lasse Schuirmann 2014-07-30 12:32:35 UTC
Created attachment 282039 [details] [review]
downloader: Add fetch_media()

This allows easy downloading of media to the users Download folder. If
the user already downloaded the media (via Boxes or externally) and left
it with the same name as the initial one in the Download folder it will
be taken from there.
Comment 140 Lasse Schuirmann 2014-07-30 12:32:42 UTC
Created attachment 282040 [details] [review]
wizard: prepare() now reports progress

Currently, only prepare_media reports progress. This patch adds
progress reporting to the whole 'prepare' call chain.

This patch makes it possible to introduce a subprogress for media
downloading in a later patch.
Comment 141 Lasse Schuirmann 2014-07-30 12:32:49 UTC
Created attachment 282041 [details] [review]
wizard: Codestyle changes

- Reduce indentation for better readability
- Correct indentation error
- Move properties up
- Remove unneeded this.
Comment 142 Lasse Schuirmann 2014-07-30 12:32:56 UTC
Created attachment 282042 [details] [review]
wizard: Check basename for URIs
Comment 143 Lasse Schuirmann 2014-07-30 12:33:03 UTC
Created attachment 282043 [details] [review]
wizard: Introduce prepare_cancellable

This cancellable will be used for media downloading in the following
patches. It may also be used later for cancelling other preparation
actions.
Comment 144 Lasse Schuirmann 2014-07-30 12:33:10 UTC
Created attachment 282044 [details] [review]
wizard: Introduce download_media ()

This adds a function to download media via HTTP or HTTPS protocol from
the internet. Functions of other objects used here were gradually
introduced with the parent commits of this.
Comment 145 Lasse Schuirmann 2014-07-30 12:33:17 UTC
Created attachment 282045 [details] [review]
wizard: Introduce media downloading feature

This adds the possibility to download installation/live media via the
HTTP or HTTPS protocol from the internet.
Comment 146 Zeeshan Ali 2014-08-01 13:44:28 UTC
Created attachment 282258 [details] [review]
downloader: Make download cancellable
Comment 147 Zeeshan Ali 2014-08-01 13:44:59 UTC
Created attachment 282259 [details] [review]
util: Generalize cache getting
Comment 148 Zeeshan Ali 2014-08-01 13:45:18 UTC
Created attachment 282260 [details] [review]
wizard-toolbar: Only click buttons when appropriate

Buttons should only be clicked if they are sensitive.
Comment 149 Zeeshan Ali 2014-08-01 13:45:36 UTC
Created attachment 282261 [details] [review]
wizard-source: Introduce download_required prop

This function will be used by the following patches to determine
whether a remote source has to be downloaded or not. A source is
considered to require download if it uses the HTTP(S) URI scheme.
Comment 150 Zeeshan Ali 2014-08-01 13:45:51 UTC
Created attachment 282262 [details] [review]
downloader: Add fetch_media()

This allows easy downloading of media to the users Download folder. If
the user already downloaded the media (via Boxes or externally) and left
it with the same name as the initial one in the Download folder it will
be taken from there.
Comment 151 Zeeshan Ali 2014-08-01 13:46:11 UTC
Created attachment 282263 [details] [review]
wizard: prepare() now reports progress

Previously, only prepare_media reported progress. This patch adds
progress reporting to the whole 'prepare' call chain.

This patch makes it possible to introduce a subprogress for media
downloading in a later patch.
Comment 152 Zeeshan Ali 2014-08-01 13:46:27 UTC
Created attachment 282264 [details] [review]
wizard: Codestyle changes

- Reduce indentation for better readability
- Correct indentation error
- Move properties up
- Remove unneeded this.
Comment 153 Zeeshan Ali 2014-08-01 13:46:40 UTC
Created attachment 282265 [details] [review]
wizard: Allow entering of HTTP(S) URIs

And also validate URI has a proper basename.
Comment 154 Zeeshan Ali 2014-08-01 13:46:52 UTC
Created attachment 282266 [details] [review]
wizard: Introduce prepare_cancellable

This cancellable will be used for media downloading in the following
patches. It may also be used later for cancelling other preparation
actions.
Comment 155 Zeeshan Ali 2014-08-01 13:47:06 UTC
Created attachment 282267 [details] [review]
wizard: Introduce download_media ()

This adds a function to download media via HTTP or HTTPS protocol from
the internet. Functions of other objects used here were gradually
introduced with the parent commits of this.
Comment 156 Zeeshan Ali 2014-08-01 13:47:22 UTC
Created attachment 282268 [details] [review]
wizard: Introduce media downloading feature

This adds the possibility to download installation/live media via the
HTTP(S) protocol from the internet.
Comment 157 Zeeshan Ali 2014-08-01 13:54:13 UTC
Created attachment 282269 [details] [review]
wizard: Introduce media downloading feature

This adds the possibility to download installation/live media via the
HTTP(S) protocol from the internet.
Comment 158 Zeeshan Ali 2014-08-01 13:56:24 UTC
Created attachment 282270 [details] [review]
wizard: Introduce download_media ()

This adds a function to download media via HTTP or HTTPS protocol from
the internet. Functions of other objects used here were gradually
introduced with the parent commits of this.
Comment 159 Zeeshan Ali 2014-08-01 13:57:03 UTC
Created attachment 282271 [details] [review]
wizard: Introduce media downloading feature

This adds the possibility to download installation/live media via the
HTTP(S) protocol from the internet.
Comment 160 Zeeshan Ali 2014-08-01 14:02:46 UTC
Created attachment 282272 [details] [review]
wizard: Introduce media downloading feature

This adds the possibility to download installation/live media via the
HTTP(S) protocol from the internet.
Comment 161 Zeeshan Ali 2014-08-01 14:09:12 UTC
Attachment 282258 [details] pushed as 1b38496 - downloader: Make download cancellable
Attachment 282259 [details] pushed as 54bb203 - util: Generalize cache getting
Attachment 282260 [details] pushed as 0454ec4 - wizard-toolbar: Only click buttons when appropriate
Attachment 282261 [details] pushed as 0482d0c - wizard-source: Introduce download_required prop
Attachment 282262 [details] pushed as 468cb95 - downloader: Add fetch_media()
Attachment 282263 [details] pushed as 6492e0b - wizard: prepare() now reports progress
Attachment 282264 [details] pushed as 85b9149 - wizard: Codestyle changes
Attachment 282265 [details] pushed as cf8ed3e - wizard: Allow entering of HTTP(S) URIs
Attachment 282266 [details] pushed as 0977fb1 - wizard: Introduce prepare_cancellable
Attachment 282270 [details] pushed as eb9f11a - wizard: Introduce download_media ()
Attachment 282272 [details] pushed as 4bbaa4d - wizard: Introduce media downloading feature