[Spice-devel] [PATCH spice-gtk] channel-main: Notify about existence of monitor

[Marc-André Lureau]

Hi Pavel

----- Original Message -----
> SpiceMainChannel should be notify about existence of monitors
> assigned to a SpiceDisplayChannel, even when the SpiceDisplayChannel
> is not connected to a SpiceDisplay widget. Otherwise the automatic
> resizing (the "resize-guest" property) of SpiceDisplay will not work
> when there is more display channels than SpiceDisplays.
> 
> Fixes:
> https://bugs.freedesktop.org/show_bug.cgi?id=90914
> ---
>  src/channel-main.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/src/channel-main.c b/src/channel-main.c
> index c55d097..fbc41da 100644
> --- a/src/channel-main.c
> +++ b/src/channel-main.c
> @@ -1580,18 +1580,36 @@ static void main_handle_mm_time(SpiceChannel
> *channel, SpiceMsgIn *in)
>      spice_session_set_mm_time(session, msg->time);
>  }
>  
> +static void main_enable_monitor(SpiceChannel *channel, GParamSpec *pspec
> G_GNUC_UNUSED,
> +                                SpiceMainChannel *main_channel)
> +{
> +    gint channel_id = spice_channel_get_channel_id(channel);
> +
> +    g_return_if_fail(SPICE_IS_DISPLAY_CHANNEL(channel));
> +    g_return_if_fail(channel_id < MAX_DISPLAY);
> +    g_return_if_fail(main_channel != NULL && main_channel->priv != NULL);
> +
> +    main_channel->priv->display[channel_id].enabled_set = TRUE;
> +}
> +

- "enabled_set" is a boolean used to indicate that the display "enabled" is set... To make this more clear, I think it should be changed for an enum instead: UNDEFINED, DISABLED, ENABLED.
- "enabled_set" is set, but what about the rest of the monitor config (enabled/x/y/w/h)
- this may overwrite user monitor config, and may be annoying in some racy situation: user start with a 2 monitors config, connects to a 1 monitor enabled guest, and reconfigure/resizes while the guest second monitor is not yet enabled: it will flip config back to disable 2nd monitor. Although this "race" already exists today with spice-gtk, I think it's worth mentioning again since it introduces again the same issue 
- the nth display is not necessarily the nth "monitor", channel 0 can have multiple monitors

>  typedef struct channel_new {
>      SpiceSession *session;
>      int type;
>      int id;
> +    SpiceMainChannel *main_channel;
>  } channel_new_t;
>  
>  /* main context */
>  static gboolean _channel_new(channel_new_t *c)
>  {
> +    SpiceChannel *channel;
>      g_return_val_if_fail(c != NULL, FALSE);
>  
> -    spice_channel_new(c->session, c->type, c->id);
> +    channel = spice_channel_new(c->session, c->type, c->id);
> +    if (SPICE_IS_DISPLAY_CHANNEL(channel)) {
> +        spice_g_signal_connect_object(channel, "notify::monitors",
> +                                      G_CALLBACK(main_enable_monitor),
> c->main_channel, 0);
> +    }
>  
>      g_object_unref(c->session);
>      g_free(c);
> @@ -1619,6 +1637,7 @@ static void main_handle_channels_list(SpiceChannel
> *channel, SpiceMsgIn *in)
>          c->session = g_object_ref(session);
>          c->type = msg->channels[i].type;
>          c->id = msg->channels[i].id;
> +        c->main_channel = SPICE_MAIN_CHANNEL(channel);
>          /* no need to explicitely switch to main context, since
>             synchronous call is not needed. */
>          /* no need to track idle, session is refed */
> --
> 2.4.3
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>