[virt-tools-list] [PATCH virt-viewer 2/3] app: Compute monitor mapping only in fullscreen

[Pavel Grunt]

Hi,

On Mon, 2016-02-15 at 08:29 +0100, Fabiano Fidêncio wrote:
> Pavel,
> 
> On Mon, Feb 15, 2016 at 8:18 AM, Pavel Grunt <pgrunt at redhat.com>
> wrote:
> > ---
> >  src/virt-viewer-app.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/src/virt-viewer-app.c b/src/virt-viewer-app.c
> > index d4ce06a..19cc0a0 100644
> > --- a/src/virt-viewer-app.c
> > +++ b/src/virt-viewer-app.c
> > @@ -481,6 +481,10 @@ void
> > virt_viewer_app_set_uuid_string(VirtViewerApp *self, const gchar
> > *uuid_stri
> > 
> >      g_free(self->priv->uuid);
> >      self->priv->uuid = g_strdup(uuid_string);
> > +
> > +    if (!virt_viewer_app_get_fullscreen(self)) /* no need to get
> > mapping */
> > +        return;
> > +
> 
> A few lines below we have comments in the format:
>  // comment
>  // foobar
> 
> Let's try to keep it standardized.
sure, will change it
> 
> >      mapping =
> > virt_viewer_app_get_monitor_mapping_for_section(self, uuid_string);
> >      if (!mapping) {
> >          g_debug("No guest-specific fullscreen config, using
> > fallback");
> > --
> > 2.5.0
> > 
> > _______________________________________________
> > virt-tools-list mailing list
> > virt-tools-list at redhat.com
> > https://www.redhat.com/mailman/listinfo/virt-tools-list
> 
> On one hand I agree with the patch, but I'm still not sure if here is
> the right place.
> I do believe this set_uuid_string() is doing too much IMO. What do
> you think?

It is doing more than it says, also it is called even when the uuid
does not change (it can get uuid from libvirt, than from spice, than
notify is emitted on session creation... I will post updated patch.

Thanks,
Pavel

> 
> Anyways, patch is good.
> 
> Reviewed-by: Fabiano Fidêncio <fidencio at redhat.com>
>