GNOME Bugzilla – Bug 686778
Allow more hardware setup
Last modified: 2016-03-31 13:58:20 UTC
There are certain common hardware settings that you might want to do on a guest that we currently don't support. We need to figure out which such settings we should support and how they should work/look. Bug 672268 is about enabling support for usb redirection, but we also need UI support to actually configure which usb devices to redirect. Other common hardware tweaks: * Enable smartcard use in guest * Mount ISOs as CDroms Are there more things we want to expose?
Created attachment 228912 [details] [review] Add get_utf8_basename helper function
Created attachment 228913 [details] [review] Add support for changing the CDROM iso
Note: These depend on the Machine.got_error patch from bug 672268
Also, we this needs latest libvirt-glib.
Review of attachment 228912 [details] [review]: Looks good, one minor comment, ACK regardless of if you change it or not. ::: src/util.vala @@ +85,3 @@ + var info = file.query_info (FileAttribute.STANDARD_DISPLAY_NAME, 0); + name = info.get_display_name (); + } catch (GLib.Error e) { I'd try to call name.get_basename() here as this seems to be more readable than file.get_parse_name()
Review of attachment 228913 [details] [review]: Looks good overall, various small nits. ::: src/libvirt-machine.vala @@ +345,3 @@ add_ram_property (ref list); add_storage_property (ref list); + Looks like unintentional whitespace change. @@ +355,3 @@ case PropertiesPage.DEVICES: + foreach (var device_config in domain_config.get_devices ()) { + if (device_config is GVirConfig.DomainDisk) { Nit: if (!device_config is GVirConfig.DomainDisk) continue; would save one indentation level @@ +368,3 @@ + + var source = disk_config.get_source (); + bool empty = source == null || source == ""; Other nit, bool empty = (source == null || source == ""); would make things more readable (took me a while to parse it) @@ +378,3 @@ + if (empty) { + button_label.set_text (_("Select")); + label.set_markup (Markup.printf_escaped ("<i>%s</i>", _("empty"))); Select/empty/Remove are probably worth translators' comments as they are very unspecific when taken out of context. @@ +467,2 @@ break; + New blank line there too, dunno if that's on purpose.
Review of attachment 228912 [details] [review]: ::: src/util.vala @@ +85,3 @@ + var info = file.query_info (FileAttribute.STANDARD_DISPLAY_NAME, 0); + name = info.get_display_name (); + } catch (GLib.Error e) { More readable, yes, but also wrong. A basename has no guarantee of being in utf8. The filesystem encoding could be something else, and additionally there is no guarantee that any particular file follows the file system encoding. In practice however, for local files info.get_display_name() will get you the basename converted to utf8 as best is possible, and some extra "(invalid encoding)" stuff if the charset conversion partially failed.
Attachment 228912 [details] pushed as 29c8a98 - Add get_utf8_basename helper function Attachment 228913 [details] pushed as f34ce3b - Add support for changing the CDROM iso
smartcard and isos work now. Closing this as I can't think of any important missing commonly used HW setup we want to add (that not in a separate bug already like network).