[virt-tools-list] [PATCH virt-viewer] Monitor config at startup sometimes leaves additional monitors enabled

[Jonathon Jongsma]

On Thu, 2015-03-19 at 11:46 +0100, Christophe Fergeau wrote:
> Hi,
> 
> On Tue, Mar 17, 2015 at 02:50:58PM -0500, Jonathon Jongsma wrote:
> > When using the configuration file to specify which remote monitors
> > should be enabled when using the --full-screen option, it sometimes left
> > additional displays enabled, or didn't place the displays on the right
> > monitor, or didn't fullscreen them. This was especially true when not
> > enabling the first display on the remote host. For example:
> > 
> >   monitor-mapping=2:2;3:3
> > 
> > (note that configuration file uses 1-based indexes, rather than 0-based
> > indexes, so the numbers used below will be 1 less than those above)
> > 
> > There were several issues that contributed to this bug. The first is
> > that when performing fullscreen auto-conf, we were configuring displays
> > starting at #0 and ending at ndisplays. So for the previous
> > configuration, we looped from i = 0 to i < 2 (i.e. display #0 and #1)
> > even though we should have configured display #1 and #2.
> 
> 
> It looks like this fix...
> 
> > 
> > The other issue is that we were creating the first display window before
> > the loading the monitor mapping configuration from the settings file. So
> > even if the first display was disabled in the configuration, the first
> > window will still be created with an id of 0, and therefore didn't get
> > set to fullscreen. Moving the main window creation to the 'constructed'
> > vfunc instead of the object init func ensures that the configuration is
> > all loaded before we attempt to do any fullscreen autoconf.
> 
> ... and that one could easily be split in 2 distinct commits.

Yep, I had the same thought after I sent the patch.  Will split.

> 
> > 
> > I also took this opportunity to change the 'constructor' vfunc to a
> > 'constructed' vfunc, since we don't need the added complexity of
> > 'constructor'.
> 
> (that one too, but it's not that important)
> 
> > 
> > Resolves: rhbz#1200750
> > ---
> >  src/virt-viewer-app.c           | 62 +++++++++++++++++++++++++----------------
> >  src/virt-viewer-app.h           |  2 +-
> >  src/virt-viewer-session-spice.c | 13 +++++----
> >  3 files changed, 47 insertions(+), 30 deletions(-)
> > 
> > diff --git a/src/virt-viewer-app.c b/src/virt-viewer-app.c
> > index 8bf728f..c0c980f 100644
> > --- a/src/virt-viewer-app.c
> > +++ b/src/virt-viewer-app.c
> > @@ -301,12 +301,35 @@ virt_viewer_app_quit(VirtViewerApp *self)
> >      gtk_main_quit();
> >  }
> >  
> > -gint virt_viewer_app_get_n_initial_displays(VirtViewerApp* self)
> > +GList* virt_viewer_app_get_initial_displays(VirtViewerApp* self)
> >  {
> > -    if (self->priv->initial_display_map)
> > -        return g_hash_table_size(self->priv->initial_display_map);
> > +    if (!self->priv->initial_display_map) {
> > +        GList *l = NULL;
> > +        gint i, n = gdk_screen_get_n_monitors(gdk_screen_get_default());
> 
> I prefer to have each declaration on its own line, especially with the
> initialization. Since negative values for monitors are filtered out in
> virt_viewer_app_parse_monitor_mappings(), most of this patch could use
> guint instead of gint, but maybe this would not go well with existing
> code.

right, will split the declarations. 

> 
> 
> >  
> > -    return gdk_screen_get_n_monitors(gdk_screen_get_default());
> > +        for (i = 0; i < n; i++) {
> > +            l = g_list_append(l, GINT_TO_POINTER(i));
> > +        }
> > +        return l;
> > +    }
> > +    return g_hash_table_get_keys(self->priv->initial_display_map);
> > +}
> > +
> > +static gint virt_viewer_app_get_first_monitor(VirtViewerApp *self)
> > +{
> > +    g_print("%s: initial_display_map = %p\n", G_STRFUNC, self->priv->initial_display_map);
> 
> Should be g_debug or removed.

Oops, that was just a test thing that accidentally got left in.  There's
no good reason to keep that as a debug statement. Will rework this patch
and send a new version soon.

Jonathon