[virt-tools-list] [PATCH 03/22] app: simplify toggling visibility

[Victor Toso]

Hi,

Some commit log suggestions but feel free to ignore it.

On Tue, Jul 31, 2018 at 03:24:43PM +0200, marcandre.lureau at redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau at redhat.com>
> 
> There is a hack to maintain the the toggle state to a desired

Removing the double 'the'

> state within the "toggled" handler. (since there is no way to

I would move this parenthesis to the end of commit log as a 'Note
that the hack was needed since there is no way to ...'

> hook a signal handler on "clicked" before "toggled" is emitted
> and handled by Gtk, to avoid the recursion)
> 
> However it is only necessary for the ask-quit case. In this
> case, we want to maintain the item active, which is simpler to
> handle than the general case. Simplify the code by folding
> virt_viewer_app_window_set_visible() and removing the static
> "reentering" hack, only maintaining "active" on the last item.

Out of curiosity, why we quit when disabling the last display
instead of not allowing the last display to be toggled?

I would say that might happen by mistake (disabling last display)
instead of intentional desire to quit the app.

> Signed-off-by: Marc-André Lureau <marcandre.lureau at redhat.com>

Patch looks fine,
Acked-by: Victor Toso <victortoso at redhat.com>

> ---
>  src/virt-viewer-app.c | 43 +++++++++++--------------------------------
>  src/virt-viewer-app.h |  2 +-
>  2 files changed, 12 insertions(+), 33 deletions(-)
> 
> diff --git a/src/virt-viewer-app.c b/src/virt-viewer-app.c
> index 2a88882..62dbf19 100644
> --- a/src/virt-viewer-app.c
> +++ b/src/virt-viewer-app.c
> @@ -502,29 +502,6 @@ virt_viewer_app_get_n_windows_visible(VirtViewerApp *self)
>      return n;
>  }
>  
> -gboolean
> -virt_viewer_app_window_set_visible(VirtViewerApp *self,
> -                                   VirtViewerWindow *window,
> -                                   gboolean visible)
> -{
> -    g_return_val_if_fail(VIRT_VIEWER_IS_APP(self), FALSE);
> -    g_return_val_if_fail(VIRT_VIEWER_IS_WINDOW(window), FALSE);
> -
> -    if (visible) {
> -        virt_viewer_window_show(window);
> -        return TRUE;
> -    } else {
> -        if (virt_viewer_app_get_n_windows_visible(self) > 1) {
> -            virt_viewer_window_hide(window);
> -            return FALSE;
> -        }
> -
> -        virt_viewer_app_maybe_quit(self, window);
> -    }
> -
> -    return FALSE;
> -}
> -
>  static void hide_one_window(gpointer value,
>                              gpointer user_data G_GNUC_UNUSED)
>  {
> @@ -2232,19 +2209,21 @@ menu_display_visible_toggled_cb(GtkCheckMenuItem *checkmenuitem,
>  {
>      VirtViewerApp *self = virt_viewer_session_get_app(virt_viewer_display_get_session(display));
>      gboolean visible = gtk_check_menu_item_get_active(checkmenuitem);
> -    static gboolean reentering = FALSE;
>      VirtViewerWindow *vwin;
>  
> -    if (reentering) /* do not reenter if I switch you back */
> -        return;
> -
> -    reentering = TRUE;
> -
>      vwin = ensure_window_for_display(self, display);
> -    visible = virt_viewer_app_window_set_visible(self, vwin, visible);
>  
> -    gtk_check_menu_item_set_active(checkmenuitem, /* will be toggled again */ !visible);
> -    reentering = FALSE;
> +    if (visible) {
> +        virt_viewer_window_show(vwin);
> +    } else {
> +        if (virt_viewer_app_get_n_windows_visible(self) > 1) {
> +            virt_viewer_window_hide(vwin);
> +        } else {
> +            virt_viewer_app_maybe_quit(self, vwin);
> +            /* the last item remains active, doesn't matter if we quit */
> +            gtk_check_menu_item_set_active(checkmenuitem, TRUE);
> +        }
> +    }
>  
>      virt_viewer_session_update_displays_geometry(virt_viewer_display_get_session(display));
>  }
> diff --git a/src/virt-viewer-app.h b/src/virt-viewer-app.h
> index 16b1c8c..0321229 100644
> --- a/src/virt-viewer-app.h
> +++ b/src/virt-viewer-app.h
> @@ -84,7 +84,7 @@ void virt_viewer_app_set_connect_info(VirtViewerApp *self,
>                                        const gchar *user,
>                                        gint port,
>                                        const gchar *guri);
> -gboolean virt_viewer_app_window_set_visible(VirtViewerApp *self, VirtViewerWindow *window, gboolean visible);
> +
>  void virt_viewer_app_show_status(VirtViewerApp *self, const gchar *fmt, ...);
>  void virt_viewer_app_show_display(VirtViewerApp *self);
>  GList* virt_viewer_app_get_windows(VirtViewerApp *self);
> -- 
> 2.18.0.321.gffc6fa0e39
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20180831/2168f3d6/attachment.sig>