Hi
On Fri, Dec 21, 2018 at 3:31 PM Victor Toso <victortoso@xxxxxxxxxx> wrote:
>
> Hi,
>
> On Wed, Sep 26, 2018 at 07:26:35PM +0400, marcandre.lureau@xxxxxxxxxx wrote:
> > From: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>
> >
> > virt_viewer_app_display_added() now handles VTE displays. They should
> > be skipped for monitor configuration, and they don't emit "show-hint".
> >
> > (a VTE display has a monitor nth == -1, which is now a valid value)
> >
> > The associated window will be hidden when virt-viewer is started.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx>
> > ---
> > src/virt-viewer-app.c | 69 ++++++++++++++++++++++++++++++++-----------
> > 1 file changed, 52 insertions(+), 17 deletions(-)
> >
> > diff --git a/src/virt-viewer-app.c b/src/virt-viewer-app.c
> > index 568af47..376fe98 100644
> > --- a/src/virt-viewer-app.c
> > +++ b/src/virt-viewer-app.c
> > @@ -112,7 +112,7 @@ struct _VirtViewerAppPrivate {
> > VirtViewerWindow *main_window;
> > GtkWidget *main_notebook;
> > GList *windows;
> > - GHashTable *displays;
> > + GHashTable *displays; /* !vte */
> > GHashTable *initial_display_map;
> > gchar *clipboard;
> > GtkWidget *preferences;
> > @@ -822,9 +822,11 @@ virt_viewer_app_window_new(VirtViewerApp *self, gint nth)
> > VirtViewerWindow* window;
> > GtkWindow *w;
> >
> > - window = virt_viewer_app_get_nth_window(self, nth);
> > - if (window)
> > - return window;
> > + if (nth >= 0) {
> > + window = virt_viewer_app_get_nth_window(self, nth);
> > + if (window)
> > + return window;
> > + }
>
> I think it is better to patch virt_viewer_app_get_nth_window() to
> return NULL if nth < 0 ...
ok
>
> > window = g_object_new(VIRT_VIEWER_TYPE_WINDOW, "app", self, NULL);
> > virt_viewer_window_set_kiosk(window, self->priv->kiosk);
> > @@ -848,11 +850,27 @@ virt_viewer_app_window_new(VirtViewerApp *self, gint nth)
> > return window;
> > }
> >
> > +static void
> > +window_weak_notify(gpointer data, GObject *where_was G_GNUC_UNUSED)
> > +{
> > + VirtViewerDisplay *display = VIRT_VIEWER_DISPLAY(data);
> > +
> > + g_object_set_data(G_OBJECT(display), "virt-viewer-window", NULL);
> > +}
> > +
> > static VirtViewerWindow *
> > ensure_window_for_display(VirtViewerApp *self, VirtViewerDisplay *display)
> > {
> > - gint nth = virt_viewer_display_get_nth(display);
> > - VirtViewerWindow *win = virt_viewer_app_get_nth_window(self, nth);
> > + VirtViewerWindow *win = NULL;
> > + gint nth = -1;
>
> ... and nth should be -1 on VTE and win NULL here, so you just
> need to add the if below.
Hmm, it's a bit less code, but it is a bit less correct somehow. We
shouldn't need to call virt_viewer_app_get_nth_window() for a vte
display.
Neverthess, l don't mind much, so I'll follow your recommendation.
>
> > + if (VIRT_VIEWER_IS_DISPLAY_VTE(display)) {
> > + win = g_object_get_data(G_OBJECT(display), "virt-viewer-window");
> > + } else {
> > + nth = virt_viewer_display_get_nth(display);
> > + win = virt_viewer_app_get_nth_window(self, nth);
> > + }
> > +
> > if (win == NULL) {
> > GList *l = self->priv->windows;
> >
> > @@ -874,12 +892,26 @@ ensure_window_for_display(VirtViewerApp *self, VirtViewerDisplay *display)
> > }
> >
> > virt_viewer_window_set_display(win, display);
> > + if (VIRT_VIEWER_IS_DISPLAY_VTE(display)) {
> > + g_object_set_data(G_OBJECT(display), "virt-viewer-window", win);
> > + g_object_weak_ref(G_OBJECT(win), window_weak_notify, display);
> > + }
> > }
> > virt_viewer_app_set_window_subtitle(self, win, nth);
> >
> > return win;
> > }
> >
> > +static VirtViewerWindow *
> > +display_show_get_window(VirtViewerApp *self, VirtViewerDisplay *display)
> > +{
>
> From the name, it seems like a simple getter but it does a bit
> more than that. Perhaps a reference to notebook would improve
> eg: display_show_notebook_get_window() but I'm not famous for my
> name picking ability.
>
Ok
> > + VirtViewerWindow *win = ensure_window_for_display(self, display);
> > + VirtViewerNotebook *nb = virt_viewer_window_get_notebook(win);
> > +
> > + virt_viewer_notebook_show_display(nb);
> > + return win;
> > +}
> > +
> > static void
> > display_show_hint(VirtViewerDisplay *display,
> > GParamSpec *pspec G_GNUC_UNUSED,
> > @@ -907,9 +939,7 @@ display_show_hint(VirtViewerDisplay *display,
> > virt_viewer_window_hide(win);
> > } else {
> > if (hint & VIRT_VIEWER_DISPLAY_SHOW_HINT_READY) {
> > - win = ensure_window_for_display(self, display);
> > - nb = virt_viewer_window_get_notebook(win);
> > - virt_viewer_notebook_show_display(nb);
> > + win = display_show_get_window(self, display);
> > virt_viewer_window_show(win);
> > } else {
> > if (!self->priv->kiosk && win) {
> > @@ -926,19 +956,24 @@ virt_viewer_app_display_added(VirtViewerSession *session G_GNUC_UNUSED,
> > VirtViewerDisplay *display,
> > VirtViewerApp *self)
> > {
> > - gint nth;
> > + if (VIRT_VIEWER_IS_DISPLAY_VTE(display)) {
> > + VirtViewerWindow *win = display_show_get_window(self, display);
> > + virt_viewer_window_hide(win);
> > + virt_viewer_app_update_menu_displays(self);
> > + } else {
>
>
> > + gint nth;
> >
> > - g_object_get(display, "nth-display", &nth, NULL);
> > + g_object_get(display, "nth-display", &nth, NULL);
> >
> > - g_debug("Insert display %d %p", nth, display);
>
> I'd move the if above below this debug, so we have a 'insert
> display -1 %p' log and add a return in the end of the if. Less
> changes and more log but feel free to keep as is.
ok
>
> > - g_hash_table_insert(self->priv->displays, GINT_TO_POINTER(nth), g_object_ref(display));
> > + g_debug("Insert display %d %p", nth, display);
> > + g_hash_table_insert(self->priv->displays, GINT_TO_POINTER(nth), g_object_ref(display));
> >
> > - g_signal_connect(display, "notify::show-hint",
> > - G_CALLBACK(display_show_hint), NULL);
> > - g_object_notify(G_OBJECT(display), "show-hint"); /* call display_show_hint */
> > + g_signal_connect(display, "notify::show-hint",
> > + G_CALLBACK(display_show_hint), NULL);
> > + g_object_notify(G_OBJECT(display), "show-hint"); /* call display_show_hint */
> > + }
> > }
> >
> > -
> > static void virt_viewer_app_remove_nth_window(VirtViewerApp *self,
> > gint nth)
> > {
>
> Just nitpicks,
> Acked-by: Victor Toso <victortoso@xxxxxxxxxx>
>
> > --
> > 2.19.0.271.gfe8321ec05
> >
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
