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

[Marc-André Lureau]

Hi

----- Original Message -----
> 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.

I agree, I will introduce another 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.

Please be more clear about the possible regressions. This patch
helps removing unwanted monitor config messages, so it's worth it.
If a regression is found, this is still a good patch and I
would rather solve further issues than working around them.