[PATCH 0/3] ui/gtk: new options, monitor and detach-all

[PATCH 0/3] ui/gtk: new options, monitor and detach-all

[Dongwon Kim]

This patch series introduces two new gtk optional parameters, monitor
and detach-all. "monitor" is for specifying a display where QEMU window
will be launched from. "detach-all" is making all VCs detached upon
QEMU's launch. The use-case we originally wanted to deal with is when
multiple displays (max_output = n) are assigned to the guest os and
each guest display needs to be full-screened to certain physical monitor.
This can be achieved when these new parameters are properly configured
together with existing full-screen option set to true.

Dongwon Kim (3):
  ui/gtk: new param monitor to specify target monitor for launching QEMU
  ui/gtk: detach_all option for making all VCs detached upon starting
  ui/gtk: specify detached window's size and location

 qapi/ui.json    | 11 +++++++--
 qemu-options.hx |  2 +-
 ui/gtk.c        | 66 +++++++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 74 insertions(+), 5 deletions(-)

-- 
2.30.2

[PATCH 1/3] ui/gtk: new param monitor to specify target monitor for launching QEMU

[Dongwon Kim]

Introducing a new integer parameter to specify the monitor where the
Qemu window is placed upon launching.

Monitor can be any number between 0 and (total number of monitors - 1).

It can be used together with full-screen=on, which will make the QEMU
window full-screened on the targeted monitor.

v2: fixed typos and updated commit subject and msg
    (Philippe Mathieu-Daudé)

    changed param name to monitor, removed unnecessary condition check
    on the parameter
    (Paolo Bonzini)

v3: updated Since version to 7.1 for monitor parameter

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    | 6 +++++-
 qemu-options.hx | 2 +-
 ui/gtk.c        | 8 ++++++++
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/qapi/ui.json b/qapi/ui.json
index 059302a5ef..ddcea7349b 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -1204,13 +1204,17 @@
 #               assuming the guest will resize the display to match
 #               the window size then.  Otherwise it defaults to "off".
 #               Since 3.1
+# @monitor:     Indicate monitor where QEMU window is lauched. monitor
+#               could be any number from 0 to (total num of monitors - 1).
+#               since 7.1
 #
 # Since: 2.12
 #
 ##
 { 'struct'  : 'DisplayGTK',
   'data'    : { '*grab-on-hover' : 'bool',
-                '*zoom-to-fit'   : 'bool'  } }
+                '*zoom-to-fit'   : 'bool',
+                '*monitor'       : 'uint32' } }
 
 ##
 # @DisplayEGLHeadless:
diff --git a/qemu-options.hx b/qemu-options.hx
index 5f69b94b8e..dbf9b223dd 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -1908,7 +1908,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=<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 c57c36749e..d9971d65ac 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -2375,6 +2375,14 @@ static void gtk_display_init(DisplayState *ds, DisplayOptions *opts)
                              vc && vc->type == GD_VC_VTE);
 #endif
 
+    if (opts->u.gtk.has_monitor &&
+        opts->u.gtk.monitor < gdk_display_get_n_monitors(window_display)) {
+        GdkRectangle mon_dest;
+        gdk_monitor_get_geometry(
+            gdk_display_get_monitor(window_display, opts->u.gtk.monitor),
+            &mon_dest);
+        gtk_window_move(GTK_WINDOW(s->window), mon_dest.x, mon_dest.y);
+    }
     if (opts->has_full_screen &&
         opts->full_screen) {
         gtk_menu_item_activate(GTK_MENU_ITEM(s->full_screen_item));
-- 
2.30.2

[PATCH 2/3] ui/gtk: detach_all option for making all VCs detached upon starting

[Dongwon Kim]

With "detach-all=on" for display, QEMU starts with all VC windows
detached automatically.

If used with "full-screen=on", it places individual windows (from
top window) starting from monitor 0 or monitor n in case monitor=n.

In case # mon < # VCs, only same number of VCs as # mon will be sent to
the monitors for full-screen while others are remaining in windowed-mode.

Target monitor number for individual VC is rotated in case monitor=n
(n != 0) (e.g. if monitor=1 and # VCs = 2, the top window will be
full-screened on monitor 1 and top second window will be full-screened
on monitor 0.)

v2: update Since version to 7.1

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 |  5 ++++-
 ui/gtk.c     | 47 ++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 48 insertions(+), 4 deletions(-)

diff --git a/qapi/ui.json b/qapi/ui.json
index ddcea7349b..a5b1550937 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -1378,6 +1378,8 @@
 # @show-cursor:   Force showing the mouse cursor (default: off).
 #                 (since: 5.0)
 # @gl:            Enable OpenGL support (default: off).
+# @detach-all:    Start QEMU with all VC windows detached (default: off)
+#                 (since: 7.1)
 #
 # Since: 2.12
 #
@@ -1387,7 +1389,8 @@
                 '*full-screen'   : 'bool',
                 '*window-close'  : 'bool',
                 '*show-cursor'   : 'bool',
-                '*gl'            : 'DisplayGLMode' },
+                '*gl'            : 'DisplayGLMode',
+                '*detach-all'    : 'bool' },
   'discriminator' : 'type',
   'data'    : {
       'gtk': { 'type': 'DisplayGTK', 'if': 'CONFIG_GTK' },
diff --git a/ui/gtk.c b/ui/gtk.c
index d9971d65ac..f1ca6a7275 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -2305,6 +2305,8 @@ static void gtk_display_init(DisplayState *ds, DisplayOptions *opts)
     GdkDisplay *window_display;
     GtkIconTheme *theme;
     char *dir;
+    int num_mon;
+    int i;
 
     if (!gtkinit) {
         fprintf(stderr, "gtk initialization failed\n");
@@ -2374,18 +2376,57 @@ static void gtk_display_init(DisplayState *ds, DisplayOptions *opts)
     gtk_widget_set_sensitive(s->copy_item,
                              vc && vc->type == GD_VC_VTE);
 #endif
-
     if (opts->u.gtk.has_monitor &&
-        opts->u.gtk.monitor < gdk_display_get_n_monitors(window_display)) {
+        opts->u.gtk.monitor <
+        (num_mon = gdk_display_get_n_monitors(window_display))) {
         GdkRectangle mon_dest;
         gdk_monitor_get_geometry(
             gdk_display_get_monitor(window_display, opts->u.gtk.monitor),
             &mon_dest);
         gtk_window_move(GTK_WINDOW(s->window), mon_dest.x, mon_dest.y);
     }
+    if (opts->has_detach_all &&
+        opts->detach_all) {
+        for (i = 0; i < s->nb_vcs - 1; i++) {
+            gtk_menu_item_activate(GTK_MENU_ITEM(s->untabify_item));
+        }
+    }
     if (opts->has_full_screen &&
         opts->full_screen) {
-        gtk_menu_item_activate(GTK_MENU_ITEM(s->full_screen_item));
+        bool no_mon_left = 0;
+        int next_mon = 0;
+        if (!opts->u.gtk.has_monitor ||
+	    (opts->u.gtk.has_monitor && opts->u.gtk.monitor < num_mon)) {
+            next_mon = (opts->u.gtk.has_monitor) ? opts->u.gtk.monitor : 0;
+            for (i = 0; i < s->nb_vcs - 1; i++) {
+                if (!s->vc[i].window) {
+                    continue;
+                }
+
+                gtk_window_fullscreen_on_monitor(
+                    GTK_WINDOW(s->vc[i].window),
+                    gdk_display_get_default_screen(window_display),
+                    next_mon++);
+
+                if (next_mon == opts->u.gtk.monitor) {
+                    no_mon_left = true;
+                    break;
+                }
+
+                if (next_mon == num_mon) {
+                    next_mon = 0;
+                }
+            }
+        }
+
+        if (!no_mon_left) {
+            GdkRectangle mon_dest;
+            gdk_monitor_get_geometry(
+                gdk_display_get_monitor(window_display, next_mon),
+                &mon_dest);
+            gtk_window_move(GTK_WINDOW(s->window), mon_dest.x, mon_dest.y);
+            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) {
-- 
2.30.2

[PATCH 3/3] ui/gtk: specify detached window's size and location

[Dongwon Kim]

Specify location and size of detached window based on top level
window's location and size info when detachment happens.

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>
---
 ui/gtk.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/ui/gtk.c b/ui/gtk.c
index f1ca6a7275..7dadf3b588 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -1338,6 +1338,8 @@ static void gd_menu_untabify(GtkMenuItem *item, void *opaque)
                                        FALSE);
     }
     if (!vc->window) {
+        gint x, y, w, h;
+        int i;
         gtk_widget_set_sensitive(vc->menu_item, false);
         vc->window = gtk_window_new(GTK_WINDOW_TOPLEVEL);
 #if defined(CONFIG_OPENGL)
@@ -1351,7 +1353,18 @@ static void gd_menu_untabify(GtkMenuItem *item, void *opaque)
         }
 #endif
         gd_widget_reparent(s->notebook, vc->window, vc->tab_item);
+        gtk_window_get_position(GTK_WINDOW(s->window), &x, &y);
+        gtk_window_get_size(GTK_WINDOW(s->window), &w, &h);
+
+        for (i = 0; i < s->nb_vcs; i++) {
+            if (vc == &s->vc[i]) {
+                break;
+            }
+        }
 
+        gtk_window_move(GTK_WINDOW(vc->window),
+                        x + w * (i % (s->nb_vcs/2) + 1), y + h * (i / (s->nb_vcs/2)));
+        gtk_window_resize(GTK_WINDOW(vc->window), w, h);
         g_signal_connect(vc->window, "delete-event",
                          G_CALLBACK(gd_tab_window_close), vc);
         gtk_widget_show_all(vc->window);
-- 
2.30.2

Re: [PATCH 2/3] ui/gtk: detach_all option for making all VCs detached upon starting

[Daniel P. Berrangé]

On Thu, Apr 28, 2022 at 04:13:03PM -0700, Dongwon Kim wrote:
> With "detach-all=on" for display, QEMU starts with all VC windows
> detached automatically.
> 
> If used with "full-screen=on", it places individual windows (from
> top window) starting from monitor 0 or monitor n in case monitor=n.
> 
> In case # mon < # VCs, only same number of VCs as # mon will be sent to
> the monitors for full-screen while others are remaining in windowed-mode.
> 
> Target monitor number for individual VC is rotated in case monitor=n
> (n != 0) (e.g. if monitor=1 and # VCs = 2, the top window will be
> full-screened on monitor 1 and top second window will be full-screened
> on monitor 0.)

I tend to wonder whether we actually need this at all, as opposed
to just changing QEMU's behaviour by default.

It makes sense to have tabs per-VC for the things like the HMP
console, serial ports, etc, but I think graphical video outputs
should always be displayed as multiple windows. Putting graphical
outputs as tabs rather defeats the purpose of having multiple
outputs IMHO. 

IOW, why won't we just create 1 gtk window per graphical output
all the time.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 1/3] ui/gtk: new param monitor to specify target monitor for launching QEMU

[Daniel P. Berrangé]

On Thu, Apr 28, 2022 at 04:13:02PM -0700, Dongwon Kim wrote:
> Introducing a new integer parameter to specify the monitor where the
> Qemu window is placed upon launching.
> 
> Monitor can be any number between 0 and (total number of monitors - 1).
> 
> It can be used together with full-screen=on, which will make the QEMU
> window full-screened on the targeted monitor.
> 
> v2: fixed typos and updated commit subject and msg
>     (Philippe Mathieu-Daudé)
> 
>     changed param name to monitor, removed unnecessary condition check
>     on the parameter
>     (Paolo Bonzini)
> 
> v3: updated Since version to 7.1 for monitor parameter
> 
> 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    | 6 +++++-
>  qemu-options.hx | 2 +-
>  ui/gtk.c        | 8 ++++++++
>  3 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/qapi/ui.json b/qapi/ui.json
> index 059302a5ef..ddcea7349b 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -1204,13 +1204,17 @@
>  #               assuming the guest will resize the display to match
>  #               the window size then.  Otherwise it defaults to "off".
>  #               Since 3.1
> +# @monitor:     Indicate monitor where QEMU window is lauched. monitor
> +#               could be any number from 0 to (total num of monitors - 1).
> +#               since 7.1
>  #
>  # Since: 2.12
>  #
>  ##
>  { 'struct'  : 'DisplayGTK',
>    'data'    : { '*grab-on-hover' : 'bool',
> -                '*zoom-to-fit'   : 'bool'  } }
> +                '*zoom-to-fit'   : 'bool',
> +                '*monitor'       : 'uint32' } }

I feel like this ought to be an array of monitors, so that we can have
explicit positioning when we have multiple graphical outputs and are
creating a separate window for each.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 3/3] ui/gtk: specify detached window's size and location

[Daniel P. Berrangé]

On Thu, Apr 28, 2022 at 04:13:04PM -0700, Dongwon Kim wrote:
> Specify location and size of detached window based on top level
> window's location and size info when detachment happens.

Can you explain what problem is being solved by this change ?
What's wrong with default size/placement logic ?

In terms of size at least, I would hope we are resizing
windows any time the guest changes the resolution of the
virtual video adapter.  If there are 2 outputs, they can
be at different resolution, so copying the size of the
existing window feels wrong - we need to copy the guest
resolution currently set.

Why do we need to mess around with position at all ?

> 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>
> ---
>  ui/gtk.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/ui/gtk.c b/ui/gtk.c
> index f1ca6a7275..7dadf3b588 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -1338,6 +1338,8 @@ static void gd_menu_untabify(GtkMenuItem *item, void *opaque)
>                                         FALSE);
>      }
>      if (!vc->window) {
> +        gint x, y, w, h;
> +        int i;
>          gtk_widget_set_sensitive(vc->menu_item, false);
>          vc->window = gtk_window_new(GTK_WINDOW_TOPLEVEL);
>  #if defined(CONFIG_OPENGL)
> @@ -1351,7 +1353,18 @@ static void gd_menu_untabify(GtkMenuItem *item, void *opaque)
>          }
>  #endif
>          gd_widget_reparent(s->notebook, vc->window, vc->tab_item);
> +        gtk_window_get_position(GTK_WINDOW(s->window), &x, &y);
> +        gtk_window_get_size(GTK_WINDOW(s->window), &w, &h);
> +
> +        for (i = 0; i < s->nb_vcs; i++) {
> +            if (vc == &s->vc[i]) {
> +                break;
> +            }
> +        }
>  
> +        gtk_window_move(GTK_WINDOW(vc->window),
> +                        x + w * (i % (s->nb_vcs/2) + 1), y + h * (i / (s->nb_vcs/2)));
> +        gtk_window_resize(GTK_WINDOW(vc->window), w, h);
>          g_signal_connect(vc->window, "delete-event",
>                           G_CALLBACK(gd_tab_window_close), vc);
>          gtk_widget_show_all(vc->window);
> -- 
> 2.30.2
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 1/3] ui/gtk: new param monitor to specify target monitor for launching QEMU

[Dongwon Kim]

On Tue, May 03, 2022 at 10:15:13AM +0100, Daniel P. Berrangé wrote:
> On Thu, Apr 28, 2022 at 04:13:02PM -0700, Dongwon Kim wrote:
> > Introducing a new integer parameter to specify the monitor where the
> > Qemu window is placed upon launching.
> > 
> > Monitor can be any number between 0 and (total number of monitors - 1).
> > 
> > It can be used together with full-screen=on, which will make the QEMU
> > window full-screened on the targeted monitor.
> > 
> > v2: fixed typos and updated commit subject and msg
> >     (Philippe Mathieu-Daudé)
> > 
> >     changed param name to monitor, removed unnecessary condition check
> >     on the parameter
> >     (Paolo Bonzini)
> > 
> > v3: updated Since version to 7.1 for monitor parameter
> > 
> > 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    | 6 +++++-
> >  qemu-options.hx | 2 +-
> >  ui/gtk.c        | 8 ++++++++
> >  3 files changed, 14 insertions(+), 2 deletions(-)
> > 
> > diff --git a/qapi/ui.json b/qapi/ui.json
> > index 059302a5ef..ddcea7349b 100644
> > --- a/qapi/ui.json
> > +++ b/qapi/ui.json
> > @@ -1204,13 +1204,17 @@
> >  #               assuming the guest will resize the display to match
> >  #               the window size then.  Otherwise it defaults to "off".
> >  #               Since 3.1
> > +# @monitor:     Indicate monitor where QEMU window is lauched. monitor
> > +#               could be any number from 0 to (total num of monitors - 1).
> > +#               since 7.1
> >  #
> >  # Since: 2.12
> >  #
> >  ##
> >  { 'struct'  : 'DisplayGTK',
> >    'data'    : { '*grab-on-hover' : 'bool',
> > -                '*zoom-to-fit'   : 'bool'  } }
> > +                '*zoom-to-fit'   : 'bool',
> > +                '*monitor'       : 'uint32' } }
> 
> I feel like this ought to be an array of monitors, so that we can have
> explicit positioning when we have multiple graphical outputs and are
> creating a separate window for each.

That would be ideal but at the same time, wouldn't it make the option to
specific/complicated? And I am not sure how to create an option that takes the
data in the form of array. Do you have any reference?

> 
> 
> With regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> 

Re: [PATCH 2/3] ui/gtk: detach_all option for making all VCs detached upon starting

[Dongwon Kim]

On Tue, May 03, 2022 at 10:12:43AM +0100, Daniel P. Berrangé wrote:
> On Thu, Apr 28, 2022 at 04:13:03PM -0700, Dongwon Kim wrote:
> > With "detach-all=on" for display, QEMU starts with all VC windows
> > detached automatically.
> > 
> > If used with "full-screen=on", it places individual windows (from
> > top window) starting from monitor 0 or monitor n in case monitor=n.
> > 
> > In case # mon < # VCs, only same number of VCs as # mon will be sent to
> > the monitors for full-screen while others are remaining in windowed-mode.
> > 
> > Target monitor number for individual VC is rotated in case monitor=n
> > (n != 0) (e.g. if monitor=1 and # VCs = 2, the top window will be
> > full-screened on monitor 1 and top second window will be full-screened
> > on monitor 0.)
> 
> I tend to wonder whether we actually need this at all, as opposed
> to just changing QEMU's behaviour by default.
> 
> It makes sense to have tabs per-VC for the things like the HMP
> console, serial ports, etc, but I think graphical video outputs
> should always be displayed as multiple windows. Putting graphical
> outputs as tabs rather defeats the purpose of having multiple
> outputs IMHO. 
> 
> IOW, why won't we just create 1 gtk window per graphical output
> all the time.

I got your point but I think this requires changes in the
policy, which I guess need community-wide agreement. Why don't we move
on with this new option and at the same time start the discussion?

One more point is, I tried to find out but I couldn't think of any good way
to distinguish between guest output consoles and other consoles. Do you
have any thought on this?

> 
> With regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> 

Re: [PATCH 3/3] ui/gtk: specify detached window's size and location

[Dongwon Kim]

I saw windows, especially, third and fourth ones are 1/4 size of
the first when detached regardless of resolutions.

And the position is also pretty random and detached windows are usually
placed somewhere on the previous window.

This patch is to make the sizes same as the original window's and make
sure all detached windows are not overlapped each other.

On Tue, May 03, 2022 at 10:17:46AM +0100, Daniel P. Berrangé wrote:
> On Thu, Apr 28, 2022 at 04:13:04PM -0700, Dongwon Kim wrote:
> > Specify location and size of detached window based on top level
> > window's location and size info when detachment happens.
> 
> Can you explain what problem is being solved by this change ?
> What's wrong with default size/placement logic ?
> 
> In terms of size at least, I would hope we are resizing
> windows any time the guest changes the resolution of the
> virtual video adapter.  If there are 2 outputs, they can
> be at different resolution, so copying the size of the
> existing window feels wrong - we need to copy the guest
> resolution currently set.
> 
> Why do we need to mess around with position at all ?
> 
> > 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>
> > ---
> >  ui/gtk.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/ui/gtk.c b/ui/gtk.c
> > index f1ca6a7275..7dadf3b588 100644
> > --- a/ui/gtk.c
> > +++ b/ui/gtk.c
> > @@ -1338,6 +1338,8 @@ static void gd_menu_untabify(GtkMenuItem *item, void *opaque)
> >                                         FALSE);
> >      }
> >      if (!vc->window) {
> > +        gint x, y, w, h;
> > +        int i;
> >          gtk_widget_set_sensitive(vc->menu_item, false);
> >          vc->window = gtk_window_new(GTK_WINDOW_TOPLEVEL);
> >  #if defined(CONFIG_OPENGL)
> > @@ -1351,7 +1353,18 @@ static void gd_menu_untabify(GtkMenuItem *item, void *opaque)
> >          }
> >  #endif
> >          gd_widget_reparent(s->notebook, vc->window, vc->tab_item);
> > +        gtk_window_get_position(GTK_WINDOW(s->window), &x, &y);
> > +        gtk_window_get_size(GTK_WINDOW(s->window), &w, &h);
> > +
> > +        for (i = 0; i < s->nb_vcs; i++) {
> > +            if (vc == &s->vc[i]) {
> > +                break;
> > +            }
> > +        }
> >  
> > +        gtk_window_move(GTK_WINDOW(vc->window),
> > +                        x + w * (i % (s->nb_vcs/2) + 1), y + h * (i / (s->nb_vcs/2)));
> > +        gtk_window_resize(GTK_WINDOW(vc->window), w, h);
> >          g_signal_connect(vc->window, "delete-event",
> >                           G_CALLBACK(gd_tab_window_close), vc);
> >          gtk_widget_show_all(vc->window);
> > -- 
> > 2.30.2
> > 
> > 
> 
> With regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> 

Re: [PATCH 2/3] ui/gtk: detach_all option for making all VCs detached upon starting

[Daniel P. Berrangé]

On Tue, May 03, 2022 at 04:21:44PM -0700, Dongwon Kim wrote:
> On Tue, May 03, 2022 at 10:12:43AM +0100, Daniel P. Berrangé wrote:
> > On Thu, Apr 28, 2022 at 04:13:03PM -0700, Dongwon Kim wrote:
> > > With "detach-all=on" for display, QEMU starts with all VC windows
> > > detached automatically.
> > > 
> > > If used with "full-screen=on", it places individual windows (from
> > > top window) starting from monitor 0 or monitor n in case monitor=n.
> > > 
> > > In case # mon < # VCs, only same number of VCs as # mon will be sent to
> > > the monitors for full-screen while others are remaining in windowed-mode.
> > > 
> > > Target monitor number for individual VC is rotated in case monitor=n
> > > (n != 0) (e.g. if monitor=1 and # VCs = 2, the top window will be
> > > full-screened on monitor 1 and top second window will be full-screened
> > > on monitor 0.)
> > 
> > I tend to wonder whether we actually need this at all, as opposed
> > to just changing QEMU's behaviour by default.
> > 
> > It makes sense to have tabs per-VC for the things like the HMP
> > console, serial ports, etc, but I think graphical video outputs
> > should always be displayed as multiple windows. Putting graphical
> > outputs as tabs rather defeats the purpose of having multiple
> > outputs IMHO. 
> > 
> > IOW, why won't we just create 1 gtk window per graphical output
> > all the time.
> 
> I got your point but I think this requires changes in the
> policy, which I guess need community-wide agreement. Why don't we move
> on with this new option and at the same time start the discussion?

Once we add a CLI option is it is more complicated to remove it again
later. So if we don't actually need it, it is better not to add it in
the first place.

> One more point is, I tried to find out but I couldn't think of any good way
> to distinguish between guest output consoles and other consoles. Do you
> have any thought on this?

There's a helper:

  bool qemu_console_is_graphic(QemuConsole *con);
 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 3/3] ui/gtk: specify detached window's size and location

[Daniel P. Berrangé]

On Tue, May 03, 2022 at 04:33:48PM -0700, Dongwon Kim wrote:
> I saw windows, especially, third and fourth ones are 1/4 size of
> the first when detached regardless of resolutions.
> 
> And the position is also pretty random and detached windows are usually
> placed somewhere on the previous window.
> 
> This patch is to make the sizes same as the original window's and make
> sure all detached windows are not overlapped each other.

In terms of size, I think you need to just honour the surface
size like this:

@@ -1354,6 +1354,9 @@ static void gd_menu_untabify(GtkMenuItem *item, void *opaque)
 
         g_signal_connect(vc->window, "delete-event",
                          G_CALLBACK(gd_tab_window_close), vc);
+        gtk_window_set_default_size(GTK_WINDOW(vc->window),
+                                    surface_width(vc->gfx.ds),
+                                    surface_height(vc->gfx.ds));
         gtk_widget_show_all(vc->window);
 
         if (qemu_console_is_graphic(vc->gfx.dcl.con)) {


for position, I don't think we should be overriding the window
manager placement, as the logic applied could result in us
placing windows off screen.

> 
> On Tue, May 03, 2022 at 10:17:46AM +0100, Daniel P. Berrangé wrote:
> > On Thu, Apr 28, 2022 at 04:13:04PM -0700, Dongwon Kim wrote:
> > > Specify location and size of detached window based on top level
> > > window's location and size info when detachment happens.
> > 
> > Can you explain what problem is being solved by this change ?
> > What's wrong with default size/placement logic ?
> > 
> > In terms of size at least, I would hope we are resizing
> > windows any time the guest changes the resolution of the
> > virtual video adapter.  If there are 2 outputs, they can
> > be at different resolution, so copying the size of the
> > existing window feels wrong - we need to copy the guest
> > resolution currently set.
> > 
> > Why do we need to mess around with position at all ?
> > 
> > > 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>
> > > ---
> > >  ui/gtk.c | 13 +++++++++++++
> > >  1 file changed, 13 insertions(+)
> > > 
> > > diff --git a/ui/gtk.c b/ui/gtk.c
> > > index f1ca6a7275..7dadf3b588 100644
> > > --- a/ui/gtk.c
> > > +++ b/ui/gtk.c
> > > @@ -1338,6 +1338,8 @@ static void gd_menu_untabify(GtkMenuItem *item, void *opaque)
> > >                                         FALSE);
> > >      }
> > >      if (!vc->window) {
> > > +        gint x, y, w, h;
> > > +        int i;
> > >          gtk_widget_set_sensitive(vc->menu_item, false);
> > >          vc->window = gtk_window_new(GTK_WINDOW_TOPLEVEL);
> > >  #if defined(CONFIG_OPENGL)
> > > @@ -1351,7 +1353,18 @@ static void gd_menu_untabify(GtkMenuItem *item, void *opaque)
> > >          }
> > >  #endif
> > >          gd_widget_reparent(s->notebook, vc->window, vc->tab_item);
> > > +        gtk_window_get_position(GTK_WINDOW(s->window), &x, &y);
> > > +        gtk_window_get_size(GTK_WINDOW(s->window), &w, &h);
> > > +
> > > +        for (i = 0; i < s->nb_vcs; i++) {
> > > +            if (vc == &s->vc[i]) {
> > > +                break;
> > > +            }
> > > +        }
> > >  
> > > +        gtk_window_move(GTK_WINDOW(vc->window),
> > > +                        x + w * (i % (s->nb_vcs/2) + 1), y + h * (i / (s->nb_vcs/2)));
> > > +        gtk_window_resize(GTK_WINDOW(vc->window), w, h);
> > >          g_signal_connect(vc->window, "delete-event",
> > >                           G_CALLBACK(gd_tab_window_close), vc);
> > >          gtk_widget_show_all(vc->window);
> > > -- 
> > > 2.30.2
> > > 
> > > 
> > 
> > With regards,
> > Daniel
> > -- 
> > |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> > |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> > |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> > 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 3/3] ui/gtk: specify detached window's size and location

[Dongwon Kim]

On Fri, May 06, 2022 at 05:34:21PM +0100, Daniel P. Berrangé wrote:
> On Tue, May 03, 2022 at 04:33:48PM -0700, Dongwon Kim wrote:
> > I saw windows, especially, third and fourth ones are 1/4 size of
> > the first when detached regardless of resolutions.
> > 
> > And the position is also pretty random and detached windows are usually
> > placed somewhere on the previous window.
> > 
> > This patch is to make the sizes same as the original window's and make
> > sure all detached windows are not overlapped each other.
> 
> In terms of size, I think you need to just honour the surface
> size like this:
> 
> @@ -1354,6 +1354,9 @@ static void gd_menu_untabify(GtkMenuItem *item, void *opaque)
>  
>          g_signal_connect(vc->window, "delete-event",
>                           G_CALLBACK(gd_tab_window_close), vc);
> +        gtk_window_set_default_size(GTK_WINDOW(vc->window),
> +                                    surface_width(vc->gfx.ds),
> +                                    surface_height(vc->gfx.ds));

Thanks for this. I will modify the code and test it.

>          gtk_widget_show_all(vc->window);
>  
>          if (qemu_console_is_graphic(vc->gfx.dcl.con)) {
> 
> 
> for position, I don't think we should be overriding the window
> manager placement, as the logic applied could result in us
> placing windows off screen.

Ok, I think what you are saying makes sense.

> 
> > 
> > On Tue, May 03, 2022 at 10:17:46AM +0100, Daniel P. Berrangé wrote:
> > > On Thu, Apr 28, 2022 at 04:13:04PM -0700, Dongwon Kim wrote:
> > > > Specify location and size of detached window based on top level
> > > > window's location and size info when detachment happens.
> > > 
> > > Can you explain what problem is being solved by this change ?
> > > What's wrong with default size/placement logic ?
> > > 
> > > In terms of size at least, I would hope we are resizing
> > > windows any time the guest changes the resolution of the
> > > virtual video adapter.  If there are 2 outputs, they can
> > > be at different resolution, so copying the size of the
> > > existing window feels wrong - we need to copy the guest
> > > resolution currently set.
> > > 
> > > Why do we need to mess around with position at all ?
> > > 
> > > > 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>
> > > > ---
> > > >  ui/gtk.c | 13 +++++++++++++
> > > >  1 file changed, 13 insertions(+)
> > > > 
> > > > diff --git a/ui/gtk.c b/ui/gtk.c
> > > > index f1ca6a7275..7dadf3b588 100644
> > > > --- a/ui/gtk.c
> > > > +++ b/ui/gtk.c
> > > > @@ -1338,6 +1338,8 @@ static void gd_menu_untabify(GtkMenuItem *item, void *opaque)
> > > >                                         FALSE);
> > > >      }
> > > >      if (!vc->window) {
> > > > +        gint x, y, w, h;
> > > > +        int i;
> > > >          gtk_widget_set_sensitive(vc->menu_item, false);
> > > >          vc->window = gtk_window_new(GTK_WINDOW_TOPLEVEL);
> > > >  #if defined(CONFIG_OPENGL)
> > > > @@ -1351,7 +1353,18 @@ static void gd_menu_untabify(GtkMenuItem *item, void *opaque)
> > > >          }
> > > >  #endif
> > > >          gd_widget_reparent(s->notebook, vc->window, vc->tab_item);
> > > > +        gtk_window_get_position(GTK_WINDOW(s->window), &x, &y);
> > > > +        gtk_window_get_size(GTK_WINDOW(s->window), &w, &h);
> > > > +
> > > > +        for (i = 0; i < s->nb_vcs; i++) {
> > > > +            if (vc == &s->vc[i]) {
> > > > +                break;
> > > > +            }
> > > > +        }
> > > >  
> > > > +        gtk_window_move(GTK_WINDOW(vc->window),
> > > > +                        x + w * (i % (s->nb_vcs/2) + 1), y + h * (i / (s->nb_vcs/2)));
> > > > +        gtk_window_resize(GTK_WINDOW(vc->window), w, h);
> > > >          g_signal_connect(vc->window, "delete-event",
> > > >                           G_CALLBACK(gd_tab_window_close), vc);
> > > >          gtk_widget_show_all(vc->window);
> > > > -- 
> > > > 2.30.2
> > > > 
> > > > 
> > > 
> > > With regards,
> > > Daniel
> > > -- 
> > > |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> > > |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> > > |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> > > 
> > 
> 
> With regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> 

Re: [PATCH 1/3] ui/gtk: new param monitor to specify target monitor for launching QEMU

[Dongwon Kim]

Daniel,

I found a way to make the monitor arguments in array type (['uint32']).
And I know how to retrieve monitor values from it but I could not find
how to pass the monitor values when starting qemu. Like,

qemu-system-x86_64 ..... gtk,gl=on.....monitor=????

I tried several different things but it keeps getting error saying
Invalid parameter type, expected 'array'.

Do you know how to pass this arg?

Thanks,
DW

On Tue, May 03, 2022 at 10:15:13AM +0100, Daniel P. Berrangé wrote:
> On Thu, Apr 28, 2022 at 04:13:02PM -0700, Dongwon Kim wrote:
> > Introducing a new integer parameter to specify the monitor where the
> > Qemu window is placed upon launching.
> > 
> > Monitor can be any number between 0 and (total number of monitors - 1).
> > 
> > It can be used together with full-screen=on, which will make the QEMU
> > window full-screened on the targeted monitor.
> > 
> > v2: fixed typos and updated commit subject and msg
> >     (Philippe Mathieu-Daudé)
> > 
> >     changed param name to monitor, removed unnecessary condition check
> >     on the parameter
> >     (Paolo Bonzini)
> > 
> > v3: updated Since version to 7.1 for monitor parameter
> > 
> > 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    | 6 +++++-
> >  qemu-options.hx | 2 +-
> >  ui/gtk.c        | 8 ++++++++
> >  3 files changed, 14 insertions(+), 2 deletions(-)
> > 
> > diff --git a/qapi/ui.json b/qapi/ui.json
> > index 059302a5ef..ddcea7349b 100644
> > --- a/qapi/ui.json
> > +++ b/qapi/ui.json
> > @@ -1204,13 +1204,17 @@
> >  #               assuming the guest will resize the display to match
> >  #               the window size then.  Otherwise it defaults to "off".
> >  #               Since 3.1
> > +# @monitor:     Indicate monitor where QEMU window is lauched. monitor
> > +#               could be any number from 0 to (total num of monitors - 1).
> > +#               since 7.1
> >  #
> >  # Since: 2.12
> >  #
> >  ##
> >  { 'struct'  : 'DisplayGTK',
> >    'data'    : { '*grab-on-hover' : 'bool',
> > -                '*zoom-to-fit'   : 'bool'  } }
> > +                '*zoom-to-fit'   : 'bool',
> > +                '*monitor'       : 'uint32' } }
> 
> I feel like this ought to be an array of monitors, so that we can have
> explicit positioning when we have multiple graphical outputs and are
> creating a separate window for each.
> 
> 
> With regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> 

Re: [PATCH 1/3] ui/gtk: new param monitor to specify target monitor for launching QEMU

[Gerd Hoffmann]

On Mon, May 09, 2022 at 02:31:05PM -0700, Dongwon Kim wrote:
> Daniel,
> 
> I found a way to make the monitor arguments in array type (['uint32']).
> And I know how to retrieve monitor values from it but I could not find
> how to pass the monitor values when starting qemu. Like,
> 
> qemu-system-x86_64 ..... gtk,gl=on.....monitor=????
> 
> I tried several different things but it keeps getting error saying
> Invalid parameter type, expected 'array'.
> 
> Do you know how to pass this arg?

qemu accepts json for -display, that should work:

-display '{ "type": "gtk", "monitor": [ 1, 2 ] }'

Not sure whenever there is some way to specify arrays using
the -display gtk,$options style.

take care,
  Gerd

Re: [PATCH 1/3] ui/gtk: new param monitor to specify target monitor for launching QEMU

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

> On Mon, May 09, 2022 at 02:31:05PM -0700, Dongwon Kim wrote:
>> Daniel,
>> 
>> I found a way to make the monitor arguments in array type (['uint32']).
>> And I know how to retrieve monitor values from it but I could not find
>> how to pass the monitor values when starting qemu. Like,
>> 
>> qemu-system-x86_64 ..... gtk,gl=on.....monitor=????
>> 
>> I tried several different things but it keeps getting error saying
>> Invalid parameter type, expected 'array'.
>> 
>> Do you know how to pass this arg?
>
> qemu accepts json for -display, that should work:
>
> -display '{ "type": "gtk", "monitor": [ 1, 2 ] }'
>
> Not sure whenever there is some way to specify arrays using
> the -display gtk,$options style.

There is, but it's somewhat ugly:

    -display gtk,monitor.0=1,monitor.1=2

See util/keyval.c.

Re: [PATCH 0/3] ui/gtk: new options, monitor and detach-all

[Dongwon Kim]

Dropping this.
New series with suggested changes from reviewers can be found at
https://lists.nongnu.org/archive/html/qemu-devel/2022-05/msg06245.html

On Thu, Apr 28, 2022 at 04:13:01PM -0700, Dongwon Kim wrote:
> This patch series introduces two new gtk optional parameters, monitor
> and detach-all. "monitor" is for specifying a display where QEMU window
> will be launched from. "detach-all" is making all VCs detached upon
> QEMU's launch. The use-case we originally wanted to deal with is when
> multiple displays (max_output = n) are assigned to the guest os and
> each guest display needs to be full-screened to certain physical monitor.
> This can be achieved when these new parameters are properly configured
> together with existing full-screen option set to true.
> 
> Dongwon Kim (3):
>   ui/gtk: new param monitor to specify target monitor for launching QEMU
>   ui/gtk: detach_all option for making all VCs detached upon starting
>   ui/gtk: specify detached window's size and location
> 
>  qapi/ui.json    | 11 +++++++--
>  qemu-options.hx |  2 +-
>  ui/gtk.c        | 66 +++++++++++++++++++++++++++++++++++++++++++++++--
>  3 files changed, 74 insertions(+), 5 deletions(-)
> 
> -- 
> 2.30.2
>