GNOME Bugzilla – Bug 690321
More useful UI at 'preparation' step
Last modified: 2016-03-31 14:01:32 UTC
Here are some patches to show progress of media preparation and keeping user informed. These patches are on top of patches in bug#690169 as use of gio API makes it very easy to track download progress.
Created attachment 231693 [details] [review] wizard,css: Separate labels for media & status info Separate labels for media and status information.
Created attachment 231694 [details] [review] wizard: Display OS info & logo ASAP during preparation
Created attachment 231695 [details] [review] installer: Refactor setup_pre_install_drivers() Separate methods to search all pre-installable drivers and scripts that accept them.
Created attachment 231696 [details] [review] util: Add class for reporting & tracking progress Add class for reporting and tracking of progress of async activities.
Created attachment 231697 [details] [review] downloader: Allow tracking of download progress
Created attachment 231698 [details] [review] wizard,installer: Track progress of media preparation Make the progressbar on wizard's preparation page more useful by showing progress of media detection and drivers download and keeping user informed about current activity.
I thought we wanted slow stuff (driver downloads) to happen after at at the end of the wizard (when the user clicked 'create') rather than in the middle of it?
(In reply to comment #7) > I thought we wanted slow stuff (driver downloads) to happen after at at the end > of the wizard (when the user clicked 'create') rather than in the middle of it? No, that was one idea proposed be Alex but I don't like that. I think this should be done at the same wizard stage as most VM setup steps (domain and volume creation etc) but currently those are done without any UI. At first I thought there should be some new transitional step between setup and review but thinking more about it, I think we can re-use this 'preparation' stage. 'Setup' step is all about setting-up unattended installation script and those don't take long so they can step at 'review->preparation' but the rest could be done at 'preparation'.
For my point of view, the unattended installation wizard is nice because you quickly go through it to answer a few questions without waiting, and then you wait until the installation is done, and then you're good to go. With these changes in driver download, this is no longer true, you start going through the wizard, wait, start the unattended install, wait, and you're good to go. This totally defeats the point of the wizard and unattended installs imo.
(In reply to comment #9) > For my point of view, the unattended installation wizard is nice because you > quickly go through it to answer a few questions without waiting, and then you > wait until the installation is done, and then you're good to go. > > With these changes in driver download, this is no longer true, you start going > through the wizard, wait, start the unattended install, wait, and you're good > to go. 1. You should keep in mind that driver download along with media detection does not happen during the wizard but asynchronosly while populating the media menu. 2. What I'm mainly proposing is that user have to only wait once and in a step which is meant for Boxes to do all the setup needed and has already the UI components to give feedback to user on whats going on. > This totally defeats the point of the wizard and unattended installs > imo. I'm not sure. The wait involved isn't that big and we cache the drivers so its shorter on subsequent installs. We need to download and copy the drivers *before* launching the install anyways although if we do so after user hits 'create', user does not need to be in front of machine anymore. Also, these patches are not really moving the driver download anywhere and not really making it hard to move the driver download somewhere else either so I'd want to discuss the matter of when/where to download separately from this.
Created attachment 231743 [details] [review] wizard,css: Separate labels for media & status info Separate labels for media and status information.
Created attachment 231744 [details] [review] wizard: Display OS info & logo ASAP during preparation
Created attachment 231745 [details] [review] installer: Refactor setup_pre_install_drivers() Separate methods to search all pre-installable drivers and scripts that accept them.
Created attachment 231746 [details] [review] util: Add class for reporting & tracking progress Add class for reporting and tracking of progress of async activities.
Created attachment 231747 [details] [review] downloader: Allow tracking of download progress
Created attachment 231748 [details] [review] wizard,installer: Track progress of media preparation Make the progressbar on wizard's preparation page more useful by showing progress of media detection and drivers download and keeping user informed about current activity.
I rebased the patches on top of git master (i-e using libsoup still) so they don't depend on patches in bug#690169 anymore.
(In reply to comment #10) > (In reply to comment #9) > > For my point of view, the unattended installation wizard is nice because you > > quickly go through it to answer a few questions without waiting, and then you > > wait until the installation is done, and then you're good to go. > > > > With these changes in driver download, this is no longer true, you start going > > through the wizard, wait, start the unattended install, wait, and you're good > > to go. > > 1. You should keep in mind that driver download along with media detection does > not happen during the wizard but asynchronosly while populating the media menu. Meh, missed the keyword: typically.
Review of attachment 231743 [details] [review]: ::: data/gtk-style.css @@ +59,3 @@ } +.boxes-wizard-label-bold { I don't like "bold" in the class name, the name should describe why you'd want to set this class, not what it does.
Review of attachment 231744 [details] [review]: ::: src/wizard.vala @@ +279,3 @@ + private void on_installer_recognized (Osinfo.Media os_media, Osinfo.Os os) { + prep_media_label.label = os.name; + Downloader.fetch_os_logo.begin (installer_image, os, 128); We don't seem to wait anymore on the os logo download to finish?
Review of attachment 231745 [details] [review]: ack
Review of attachment 231746 [details] [review]: ::: src/util.vala @@ +298,3 @@ + public class ActivityProgress : GLib.Object { + public double progress { get; set; } This seems like a recipe for poor performance. Any time you download a single byte you'll calculate a new double value and emit notify, causing redraws for what will essentially display as the same value. You need to rate limit the number of times you change the progress somewhere.
Review of attachment 231747 [details] [review]: ::: src/downloader.vala @@ +50,3 @@ if (downloads.contains (uri)) // Already being downloaded + return yield await_download (uri, cached_path); // FIXME: No progress report in this case. You should at least set progress to 1.0 at the end.
Review of attachment 231748 [details] [review]: ::: src/unattended-installer.vala @@ +608,3 @@ + driver_progress.notify["progress"].connect (() => { + progress.progress = original_progress + (driver_progress.progress / num_iterations); + }); This happens a lot, maybe we should add "child activity" support to ActivityProgress itself? All you need to specify is the scale and the child activity progress.
Also, while the download is fast and cached so this is not a huge problem in practice i think its major stupid to have a "initial setup so you can then leave it to install by itself" system and then add a step in the initial setup where the user is left just waiting on something that could have been done as part of the unattended part.
Review of attachment 231744 [details] [review]: ::: src/wizard.vala @@ +279,3 @@ + private void on_installer_recognized (Osinfo.Media os_media, Osinfo.Os os) { + prep_media_label.label = os.name; + Downloader.fetch_os_logo.begin (installer_image, os, 128); True but come to think of it, do we really want to? Expecially since now we show the name of OS already.
Review of attachment 231747 [details] [review]: ::: src/downloader.vala @@ +50,3 @@ if (downloads.contains (uri)) // Already being downloaded + return yield await_download (uri, cached_path); // FIXME: No progress report in this case. The caller does that. The idea is that progress should be concidered at 1.0 after the async task returns successfully.
Review of attachment 231748 [details] [review]: ::: src/unattended-installer.vala @@ +608,3 @@ + driver_progress.notify["progress"].connect (() => { + progress.progress = original_progress + (driver_progress.progress / num_iterations); + }); Good point, that would also make this class more useful.
(In reply to comment #25) > Also, while the download is fast and cached so this is not a huge problem in > practice i think its major stupid to have a "initial setup so you can then > leave it to install by itself" system and then add a step in the initial setup > where the user is left just waiting on something that could have been done as > part of the unattended part. All valid points. I'll look into moving the driver download step after these changes. Most of these are about tracking of progress and that would be nice to have even at that step. Do you agree with this part from comment#8 btw? "At first I thought there should be some new transitional step between setup and review but thinking more about it, I think we can re-use this 'preparation' stage. 'Setup' step is all about setting-up unattended installation script and those don't take long so they can step at 'review->preparation' but the rest could be done at 'preparation'."
Review of attachment 231746 [details] [review]: ::: src/util.vala @@ +298,3 @@ + public class ActivityProgress : GLib.Object { + public double progress { get; set; } However, in practice I haven't seen <100bytes being downloaded at a time but yeah, i can put some guards to ensure notify is not emitted needlessly.
Created attachment 231833 [details] [review] wizard,css: Separate labels for media & status info
Created attachment 231834 [details] [review] wizard: Display OS info & logo ASAP during preparation
Created attachment 231835 [details] [review] util: Add class for reporting & tracking progress
Created attachment 231836 [details] [review] wizard,installer: Track progress of media preparation
Created attachment 231848 [details] [review] wizard,installer: Track progress of media preparation rebased on current git master.
Review of attachment 231833 [details] [review]: ack
Review of attachment 231834 [details] [review]: ::: src/wizard.vala @@ +279,3 @@ + private void on_installer_recognized (Osinfo.Media os_media, Osinfo.Os os) { + prep_media_label.label = os.name; + Downloader.fetch_os_logo.begin (installer_image, os, 128); This still doesn't seem to wait until the os logo has been downloaded, so we will block later when we need this, won't we?
Review of attachment 231835 [details] [review]: ::: src/util.vala @@ +308,3 @@ + _progress = value; + + notify_property ("progress"); This doesn't really change anything. Any increase, say of 1 bytes will still cause a change of the value, even if its only 0.00000001%. This is why this is problematic. You need to do some kind of "difference since last signal is > 1%" kind of thing.
Review of attachment 231835 [details] [review]: ::: src/util.vala @@ +308,3 @@ + _progress = value; + + notify_property ("progress"); We probably need the normal notification to be always sent, in order to not miss updates to the parent from a child change. However, when updating the actual progressbar widget you need to limit the number of updates, because thats what is expensive.
Review of attachment 231836 [details] [review]: ack
Review of attachment 231834 [details] [review]: ::: src/wizard.vala @@ +279,3 @@ + private void on_installer_recognized (Osinfo.Media os_media, Osinfo.Os os) { + prep_media_label.label = os.name; + Downloader.fetch_os_logo.begin (installer_image, os, 128); No, I don't see where we block to wait for the logo. At least we shouldn't be since we always have the CD icon.
Created attachment 231875 [details] [review] util: Add class for reporting & tracking progress Turn 'progress' property back to automatic.
Created attachment 231876 [details] [review] wizard,installer: Track progress of media preparation Don't update progressbar if progress is less than 1%.
Review of attachment 231876 [details] [review]: ::: src/wizard.vala @@ +273,3 @@ + var progress = new ActivityProgress (); + progress.notify["progress"].connect (() => { + if (progress.progress >= 0.01) not if progress > 1%, if it *changed* more than 1%
Review of attachment 231876 [details] [review]: ::: src/wizard.vala @@ +273,3 @@ + var progress = new ActivityProgress (); + progress.notify["progress"].connect (() => { + if (progress.progress >= 0.01) My name isn't Zeeshan if i dont make silly mistakes. :) I'll correct..
Created attachment 231880 [details] [review] wizard,installer: Track progress of media preparation Don't update progressbar if progress didn't change more than 1%.
Review of attachment 231834 [details] [review]: ::: src/wizard.vala @@ +279,3 @@ + private void on_installer_recognized (Osinfo.Media os_media, Osinfo.Os os) { + prep_media_label.label = os.name; + Downloader.fetch_os_logo.begin (installer_image, os, 128); What i mean is, isn't the point of downloading the logo in the preparation step that it is available in the next steps. If we don't actually wait for the download to finish that will not be true. I guess having started the download early means we'll get it somewhat faster when needed though...
Not sure this is the right bug to mention this, but the 'continue' button is sensitive during driver download.
Review of attachment 231834 [details] [review]: ::: src/wizard.vala @@ +279,3 @@ + private void on_installer_recognized (Osinfo.Media os_media, Osinfo.Os os) { + prep_media_label.label = os.name; + Downloader.fetch_os_logo.begin (installer_image, os, 128); The thing is that we don't exactly *need* the logo for anything. Some essential OS don't even have a logo (and they wont ever). Its just something thats nice to have. Also for medias automatically discovered and put in the wizard's menu, the logo download will start much before this step (actually for them, this step doesn't even apply).
(In reply to comment #48) > Not sure this is the right bug to mention this, but the 'continue' button is > sensitive during driver download. It is? that weird cause I just checked here with and without these patches and its insensitive for me.
Created attachment 231988 [details] Preparation step w/o patches
Created attachment 231989 [details] Preparation step with patches
Without patches I get this https://bugzilla.gnome.org/show_bug.cgi?id=690321 My build is from commit 3cfaca92e7b6ba4df9431bbd3bc849ff0ee9dab1 Author: Nilamdyuti Goswami <ngoswami@redhat.com> Date: Thu Dec 20 13:53:49 2012 +0530 Assamese translation updated
The image in comment #52 looks seriosly busted in the sidebar. I guess that is unrelated to the patch though. Christophe: I think you pasted the wrong url.
Review of attachment 231834 [details] [review]: ::: src/wizard.vala @@ +279,3 @@ + private void on_installer_recognized (Osinfo.Media os_media, Osinfo.Os os) { + prep_media_label.label = os.name; + Downloader.fetch_os_logo.begin (installer_image, os, 128); So, for a case where we *do* have a logo, and its download is not done before we exit the prepare phase, what happens? Do we temporarily show a cdrom icons somewhere and then switch it to the real logo?
(In reply to comment #54) > > Christophe: I think you pasted the wrong url. Oops. At least it was not something embarrassing! http://people.gnome.org/~teuf/gnome-boxes/boxes-wizard.png is what I meant
(In reply to comment #55) > Review of attachment 231834 [details] [review]: > > ::: src/wizard.vala > @@ +279,3 @@ > + private void on_installer_recognized (Osinfo.Media os_media, Osinfo.Os os) > { > + prep_media_label.label = os.name; > + Downloader.fetch_os_logo.begin (installer_image, os, 128); > > So, for a case where we *do* have a logo, and its download is not done before > we exit the prepare phase, what happens? Do we temporarily show a cdrom icons > somewhere and then switch it to the real logo? Yes, though its unrelated to exit of prepare phase.
(In reply to comment #56) > (In reply to comment #54) > > > > Christophe: I think you pasted the wrong url. > > Oops. At least it was not something embarrassing! > http://people.gnome.org/~teuf/gnome-boxes/boxes-wizard.png is what I meant Thats the same as attachment#231988 [details], which is how it is currently. Things get better with these patches: attachment#231989 [details]. :)
(In reply to comment #58) > (In reply to comment #56) > > http://people.gnome.org/~teuf/gnome-boxes/boxes-wizard.png is what I meant > > Thats the same as attachment#231988 [details] No it's not, the 'continue' button is sensitive (NB: this seems kind of related to this bug, but I can split this discussion off to another bug as things are getting a bit mixed up)
zeeshan: How is it unrelated? Switching around the icon looks pretty ugly, and its a behavioural change from this patch. I.e. before the patch it did not exit the prepare phase before the logo download is finished.
(In reply to comment #60) > zeeshan: > How is it unrelated? Switching around the icon looks pretty ugly, and its a > behavioural change from this patch. I.e. before the patch it did not exit the > prepare phase before the logo download is finished. Then I misunderstand what the issue is. Let me ask some questions to clarify: Do you think we should wait for the real logo to download (and loaded/rendered into the image) *before* we leave preparation stage? If so, why exactly? How does switching look uglier than before if these patches are making it more unlikely that user sees the switch happening?
The behaviour before the patch is to wait for the logo download to finish. This seems like the right behaviour, as it avoids a visual glitch later (we don't have to show the placeholder logo for a while). Your patch changes this behaviour to not wait for the download to finish. Or maybe i'm confused about what the logo is needed for?
(In reply to comment #62) > The behaviour before the patch is to wait for the logo download to finish. This > seems like the right behaviour, as it avoids a visual glitch later (we don't > have to show the placeholder logo for a while). The logo download still continues and logo is cached so only wait that is involved in subsequent usage is the time it take to load & render it to the GtkImage. > Or maybe i'm confused about what the logo is needed for? As I said before, logo is not exactly needed, per say. Before the patches, weren't show any info about the OS or progress but now we do so showing of logo is even less important now.
I still don't understand this. If we're starting the logo download, that seems to imply that later stages may need the logo (or why else would we start the download?). If that is so, isn't it better to also wait until its downloaded (it should be fast), rather than having the later stage that needs the logo possibly show a fallback icon for a short while. This is how it used to work, i just don't see a good explanation for the change in this behaviour. But its not like the current patch doesn't work, so, in order to get this in before everyone leaves for xmas, ack.
Review of attachment 231875 [details] [review]: ack
Review of attachment 231880 [details] [review]: ack
(In reply to comment #64) > I still don't understand this. If we're starting the logo download, that seems > to imply that later stages may need the logo (or why else would we start the > download?). That is not correct. We don't show the logo in the later stages. We show the logo if it downloads fast enough, not otherwise. Once downloaded, it'll be shown in the subsequent installs for the same OS (as it'll just be about loading). It'll definitely makes sense to keep the current behavior if we were to show the logo in any later stage. > But its not like the current patch doesn't work, so, in order to get this in > before everyone leaves for xmas, ack. Cool! We can always change it back again, assuming Mayans weren't correct about today being the last day. :)
Attachment 231745 [details] pushed as 8621863 - installer: Refactor setup_pre_install_drivers() Attachment 231747 [details] pushed as 1ea950b - downloader: Allow tracking of download progress Attachment 231833 [details] pushed as 3140f86 - wizard,css: Separate labels for media & status info Attachment 231834 [details] pushed as cc4a8bb - wizard: Display OS info & logo ASAP during preparation Attachment 231875 [details] pushed as 47dcb6b - util: Add class for reporting & tracking progress Attachment 231880 [details] pushed as 6be8eb2 - wizard,installer: Track progress of media preparation