GNOME Bugzilla – Bug 704012
Notify when a guest doesn't have the the spice agent installed
Last modified: 2016-09-20 08:16:03 UTC
It is easy to run a guest that isn't performing well and not realise that it needs to have the spice agent installed and running. It would be great if Boxes could detect this and inform the user that they need to install it. (It would be even better if there was a button that would automatically install the agent in the guest.) I'm sure I've seen something similar in other VM clients.
Not too sure about this. Its kinda internal detail and: 1. Most Linux installers setup agent anyway. 2. Users should do express installation unless they know what they are doing. Anyway, we'll need API from spice for this: https://bugs.freedesktop.org/show_bug.cgi?id=85089 and mockup would help too. :)
Turns out that spice-gtk already provides API for this. Now the question is how we should expose this information?
(In reply to Zeeshan Ali (Khattak) from comment #1) > Not too sure about this. Its kinda internal detail Just to be clear, just saying "no agent installed, Please install one" or something like that sounded like exposing internal details. If there was a way to do that ourselves after installation, would have been much easier. I'll ask some virt folks around if this is possible somehow.
How about in properties view, we show a property in General section: "Guest tools: Installed/Not installed <i>These tools enable a much smoother experience overall and enables host and box interactions, such as copy&paste. Please visit http:XYZ to download and install these tools from within the box.</i>"
Created attachment 328762 [details] [review] i-properties-provider: Enable markup for string property We need this because we currently have no way of formatting or adding links to a string property.
Created attachment 328763 [details] [review] spice-display: Add spice agent property Currently a user has no feedback regarding the existence of spice agent in the guest machine, which can result in a loss of performace and missing features. Add a propery that notifies the user if spice-agent is installed in guest and show a helpful message in case it is not installed.
Review of attachment 328762 [details] [review]: ACK
Review of attachment 328763 [details] [review]: This info being a property, is an implementation detail and mentioning that only gives the impression that you are adding a GObject/vala (as in class) property. I'd rather not mention that in the log. Pretty good effort with details section of the log. You're getting near to perfection slowly. :) Some nitpicks still though: * spice -> SPICE. * "Add a propery that notifies the user.." -> "Inform the user about missing guest agent and where to download and install it from, on the 'general' page of properties view". This is assuming we drop the "Guest installed" property. ::: src/spice-display.vala @@ +26,3 @@ private BoxConfig.SavedProperty[] gtk_session_saved_properties; private bool closed; + private bool spice_agent_installed; there is no need to keep around a data that is already kept around by main_channel. You can just access the main_channel.agent_connected directly where it's needed and remove this and 7 other lines you're adding after. @@ +333,3 @@ add_property (ref list, _("Share Clipboard"), toggle); + + string guest_tools = spice_agent_installed ? _("Installed") : _("Not installed"); use 'var' wherever possible. @@ +334,3 @@ + + string guest_tools = spice_agent_installed ? _("Installed") : _("Not installed"); + add_string_property (ref list, _("Guest Tools"), guest_tools); I don't think we need to inform user of this. We only need to notify them if agent is not installed. @@ +336,3 @@ + add_string_property (ref list, _("Guest Tools"), guest_tools); + + string message = _("<i>These tools enable a much smoother experience overall and enables host and box interactions, such as copy&paste.\nPlease visit <a href=\"http://www.spice-space.org/download.html\" title=\"Download spice agent\">spice-space</a> to download and install these tools from within the box.</i>"); * use 'var'. * declare and init in the if block below, where it's needed.
Created attachment 328985 [details] [review] spice-display: Inform user if SPICE agent not installed Currently a user has no feedback regarding the existence of SPICE agent in the guest machine, which can result in a loss of performace and missing features. Inform the user about missing guest agent and where to download and install it from, on the 'general' page of properties view.
Review of attachment 328985 [details] [review]: ::: src/spice-display.vala @@ +323,3 @@ + add_string_property (ref list, _("Guest Tools"), _("Not installed")); + var message = _("<i>These tools enable a much smoother experience overall and enables host and box interactions, such as copy&paste.\nPlease visit <a href=\"http://www.spice-space.org/download.html\" title=\"Download spice agent\">spice-space</a> to download and install these tools from within the box.</i>"); + add_string_property (ref list, "", message); * There is no need for a separate "Guest Tools: Status" if we'll only be showing it for one status type. Just change your string below. * You want to use an infobar (like "There is not enough space.." label in libvirt-machine-properties). Then you don't need to use markup (which should be avoided when possible in translated strings) for styling at least.
Created attachment 329089 [details] [review] spice-display: Inform user if SPICE agent not installed Currently a user has no feedback regarding the existence of SPICE agent in the guest machine, which can result in a loss of performace and missing features. Inform the user about missing guest agent and where to download and install it from, on the 'general' page of properties view.
Review of attachment 329089 [details] [review]: ::: src/spice-display.vala @@ +325,3 @@ add_property (ref list, _("Share Clipboard"), toggle); + + if (connected && !main_channel.agent_connected) { I prefer avoiding indenting blocks of code and doing so would be be consistent with DEVICES case below, so rather you do: if (!connected || main_channel.agent_connected) break; // Rest of the code @@ +336,3 @@ + content.add (image); + + var content = infobar.get_content_area (); with separate status label now removed, you need to first tell that guest tools are missing.
Created attachment 330387 [details] [review] spice-display: Inform user if SPICE agent not installed Currently a user has no feedback regarding the existence of SPICE agent in the guest machine, which can result in a loss of performace and missing features. Inform the user about missing guest agent and where to download and install it from, on the 'general' page of properties view.
Created attachment 330616 [details] [review] The infobar looked really ugly in this case, so i changed it to a label (on Allan's advise) and moved it below.