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

[Jonathon Jongsma]

On Fri, 2015-04-03 at 11:25 -0400, Marc-André Lureau wrote:
> 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.

Yes, it removes unwanted monitor config messages, but it may also remove
*wanted* monitor config messages. I'm not sure that I can be a lot more
clear about possible regressions because that's the nature of
regressions -- you don't know about them until they happen. But I do
know that this general area is one that seems to be very tricky to get
right. 

Oh, I just thought of one potential example (note that I haven't
actually tested it with your patches):

- User connects a fullscreen (1600x900) virt-viewer to a guest
- user reboots guest
- during reboot, guest switches to e.g. 720x400 (which results in a
scaled display widget in the fullscreen window)
- There is no vdagent running during boot
- The graphical login screen (gdm) comes up and vdagent connects
- the gdm greeter will default to e.g. 1024x768
- There are no pending monitor changes, so your patch prevents us from
sending a monitors config message to the server

Result: the gdm greeter is a 1024x768 widget scaled up to a fullscreen
1600x900 window. In other words, it is zoomed in, "fuzzy", and with
black bars along the sides.

Previously, we would have unconditionally sent a monitors config update
to the server when the vdagent became connected, which would have
resulted in the gdm screen being configured to the fullscreen window
size.

This is just one example off the top of my head, but I'm sure there are
several other scenarios that I can't think of immediately (VT switching?
vdagent restarting?).

As I said in my previous email, I think that if we change this behavior,
it would probably be better to remove it completely and leave it up to
the application to handle.

Jonathon