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
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
