GNOME Bugzilla – Bug 684233
"customize" wizard button not working for spice:// boxes
Last modified: 2016-03-31 13:54:00 UTC
Create a new box Enter a spice:// URL (eg spice://localhost:5900) Click on "customize" on the review page -> a warning is printed on the console, and no review page (gnome-boxes:26010): Boxes-CRITICAL **: boxes_app_select_item: assertion `item != NULL' failed Expected result: properties are shown to enable/disable USB redir, cut&paste, ...
Meh, this is really bad! I suggest we temporarily work around this by only showing the 'customize' button for VMs.
Sounds good if the "is vm' check is not too ugly..
Created attachment 225901 [details] [review] wizard: Disable customization for non-VMs for now Customization assumes a machine ready at 'review' page and that is currently only true when creating VMs. Since this feature is most useful for VMs (people wanting to change storage capacity and RAM) and its still very much possible to change machine properties after creation, lets go with this least intrusive solution for now.
Review of attachment 225901 [details] [review]: Looks good ::: src/wizard.vala @@ +417,3 @@ } + if (machine != null) Maybe a comment explaining when machine can be null here?
Comment on attachment 225901 [details] [review] wizard: Disable customization for non-VMs for now Attachment 225901 [details] pushed as 98bc991 - wizard: Disable customization for non-VMs for now + comment added as suggested in the review.
This isn't fixed really, we should be allowing customization of spice (or any box) before creation, just that its not as important as in case of VM. So I'll keep this open.
Created attachment 227528 [details] [review] wizard: be more strict about spice uri A spice URI need either a port or a query string with port. Avoid the follwing critical: (gnome-boxes:9570): Boxes-CRITICAL **: boxes_query_construct: assertion `query != NULL' failed
Created attachment 227529 [details] [review] wizard: split libvirt_machine and machine machine can be reused for non-libvirt cases.
Created attachment 227530 [details] [review] remote-machine: return properties when display isn't connected Currently, get_properties () returns correctly only when the display is connected, because the display isn't created until then.
Created attachment 227531 [details] [review] remote-machine: verify the kind of remote is known Warn only in the console if not, this is a programming error.
Created attachment 227532 [details] [review] wizard: allow properties access for remote machine
Review of attachment 227528 [details] [review]: Looks good otherwise. ::: src/spice-display.vala @@ +215,3 @@ } + +// FIXME: this kind of function should be part of spice-gtk * I'm guessing that we are putting it here for not bumping spice-gtk dep? I think we should special-case some of our deps so that we can bump the deps: libvirt-glib, libosinfo and spice-gtk. * Why a global function in this module rather than static method? * Why 'static' keyword if not in a class? ::: src/wizard.vala @@ +393,3 @@ + } catch (Boxes.Error error) { + // this shouldn't happen, since the URI was validated before + critical (error.message); I prefer g_assert*() for situations that are not supposed to happen. The idea is that during testing phases, an assert will be hard to ignore and will most likely be reported, treated as high priority and fixed ASAP. In production, asserts could simply be disabled at buildtime. You can disagree of course but this is one of the things that we should have a project-wide policy/rule about.
Review of attachment 227529 [details] [review]: ACK
Review of attachment 227530 [details] [review]: Looks ok but *IMHO* a better title would be " remote-machine: Separate display creation from connection".
Review of attachment 227531 [details] [review]: Good otherwise. ::: src/app.vala @@ +317,3 @@ + collection.add_item (machine); + } catch (Boxes.Error error) { + warning (error.message); like in the previous patch, I think this one should be dealt with g_assert*. ::: src/remote-machine.vala @@ +6,3 @@ + public RemoteMachine (CollectionSource source) throws Boxes.Error { + if (source.source_type != "spice" && + source.source_type != "vnc") nitpick: both cases nicely fit in one line so no need to split lines..
Review of attachment 227532 [details] [review]: "access for" -> "access to" in commit log. Looks good otherwise.
Review of attachment 227528 [details] [review]: ::: src/spice-display.vala @@ +215,3 @@ } + +// FIXME: this kind of function should be part of spice-gtk - we put it in Boxes, because it does things specific to Boxes (return ports values) and we don't have a way to make it work nicely with SpiceSession, and I am not sure it will do what Boxes need in the end (returned exact parsed values and errors). - global or static I don't think it matters anyway. ::: src/wizard.vala @@ +393,3 @@ + } catch (Boxes.Error error) { + // this shouldn't happen, since the URI was validated before + critical (error.message); I don't think we need a rule for that, I don't like assert() in code in general, I run with G_DEBUG=fatal_criticals by default, all devs should do the same imho, and keep using return_if_fail or critical if he likes to. Removing assert() line in production is not a good idea, I much prefer keeping a critical trace...
Review of attachment 227531 [details] [review]: ::: src/remote-machine.vala @@ +6,3 @@ + public RemoteMachine (CollectionSource source) throws Boxes.Error { + if (source.source_type != "spice" && + source.source_type != "vnc") imho, "equivalent" cases can be put at same level of indentation, so it is easy to see which values/conditions are handled. For similiar reason we do foo ("prop_a", 0, "prop_b", 42, null) and not a single line with all the arguments.
Review of attachment 227531 [details] [review]: OK then..
Review of attachment 227528 [details] [review]: k
Attachment 227528 [details] pushed as c94e493 - wizard: be more strict about spice uri Attachment 227529 [details] pushed as d2b6eba - wizard: split libvirt_machine and machine Attachment 227530 [details] pushed as f970cb4 - remote-machine: return properties when display isn't connected Attachment 227531 [details] pushed as f36f523 - remote-machine: verify the kind of remote is known Attachment 227532 [details] pushed as 6ba65a9 - wizard: allow properties access for remote machine