RE: [PATCH v2] ui/gtk: Allow user to select monitor number to display qemu in full screen through new gtk display option

[Khor, Swee Aun]

Hi Markus,
Thanks for include Graphic maintainer and the coding style comments.  Yes, 
sweeaun is my name 😊

For  "full screen" be "full-screen" or even "fullscreen"? These 3 words have 
been being used in QEMU repo, but full-screen mostly used for variable/member.
Thus, I felt full screen should be fine to be used as documentation. What do 
you think?
Example:
"FullScreen" in hw/arm/nseries.c
"toggle full screen" in softmmu/vl.c 
""full-screen" in /qemu-options.hx

For subsequence questions:
" Your new option argument seems to count monitors from 1, while GTK counts 
them from zero.  Why the difference?"
sweeaun: It is due to gtk_window_fullscreen_on_monitor monitor index is started 
from zero. I am not using zero as starting index of new option argument to make 
easier for user. Example, if there is 2 monitors, then argument option can be 
monitor 1 or 2. Last number will matched with total monitors which can avoid 
confusion for user. That is my thought.   

"Your documentation states that @monitor applies only "in full screen", but 
this code is not guarded by if (opts->full_screen).  Why is that okay?"
sweeaun: It doesn’t need to be guarded by if (opts->full_screen), as 
gtk_window_fullscreen_on_monitor will enable the full screen by itself.  

"From gdk_display_get_n_monitors()'s documentation: "The returned number is 
valid until the next emission of the “monitor-added” or “monitor-removed” 
signal."  This suggests monitors can come and go at any time.  If they can, 
what happens when the monitor we're trying to use here has gone away since we 
called gdk_display_get_n_monitors()?"
sweeaun: Based on my observation, when specific monitor device disconnected 
after QEMU launched on it, QEMU application will not be visible but QEMU 
application still running and screen framebuffer size is not being changed at 
all. QEMU application will be visible once you connect back the monitor. 
Yes the return number from gdk_display_get_n_monitors() will minus 1 compared 
to previous once 1 monitor has been disconnected. 

Regards,
SweeAun

-----Original Message-----
From: Markus Armbruster <armbru@redhat.com> 
Sent: Friday, June 18, 2021 7:07 PM
To: Khor, Swee Aun <swee.aun.khor@intel.com>
Cc: qemu-devel@nongnu.org; Romli, Khairul Anuar 
<khairul.anuar.romli@intel.com>; eblake@redhat.com; Kasireddy, Vivek 
<vivek.kasireddy@intel.com>; Gerd Hoffmann <kraxel@redhat.com>
Subject: Re: [PATCH v2] ui/gtk: Allow user to select monitor number to display 
qemu in full screen through new gtk display option

You neglected to cc: the Graphics maintainer.  I'm doing that for you now.

sweeaun <swee.aun.khor@intel.com> writes:

>  -display gtk,monitor=<value>
>
> Signed-off-by: sweeaun <swee.aun.khor@intel.com>

Your commit message is formatted badly.  What about this:

    ui/gtk: New -display gtk parameter 'monitor'.

    This lets the user select monitor number to display QEMU in full
    screen with -display gtk,monitor=<value>.

Furthermore, you're Signed-off-by line may be off.  It should be of the form

    Signed-off-by: REAL NAME <EMAIL>

Is "sweeaun" your real name?

> ---
>  qapi/ui.json    |  4 +++-
>  qemu-options.hx |  2 +-
>  ui/gtk.c        | 15 +++++++++++++++
>  3 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/qapi/ui.json b/qapi/ui.json index 1052ca9c38..1616f3ffbd 
> 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -1035,13 +1035,15 @@
>  #               assuming the guest will resize the display to match
>  #               the window size then.  Otherwise it defaults to "off".
>  #               Since 3.1
> +# @monitor: Monitor number to display qemu in full screen.

We spell it QEMU.

Should "full screen" be "full-screen" or even "fullscreen"?

>  #
>  # Since: 2.12
>  #
>  ##
>  { 'struct'  : 'DisplayGTK',
>    'data'    : { '*grab-on-hover' : 'bool',
> -                '*zoom-to-fit'   : 'bool'  } }
> +                '*zoom-to-fit'   : 'bool',
> +                '*monitor' : 'int' } }

Best to make your addition "blend in" like this

   { 'struct'  : 'DisplayGTK',
     'data'    : { '*grab-on-hover' : 'bool',
                   '*zoom-to-fit'   : 'bool',
                   '*monitor'       : 'int' } }

>  
>  ##
>  # @DisplayEGLHeadless:
> diff --git a/qemu-options.hx b/qemu-options.hx index 
> 14258784b3..e4b89b6a72 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1787,7 +1787,7 @@ DEF("display", HAS_ARG, QEMU_OPTION_display,
>      "            [,window_close=on|off][,gl=on|core|es|off]\n"
>  #endif
>  #if defined(CONFIG_GTK)
> -    "-display gtk[,grab_on_hover=on|off][,gl=on|off]|\n"
> +    "-display gtk[,grab_on_hover=on|off][,gl=on|off][,monitor=<value>]\n"
>  #endif
>  #if defined(CONFIG_VNC)
>      "-display vnc=<display>[,<optargs>]\n"
> diff --git a/ui/gtk.c b/ui/gtk.c
> index 98046f577b..84da126611 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -2268,6 +2268,21 @@ static void gtk_display_init(DisplayState *ds, 
> DisplayOptions *opts)
>          gtk_menu_item_activate(GTK_MENU_ITEM(s->grab_on_hover_item));
>      }
>      gd_clipboard_init(s);
> +
> +    if (opts->u.gtk.has_monitor) {
> +        int n_monitor;
> +        n_monitor = gdk_display_get_n_monitors(window_display);

Terser:

           int n_monitor = gdk_display_get_n_monitors(window_display);

> +
> +        if ((opts->u.gtk.monitor <= n_monitor) &&
> +            (opts->u.gtk.monitor > 0)) {

Suggest to drop the superfluous parenthesis:

           if (opts->u.gtk.monitor <= n_monitor &&
               opts->u.gtk.monitor > 0) {

> +            GdkScreen *gdk_screen;
> +            gdk_screen = gdk_display_get_default_screen(window_display);
> +            gtk_window_fullscreen_on_monitor(GTK_WINDOW(s->window), 
> gdk_screen,
> +                                             (opts->u.gtk.monitor - 
> + 1));

Drop the superfluous parenthesis around the last argument.

Your new option argument seems to count monitors from 1, while GTK counts them 
from zero.  Why the difference?

Your documentation states that @monitor applies only "in full screen", but this 
code is not guarded by if (opts->full_screen).  Why is that okay?

From gdk_display_get_n_monitors()'s documentation: "The returned number is 
valid until the next emission of the “monitor-added” or “monitor-removed” 
signal."  This suggests monitors can come and go at any time.  If they can, 
what happens when the monitor we're trying to use here has gone away since we 
called gdk_display_get_n_monitors()?

> +        } else {
> +            fprintf(stderr, "Invalid GTK monitor argument\n");
> +        }
> +    }
>  }
>  
>  static void early_gtk_display_init(DisplayOptions *opts)