[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [virt-tools-list] [PATCH] Do all display alignment in virt-viewer




----- Original Message -----
> Don't rely on spice-gtk to do any alignment of displays.  This patch sets the
> disable-display-align property on the SpiceMainChannel, and makes the
> VirtViewerSession in charge of doing all alignment. This means that every
> display has to tell the VirtViewerSession when its "virtual monitor" has
> changed
> configuration (and wants to reconfigure its display on the guest), rather
> than
> sending it directly to the Main Channel.  The session will then align the
> displays (if necessary), and the spice session will send down new
> configuration
> for all displays at once. This solves a couple of problems:
> 
> 1. It allows the session to send down absolute coordinates only in the case
>    where *all* windows are fullscreen (so that we can still support
>    vertically-stacked displays, etc).  But it auto-aligns displays if only a
>    subset of the displays are in fullscreen mode. This solves the problem of
>    overlapping regions on different displays when one monitor is in
>    fullscreen
>    because only one monitor's configuration was updated and the others were
>    not
>    aligned.
> 2. Allows us to always align based on the current position of each display.
> This
>    contrasts with the earlier behavior where the position used for alignment
>    was
>    the window's position at the time when it was last resized. This caused
>    displays to be arranged in a seemingly non-deterministic manner if one
>    window
>    was moved and then another window was resized (causing a display
>    re-configuration).
> 
> Solves rhbz#1002156
> ---
> 
> This patch is an attempt to address Marc-Andre's concerns with the previous
> patch.  Most of the alignment logic is moved down to the Session base class.
> It
> uses an array instead of a GHashTable.
> 
> It still does some extra at display-reconfiguration, but this seems
> preferable
> to me to pushing the aligned rects into e.g. VirtViewerDisplay, for a couple
> of
> reasons:
> - the aligned rects are only used by the session to send down new geometry to
>   the spice server. They have no significance to the display object. The
>   display
>   object will get updated with its proper geometry when the server sends back
>   a
>   monitors configuration message (after the monitors have been successfully
>   configured).
> - display configuration is not something that happens particularly often, nor
> is
>   this a performance-critical path, so a bit of extra allocation doesn't hurt
>   anything, and it keeps all of the data encapsulated in one place.
> 
>  src/virt-viewer-display-spice.c | 90
>  +++++++++--------------------------------
>  src/virt-viewer-display.c       | 72 +++++++++++++++++++++++++++++++++
>  src/virt-viewer-display.h       |  2 +
>  src/virt-viewer-session-spice.c | 26 +++++++++---
>  src/virt-viewer-session.c       | 89
>  ++++++++++++++++++++++++++++++++++++++++
>  src/virt-viewer-session.h       |  1 +
>  6 files changed, 203 insertions(+), 77 deletions(-)
> 
> diff --git a/src/virt-viewer-display-spice.c
> b/src/virt-viewer-display-spice.c
> index 54c1672..fd85e29 100644
> --- a/src/virt-viewer-display-spice.c
> +++ b/src/virt-viewer-display-spice.c
> @@ -95,22 +95,31 @@ get_main(VirtViewerDisplay *self)
>  }
>  
>  static void
> +virt_viewer_display_spice_monitor_geometry_changed(VirtViewerDisplaySpice
> *self)
> +{
> +
> +    if (virt_viewer_display_get_auto_resize(VIRT_VIEWER_DISPLAY(self)) ==
> FALSE)
> +        return;
> +
> +    g_signal_emit_by_name(self, "monitor-geometry-changed", NULL);
> +
> +}
> +
> +static void
>  show_hint_changed(VirtViewerDisplay *self)
>  {
>      SpiceMainChannel *main_channel = get_main(self);
> -    guint enabled = TRUE;
> -    guint nth, hint = virt_viewer_display_get_show_hint(self);
> +    guint enabled = virt_viewer_display_get_enabled(self);
> +    guint nth;
>  
>      /* this may happen when finalizing */
>      if (!main_channel)
>          return;
>  
>      g_object_get(self, "nth-display", &nth, NULL);
> -    if (!(hint & VIRT_VIEWER_DISPLAY_SHOW_HINT_SET) ||
> -        hint & VIRT_VIEWER_DISPLAY_SHOW_HINT_DISABLED)
> -        enabled = FALSE;
> -
>      spice_main_set_display_enabled(main_channel, nth, enabled);
> +
> +
> virt_viewer_display_spice_monitor_geometry_changed(VIRT_VIEWER_DISPLAY_SPICE(self));
>  }
>  
>  static void
> @@ -182,70 +191,12 @@ virt_viewer_display_spice_mouse_grab(SpiceDisplay
> *display G_GNUC_UNUSED,
>  
>  
>  static void
> -virt_viewer_display_spice_resize(VirtViewerDisplaySpice *self,
> -                                 GtkAllocation *allocation,
> -                                 gboolean resize_guest)
> -{
> -    gdouble dw = allocation->width, dh = allocation->height;
> -    guint zoom = 100;
> -    guint nth;
> -    gint x = 0, y = 0;
> -    gboolean disable_display_position = TRUE;
> -
> -    if (virt_viewer_display_get_auto_resize(VIRT_VIEWER_DISPLAY(self)) ==
> FALSE)
> -        return;
> -
> -    if (virt_viewer_display_get_show_hint(VIRT_VIEWER_DISPLAY(self)) &
> VIRT_VIEWER_DISPLAY_SHOW_HINT_DISABLED)
> -        return;
> -
> -    if (self->priv->auto_resize == AUTO_RESIZE_FULLSCREEN) {
> -        GdkRectangle monitor;
> -        GdkScreen *screen = gtk_widget_get_screen(GTK_WIDGET(self));
> -        int n = virt_viewer_display_get_monitor(VIRT_VIEWER_DISPLAY(self));
> -        if (n == -1)
> -            n = gdk_screen_get_monitor_at_window(screen,
> -
> gtk_widget_get_window(GTK_WIDGET(self)));
> -        gdk_screen_get_monitor_geometry(screen, n, &monitor);
> -        disable_display_position = FALSE;
> -        x = monitor.x;
> -        y = monitor.y;
> -        dw = monitor.width;
> -        dh = monitor.height;
> -    } else {
> -        GtkWidget *top = gtk_widget_get_toplevel(GTK_WIDGET(self));
> -        gtk_window_get_position(GTK_WINDOW(top), &x, &y);
> -        if (x < 0)
> -            x = 0;
> -        if (y < 0)
> -            y = 0;
> -    }
> -
> -    if (virt_viewer_display_get_zoom(VIRT_VIEWER_DISPLAY(self))) {
> -        zoom =
> virt_viewer_display_get_zoom_level(VIRT_VIEWER_DISPLAY(self));
> -
> -        dw = round(dw * 100 / zoom);
> -        dh = round(dh * 100 / zoom);
> -    }
> -
> -    g_object_get(self, "nth-display", &nth, NULL);
> -
> -    if (resize_guest) {
> -        g_object_set(get_main(VIRT_VIEWER_DISPLAY(self)),
> -                     "disable-display-position", disable_display_position,
> -                     "disable-display-align", !disable_display_position,
> -                     NULL);
> -        spice_main_set_display(get_main(VIRT_VIEWER_DISPLAY(self)),
> -                               nth, x, y, dw, dh);
> -    }
> -}
> -
> -static void
>  virt_viewer_display_spice_size_allocate(VirtViewerDisplaySpice *self,
> -                                        GtkAllocation *allocation,
> +                                        GtkAllocation *allocation
> G_GNUC_UNUSED,
>                                          gpointer data G_GNUC_UNUSED)
>  {
> -    virt_viewer_display_spice_resize(self, allocation,
> -                                     self->priv->auto_resize !=
> AUTO_RESIZE_NEVER);
> +    if (self->priv->auto_resize != AUTO_RESIZE_NEVER)
> +        virt_viewer_display_spice_monitor_geometry_changed(self);
>  
>      if (self->priv->auto_resize == AUTO_RESIZE_FULLSCREEN)
>          self->priv->auto_resize = AUTO_RESIZE_NEVER;
> @@ -256,13 +207,10 @@ zoom_level_changed(VirtViewerDisplaySpice *self,
>                     GParamSpec *pspec G_GNUC_UNUSED,
>                     VirtViewerApp *app G_GNUC_UNUSED)
>  {
> -    GtkAllocation allocation;
> -
>      if (self->priv->auto_resize != AUTO_RESIZE_NEVER)
>          return;
>  
> -    gtk_widget_get_allocation(GTK_WIDGET(self), &allocation);
> -    virt_viewer_display_spice_resize(self, &allocation, TRUE);
> +    virt_viewer_display_spice_monitor_geometry_changed(self);
>  }
>  
>  static void
> diff --git a/src/virt-viewer-display.c b/src/virt-viewer-display.c
> index b6ef018..feefcca 100644
> --- a/src/virt-viewer-display.c
> +++ b/src/virt-viewer-display.c
> @@ -255,6 +255,16 @@ virt_viewer_display_class_init(VirtViewerDisplayClass
> *class)
>                   G_TYPE_NONE,
>                   0);
>  
> +    g_signal_new("monitor-geometry-changed",
> +                 G_OBJECT_CLASS_TYPE(object_class),
> +                 G_SIGNAL_RUN_LAST | G_SIGNAL_NO_HOOKS,
> +                 0,
> +                 NULL,
> +                 NULL,
> +                 g_cclosure_marshal_VOID__VOID,
> +                 G_TYPE_NONE,
> +                 0);
> +
>      g_type_class_add_private(class, sizeof(VirtViewerDisplayPrivate));
>  }
>  
> @@ -668,6 +678,12 @@ void virt_viewer_display_set_enabled(VirtViewerDisplay
> *self, gboolean enabled)
>      virt_viewer_display_set_show_hint(self,
>      VIRT_VIEWER_DISPLAY_SHOW_HINT_DISABLED, !enabled);
>  }
>  
> +gboolean virt_viewer_display_get_enabled(VirtViewerDisplay *self)
> +{
> +    return ((self->priv->show_hint & VIRT_VIEWER_DISPLAY_SHOW_HINT_SET) &&
> +        !(self->priv->show_hint & VIRT_VIEWER_DISPLAY_SHOW_HINT_DISABLED));
> +}
> +
>  VirtViewerSession* virt_viewer_display_get_session(VirtViewerDisplay *self)
>  {
>      g_return_val_if_fail(VIRT_VIEWER_IS_DISPLAY(self), NULL);
> @@ -759,6 +775,62 @@ gboolean
> virt_viewer_display_get_fullscreen(VirtViewerDisplay *self)
>      return self->priv->fullscreen;
>  }
>  
> +void virt_viewer_display_get_preferred_monitor_geometry(VirtViewerDisplay*
> self,
> +                                                        GdkRectangle*
> preferred)
> +{
> +    GtkWidget *top = NULL;
> +    gint topx = 0, topy = 0;
> +
> +    g_return_if_fail(preferred != NULL);
> +
> +    if (!virt_viewer_display_get_enabled(VIRT_VIEWER_DISPLAY(self))) {
> +        preferred->width = 0;
> +        preferred->height = 0;
> +        preferred->x = 0;
> +        preferred->y = 0;
> +        return;
> +    }
> +
> +    top = gtk_widget_get_toplevel(GTK_WIDGET(self));
> +    gtk_window_get_position(GTK_WINDOW(top), &topx, &topy);
> +    topx = MAX(topx, 0);
> +    topy = MAX(topy, 0);
> +
> +    if (virt_viewer_display_get_auto_resize(VIRT_VIEWER_DISPLAY(self)) ==
> FALSE) {
> +        guint w, h;
> +        virt_viewer_display_get_desktop_size(self, &w, &h);
> +        preferred->width = w;
> +        preferred->height = h;
> +        preferred->x = topx;
> +        preferred->y = topy;
> +    } else {
> +        if (virt_viewer_display_get_fullscreen(VIRT_VIEWER_DISPLAY(self))) {
> +            GdkRectangle physical_monitor;
> +            GdkScreen *screen = gtk_widget_get_screen(GTK_WIDGET(self));
> +            int n =
> virt_viewer_display_get_monitor(VIRT_VIEWER_DISPLAY(self));
> +            if (n == -1)
> +                n = gdk_screen_get_monitor_at_window(screen,
> +
> gtk_widget_get_window(GTK_WIDGET(self)));
> +            gdk_screen_get_monitor_geometry(screen, n, &physical_monitor);
> +            preferred->x = physical_monitor.x;
> +            preferred->y = physical_monitor.y;
> +            preferred->width = physical_monitor.width;
> +            preferred->height = physical_monitor.height;
> +        } else {
> +            gtk_widget_get_allocation(GTK_WIDGET(self), preferred);
> +            preferred->x = topx;
> +            preferred->y = topy;
> +        }
> +
> +        if (virt_viewer_display_get_zoom(VIRT_VIEWER_DISPLAY(self))) {
> +            guint zoom =
> virt_viewer_display_get_zoom_level(VIRT_VIEWER_DISPLAY(self));
> +
> +            preferred->width = round(preferred->width * 100 / zoom);
> +            preferred->height = round(preferred->height * 100 / zoom);
> +        }
> +    }
> +}
> +
>  /*
>   * Local variables:
>   *  c-indent-level: 4
> diff --git a/src/virt-viewer-display.h b/src/virt-viewer-display.h
> index 99844c4..195eeee 100644
> --- a/src/virt-viewer-display.h
> +++ b/src/virt-viewer-display.h
> @@ -124,8 +124,10 @@ void
> virt_viewer_display_release_cursor(VirtViewerDisplay *display);
>  
>  void virt_viewer_display_close(VirtViewerDisplay *display);
>  void virt_viewer_display_set_enabled(VirtViewerDisplay *display, gboolean
>  enabled);
> +gboolean virt_viewer_display_get_enabled(VirtViewerDisplay *display);
>  gboolean virt_viewer_display_get_selectable(VirtViewerDisplay *display);
>  void virt_viewer_display_queue_resize(VirtViewerDisplay *display);
> +void virt_viewer_display_get_preferred_monitor_geometry(VirtViewerDisplay
> *self, GdkRectangle* preferred);
>  
>  G_END_DECLS
>  
> diff --git a/src/virt-viewer-session-spice.c
> b/src/virt-viewer-session-spice.c
> index b42d48e..21a0b38 100644
> --- a/src/virt-viewer-session-spice.c
> +++ b/src/virt-viewer-session-spice.c
> @@ -80,6 +80,7 @@ static void
> virt_viewer_session_spice_channel_destroy(SpiceSession *s,
>  static void virt_viewer_session_spice_smartcard_insert(VirtViewerSession
>  *session);
>  static void virt_viewer_session_spice_smartcard_remove(VirtViewerSession
>  *session);
>  static gboolean
>  virt_viewer_session_spice_fullscreen_auto_conf(VirtViewerSessionSpice
>  *self);
> +static void
> virt_viewer_session_spice_monitor_geometry_changed(VirtViewerSession *self,
> GdkRectangle *monitors, guint nmonitors);
>  
>  static void
>  virt_viewer_session_spice_get_property(GObject *object, guint property_id,
> @@ -155,6 +156,7 @@
> virt_viewer_session_spice_class_init(VirtViewerSessionSpiceClass *klass)
>      dclass->smartcard_insert = virt_viewer_session_spice_smartcard_insert;
>      dclass->smartcard_remove = virt_viewer_session_spice_smartcard_remove;
>      dclass->mime_type = virt_viewer_session_spice_mime_type;
> +    dclass->monitor_geometry_changed =
> virt_viewer_session_spice_monitor_geometry_changed;
>  
>      g_type_class_add_private(klass, sizeof(VirtViewerSessionSpicePrivate));
>  
> @@ -675,6 +677,10 @@ virt_viewer_session_spice_channel_new(SpiceSession *s,
>          g_signal_connect(channel, "channel-event",
>                           G_CALLBACK(virt_viewer_session_spice_main_channel_event),
>                           self);
>          self->priv->main_channel = SPICE_MAIN_CHANNEL(channel);
> +        g_object_set(G_OBJECT(channel),
> +                     "disable-display-position", FALSE,
> +                     "disable-display-align", TRUE,
> +                     NULL);
>  
>          g_signal_connect(channel, "notify::agent-connected",
>          G_CALLBACK(agent_connected_changed), self);
>          virt_viewer_session_spice_fullscreen_auto_conf(self);
> @@ -742,12 +748,6 @@
> virt_viewer_session_spice_fullscreen_auto_conf(VirtViewerSessionSpice *self)
>          return FALSE;
>      }
>  
> -    DEBUG_LOG("Performing full screen auto-conf, %d host monitors",
> -              gdk_screen_get_n_monitors(screen));
> -    g_object_set(G_OBJECT(cmain),
> -                 "disable-display-position", FALSE,
> -                 "disable-display-align", TRUE,
> -                 NULL);
>      spice_main_set_display_enabled(cmain, -1, FALSE);
>      for (i = 0; i < gdk_screen_get_n_monitors(screen); i++) {
>          gdk_screen_get_monitor_geometry(screen, i, &dest);
> @@ -845,6 +845,20 @@
> virt_viewer_session_spice_smartcard_remove(VirtViewerSession *session
> G_GNUC_UNU
>      spice_smartcard_manager_remove_card(spice_smartcard_manager_get());
>  }
>  
> +void
> +virt_viewer_session_spice_monitor_geometry_changed(VirtViewerSession
> *session, GdkRectangle *monitors, guint nmonitors)
> +{
> +    guint i;
> +    VirtViewerSessionSpice *self = VIRT_VIEWER_SESSION_SPICE(session);
> +
> +    for (i = 0; i < nmonitors; i++) {
> +        GdkRectangle* rect = &monitors[i];
> +
> +        spice_main_set_display(self->priv->main_channel, i, rect->x,
> +                               rect->y, rect->width, rect->height);
> +    }
> +}
> +
>  /*
>   * Local variables:
>   *  c-indent-level: 4
> diff --git a/src/virt-viewer-session.c b/src/virt-viewer-session.c
> index 595dcd0..e82ed5d 100644
> --- a/src/virt-viewer-session.c
> +++ b/src/virt-viewer-session.c
> @@ -25,6 +25,7 @@
>  #include <config.h>
>  
>  #include <locale.h>
> +#include <math.h>
>  
>  #include "virt-viewer-session.h"
>  #include "virt-viewer-util.h"
> @@ -337,6 +338,90 @@ virt_viewer_session_init(VirtViewerSession *session)
>      session->priv = VIRT_VIEWER_SESSION_GET_PRIVATE(session);
>  }
>  
> +/* simple sorting of monitors. Primary sort left-to-right, secondary sort
> from
> + * top-to-bottom, finally by monitor id */
> +static int
> +displays_cmp(const void *p1, const void *p2, gpointer user_data)
> +{
> +    guint diff;
> +    GdkRectangle *displays = user_data;
> +    guint i = *(guint*)p1;
> +    guint j = *(guint*)p2;
> +    GdkRectangle *m1 = &displays[i];
> +    GdkRectangle *m2 = &displays[j];
> +    diff = m1->x - m2->x;
> +    if (diff == 0)
> +        diff = m1->y - m2->y;
> +    if (diff == 0)
> +        diff = i - j;
> +
> +    return diff;
> +}
> +
> +static void
> +virt_viewer_session_align_monitors_linear(GdkRectangle *displays, guint
> ndisplays)
> +{
> +    gint i, x = 0;
> +    guint *sorted_displays;
> +
> +    g_return_if_fail(displays != NULL);
> +
> +    if (ndisplays == 0)
> +        return;
> +
> +    sorted_displays = g_new0(guint, ndisplays);
> +    for (i = 0; i < ndisplays; i++)
> +        sorted_displays[i] = i;
> +    g_qsort_with_data(sorted_displays, ndisplays, sizeof(guint),
> displays_cmp, displays);
> +
> +    /* adjust monitor positions so that there's no gaps or overlap between
> +     * monitors */
> +    for (i = 0; i < ndisplays; i++) {
> +        guint nth = sorted_displays[i];
> +        g_assert(nth < ndisplays);
> +        GdkRectangle *rect = &displays[nth];
> +        rect->x = x;
> +        rect->y = 0;
> +        x += rect->width;
> +    }
> +    g_free(sorted_displays);
> +}
> +
> +static void
> +virt_viewer_session_on_monitor_geometry_changed(VirtViewerSession* self,
> +                                                VirtViewerDisplay* display
> G_GNUC_UNUSED)
> +{
> +    VirtViewerSessionClass *klass;
> +    gboolean all_fullscreen = TRUE;
> +    guint nmonitors = g_list_length(self->priv->displays);
> +    GdkRectangle *monitors = g_new0(GdkRectangle, nmonitors);
> +
> +    klass = VIRT_VIEWER_SESSION_GET_CLASS(self);
> +    if (!klass->monitor_geometry_changed)
> +        return;
> +
> +    for (GList *l = self->priv->displays; l; l = l->next) {
> +        VirtViewerDisplay *d = VIRT_VIEWER_DISPLAY(l->data);
> +        guint nth = 0;
> +        GdkRectangle *rect = NULL;
> +
> +        g_object_get(d, "nth-display", &nth, NULL);
> +        g_return_if_fail(nth < nmonitors);
> +        rect = &monitors[nth];
> +        virt_viewer_display_get_preferred_monitor_geometry(d, rect);
> +
> +        if (virt_viewer_display_get_enabled(d) &&
> +            !virt_viewer_display_get_fullscreen(d))
> +            all_fullscreen = FALSE;
> +    }
> +
> +    if (!all_fullscreen)
> +        virt_viewer_session_align_monitors_linear(monitors, nmonitors);
> +
> +    klass->monitor_geometry_changed(self, monitors, nmonitors);

I guess this code path shouldn't be reached by vnc backend. A g_return_if_fail(klass->monitor_geometry_changed) wouldn't hurt though.

Also to help showing different phases of monitor_geometry_changed, perhaps "apply_changes" or "commit_config", would help mixing the various callbacks and signals.

The rest looks good to me

> +    g_free(monitors);
> +}
> +
>  void virt_viewer_session_add_display(VirtViewerSession *session,
>                                       VirtViewerDisplay *display)
>  {
> @@ -346,6 +431,10 @@ void virt_viewer_session_add_display(VirtViewerSession
> *session,
>      session->priv->displays = g_list_append(session->priv->displays,
>      display);
>      g_object_ref(display);
>      g_signal_emit_by_name(session, "session-display-added", display);
> +
> +    virt_viewer_signal_connect_object(display, "monitor-geometry-changed",
> +
> G_CALLBACK(virt_viewer_session_on_monitor_geometry_changed),
> session,
> +                                      G_CONNECT_SWAPPED);
>  }
>  
>  
> diff --git a/src/virt-viewer-session.h b/src/virt-viewer-session.h
> index 0467724..f12cc1c 100644
> --- a/src/virt-viewer-session.h
> +++ b/src/virt-viewer-session.h
> @@ -94,6 +94,7 @@ struct _VirtViewerSessionClass {
>      void (*session_cut_text)(VirtViewerSession *session, const gchar *str);
>      void (*session_bell)(VirtViewerSession *session);
>      void (*session_cancelled)(VirtViewerSession *session);
> +    void (*monitor_geometry_changed)(VirtViewerSession *session,
> GdkRectangle* monitors, guint nmonitors);
>  };
>  
>  GType virt_viewer_session_get_type(void);
> --
> 1.8.3.1
> 
> _______________________________________________
> virt-tools-list mailing list
> virt-tools-list redhat com
> https://www.redhat.com/mailman/listinfo/virt-tools-list
> 


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]