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

[Dongwon Kim]

On Thu, Jun 30, 2022 at 05:19:26PM +0200, Markus Armbruster wrote:
> Dongwon Kim <dongwon.kim@intel.com> writes:
> 
> > New integer array parameter, 'monitor' is for specifying the target
> > monitors where individual GTK windows are placed upon launching.
> >
> > Monitor numbers in the array are associated with virtual consoles
> > in the order of [VC0, VC1, VC2 ... VCn].
> >
> > Every GTK window containing each VC will be placed in the region
> > of corresponding monitors.
> >
> > Usage: -display gtk,monitor.<id of VC>=<target monitor>,..
> >        ex)-display gtk,monitor.0=1,monitor.1=0
> >
> > v3: - Revised commit message
> >     - Rewrote desription of the new parameter (Markus Armbruster)
> >     - Replaced unnecessary 'for' loop with 'if' condition
> >       (Markus Armbruster)
> 
> Again, patch history ...
> > 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>
> > ---
> 
> ... goes here.

No problem moving down the version history but may I ask you if that
is current rule? We don't want to include the history anymore in the
git history?

And FYI, the cover letter has the whole history already. I guess I can
simply remove the history from individual patches then?

Thanks!!

> 
> >  qapi/ui.json    |  9 ++++++++-
> >  qemu-options.hx |  3 ++-
> >  ui/gtk.c        | 31 +++++++++++++++++++++++++++++--
> >  3 files changed, 39 insertions(+), 4 deletions(-)
> >
> > diff --git a/qapi/ui.json b/qapi/ui.json
> > index 413371d5e8..7b4c098bb4 100644
> > --- a/qapi/ui.json
> > +++ b/qapi/ui.json
> > @@ -1195,12 +1195,19 @@
> >  #               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 GTK window containing a given VC will be
> > +#               placed. Each monitor number in the array will be
> > +#               associated with a virtual-console starting from VC0.
> 
> Drop the hyphen in "virtual-console".
> 
> Is the term "virtual console" obvious?  Gerd?
> 

I will do so.

> > +#
> > +#               since 7.1
> >  #
> >  # 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..aabdfb0636 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -1938,7 +1938,8 @@ 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.<id of VC>=<monitor 
> > number>][,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..935176e614 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,10 +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 &&
> > -        opts->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 && i < s->nb_vcs) {
> > +                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++;
> > +            }
> > +        }
> > +    } else if (opts->has_full_screen &&
> > +               opts->full_screen) {
> 
> I'd join these two lines, like
> 
>        } else if (opts->has_full_screen && opts->full_screen) {
> 
> or even exploit the fact that a opts->full_screen is false when absent
> 
>        } else if (opts->full_screen) {

This is simpler. I will go with this.

> 
> >          gtk_menu_item_activate(GTK_MENU_ITEM(s->full_screen_item));
> >      }
> > +
> >      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));
>