Re: [Qemu-devel] [PATCH v2 04/12] gtk: add and use DisplayOptions + DisplayGTK

[Markus Armbruster]

Got a QAPI remark, cc: Eric.

Gerd Hoffmann <address@hidden> writes:

> Add QAPI DisplayType enum, DisplayOptions union and DisplayGTK struct.
> Switch gtk configuration to use the qapi type.
>
> Some bookkeeping (fullscreen for example) is done twice now, this is
> temporary until more/all UIs are switched over to qapi configuration.
>
> Signed-off-by: Gerd Hoffmann <address@hidden>
> ---
>  include/ui/console.h |  9 ++++----
>  ui/gtk.c             | 32 +++++++++++++++-------------
>  vl.c                 | 23 +++++++++++++++-----
>  qapi/ui.json         | 59 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 99 insertions(+), 24 deletions(-)
>
> diff --git a/include/ui/console.h b/include/ui/console.h
> index 7b35778444..58d1a3d27c 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -511,18 +511,17 @@ int index_from_key(const char *key, size_t key_length);
>  
>  /* gtk.c */
>  #ifdef CONFIG_GTK
> -void early_gtk_display_init(int opengl);
> -void gtk_display_init(DisplayState *ds, bool full_screen, bool 
> grab_on_hover);
> +void early_gtk_display_init(DisplayOptions *opts);
> +void gtk_display_init(DisplayState *ds, DisplayOptions *opts);
>  #else
> -static inline void gtk_display_init(DisplayState *ds, bool full_screen,
> -                                    bool grab_on_hover)
> +static inline void gtk_display_init(DisplayState *ds, DisplayOptions *opts)
>  {
>      /* This must never be called if CONFIG_GTK is disabled */
>      error_report("GTK support is disabled");
>      abort();
>  }
>  
> -static inline void early_gtk_display_init(int opengl)
> +static inline void early_gtk_display_init(DisplayOptions *opts)
>  {
>      /* This must never be called if CONFIG_GTK is disabled */
>      error_report("GTK support is disabled");
> diff --git a/ui/gtk.c b/ui/gtk.c
> index f0ad63e431..c12d5e020c 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -229,6 +229,8 @@ struct GtkDisplayState {
>  
>      bool modifier_pressed[ARRAY_SIZE(modifier_keycode)];
>      bool ignore_keys;
> +
> +    DisplayOptions *opts;
>  };
>  
>  typedef struct VCChardev {
> @@ -777,9 +779,14 @@ static gboolean gd_window_close(GtkWidget *widget, 
> GdkEvent *event,
>                                  void *opaque)
>  {
>      GtkDisplayState *s = opaque;
> +    bool allow_close = true;
>      int i;
>  
> -    if (!no_quit) {
> +    if (s->opts->has_window_close && !s->opts->window_close) {
> +        allow_close = false;
> +    }
> +
> +    if (allow_close) {
>          for (i = 0; i < s->nb_vcs; i++) {
>              if (s->vc[i].type != GD_VC_GFX) {
>                  continue;
> @@ -2289,7 +2296,7 @@ static void gd_create_menus(GtkDisplayState *s)
>  
>  static gboolean gtkinit;
>  
> -void gtk_display_init(DisplayState *ds, bool full_screen, bool grab_on_hover)
> +void gtk_display_init(DisplayState *ds, DisplayOptions *opts)
>  {
>      VirtualConsole *vc;
>  
> @@ -2301,6 +2308,8 @@ void gtk_display_init(DisplayState *ds, bool 
> full_screen, bool grab_on_hover)
>          fprintf(stderr, "gtk initialization failed\n");
>          exit(1);
>      }
> +    assert(opts->type == DISPLAY_TYPE_GTK);
> +    s->opts = opts;
>  
>  #if !GTK_CHECK_VERSION(3, 0, 0)
>      g_printerr("Running QEMU with GTK 2.x is deprecated, and will be 
> removed\n"
> @@ -2387,15 +2396,17 @@ void gtk_display_init(DisplayState *ds, bool 
> full_screen, bool grab_on_hover)
>                               vc && vc->type == GD_VC_VTE);
>  #endif
>  
> -    if (full_screen) {
> +    if (opts->has_full_screen &&
> +        opts->full_screen) {
>          gtk_menu_item_activate(GTK_MENU_ITEM(s->full_screen_item));
>      }
> -    if (grab_on_hover) {
> +    if (opts->u.gtk.has_grab_on_hover &&
> +        opts->u.gtk.grab_on_hover) {
>          gtk_menu_item_activate(GTK_MENU_ITEM(s->grab_on_hover_item));
>      }
>  }
>  
> -void early_gtk_display_init(int opengl)
> +void early_gtk_display_init(DisplayOptions *opts)
>  {
>      /* The QEMU code relies on the assumption that it's always run in
>       * the C locale. Therefore it is not prepared to deal with
> @@ -2421,11 +2432,8 @@ void early_gtk_display_init(int opengl)
>          return;
>      }
>  
> -    switch (opengl) {
> -    case -1: /* default */
> -    case 0:  /* off */
> -        break;
> -    case 1: /* on */
> +    assert(opts->type == DISPLAY_TYPE_GTK);
> +    if (opts->has_gl && opts->gl) {
>  #if defined(CONFIG_OPENGL)
>  #if defined(CONFIG_GTK_GL)
>          gtk_gl_area_init();
> @@ -2433,10 +2441,6 @@ void early_gtk_display_init(int opengl)
>          gtk_egl_init();
>  #endif
>  #endif
> -        break;
> -    default:
> -        g_assert_not_reached();
> -        break;
>      }
>  
>      keycode_map = gd_get_keymap(&keycode_maplen);
> diff --git a/vl.c b/vl.c
> index a2478412c7..4a555de0cf 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -150,9 +150,9 @@ static int rtc_date_offset = -1; /* -1 means no change */
>  QEMUClockType rtc_clock;
>  int vga_interface_type = VGA_NONE;
>  static int full_screen = 0;
> +static DisplayOptions dpy;
>  int no_frame;
>  int no_quit = 0;
> -static bool grab_on_hover;
>  Chardev *serial_hds[MAX_SERIAL_PORTS];
>  Chardev *parallel_hds[MAX_PARALLEL_PORTS];
>  Chardev *virtcon_hds[MAX_VIRTIO_CONSOLES];
> @@ -2191,24 +2191,29 @@ static LegacyDisplayType select_display(const char *p)
>      } else if (strstart(p, "gtk", &opts)) {
>  #ifdef CONFIG_GTK
>          display = DT_GTK;
> +        dpy.type = DISPLAY_TYPE_GTK;
>          while (*opts) {
>              const char *nextopt;
>  
>              if (strstart(opts, ",grab_on_hover=", &nextopt)) {
>                  opts = nextopt;
> +                dpy.u.gtk.has_grab_on_hover = true;
>                  if (strstart(opts, "on", &nextopt)) {
> -                    grab_on_hover = true;
> +                    dpy.u.gtk.grab_on_hover = true;
>                  } else if (strstart(opts, "off", &nextopt)) {
> -                    grab_on_hover = false;
> +                    dpy.u.gtk.grab_on_hover = false;
>                  } else {
>                      goto invalid_gtk_args;
>                  }
>              } else if (strstart(opts, ",gl=", &nextopt)) {
>                  opts = nextopt;
> +                dpy.has_gl = true;
>                  if (strstart(opts, "on", &nextopt)) {
>                      request_opengl = 1;
> +                    dpy.gl = true;
>                  } else if (strstart(opts, "off", &nextopt)) {
>                      request_opengl = 0;
> +                    dpy.gl = false;
>                  } else {
>                      goto invalid_gtk_args;
>                  }
> @@ -2225,6 +2230,7 @@ static LegacyDisplayType select_display(const char *p)
>  #endif
>      } else if (strstart(p, "none", &opts)) {
>          display = DT_NONE;
> +        dpy.type = DISPLAY_TYPE_NONE;
>      } else {
>          error_report("unknown display type");
>          exit(1);
> @@ -3259,6 +3265,7 @@ int main(int argc, char **argv, char **envp)
>                  qemu_opts_parse_noisily(olist, "graphics=off", false);
>                  nographic = true;
>                  display_type = DT_NONE;
> +                dpy.type = DISPLAY_TYPE_NONE;
>                  break;
>              case QEMU_OPTION_curses:
>  #ifdef CONFIG_CURSES
> @@ -3646,6 +3653,8 @@ int main(int argc, char **argv, char **envp)
>                  break;
>              case QEMU_OPTION_full_screen:
>                  full_screen = 1;
> +                dpy.has_full_screen = true;
> +                dpy.full_screen = true;
>                  break;
>              case QEMU_OPTION_no_frame:
>                  g_printerr("The -no-frame switch is deprecated, and will 
> be\n"
> @@ -3664,6 +3673,8 @@ int main(int argc, char **argv, char **envp)
>                  break;
>              case QEMU_OPTION_no_quit:
>                  no_quit = 1;
> +                dpy.has_window_close = true;
> +                dpy.window_close = false;
>                  break;
>              case QEMU_OPTION_sdl:
>  #ifdef CONFIG_SDL
> @@ -4331,6 +4342,7 @@ int main(int argc, char **argv, char **envp)
>      if (display_type == DT_DEFAULT && !display_remote) {
>  #if defined(CONFIG_GTK)
>          display_type = DT_GTK;
> +        dpy.type = DISPLAY_TYPE_GTK;
>  #elif defined(CONFIG_SDL)
>          display_type = DT_SDL;
>  #elif defined(CONFIG_COCOA)
> @@ -4339,6 +4351,7 @@ int main(int argc, char **argv, char **envp)
>          vnc_parse("localhost:0,to=99,id=default", &error_abort);
>  #else
>          display_type = DT_NONE;
> +        dpy.type = DISPLAY_TYPE_NONE;
>  #endif
>      }
>  
> @@ -4352,7 +4365,7 @@ int main(int argc, char **argv, char **envp)
>      }
>  
>      if (display_type == DT_GTK) {
> -        early_gtk_display_init(request_opengl);
> +        early_gtk_display_init(&dpy);
>      }
>  
>      if (display_type == DT_SDL) {
> @@ -4697,7 +4710,7 @@ int main(int argc, char **argv, char **envp)
>          cocoa_display_init(ds, full_screen);
>          break;
>      case DT_GTK:
> -        gtk_display_init(ds, full_screen, grab_on_hover);
> +        gtk_display_init(ds, &dpy);
>          break;
>      default:
>          break;
> diff --git a/qapi/ui.json b/qapi/ui.json
> index 07b468f625..2a0772a53a 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -982,3 +982,62 @@
>    'data': { '*device': 'str',
>              '*head'  : 'int',
>              'events' : [ 'InputEvent' ] } }
> +
> +
> +##
> +# @DisplayNoOpts:
> +#
> +# Empty struct for displays without config options.
> +#
> +# Since: 2.12
> +#
> +##
> +{ 'struct'  : 'DisplayNoOpts',
> +  'data'    : { } }

This is the fifth empty struct (QCryptoBlockInfoQCow, NetdevNoneOptions,
Abort, CpuInfoOther), not counting the QAPI frontend's internal one.
Perhaps we should make the internal one a full built-in type.  Not this
patch's problem, of course.

> +
> +##
> +# @DisplayGTK:
> +#
> +# GTK display options.
> +#
> +# @grab-on-hover: Grab keyboard input on mouse hover.
> +#
> +# Since: 2.12
> +#
> +##
> +{ 'struct'  : 'DisplayGTK',
> +  'data'    : { '*grab-on-hover' : 'bool' } }
> +
> +##
> +# @DisplayType:
> +#
> +# Display (user interface) type.
> +#
> +# Since: 2.12
> +#
> +##
> +{ 'enum'    : 'DisplayType',
> +  'data'    : [ 'default', 'none', 'gtk' ] }

Member 'default' is not used in this patch.  Suggest to introduce it
with the patch that uses it.

> +
> +##
> +# @DisplayOptions:
> +#
> +# Display (user interface) options.
> +#
> +# @type:          Which DisplayType qemu should use.
> +# @full-screen:   Start user interface in fullscreen mode (default: off).
> +# @window-close:  Allow to quit qemu with window close button (default: on).
> +# @gl:            Enable OpenGL support (default: off).
> +#
> +# Since: 2.12
> +#
> +##
> +{ 'union'   : 'DisplayOptions',
> +  'base'    : { 'type'           : 'DisplayType',
> +                '*full-screen'   : 'bool',
> +                '*window-close'  : 'bool',
> +                '*gl'            : 'bool' },
> +  'discriminator' : 'type',
> +  'data'    : { 'default'        : 'DisplayNoOpts',
> +                'none'           : 'DisplayNoOpts',
> +                'gtk'            : 'DisplayGTK' } }

Case 'default' might be slightly awkward.  Can't say, because it isn't
visible in this patch.  No biggie anyway.