GNOME Bugzilla – Bug 669409
Several improvements during FOSDEM
Last modified: 2016-03-31 14:01:44 UTC
Sorry too busy drinking beers, I just opened 1 bug for these various patches.
Created attachment 206827 [details] [review] display-page: wait until the widget is realized It can be that the widget isn't yet realized when "show_display" is called. Use the first "draw" even to hook the cursor change event to the display widget.
Created attachment 206828 [details] [review] Teach the Wizard to accept qemu..:// sources
Created attachment 206829 [details] [review] source: do not save automatically if it didn't exist
Created attachment 206830 [details] [review] wizard: update the URL page when entry is updated Update the URL page depending on the URI. We currently do simple guesses based on simple URI parsing, but later on, we could query the URI to gather informations etc..
Created attachment 206831 [details] [review] Don't choke on unknown media, use default values
Created attachment 206832 [details] [review] libvirt-machine: update domain xml when connecting display When the machine is started, the Spice port is added to the domain XML. But the "updated" signal is not emitted in this case, and it fails to lookup connection details.
Review of attachment 206827 [details] [review]: ACK with following change to log: "draw" even -> 'draw' event
Review of attachment 206828 [details] [review]: This is more like a new feature, not a bugfix. Please file into separate bug.
Review of attachment 206829 [details] [review]: ACK
Review of attachment 206830 [details] [review]: Overall it looks OK. Assuming you tested this already? ::: configure.ac @@ -49,3 @@ LIBVIRT_GCONFIG_MIN_VERSION=0.0.4 LIBXML2_MIN_VERSION=2.7.8 -SPICE_GTK_MIN_VERSION=0.7.98 I'm assuming you have a tarball ready soon for 0.9 or is there already? ::: src/wizard.vala @@ +118,3 @@ + prepare_for_location (this.wizard_source.uri, true); + if (source != null) + message (source.source_type); i guess you wanted to remove this but forgot? @@ -115,0 +114,11 @@ + + var text = _("Please enter desktop or collection URI"); + var icon = "preferences-desktop-remote-desktop"; ... 8 more ... coding-style: no {} needed here @@ -181,2 +202,2 @@ source = new CollectionSource (uri.server ?? uri_as_text, uri.scheme, uri_as_text); - + message (uri.scheme); same here. If you intend to put something to console for debugging, use 'debug' and print a bit more verbose information
Review of attachment 206831 [details] [review]: Other than these nitpicks, ACK! ::: src/wizard.vala @@ -227,3 +227,3 @@ try { install_media = InstallerMedia.instantiate.end (result); - resources = os_db.get_resources_for_os (install_media.os, install_media.os_media.architecture); + // XXX: these values could be made editable somehow 1. editable how? 2. XXX -> FIXME. XXX sound like adult movie. :) @@ -229,1 +229,4 @@ - resources = os_db.get_resources_for_os (install_media.os, install_media.os_media.architecture); + // XXX: these values could be made editable somehow + var architecture = "i686"; + if (install_media.os_media != null) { + architecture = install_media.os_media.architecture; I thought we preferred this syntax: var architecture = (install_media.os_media != null) ? install_media.os_media.architecture : "i686";
Review of attachment 206832 [details] [review]: ACK
Attachment 206829 [details] pushed as e57da6f - source: do not save automatically if it didn't exist Attachment 206832 [details] pushed as 1317189 - libvirt-machine: update domain xml when connecting display
Created attachment 206925 [details] [review] Don't choke on unknown media, use default values
Comment on attachment 206925 [details] [review] Don't choke on unknown media, use default values Attachment 206925 [details] pushed as 753ac40 - Don't choke on unknown media, use default values
Review of attachment 206830 [details] [review]: ::: configure.ac @@ -49,3 @@ LIBVIRT_GCONFIG_MIN_VERSION=0.0.4 LIBXML2_MIN_VERSION=2.7.8 -SPICE_GTK_MIN_VERSION=0.7.98 Yes, since last week. ::: src/wizard.vala @@ +118,3 @@ + prepare_for_location (this.wizard_source.uri, true); + if (source != null) + message (source.source_type); indeed. removed. @@ -115,0 +114,11 @@ + } else { + var text = _("Please enter desktop or collection URI"); + var icon = "preferences-desktop-remote-desktop"; ... 8 more ... ok @@ -181,2 +202,2 @@ source = new CollectionSource (uri.server ?? uri_as_text, uri.scheme, uri_as_text); - + message (uri.scheme); leftover again
Created attachment 206942 [details] [review] wizard: update the URL page when entry is updated Update the URL page depending on the URI. We currently do simple guesses based on simple URI parsing, but later on, we could query the URI to gather informations etc..
Review of attachment 206942 [details] [review]: Ok, assuming you have tested it, ACK! ::: src/wizard.vala @@ -114,0 +113,17 @@ + + var text = _("Please enter desktop or collection URI"); + var icon = "preferences-desktop-remote-desktop"; ... 14 more ... nitpick: should be inside try block so we dont keep updating text/icon for no reason?
Review of attachment 206942 [details] [review]: ::: src/wizard.vala @@ -114,0 +113,17 @@ + wizard_source.update_url_page (_("Desktop Access"), text, icon); + var text = _("Please enter desktop or collection URI"); + var icon = "preferences-desktop-remote-desktop"; ... 14 more ... here we keep "desktop-remote" most of the time, only changing it to "network-workgroup" when we are sure it's a collection server. avoiding the text/icon update is a micro optimization, that could be handled later in the update_url_page()
Attachment 206942 [details] pushed as b2add0b - wizard: update the URL page when entry is updated