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

[Marc-André Lureau]

On Thu, Oct 31, 2013 at 11:28 PM, Jonathon Jongsma <jjongsma at redhat.com> wrote:
> +static int displays_cmp(const void *p1, const void *p2, gpointer user_data)
> +{
> +    GHashTable *displays = user_data;
> +    guint i = *(guint*)p1;
> +    guint j = *(guint*)p2;
> +    GdkRectangle *m1 = g_hash_table_lookup(displays, GINT_TO_POINTER(i));
> +    GdkRectangle *m2 = g_hash_table_lookup(displays, GINT_TO_POINTER(j));
> +    double d1 = sqrt(m1->x * m1->x + m1->y * m1->y);
> +    double d2 = sqrt(m2->x * m2->x + m2->y * m2->y);
> +    int diff = d1 - d2;
> +
> +    return diff == 0 ? i - j : diff;
> +}

My gut feeling is that all this code is not clean enough. Probably if
moving most of the logic in session, that would help a lot. You could
also change the GList of displays there to a GArray, add a
GdkRectangle directly to VirtViewerDisplay. Then all the align could
be done there, and once over, update spice-session (via signals or
vtable).

At least I don't really get why you use a hash table here, and not
just an array (since monitors are indexed, and even if sparse, you
could easily detect that, 0x0 == disabled in spice)

> +static void align_displays(GHashTable *displays)
> +{
> +    gint i, j, x = 0;
> +    guint *sorted_displays;
> +    guint ndisplays = 0;
> +
> +    g_return_if_fail(displays != NULL);
> +
> +    ndisplays = g_hash_table_size(displays);
> +
> +    if (ndisplays == 0)
> +        return;
> +
> +    /* sort by distance from origin */
> +    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);
> +
> +    for (i = 0; i < ndisplays; i++) {
> +        j = sorted_displays[i];
> +        g_assert(j < ndisplays);
> +        GdkRectangle *rect = g_hash_table_lookup(displays, GINT_TO_POINTER(j));
> +        rect->x = x;
> +        rect->y = 0;
> +        x += rect->width;
> +        if (rect->width || rect->height)
> +            DEBUG_LOG("%s: monitor config: #%d %dx%d+%d+%d", G_STRFUNC,
> +                      j, rect->width, rect->height, rect->x, rect->y);
> +    }
> +    g_free(sorted_displays);
> +}
> +
> +void
> +virt_viewer_session_spice_update_displays(VirtViewerSessionSpice *self)
> +{
> +    gboolean all_fullscreen = TRUE;
> +    GHashTable *display_rects = g_hash_table_new_full(g_direct_hash, g_direct_equal, NULL, g_free);
> +    GHashTableIter iter;
> +    gpointer key, value;
> +    GList *displays = virt_viewer_session_get_displays(VIRT_VIEWER_SESSION(self));
> +    for (GList *l = displays; l; l = l->next) {
> +        GdkRectangle *rect = g_new0(GdkRectangle, 1);
> +        VirtViewerDisplay *d = VIRT_VIEWER_DISPLAY(l->data);
> +        gint nth;
> +        gboolean enabled = virt_viewer_display_get_enabled(d);
> +
> +        g_object_get(d, "nth-display", &nth, NULL);
> +
> +        spice_main_set_display_enabled(self->priv->main_channel, nth, enabled);
> +        if (enabled && !virt_viewer_display_get_fullscreen(d))
> +            all_fullscreen = FALSE;
> +
> +        virt_viewer_display_spice_get_requested_size(VIRT_VIEWER_DISPLAY_SPICE(d), rect);
> +        DEBUG_LOG("%s: Got requested size for display %d: (%dx%d) @ (%d,%d)",
> +                  G_STRFUNC, nth, rect->width, rect->height, rect->x, rect->y);
> +        g_hash_table_insert(display_rects, GINT_TO_POINTER(nth), rect);
> +    }
> +
> +    if (!all_fullscreen)
> +        align_displays(display_rects);
> +
> +    g_hash_table_iter_init(&iter, display_rects);
> +    while (g_hash_table_iter_next(&iter, &key, &value)) {
> +        gint nth = GPOINTER_TO_INT(key);
> +        GdkRectangle* rect = (GdkRectangle*) value;
> +        spice_main_set_display(self->priv->main_channel, nth, rect->x,
> +                               rect->y, rect->width, rect->height);
> +    }
> +    g_hash_table_unref(display_rects);
> +}
> +




-- 
Marc-André Lureau