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