Re: [PATCH v2 2/2] ui/gtk: a new array param monitor to specify the target displays

[Markus Armbruster <armbru@redhat.com>]

Dongwon Kim <dongwon.kim@intel.com> writes:

> New integer array parameter, 'monitor' is for specifying the target
> displays where individual QEMU windows are placed upon launching.
>
> The array contains a series of numbers representing the monitor where
> QEMU windows are placed.
>
> Numbers in the array are mapped to QEMU windows like,
>
> [1st detached window, 2nd detached window,.... Main window]
>
> Usage example: -display gtk,monitor.0=0,monitor.1=1.....
>
> Cc: Daniel P. Berrangé <berrange@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Vivek Kasireddy <vivek.kasireddy@intel.com>
> Signed-off-by: Dongwon Kim <dongwon.kim@intel.com>
> ---
>  qapi/ui.json    |  7 ++++++-
>  qemu-options.hx |  2 +-
>  ui/gtk.c        | 32 +++++++++++++++++++++++++++++++-
>  3 files changed, 38 insertions(+), 3 deletions(-)
>
> diff --git a/qapi/ui.json b/qapi/ui.json
> index 413371d5e8..5980f30c7f 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -1195,12 +1195,17 @@
>  #               assuming the guest will resize the display to match
>  #               the window size then.  Otherwise it defaults to "off".
>  #               Since 3.1
> +# @monitor:     Array of numbers, each of which represents physical
> +#               monitor where individual QEMU window is placed in case
> +#               there are multiple of them

End you sentence with a period, and ...

> +#               since 7.1

... start the next phrase with a capital letter.

The documentation text feels vague.  Possibly because I lack familiarity
with this part of the user interface.  What are the "individual QEMU
windows"?  How are they numbered?

>  #
>  # Since: 2.12
>  ##
>  { 'struct'  : 'DisplayGTK',
>    'data'    : { '*grab-on-hover' : 'bool',
> -                '*zoom-to-fit'   : 'bool'  } }
> +                '*zoom-to-fit'   : 'bool',
> +                '*monitor'       : ['uint16']  } }
>  
>  ##
>  # @DisplayEGLHeadless:
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 377d22fbd8..f79f533e9d 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1938,7 +1938,7 @@ DEF("display", HAS_ARG, QEMU_OPTION_display,
>  #endif
>  #if defined(CONFIG_GTK)
>      "-display gtk[,full-screen=on|off][,gl=on|off][,grab-on-hover=on|off]\n"
> -    "            [,show-cursor=on|off][,window-close=on|off]\n"
> +    "            [,monitor.<order>=<value>][,show-cursor=on|off][,window-close=on|off]\n"
>  #endif
>  #if defined(CONFIG_VNC)
>      "-display vnc=<display>[,<optargs>]\n"
> diff --git a/ui/gtk.c b/ui/gtk.c
> index e6878c3209..fc9bf04680 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -2316,6 +2316,10 @@ static void gtk_display_init(DisplayState *ds, DisplayOptions *opts)
>      GtkDisplayState *s = g_malloc0(sizeof(*s));
>      GdkDisplay *window_display;
>      GtkIconTheme *theme;
> +    GtkWidget *win;
> +    GdkRectangle dest;
> +    uint16List *mon;
> +    int n_mon;
>      int i;
>      char *dir;
>  
> @@ -2393,7 +2397,33 @@ static void gtk_display_init(DisplayState *ds, DisplayOptions *opts)
>              gtk_menu_item_activate(GTK_MENU_ITEM(s->untabify_item));
>          }
>      }
> -    if (opts->has_full_screen &&
> +    if (opts->u.gtk.has_monitor) {
> +        i = 0;
> +        n_mon = gdk_display_get_n_monitors(window_display);
> +        for (mon = opts->u.gtk.monitor; mon; mon = mon->next) {
> +            if (mon->value < n_mon) {
> +                for (; i < s->nb_vcs; i++) {
> +                    win = s->vc[i].window ? s->vc[i].window : s->window;
> +                    if (opts->has_full_screen && opts->full_screen) {
> +                        gtk_window_fullscreen_on_monitor(
> +                            GTK_WINDOW(win),
> +                            gdk_display_get_default_screen(window_display),
> +                            mon->value);
> +                    } else {
> +                        gdk_monitor_get_geometry(
> +                            gdk_display_get_monitor(window_display, mon->value),
> +                            &dest);
> +                        gtk_window_move(GTK_WINDOW(win),
> +                                        dest.x, dest.y);
> +                    }
> +                    i++;
> +                    break;
> +                }

This loop is odd.  It's of the form

                   for (; COND; STEP) {
                       ...
                       break;
                   }

STEP is unreachable.  The whole thing boils down to

                   if (COND) {
                       ....
                   }

doesn't it?

> +            }
> +        }
> +    }
> +    if (!opts->u.gtk.has_monitor &&
> +        opts->has_full_screen &&
>          opts->full_screen) {
>          gtk_menu_item_activate(GTK_MENU_ITEM(s->full_screen_item));
>      }


This is

       if (COND1) {
           ...
       }
       if (!COND1 && COND2) {
           ...
       }

I'd prefer

       if (COND1) {
           ...
       } else if (COND2) {
           ...
       }