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

[Christophe Fergeau]

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.

> 
> 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.


>  
> -    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.

Apart from these details, the patch makes a lot of sense.

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20150319/89243c3b/attachment.sig>