Re: [PATCH] gtk: Add show_tabs=on|off command line option.

[Hanna Reitz <hreitz@redhat.com>]

Hi,

(Thanks for the patch!)

On 27.06.22 18:44, Felix xq Queißner wrote:
> The patch adds "show_tabs" command line option for GTK ui similar to "grab_on_hover". This option allows tabbed view mode to not have to be enabled by hand at each start of the VM.

I’m not sure we have a hard rule on it, but I think generally commit 
messages should be wrapped at 72 characters.

> Signed-off-by: Felix "xq" Queißner <xq@random-projects.net>
> ---
>   qapi/ui.json    | 5 ++++-
>   qemu-options.hx | 2 +-
>   ui/gtk.c        | 4 ++++
>   3 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/qapi/ui.json b/qapi/ui.json
> index 413371d5e8..049e7957a9 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -1195,12 +1195,15 @@
>   #               assuming the guest will resize the display to match
>   #               the window size then.  Otherwise it defaults to "off".
>   #               Since 3.1
> +# @show-tabs:   Displays the tab items by default.
> +#               Since 7.1

I don’t really understand what “tab items” is supposed to mean. Perhaps 
“tabs item”?

But a bit more verbosity might be nice, too.  What about “Display the 
tab bar for switching between the various graphical interfaces (e.g. VGA 
and virtual console character devices) by default”?  (Note the 
imperative on “Display”, I think we generally use the imperative to 
document options.)

>   #
>   # Since: 2.12
>   ##
>   { 'struct'  : 'DisplayGTK',
>     'data'    : { '*grab-on-hover' : 'bool',
> -                '*zoom-to-fit'   : 'bool'  } }
> +                '*zoom-to-fit'   : 'bool',
> +                '*show-tabs'     : 'bool'  } }
>   
>   ##
>   # @DisplayEGLHeadless:
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 377d22fbd8..2b279afff7 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1937,7 +1937,7 @@ DEF("display", HAS_ARG, QEMU_OPTION_display,
>       "            [,window-close=on|off]\n"
>   #endif
>   #if defined(CONFIG_GTK)
> -    "-display gtk[,full-screen=on|off][,gl=on|off][,grab-on-hover=on|off]\n"
> +    "-display gtk[,full-screen=on|off][,gl=on|off][,grab-on-hover=on|off][,show-tabs=on|off]\n"
>       "            [,show-cursor=on|off][,window-close=on|off]\n"

There is some documentation further below that explains the other 
options.  I think this new option should be explained there as well.  
(Probably reusing the documentation from qapi/ui.json.)

>   #endif
>   #if defined(CONFIG_VNC)
> diff --git a/ui/gtk.c b/ui/gtk.c
> index 2a791dd2aa..1467b8c7d7 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -2390,6 +2390,10 @@ static void gtk_display_init(DisplayState *ds, DisplayOptions *opts)
>           opts->u.gtk.grab_on_hover) {
>           gtk_menu_item_activate(GTK_MENU_ITEM(s->grab_on_hover_item));
>       }
> +    if (opts->u.gtk.has_show_tabs &&
> +        opts->u.gtk.show_tabs) {
> +        gtk_menu_item_activate(GTK_MENU_ITEM(s->show_tabs_item));
> +    }
>       gd_clipboard_init(s);
>   }

Not that I’d know any of this code, but FWIW, makes sense and looks good 
to me.

Thanks!

Hanna