Hans,
Thanks for such quick review! Few comments/questions below.
On Mon, Jul 29, 2013 at 10:26 AM, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> Hi,
>
>
> On 07/28/2013 10:43 PM, Fedor Lyakhov wrote:
>>
>> Hi,
>>
>> Bug62033 is a request by Gnome3 developers. They need means to detect
>> that spice client runs locally so they shouldn't disable any desktop
>> effects on the guest running Gnome3. On the other hand, for slow
>> remote connections, they want to disable the effects.
>>
>> My solution approaches the question from another side: allow user to
>> decide whether he needs the effects or not. Sure, he can configure the
>> guest manually as he wants, but it isn't very convenient.
>
>
> Many thanks for working on this!
>
>
>>
>> The user interface for this task already exists in Spice GTK client,
>> this is CLI option '--spice-disable-effects=<wallpaper, font-smooth,
>> animation, all>. Right now this options works for Windows guest only.
>> In next patch series I add following:
>> 0001. Unrelated small fix
>
>
> Added to my local repo and pushed to the official repo.
>
>
>> 0002. Announce capabilities of Display Config in spice-vdagentd system
>> daemon, handle Display Config message in spice-vdagentd - send it to
>> spice-vdagent session daemon
>
>
> Looks good.
>
>
>> 0003. Handle Display Config message in spice-vdagent - implementation
>> uses GSettings API from GIO 2.26+; at run-time: xsettings plugin for
>> gnome-settings-manager should be installed and enabled - otherwise
>> font smooth won't be affected.
>> Better implementation with g_settings_get_range() introspection to get
>> enum nicks requires GIO 2.28+ - I need approval on this.
>
>
> 2.28 is too new, 2.26 is the newest version we can require given
> the various distros we want to support. Note there is no need to do
> conditionally enable the code based on 2.26 being there and too support
> even older glib versions. 2.26 as a requirement is fine, newer is not.
Ok, I see.
>
>
>>
>> Please note patches 2, 3 are for review mostly, the feature isn't
>> complete yet: user doesn't have convenient means to restore the
>> effects once they're disabled. I see 2 potential solutions (better
>> ideas are welcome!):
>>
>> (i) Introduce '--spice-reset-effects' option.
>> We don't want to change protocol, and there is one possibility to
>> explore - send Display Config with empty flags to indicate the reset
>> is required.
>> Right now Display Config is sent regardles of --spice-reset-effects
>> and --spice-color-depth options - i.e. empty one is sent when the
>> options aren't set. I want to make this empty Display Config to be
>> sent only when --spice-reset-effects option is enabled (need to check
>> whether this is acceptable at Windows agent). I've added handling code
>> of this case to vdagent: reset all the settings we change to default
>> (it is commented out for now). There are following limitations I know
>> about:
>> 1. Inability to set color depth and reset effects in one connection
>> (not that bad - reset can be done once, then re-connect with desired
>> color depth; also color depth change isn't supported on Linux guests
>> yet)
>> 2. If new flags are added to Display Config, all them need to be
>> treated the same way, i.e. reset should reset them all (there can be
>> only 1 reset message, flags=0).
>
>
> I would prefer to have a VD_AGENT_DISPLAY_CONFIG_FLAG_RE_ENABLE_ALL or
> some such rather then an empty message.
My idea was based on preserving current protocol - if it is OK to
introduce new message, I'm willing to do so.
>
>> (ii) Make vdagent stateful - remember that previous connection was
>> disabling some effects, and if current one isn't disabling these
>> effects, reset them. No need for '--spice-reset-effects' option in
>> this case. We can even remember previous states of effects and restore
>> them instead of resetting them to default. This seems to be the most
>> user-friendly approach, but it is harder to implement. If this
>> approach is selected, I see implementation via GSettings (afaik there
>> is no config for vdagent right now, correct?)
>
>
> I would not go here, --spice-disable-effects is mostly used by the
> ovirt user-portal which does this without the user knowing, so the user
> is not really aware this is happening, what if the user changes some
> settings after the agent has made its changes, then the client disconnecting
> will restore the wrong settings?
Ok, I'll go with (i).
I don't have experience with ovirt user-portal - need to look into it
probably. One concern: I find --spice-disable-effects=font-smooth
really annoying - font quality is extremely degraded (even without
setting hinting to none)... As a user I wouldn't like this to happen
behind the scenes without my opinion asked - guess I need to check
ovirt workflow here. Is --spice-disable-effects usage in ovirt
configurable by e.g. administrator?
Just to elaborate on (ii): you're right, wrong settings will be
restored - if we accept user has only one set of settings. But if we
consider user has 2 profiles - normal and disabled (with effects
disabled), it isn't so bad:
On connect with disabled effects:
If (!disabled) {
1. Stash current effect settings to normal_profile
2. Apply stashed disabled_profile (if nothing is stored, disable
effects by default)
3. Set disabled=true
}
On normal connect:
If (disabled) {
1. Stash current effect settings to disabled_profile
2. Apply stashed normal_profile (if nothing is stored, reset effect settings)
3. Set disabled=false
}
This way we'd loose imperative meaning of --spice-disable-effects
option though - this idea with profiles maybe makes sense, but for
different CLI option...
We can also stash only normal_profile, always enforcing disabling
effects when requested - but in this case we're back to where we've
started from.
>
> I also wonder how the windows agent handles this, does it maybe see the
> lack of a VD_AGENT_DISPLAY_CONFIG_FLAG_DISABLE_* flag as indication to
> enable things ?
>
> <snip>
I'll figure this out. But don't think this is the right way - it will
certainly break current behavior on Linux, making users, who have
their effects already configured, unhappy... Another complication is
we cannot simply 'enable' some effects because they have different
levels/choices...
>
>
>> Note1: Fedora 19 with Spice graphics at Host-1 crashes at startup,
>> haven't tried at Host-2.
>
> Sounds like a known issue, have you installed all updates for Fedora-19 ?
I'll try once more with fully updated Fedora 19.
>
>
>> Note2: I've got 100% reproducible crash at Host-1 Guest1.1: qemu
>> crashes in following scenario1:
>> 1. Start VM, connect with Spice client, open System
>> settings->Background, select new background [or just open
>> dconf-editor], close it
>> 2. Re-connect client
>> 3. Open System settings->Background->select background [or open
>> dconf-editor] -> VM crashes
>> Scenario2:
>> 1. Start VM, connect with Spice client, open System
>> settings->Background->select new background (do not close the window)
>> 2. Re-connect client -> VM crashes
>> It seems to be unrelated to my changes though, because it is
>> reproducible even without vdagentd/vdagent running. Does anybody know
>> whether it is
>> already reported?
>
> This sounds like a new bug, can you file a bug for this please?
>
OK, sure. What do I need to do to collect enough debug information
about the crash? I've never debugged qemu/spice server...
-Fedor
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel