GNOME Bugzilla – Bug 729026
Allow entering remote (HTTP & HTTPS) ISOs
Last modified: 2016-03-31 13:22:07 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.
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.
Fwiw, the libosinfo database already has some URLs to installers ISOs.
(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.
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.)
(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. :)
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
(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.
The following patches represent the first iteration.
Created attachment 278792 [details] [review] downloader: Make download cancellable This will be used in the following patches to introduce ISO downloading.
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.
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.
Created attachment 278795 [details] [review] wizard: Add ISO downloading support
Created attachment 278796 [details] [review] wizard: Change indentation for better readability
Created attachment 278797 [details] [review] wizard: Show negative progress when analyzing
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?
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.
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.
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. :(
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?
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.
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 () ?
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.
Review of attachment 278792 [details] [review]: ::: src/downloader.vala @@ +122,3 @@ + session.cancel_message (msg, Soup.Status.CANCELLED); + }); + ok.
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.
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!
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.
Review of attachment 278796 [details] [review]: I am rebasing this to the new parents. Also adding few more stylefixes.
Created attachment 279033 [details] [review] downloader: Make download cancellable
Created attachment 279034 [details] [review] util: Remove get_cache () (unneeded) This function was introduced when solving bug#698144 but never used.
Created attachment 279035 [details] [review] util: Generalize cache getting
Created attachment 279036 [details] [review] util: Introduce iso cache ISO cache will be needed by the following patches to introduce ISO downloading.
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.
Created attachment 279038 [details] [review] wizard: Add ISO downloading support
Created attachment 279039 [details] [review] wizard: Reduce indentation for better readability
Created attachment 279040 [details] [review] wizard: Show negative progress when analyzing This is necessary for e.g. resetting the progress bar.
Created attachment 279041 [details] [review] wizard-toolbar: Only click buttons when appropriate Buttons should only be clicked if they are sensitive.
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.
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.
Created attachment 279093 [details] [review] downloader: Make download cancellable
Created attachment 279094 [details] [review] util: Remove get_cache () (unneeded) This function was introduced when solving bug#698144 but never used.
Created attachment 279095 [details] [review] util: Generalize cache getting
Created attachment 279096 [details] [review] util: Introduce iso cache ISO cache will be needed by the following patches to introduce ISO downloading.
Created attachment 279097 [details] [review] wizard-toolbar: Only click buttons when appropriate Buttons should only be clicked if they are sensitive.
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.
Created attachment 279099 [details] [review] wizard: Add ISO downloading support
Created attachment 279100 [details] [review] wizard: Codestyle changes - Reduce indentation for better readability - Correct indentation error - Move properties up
Created attachment 279101 [details] [review] wizard: Show negative progress when analyzing This is necessary for e.g. resetting the progress bar.
I just reordered the patches and merged the codestyle patches. No need to look at any old ones.
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.
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.
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.
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.
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
Review of attachment 279094 [details] [review]: Why not 'Remove redunant get_cache()' ? Space before '(' in commit logs is optional btw. :)
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.
Review of attachment 279095 [details] [review]: Looks good.
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.
Review of attachment 279096 [details] [review]: g_get_user_special_dir (G_USER_DIRECTORY_DOWNLOAD) in C.
Review of attachment 279097 [details] [review]: ACK
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.
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.
Review of attachment 279100 [details] [review]: ack
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.
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.
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.
Created attachment 279322 [details] [review] downloader: Make download cancellable
Created attachment 279323 [details] [review] util: Generalize cache getting
Created attachment 279324 [details] [review] wizard-toolbar: Only click buttons when appropriate Buttons should only be clicked if they are sensitive.
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.
Created attachment 279326 [details] [review] downloader: Add fetch_iso ()
Created attachment 279327 [details] [review] wizard: Make ActivityProgress for prepare_media() a member This allows introducing subprocesses for the preparation step.
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);
Created attachment 279328 [details] [review] wizard: Add ISO downloading support
Created attachment 279329 [details] [review] wizard: Codestyle changes - Reduce indentation for better readability - Correct indentation error - Move properties up
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);
Review of attachment 279322 [details] [review]: ack
Review of attachment 279323 [details] [review]: ack, still
Review of attachment 279324 [details] [review]: ACK
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.
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.
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)
Review of attachment 279329 [details] [review]: ack
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.
Review of attachment 279328 [details] [review]: ::: src/wizard.vala @@ +349,3 @@ } + private bool prepare (bool just_downloaded = false) { its down in line 576
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.
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.
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.
Created attachment 280224 [details] [review] wizard: Codestyle changes - Reduce indentation for better readability - Correct indentation error - Move properties up - Remove unneeded this.
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
Review of attachment 280221 [details] [review]: Log calls it function while its a property. Good otherwise. Just fix the log when pushing.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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?
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?
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.
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.
Created attachment 280654 [details] [review] wizard: Codestyle changes - Reduce indentation for better readability - Correct indentation error - Move properties up - Remove unneeded this.
Created attachment 280655 [details] [review] wizard: Check basename for URIs
Created attachment 280656 [details] [review] wizard: Introduce media_download_cancellable This cancellable will be used for media downloading in the following patches.
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.
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.
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.
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.
Review of attachment 280653 [details] [review]: * 'now report' -> 'now reports' * ISO -> media Good otherwise
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.
Review of attachment 280654 [details] [review]: ::: src/wizard.vala @@ +51,3 @@ + private Cancellable? review_cancellable; + ah, yes sorry. I missed that :s
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'.
Review of attachment 280656 [details] [review]: Shouldn't we have same cancellable for all of 'prepare' operations?
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
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.
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.
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.
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.
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.
Created attachment 280705 [details] [review] wizard: Codestyle changes - Reduce indentation for better readability - Correct indentation error - Move properties up - Remove unneeded this.
Created attachment 280706 [details] [review] wizard: Check basename for URIs
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.
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.
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.
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.
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.
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.
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.
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.
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.
Created attachment 282035 [details] [review] downloader: Make download cancellable
Created attachment 282036 [details] [review] util: Generalize cache getting
Created attachment 282037 [details] [review] wizard-toolbar: Only click buttons when appropriate Buttons should only be clicked if they are sensitive.
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.
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.
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.
Created attachment 282041 [details] [review] wizard: Codestyle changes - Reduce indentation for better readability - Correct indentation error - Move properties up - Remove unneeded this.
Created attachment 282042 [details] [review] wizard: Check basename for URIs
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.
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.
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.
Created attachment 282258 [details] [review] downloader: Make download cancellable
Created attachment 282259 [details] [review] util: Generalize cache getting
Created attachment 282260 [details] [review] wizard-toolbar: Only click buttons when appropriate Buttons should only be clicked if they are sensitive.
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.
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.
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.
Created attachment 282264 [details] [review] wizard: Codestyle changes - Reduce indentation for better readability - Correct indentation error - Move properties up - Remove unneeded this.
Created attachment 282265 [details] [review] wizard: Allow entering of HTTP(S) URIs And also validate URI has a proper basename.
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.
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.
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.
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.
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.
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.
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.
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