[Spice-devel] [PATCH spice-gtk 2/4] main: send only pending monitor config changes

[Jonathon Jongsma]

This patch is both confusing and (I think) unwise. 

first of all, as it is, the -1 will cause problems. If somebody calls
update_display_timer() while the timer id is -1, it will try to call
g_source_remove(-1). g_source_remove() expects a guint, so -1 will
presumably be coerced into an unsigned integer and then almost certainly
print a warning about in invalid source ID.

But if that is fixed, the -1 is still quite confusing. presumably you're
using it as a sort of signal that we tried to send a display update but
weren't able to? I'm really not a fan of using special magic numeric
values as a flag. It's much better to simply introduce a boolean flag to
be explicit about what you mean that trying to shoehorn that
functionality into a different variable.

But even if those things were fixed, I still don't think that this is
the right approach. I think that it's basically guaranteed to cause
regressions -- probably in scenarios that we don't test regularly and
won't catch until much later. I think that either we should leave the
behavior as-is, OR we should completely remove the
automatic-display-update-on-vdagent-connect from spice-gtk and leave it
to the application to handle.


On Thu, 2015-04-02 at 23:25 +0200, Marc-André Lureau wrote:
> When agent is ready, do not send current monitor configuration
> immediately unless there are pending changes.
> ---
>  gtk/channel-main.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/gtk/channel-main.c b/gtk/channel-main.c
> index 3150208..c132ffa 100644
> --- a/gtk/channel-main.c
> +++ b/gtk/channel-main.c
> @@ -1290,8 +1290,11 @@ static gboolean timer_set_display(gpointer data)
>      SpiceSession *session;
>      gint i;
>  
> -    if (!c->agent_connected)
> +    c->display_timer_id = 0;
> +    if (!c->agent_connected) {
> +        c->display_timer_id = -1;
>          return FALSE;
> +    }
>  
>      session = spice_channel_get_session(SPICE_CHANNEL(channel));
>  
> @@ -1789,7 +1792,9 @@ static void main_agent_handle_msg(SpiceChannel *channel,
>          }
>          c->agent_caps_received = true;
>          g_coroutine_signal_emit(self, signals[SPICE_MAIN_AGENT_UPDATE], 0);
> -        update_display_timer(SPICE_MAIN_CHANNEL(channel), 0);
> +
> +        if (c->display_timer_id)
> +            update_display_timer(SPICE_MAIN_CHANNEL(channel), 0);
>  
>          if (caps->request)
>              agent_announce_caps(self);