GNOME Bugzilla – Bug 682300
Track & show installation progress
Last modified: 2016-03-31 13:55:12 UTC
Here is the first implementation of showing to user some progress of the installation. It works pretty well from what I can tell from my testing so far, athough i kept changing the code so I might have broken something after last test (which was just an hour ago). I will run more tests while I get a review..
Created attachment 221913 [details] [review] vm-creator: Start under-installation domain When domain is still under installation at shutdown, its most probably because we ask libvirt to shutdown on guest reboot (to activate the new configuration).
Created attachment 221914 [details] [review] Make Machine.info be a public automatic property This means that now LibvirtMachine sets this property appropriately.
Created attachment 221915 [details] [review] installer-media: Add installed_size property It represents the storage allocation of the OS immediately after installation. We only know the value of this property for express installations. This information should really come from libosinfo and hopefully it soon will be, along with the rest of the OS-specific express installation code/data.
Created attachment 221916 [details] [review] vm-creator: Track & show installation progress Its completely based on us knowing the guest disk usage after installation and we only know that in case of express installation. We do the same for non-express installations and live VMs. Another limitation is that we lose track of installations if user quits boxes in btw installation. On the bright side, we now have a more reliable way of knowing when installation is really complete for express installs.
Created attachment 221941 [details] [review] vm-creator: Track & show installation progress V2: A small change needed when patch from bug#682311 is applied to avoid a crash. W/o patch from bug#682311, we don't get rid of "Installing.." string in the UI.
Review of attachment 221914 [details] [review]: ::: src/vm-creator.vala @@ +22,3 @@ + else + machine.info = _("Live"); + it would make more sense to factor this in a seperate update_info() to avoid logic and strings duplication
Review of attachment 221915 [details] [review]: ack
Review of attachment 221941 [details] [review]: ::: src/vm-creator.vala @@ +213,3 @@ + GVir.StorageVolInfo volume_info; + try { + volume_info = volume.get_info (); I guess we want to make that call async, since we do it regularly. otherwise, it may freeze UI intermitently...
(In reply to comment #8) > Review of attachment 221941 [details] [review]: > > ::: src/vm-creator.vala > @@ +213,3 @@ > + GVir.StorageVolInfo volume_info; > + try { > + volume_info = volume.get_info (); > > I guess we want to make that call async, since we do it regularly. otherwise, > it may freeze UI intermitently... Yeah but this will require changes to libvirt-glib and we'll have to dump dep again. Since Daniel release it yesterday on a quick notice for us, I don't dare to ask him to rush in another release. :) Could we live with sync for now?
(In reply to comment #9) > (In reply to comment #8) > > Review of attachment 221941 [details] [review] [details]: > > > > ::: src/vm-creator.vala > > @@ +213,3 @@ > > + GVir.StorageVolInfo volume_info; > > + try { > > + volume_info = volume.get_info (); > > > > I guess we want to make that call async, since we do it regularly. otherwise, > > it may freeze UI intermitently... > > Yeah but this will require changes to libvirt-glib and we'll have to dump dep > again. Since Daniel release it yesterday on a quick notice for us, I don't dare > to ask him to rush in another release. :) Could we live with sync for now? you can use run_in_thread() in the meantime, we already do that
Created attachment 221990 [details] [review] Make Machine.info be a public automatic property V2: Factored out some code in update_machine_info().
Created attachment 221991 [details] [review] vm-creator: Track & show installation progress V2: * The periodic calls to get_progress are now done async. * Reduced margin of error to better suit Fedora 17.
These patches got in, didn't they? Just confused by the patch status. I've played a bit with this today (winxp/win7 installations), and it's quite easy to get to 100% while the installation is still in progress by using non-English ISOs, or using different Windows versions (ultimate, professional, home, ...). When this happens, I've seen the 'Installing' label disappear early in one case, and I've also seen it replace the '100% Installed' label in another case (I think). I've also been able to finish an installation while Boxes thinks it's only at 65% (en_windows_7_professional_n_with_sp1_x86_dvd_u_677328.iso). In this case, the "65% Installed" label stays there, and the completion percentage grows as you use the VM (ie as more disk space is used), and ultimately disappears if the disk grows big enough. Getting this to work nicely in a majority of cases probably means adding lots of missing data to libosinfo...
Created attachment 222524 [details] [review] Use ngettext to tranlate "%d%% installed" 'installed' is invariant in English, but this is not necessarily true in other languages, so it's better to use ngettext to translate it. This also changes the initial 'I' of 'Installed' to be lower case. This will need approval for breaking the string freeze.
Review of attachment 221915 [details] [review]: ::: src/fedora-installer.vala @@ +4,3 @@ + public override uint64 installed_size { + get { + return express_install? 2922930176 : 0; A comment explaining where this value is coming from would have been nice.
Created attachment 222562 [details] [review] Use ngettext to tranlate "%d%% installed" 'installed' is invariant in English, but this is not necessarily true in other languages, so it's better to use ngettext to translate it. This also changes the initial 'I' of 'Installed' to be lower case.
Created attachment 222563 [details] [review] Use ngettext to translate "%d%% installed" 'installed' is invariant in English, but this is not necessarily true in other languages, so it's better to use ngettext to translate this string. This commit also changes the initial 'I' of 'Installed' to be lower case.
(In reply to comment #17) > Created an attachment (id=222563) [details] [review] > Use ngettext to translate "%d%% installed" > > 'installed' is invariant in English, but this is not necessarily true > in other languages, so it's better to use ngettext to translate this > string. ACK for this part of the change. > This commit also changes the initial 'I' of 'Installed' to be > lower case. Its capital in the design mockup so I kept it this way. Ask mccann as both a designer and native english speaker? :)
(In reply to comment #13) > These patches got in, didn't they? Just confused by the patch status. Yikes, I guess git bz failed to make changes and I didn't notice. Fixed now. > I've played a bit with this today (winxp/win7 installations), and it's quite > easy to get to 100% while the installation is still in progress by using > non-English ISOs, or using different Windows versions (ultimate, professional, > home, ...). When this happens, I've seen the 'Installing' label disappear early > in one case, and I've also seen it replace the '100% Installed' label in > another case (I think). The plan is to get this property from libosinfo but it will have to be based on the new automated installer APIs as the installed size is usually dependent what you choose during installation and automated install is the only thing that can know that. One important detailed I missed when I came-up with this plan (and now noticed, thanks to you) was that install size is also dependent on the installer media being used so need to think how the libosinfo API/XML will look like now. > I've also been able to finish an installation while Boxes thinks it's only at > 65% (en_windows_7_professional_n_with_sp1_x86_dvd_u_677328.iso). In this case, > the "65% Installed" label stays there, and the completion percentage grows as > you use the VM (ie as more disk space is used), and ultimately disappears if > the disk grows big enough. I think this is a bigger issue than the one above. We can work around this in Boxes by having a timeout: if install percentage doesn't increase for a while, we assume installation is done! > Getting this to work nicely in a majority of cases probably means adding lots > of missing data to libosinfo... Sure but once the needed APIs are in place in libosinfo, we'll only assume the correct installed size or not at all.
(In reply to comment #15) > Review of attachment 221915 [details] [review]: > > ::: src/fedora-installer.vala > @@ +4,3 @@ > + public override uint64 installed_size { > + get { > + return express_install? 2922930176 : 0; > > A comment explaining where this value is coming from would have been nice. Sorry, was in a bit of a hurry because of feature freeze. I got it from Fedora-17-x86_64-DVD.iso.
(In reply to comment #19) > > Sure but once the needed APIs are in place in libosinfo, we'll only assume the > correct installed size or not at all. Any ETA on that? Imo until we get this API we shouldn't try to display a percentage except for a few whitelisted ISOs where we know the data is accurate, otherwise this will look weird/buggy.
(In reply to comment #20) > (In reply to comment #15) > > Review of attachment 221915 [details] [review] [details]: > > > > ::: src/fedora-installer.vala > > @@ +4,3 @@ > > + public override uint64 installed_size { > > + get { > > + return express_install? 2922930176 : 0; > > > > A comment explaining where this value is coming from would have been nice. > > Sorry, was in a bit of a hurry because of feature freeze. I got it from > Fedora-17-x86_64-DVD.iso. I mean, if this value is wrong for some reason, how can I compute it myself? Same question for the windows values actually.
(In reply to comment #21) > (In reply to comment #19) > > > > Sure but once the needed APIs are in place in libosinfo, we'll only assume the > > correct installed size or not at all. > > Any ETA on that? Imo until we get this API we shouldn't try to display a > percentage except for a few whitelisted ISOs where we know the data is > accurate, otherwise this will look weird/buggy. If i fail to fix it this week, we'll remove the hard-coded values but we should keep rest of the code, especially the strings so there is a possiblity to get the feature back into 3.6 without breaking freezes.
(In reply to comment #22) > (In reply to comment #20) > > (In reply to comment #15) > > > Review of attachment 221915 [details] [review] [details] [details]: > > > > > > ::: src/fedora-installer.vala > > > @@ +4,3 @@ > > > + public override uint64 installed_size { > > > + get { > > > + return express_install? 2922930176 : 0; > > > > > > A comment explaining where this value is coming from would have been nice. > > > > Sorry, was in a bit of a hurry because of feature freeze. I got it from > > Fedora-17-x86_64-DVD.iso. > > I mean, if this value is wrong for some reason, how can I compute it myself? > Same question for the windows values actually. Immediately after installation is done, do `virsh vol-info NAME`. Oh, and i had a local virsh hack to make it display the whole value: http://fpaste.org/bzoU/
(In reply to comment #23) > (In reply to comment #21) > > (In reply to comment #19) > > > > > > Sure but once the needed APIs are in place in libosinfo, we'll only assume the > > > correct installed size or not at all. > > > > Any ETA on that? Imo until we get this API we shouldn't try to display a > > percentage except for a few whitelisted ISOs where we know the data is > > accurate, otherwise this will look weird/buggy. > > If i fail to fix it this week, we'll remove the hard-coded values but we should > keep rest of the code, especially the strings so there is a possiblity to get > the feature back into 3.6 without breaking freezes. I didn't advocate removing any code, just disabling it in most cases
Attachment 222563 [details] pushed as f183878 - Use ngettext to translate "%d%% installed"
I've changed installed to Installed and pushed this patch since it wasn't obvious which string we want to display when I discussed this with mccann on IRC
Closed this by mistake I think (git bz..)
Created attachment 223094 [details] [review] installer: Drop all install size assumptions Turns out this informtion is not only dependent on the OS and our control over the installer but also on the exact media and architecture as well.
Comment on attachment 223094 [details] [review] installer: Drop all install size assumptions looks good
Comment on attachment 223094 [details] [review] installer: Drop all install size assumptions Attachment 223094 [details] pushed as a5b457f - installer: Drop all install size assumptions
*** Bug 679107 has been marked as a duplicate of this bug. ***
I'm pretty skeptical about the possibility of this ever being truly reliable. It just depends on so many details that we don't control, like the exact version of the OS installer, the arch, decisions done at runtime, installation of not-on-iso data like updates, etc, etc. And I also don't think showing a non-ideal percentage completed value is all that useful to the user (the *only* really useful thing would be to know how long the install would take, and this measure doesn't really give us that).
I guess we can give up on this one now.
Bumped into some related research about this and seems I was right all along: "People didn’t mind so much if it was inaccurate,” Myers says. “They still preferred the progress bar to not having anything at all.” http://mobile.nytimes.com/2014/03/09/magazine/who-made-that-progress-bar.html?_r=1 Based on this I would really want to bring this back. I recall at least mccann agreed with me about this assertion.
Lesson: Should stay away from bugzilla when even slightly intoxicated and tired. :) The last comment still makes a lot of sense though, only that we have no good (even remotely reliable) way to track installation.