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