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

[Jonathon Jongsma]

----- Original Message -----
> From: "Marc-André Lureau" <mlureau at redhat.com>
> To: "Jonathon Jongsma" <jjongsma at redhat.com>
> Cc: virt-tools-list at redhat.com
> Sent: Tuesday, November 5, 2013 12:17:00 PM
> Subject: Re: [virt-tools-list] [PATCH] Do all display alignment in virt-viewer
> 
> 

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


Admittedly, it's a bit easy to overlook because it's separated from the vfunc call by a block of code, but there is an early return near the beginning of the function to avoid doing all of the alignment if there is no vfunc registered by the subclass. (although now I notice that we leak in that case since I'm allocating the monitors array before that point...  I'll fix that).


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

OK, I'll update the session vfunc to apply_monitor_geometry() so that it doesn't get confused with the display::monitor-geometry-changed signal.

Jonathon