[Spice-devel] [PATCH v2 2/2] Don't send monitors config when Display widget is created

[Pavel Grunt]

Hi Jonathon,
looks good, just one question below.

On Thu, 2015-08-06 at 15:22 -0500, Jonathon Jongsma wrote:
> When a display channel is associated with a particular SpiceDisplay
> widget, it previously set the display to 'enabled' unconditionally.
> There is a couple of problems with this behavior.
> 
> First, simply because a display widget has an associated display
> channel, it doesn't necessarily mean that the display is enabled. On
> linux guests, for instance, a display channel can have up to 4 displays
> for one channel, and perhaps only one of them is enabled. So, we
> shouldn't set the display to 'enabled' until we actually receive a
> monitors configuration message indicating that this display is enabled.
> 
> The second problem is that this was triggering the client to send down a
> new monitors-config message to the server. This message is completely
> unnecessary since it is triggered by a message from the server. We
> should only be sending down new monitor configurations in response to
> changes from the client, not from the server.
> ---
>  src/spice-widget.c | 38 ++++++++++++++++++++++++--------------
>  1 file changed, 24 insertions(+), 14 deletions(-)
> 
> diff --git a/src/spice-widget.c b/src/spice-widget.c
> index 3ec2e65..1268d78 100644
> --- a/src/spice-widget.c
> +++ b/src/spice-widget.c
> @@ -219,6 +219,19 @@ static void update_keyboard_focus(SpiceDisplay *display, 
> gboolean state)
>      spice_gtk_session_request_auto_usbredir(d->gtk_session, state);
>  }
>  
> +static gint get_display_id(SpiceDisplay *display)
> +{
> +    SpiceDisplayPrivate *d = display->priv;
> +
> +    /* supported monitor_id only with display channel #0 */
> +    if (d->channel_id == 0 && d->monitor_id >= 0)
> +        return d->monitor_id;
> +
> +    g_return_val_if_fail(d->monitor_id <= 0, -1);
> +
> +    return d->channel_id;
> +}
> +
>  static void update_ready(SpiceDisplay *display)
>  {
>      SpiceDisplayPrivate *d = display->priv;
> @@ -226,6 +239,16 @@ static void update_ready(SpiceDisplay *display)
>  
>      ready = d->mark != 0 && d->monitor_ready;
>  
> +    /* If the 'resize-guest' property is set, the application expects spice
> -gtk
> +     * to manage the size and state of the displays, so update the 'enabled'
> +     * state here. If 'resize-guest' is false, we can assume that the
> +     * application will manage the state of the displays.
> +     */
> +    if (d->resize_guest_enable) {
> +        spice_main_update_display_enabled(d->main, get_display_id(display),
> +                                          ready, FALSE);
Shouldn't 'update' be TRUE if the app expects spice-gtk to handle the state of
the display?

Thanks,
Pavel
> +    }
> +
>      if (d->ready == ready)
>          return;
>  
> @@ -244,19 +267,6 @@ static void set_monitor_ready(SpiceDisplay *self, 
> gboolean ready)
>      update_ready(self);
>  }
>  
> -static gint get_display_id(SpiceDisplay *display)
> -{
> -    SpiceDisplayPrivate *d = display->priv;
> -
> -    /* supported monitor_id only with display channel #0 */
> -    if (d->channel_id == 0 && d->monitor_id >= 0)
> -        return d->monitor_id;
> -
> -    g_return_val_if_fail(d->monitor_id <= 0, -1);
> -
> -    return d->channel_id;
> -}
> -
>  static void update_monitor_area(SpiceDisplay *display)
>  {
>      SpiceDisplayPrivate *d = display->priv;
> @@ -344,6 +354,7 @@ static void spice_display_set_property(GObject     
>  *object,
>          break;
>      case PROP_RESIZE_GUEST:
>          d->resize_guest_enable = g_value_get_boolean(value);
> +        update_ready(display);
>          update_size_request(display);
>          break;
>      case PROP_SCALING:
> @@ -2439,7 +2450,6 @@ static void channel_new(SpiceSession *s, SpiceChannel 
> *channel, gpointer data)
>              mark(display, primary.marked);
>          }
>          spice_channel_connect(channel);
> -        spice_main_set_display_enabled(d->main, get_display_id(display), 
> TRUE);
>          return;
>      }
>