Created attachment 225644 [details] [review]
vm-creator: Derive VMCreator from GLib.Object

This is so that we can make use of GLib.Object.notify signal on
VMCreator properties.

Created attachment 225645 [details] [review]
vm-creator: Set post-install config after install

The main reason we have been setting post-installation configuration at
first domain launch was the scenerio of first post-install reboot
happening while Boxes is not runnnig: User launches the saved
(in-installation) domain from another (libvirt-based) app.

While this scenerio is still real, it is:

a. a corner case.
b. very hard to maintain *proper* support for if we want to be able to
   better track the installation. The next patch in this series is along
   that line and it requires this change.
c. not complete broken by this patch: Boxes will just not know that
   installation is finished until another reboot/shutdown.

Created attachment 225646 [details] [review]
vm-creator: Track installation using num. of reboots

For known OSs, make use of the new libosinfo API to track installations
by simply counting the number of reboots the VM takes.

Created attachment 225832 [details] [review]
vm-creator: Track installation using num. of reboots

For known OSs, make use of the new libosinfo API to track installations
by simply counting the number of reboots the VM takes.

Created attachment 225859 [details] [review]
Make MediaManager a singleton

Created attachment 225860 [details] [review]
vm-creator: Also track saved installations

We were loosing our ability to track installation if Boxes was quit
during installation. This patches fixes it.

Created attachment 225861 [details] [review]
vm-creator: Don't show 0% progress

Created attachment 225862 [details] [review]
vm-creator: Set post-install config after install

v2

Created attachment 225863 [details] [review]
vm-creator: Track installation using num. of reboots

v2

Fwiw I'm strongly against getting this series in 3.6, it's quite big, and the bug it fixes ("Install in progress" label going away too soon) is too cosmetic to deserve such big changes (more code = more risk for bugs/regressions). Moreover, all the percentage stuff adds unnecessary complication as it's not used for now if I'm not mistaken.

It seems that the code would be much simpler if we didn't try to track number of reboots across boxes restarts, and only counted reboots while boxes is running. We can then get the more complete but bigger change in 3.7, while still fixing most of the bug for 3.6. I'd favour such an approach (and a series doing this first, and then the rest of the changes would be very appreciated ;)

I've run some tests with it and the winxp behaviour is not very good, it starts with "Installing...", then jumps to 50%, stays there during the installation, then reboots, briefly shows 100%, and then the label disappears as if installation was done.

However, after this reboot, Windows still runs through various post installation stuff which took 5 to 10 minutes there, so we cannot consider the install is over right after reboot if we want perfect feedback.

(In reply to comment #11)
> I've run some tests with it and the winxp behaviour is not very good, it starts
> with "Installing...", then jumps to 50%, stays there during the installation,
> then reboots, briefly shows 100%, and then the label disappears as if
> installation was done.

As i said on IRC, this is still an improvement over current behavior. Currently we say "Installing.." and give no feedback and then remove this label (which really means the same as 100% done but just not as explicit) long before installation is anywhere near done. I think telling 50% while installation is only ~10% done is a LOT better than telling user installation is 100% done.

> However, after this reboot, Windows still runs through various post
> installation stuff which took 5 to 10 minutes there, so we cannot consider the
> install is over right after reboot if we want perfect feedback.

A perfect feedback would be awesome indeed but point of these patches is only to improve the current situation for now. Also stress on the word "post" here.

Just to be clear, here is the two issues these patches fix: Don't assume/communicate that installation is done:

1. before it really is. This is not completely fixed (as you pointed out) but it is very much improved.
2. just because user quit Boxes before installation was finished.

(In reply to comment #12)
> 
> A perfect feedback would be awesome indeed but point of these patches is only
> to improve the current situation for now.

This is not what I thought initially, I thought they made things work flawlessly...

> 
> Just to be clear, here is the two issues these patches fix: Don't
> assume/communicate that installation is done:
> 
> 1. before it really is. This is not completely fixed (as you pointed out) but
> it is very much improved.

This is marginally better than before, but "Installing..." still disappears quite some time before the installation is done. It's just more deterministic now as it always disappears after the last reboot

> 2. just because user quit Boxes before installation was finished.

Given the code complication that goes with handling that, I'd just give up on this for 3.6 and make sure we get rid of the "installing" label when the user does that. This is a corner case for which I don't feel bad saying the user this is a known bug and that he should not do that.
Again, if this was a one-line fix, I'd be totally fine with having this in, but it seems to me that handling 2) is about half of the changes in this series.


As said on IRC, after testing this, I'm nearly leaning towards saying that the whole series is 3.7 material as this does not fully fix the issue and is quite complicated code. I could probably get convinced to get a minimal fix for 1) for 3.6 provided a few small/obvious patches...

Your list is also missing a "3. Show a "50%" progress label during installation"

(In reply to comment #13)
> (In reply to comment #12)
> > 
> > A perfect feedback would be awesome indeed but point of these patches is only
> > to improve the current situation for now.
> 
> This is not what I thought initially, I thought they made things work
> flawlessly...

I wonder why because all this was based on number of reboots and there is no way to figure out when post-installation steps (part of the first boo after install) have been taken though number of reboots.
 
> > Just to be clear, here is the two issues these patches fix: Don't
> > assume/communicate that installation is done:
> > 
> > 1. before it really is. This is not completely fixed (as you pointed out) but
> > it is very much improved.
> 
> This is marginally better than before,

That assessment is quite unfair. Installation itself really *is* complete, it would just be very nice to cover also the post installation changes as part of installation even though the underlying OS doesn't agree with that.

> As said on IRC, after testing this, I'm nearly leaning towards saying that the
> whole series is 3.7 material as this does not fully fix the issue and is quite
> complicated code.

Leaning towards? Thats what you have been saying since even before looked at the patches. :)

> I could probably get convinced to get a minimal fix for 1)
> for 3.6 provided a few small/obvious patches...

As I've been trying to tell you that I do not have any hope of convincing you (actually on anything on which you have a strong opinion). OTOH I will not decide whether these patches go to 3.6 or not but instead leave it to any other active developers (Marc-Andre or Alex).
 
> Your list is also missing a "3. Show a "50%" progress label during
> installation"

That is not the point of these patches but rather a bonus we get by reusing some existing code and translated strings.

(In reply to comment #14)
> (In reply to comment #13)
> > This is not what I thought initially, I thought they made things work
> > flawlessly...
> 
> I wonder why because all this was based on number of reboots and there is no
> way to figure out when post-installation steps

Just that I didn't think about the post installation steps before retesting Windows installations. This was not mentioned anywhere in this bug report, so I just didn't think about this. 


> > This is marginally better than before,
> 
> That assessment is quite unfair.

This not unfair, this is entirely subjective, and we agreed to disagree on this kind of things ;)

> 
> > As said on IRC, after testing this, I'm nearly leaning towards saying that the
> > whole series is 3.7 material as this does not fully fix the issue and is quite
> > complicated code.
> 
> Leaning towards? Thats what you have been saying since even before looked at
> the patches. :)
> 
> > I could probably get convinced to get a minimal fix for 1)
> > for 3.6 provided a few small/obvious patches...
> 
> As I've been trying to tell you that I do not have any hope of convincing you
> (actually on anything on which you have a strong opinion). OTOH I will not
> decide whether these patches go to 3.6 or not but instead leave it to any other
> active developers (Marc-Andre or Alex).

Ok, so now you are being totally unfair and misrepresenting the truth. I was initially against the patches, then thought more about it and suggested a way forward, splitting and reordering the patches to start with changes that are likely to be uncontroversial and could easily be reviewed and get in, and then making a decision on the controversial bits. For some reasons (which I honestly don't know what they are), you didn't seem very interested in going this way.

> 
> > Your list is also missing a "3. Show a "50%" progress label during
> > installation"
> 
> That is not the point of these patches but rather a bonus we get by reusing
> some existing code and translated strings.

Yes, but this is still a user-visible change, so worth mentioning, especially as this causes a behaviour change and is not really a bug fix.

Review of attachment 225860 [details] [review]:

::: src/app.vala
@@ +279,2 @@
         foreach (var domain in connection.get_domains ())
+            try_add_existing_domain.begin (source, connection, domain);

Doing this in parallel like this sems like somewhat of a race wrt the domain_removed signal handler below. Otoh, you can't just wait with connecting to domain_removed, as that may cause you to miss domain removals during the execution of try_add_existing_domain.

Maybe you need to separate the creation of the domain object from the async part of starting it up.

::: src/vm-configurator.vala
@@ +177,3 @@
+                break;
+            }
+        }

This seems to duplicate some code inside post_install_setup, except this copy doesn't handle floppy install media.

Review of attachment 225862 [details] [review]:

::: src/vm-creator.vala
@@ -165,3 @@
-            var config = machine.domain.get_config (GVir.DomainXMLFlags.INACTIVE);
-            VMConfigurator.mark_as_installed (config);
-            machine.domain.set_config (config);

It seems like this set_config() call is lost with the move to set_post_install_config. Is it not needed anymore?

Review of attachment 225863 [details] [review]:

::: src/vm-configurator.vala
@@ +128,3 @@
+            domain.set_custom_xml (xml, BOXES_NS, BOXES_NS_URI);
+        } catch (GLib.Error error) { assert_not_reached (); /* We are so screwed if this happens */ }
+    }

This is just too ugly to live.
I think all the xml setters should be using the libxml Node and Doc api to build up a tree of node and then replace the ones you need to. That way you can propely parse things while keeping the other unrelated nodes as-is. This means e.g. that we don't trample over some future extension to the xml if you happen to start an older boxes.


Honestly, the gvir_config_domain_get_custom_xml() function is sort of broken as it only returns the first namespace match. I don't see any reason we should have to stuff all the boxes specific metadata into a single tag.
I.e. We should be able to have:
  <metadata>
    <boxes:os-state xmlns:boxes="http://live.gnome.org/Boxes/">installed</boxes:os-state>
    <boxes:num-reboots xmlns:boxes="http://live.gnome.org/Boxes/">2</boxes:num-reboots>
  </metadata>

Rather than:
  <metadata>
    <boxes:gnome-boxes  xmlns:boxes="http://live.gnome.org/Boxes/">installed>
      <boxes:os-state xmlns:boxes="http://live.gnome.org/Boxes/">installed</boxes:os-state>
      <boxes:num-reboots xmlns:boxes="http://live.gnome.org/Boxes/">2</boxes:num-reboots>
    </boxes:gnome-boxes>
  </metadata>

@@ +201,3 @@
         try {
+            var custom_xml = BOXES_XML.printf (INSTALLED_XML);
+            domain.set_custom_xml (custom_xml, BOXES_NS, BOXES_NS_URI);

This overrides the num-reboots node. I realize that this is what you want in this particular case, but this is not exactly clear from the code...

This should also create a parse tree for the xml and modify only the nodes necessary.

@@ +324,2 @@
                     return reader.read_string ();
+            } while (reader.read () == 1);

This is a slight change in semantics which i'm not sure is right. It goes into children whereas before it just looked for "toplevel" nodes. This is right for the newly introduced gnome-boxes toplevel node, but we should not go deeper into other nodes.

Also, while this change is backwards compatible in the sense that we pick up old os-status:es its not in the sense of running an older boxes version on the same homedir that another one saved the state on.

::: src/vm-creator.vala
@@ +270,3 @@
+        } else
+            // This must mean: install_media.os_media != null
+            percent = (int) num_reboots * 100 / install_media.os_media.installer_reboots;

Is installer_reboots always set? I.e. is it "1" as the fallback if we don't know (which would give the previous behaviour).

I don't oppose this as strongly as Christophe, although it clearly needs some work. For instance, the backwards compat issue seems problematic inside a stable series for sure (and probably also outside of it) and the xml handling is not very awesome. But I also think that this is not exactly super important to work on right now compared to some other bugs, as the current code is basically usable even though we lie in the UI a bit.

I also question the final result a bit. Its a good thing that we don't claim the installation is done before we *know* its not. *BUT* it is also imho always a complete UI failures every time an installer shows a progress bar or percentage that goes from 0 to 100 in any other linear fashion than "actual time the user has to wait". And, since this is impossible to get right (even the installed_size tracking we use gets this wrong in the time scale and the fact that its not actually *finished* just because all the bits are on the disk).
Its even more broken to show something that goes from 0 to 50% to 100% in extremely long and timewise irregular jumps. 

So, I think we should remove all the percentage displayed from the UI and just go with a single "Installing" note for as long as we know it is true.

Review of attachment 225863 [details] [review]:

::: src/vm-creator.vala
@@ +270,3 @@
+        } else
+            // This must mean: install_media.os_media != null
+            percent = (int) num_reboots * 100 / install_media.os_media.installer_reboots;

Yes, the default is "1" if unknown but you get a "-1" if you try to get this value from a media w/o an installer on it.

(In reply to comment #19)
> I don't oppose this as strongly as Christophe, although it clearly needs some
> work. For instance, the backwards compat issue seems problematic inside a
> stable series for sure (and probably also outside of it)

I'm going to release libosinfo tomorrow (today by calendar actually) and push it to F18 *before* next boxes release. So I don't see any problem but if you insist, this is trivial to work around.

> and the xml handling
> is not very awesome. But I also think that this is not exactly super important
> to work on right now compared to some other bugs, as the current code is
> basically usable even though we lie in the UI a bit.

OK, two devs against one so I won't argue to get these changes to F16 anymore and as you have seen, I have been trying to fix more important bugs lately. I will try to submit a much simpler patch that solves the issue for at least the case of user keeping the boxes alive while installation is going on and does not show progress.

> I also question the final result a bit. Its a good thing that we don't claim
> the installation is done before we *know* its not. *BUT* it is also imho always
> a complete UI failures every time an installer shows a progress bar or
> percentage that goes from 0 to 100 in any other linear fashion than "actual
> time the user has to wait".

While this is something very annoying for us geeks who know more than we should on whats really going on (or at least we think we do) and crave for precision in data presented to us, AFACT most people prefer "some" feedback on progress over none. People do get annoyed when you show them "remaining time" and then the value jumps around, 3 seconds remaining.. 10 days remaining.. and so on. Fortunately we are not doing that and its pretty clear from the past tense in the label that we are talking about how much is done rather than how much remains.

> And, since this is impossible to get right (even
> the installed_size tracking we use gets this wrong in the time scale and the
> fact that its not actually *finished* just because all the bits are on the
> disk).
> Its even more broken to show something that goes from 0 to 50% to 100% in
> extremely long and timewise irregular jumps. 

This part I kinda agree with. I think a better way would be to combine both these approaches. i-e track progress through disk allocation but base the decision of 100% on reboots.

Review of attachment 225862 [details] [review]:

::: src/vm-creator.vala
@@ -165,3 @@
-            var config = machine.domain.get_config (GVir.DomainXMLFlags.INACTIVE);
-            VMConfigurator.mark_as_installed (config);
-            machine.domain.set_config (config);

lost how? here we are deleting a method we dont use anymore: mark_as_installed. set_post_install_config() still does set_config() as it has been.

Created attachment 226305 [details] [review]
vm-creator: Set post-install config after install

v2: rebased on master

Created attachment 226306 [details] [review]
vm-creator: Track installation using num. of reboots

v2:

* Doesn't save anything to XML
* No progress report

Review of attachment 225862 [details] [review]:

::: src/vm-creator.vala
@@ -165,3 @@
-            var config = machine.domain.get_config (GVir.DomainXMLFlags.INACTIVE);
-            VMConfigurator.mark_as_installed (config);
-            machine.domain.set_config (config);

vm-configurator:post_install_setup () calls set_post_install_os_config() first, which indeed calls set_config().

Then we used to wait until mark_as_installed which updated the xml and saved. However, now post_install_setup() immediately calls a new mark_as_installed which does not save the xml, *after* the call to post_install_setup() wich does save the xml.

Ah, sorry i misread set_post_install_os_config vs set_post_install_config

(In reply to comment #26)
> Ah, sorry i misread set_post_install_os_config vs set_post_install_config

So patches are good to go then?

Review of attachment 226306 [details] [review]:

Looks good

Review of attachment 226305 [details] [review]:

I can't really wrap my head around it, let's say it's good. "scenario" in the commit log.

Attachment 226305 [details] pushed as a4a6229 - vm-creator: Set post-install config after install
Attachment 226306 [details] pushed as d62ce99 - vm-creator: Track installation using num. of reboots