GNOME Bugzilla – Bug 695207
USB properties improvements/bug fixes
Last modified: 2016-03-31 13:22:07 UTC
Currently, when an USB device is plugged/unplugged, if the user is looking at the devices property page of a box, the list of devices does not automatically add/remove the new device. The IPropertiesProvider interface does not provide an easy way to dynamically add/remove properties, however given one property, it's possible to trigger a refresh of the whole property page by emitting the Property::refresh-properties signal. This commit uses the 'automatic USB redirection' property to trigger a page refresh as it will always be present when USB redirection is supported.
Created attachment 238123 [details] [review] Dynamically refresh USB devices properties
Created attachment 238124 [details] [review] Order the list of USB devices When USB devices are unplugged and replugged, they won't always reappear in the same place, which can be disturbing. This commit alphabetically sort that list so that a given device always reappear at the same place. The sort could be made more sophisticated in the future if we want to group the devices by type, this kind of things.
Created attachment 238132 [details] [review] Use ref for IPropertyProvider::get_properties 'flag' arg It seems this is what LibvirtMachineProperties::get_properties() as it is modifying its 'flag' argument to indicate USB support is missing. SpiceDisplay::get_properties() is then checking for the presence of this flag, and hides the list of USB devices if the box does not have USB redirection support. However, without using 'ref' on the get_properties 'flag' argument, the change made in LibvirtMachineProperties won't get propagated to the caller (this is LibvirtMachine::get_properties), and then SpiceDisplay::get_properties will get unmodified flags and won't know it should disable USB support.
Retitling this bug as it was initially only about the first patch, but then I added to unrelated patches which are also USB-related.
Review of attachment 238123 [details] [review]: ::: src/spice-display.vala @@ +328,3 @@ + }); + manager.device_removed.connect ( (manager, dev) => { + Idle.add (() => { I've already added the missing space before the second '('
Review of attachment 238123 [details] [review]: ::: src/spice-display.vala @@ +328,3 @@ + }); + manager.device_removed.connect ( (manager, dev) => { + Idle.add (() => { On second thought, I did not change anything as there are more uses of (() => than ( () =>
Review of attachment 238123 [details] [review]: Looks good otherwise. ::: src/spice-display.vala @@ +328,3 @@ + }); + manager.device_removed.connect ( (manager, dev) => { + Idle.add (() => { Actually, "( (" isn't correct according to coding-style. Space is only between keywork/function name and "(". @@ +329,3 @@ + manager.device_removed.connect ( (manager, dev) => { + Idle.add (() => { + // This is done in an idle to workaround a bug in spice-gtk 0.18 where calling Bug reference please. Also "FIXME: " prefix would be nice to get attention of future developers.
Review of attachment 238124 [details] [review]: Looks good, ACK.
Review of attachment 238132 [details] [review]: In the log: "It seems this is what LibvirtMachineProperties::get_properties()" -> "It seems this is what LibvirtMachineProperties::get_properties() supposed to do" ? Good otherwise.
Attachment 238123 [details] pushed as d4a4400 - Dynamically refresh USB devices properties Attachment 238124 [details] pushed as c9c7d3c - Order the list of USB devices Attachment 238132 [details] pushed as 28148f6 - Use ref for IPropertyProvider::get_properties 'flag' arg