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

[Marc-André Lureau]

Hi

On Fri, Aug 31, 2018 at 7:53 AM, Victor Toso <victortoso at redhat.com> wrote:
> 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'

ok

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

ok

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

No idea, I like your suggestion. Feel free to send a patch :)

>> Signed-off-by: Marc-André Lureau <marcandre.lureau at redhat.com>
>
> Patch looks fine,
> Acked-by: Victor Toso <victortoso at redhat.com>

thanks

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