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);
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel