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

[Dongwon Kim]

Hi Markus,

On Mon, Jun 20, 2022 at 09:07:04AM +0200, Markus Armbruster wrote:
> 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?
> 

Will rework on the phrase. So there is only one QEMU window normally
when you start the guest os. And this window usually contains multiple
virtual consoles by default. You can detach any of them by clicking detach
menu item from UI. In this case, a new window will be created for the
detached VC. By doing this, we could have primary window and newly
created n number of windows. Individual windows means these windows.

In this patch, VC number is being used as windows number. The primary
one - guest display is '0'. The guest's secondary display will be '1'
and so on.

Thanks for the feedback.

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

You are definitely right. if (COND) should be the one here. I will fix this in 
the
next version.

> 
> > +            }
> > +        }
> > +    }
> > +    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) {
>            ...
>        }
> 

I will take a look at this as well.