GNOME Bugzilla – Bug 683417
Fix running "gnome-boxes live.iso"
Last modified: 2016-03-31 13:53:48 UTC
The first patch is a small improvement to allow cancelling review() step.
Created attachment 223528 [details] [review] wizard: make review cancellable Do not disable all wizard navigation buttons, but allow to cancel review step.
Created attachment 223529 [details] [review] wizard: fix running from command line "gnome-boxes live.iso" Now that we create the domain asynchronously in review() we need to wait until it is ready before create() can finish. Fix related smaller bugs.
Review of attachment 223528 [details] [review]: Makes sense to me, but I'm not comfortable enough with this stuff to get what it will be used for :-/ Could you tweak the commit log a bit, at first I thought the 'cancel' button was disabled on the Review page, and that this bug was about that. I think there's one line that needs to be moved from the next commit to this one.
Review of attachment 223529 [details] [review]: ::: src/wizard.vala @@ +33,2 @@ private VMCreator? vm_creator; + protected LibvirtMachine? machine { get; set; } Is this change really needed? machine doesn't seem to be used in a class inheriting from Wizard @@ -83,2 @@ case WizardPage.LAST: - skip_review_for_live = false; This looks like it belongs in the previous commit? @@ +182,3 @@ + // wait until the machine is ready or not + var wait = notify["machine"].connect (() => { + create.callback (); If machine = yield vm_creator.create_vm (null); fails, won't we get stuck there until the next time a VM creation succeeds? Is this handled by this cancellable work in the previous commit? @@ +693,2 @@ public void append_customize_button (CustomizeFunc cutomize_func) { + // there is nothing to customize if review page is empty This belongs in a separate commit
Review of attachment 223529 [details] [review]: ::: src/wizard.vala @@ +33,2 @@ private VMCreator? vm_creator; + protected LibvirtMachine? machine { get; set; } to use notify signal, we need to make it either public or protected. It is a gobject limitation. @@ +182,3 @@ + // wait until the machine is ready or not + var wait = notify["machine"].connect (() => { + create.callback (); if it fails, machine = null will trigger the notify signal and create() will fail later on. @@ +693,2 @@ public void append_customize_button (CustomizeFunc cutomize_func) { + // there is nothing to customize if review page is empty ok
Review of attachment 223529 [details] [review]: ::: src/wizard.vala @@ +33,2 @@ private VMCreator? vm_creator; + protected LibvirtMachine? machine { get; set; } Ah ok, thanks. @@ +182,3 @@ + // wait until the machine is ready or not + var wait = notify["machine"].connect (() => { + create.callback (); Sorry, which machine = null? I could only find one in create() after this code, and one in destroy_machine(), I don't think either of these will get called when review () fails?
Review of attachment 223529 [details] [review]: ::: src/wizard.vala @@ +182,3 @@ + // wait until the machine is ready or not + var wait = notify["machine"].connect (() => { + create.callback (); right, we need to notify the machine property when VM creator failed. thanks for pointing out, updating patch.
Created attachment 223557 [details] [review] wizard: skipping Review page should still run through review() During review(), the VM is now created. We can't skip that step from Setup page entirely anymore. This is part of fixing running boxes from command line with "gnome-boxes live.iso".
Created attachment 223558 [details] [review] wizard: make review cancellable Do not disable all wizard navigation buttons, but allow to cancel review step.
Created attachment 223559 [details] [review] wizard: fix running from command line "gnome-boxes live.iso" Now that we create the domain asynchronously in review() we need to wait until it is ready before create() can finish. Fix related smaller bugs.
Created attachment 223560 [details] [review] wizard: fix critical when running wizard automatically from cli When running from CLI, the summary is not generated (machine is null after create()), current_row = 0, and append_customize() will end up with a critical since there is no row to append to.
Review of attachment 223557 [details] [review]: NB: no clue about this code, trusting you on this one ;)
Review of attachment 223558 [details] [review]: Same comments as for the previous version of this patch https://bugzilla.gnome.org/review?bug=683417&attachment=223528
Review of attachment 223560 [details] [review]: The last 'to.' in the log is weirdly spaced, looks good otherwise. It seems the log does not apply to all ISOs ran from the command line, I got a summary and a customize button when testing with a f17 installer.
Review of attachment 223559 [details] [review]: Looks good apart from a minor question. ::: src/wizard.vala @@ -83,2 @@ case WizardPage.LAST: - skip_review_for_live = false; Same question as in the previous review, shouldn't it go in the commit making the review cancellable?
Created attachment 223623 [details] [review] wizard: skipping Review page should still run through review() During review(), the VM is now created. We can't skip that step from Setup page entirely anymore. This is part of fixing running boxes from command line with "gnome-boxes live.iso".
Created attachment 223624 [details] [review] wizard: make review cancellable Do not disable all wizard navigation buttons, but allow to cancel review step (back and cancel button should be always sensitive).
Created attachment 223625 [details] [review] wizard: fix running from command line "gnome-boxes live.iso" Now that we create the domain asynchronously in review() we need to wait until it is ready before create() can finish.
Created attachment 223626 [details] [review] wizard: clear skip_review_for_live at end of review Since review() is async, it may continue while the LAST page has already been reached, we want to keep that variable set until review finishes.
Created attachment 223627 [details] [review] wizard: fix critical when running wizard automatically from cli When running from CLI, with live media, the summary is not generated (machine is null after create()), current_row = 0, and append_customize() will end up with a critical since there is no row to append to.
ACK all
Attachment 223623 [details] pushed as a5b7525 - wizard: skipping Review page should still run through review() Attachment 223624 [details] pushed as c61e305 - wizard: make review cancellable Attachment 223625 [details] pushed as ebd2048 - wizard: fix running from command line "gnome-boxes live.iso" Attachment 223626 [details] pushed as 76c414a - wizard: clear skip_review_for_live at end of review Attachment 223627 [details] pushed as 3e14f7f - wizard: fix critical when running wizard automatically from cli