Re: [PATCH v5] ui/gtk: New -display gtk option 'full-screen-on-monitor'.

[Markus Armbruster]

"Khor, Swee Aun" <swee.aun.khor@intel.com> writes:

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

The part from here ...

> v2:
>  - https://patchew.org/QEMU/20210617020609.18089-1-swee.aun.khor@intel.com/.
>  - Added documentation for new member.
>  - Renamed member name from monitor-num to monitor.
>
> v3:
> - Changed commit message subject.
> - Cleaned up signed-off format.
> - Renamed member name from monitor to full-screen-on-monitor to make clear 
> this option automatically enables full screen.
> - Added more detail documentation to specify full-screen-on-monitor option 
> index started from 1.
> - Added code to check windows has been launched successfully at specified 
> monitor.
>
> v4:
> - Used PRId64 format specifier for int64_t variable in warn_report().
>
> v5:
> - Removed gdk_screen and gdk_monitor variables as it used once only.
> - Fixed issue where v3 and v4 doesn't showing up in https://patchew.org/QEMU/.

... to here should go ...

> Signed-off-by: Khor Swee Aun <swee.aun.khor@intel.com>
> ---

... here.  If nothing else needs to be improved, I hope the maintainer
can take care of this one for you.

>  qapi/ui.json    | 10 +++++++---
>  qemu-options.hx |  2 +-
>  ui/gtk.c        | 30 ++++++++++++++++++++++++++++++
>  3 files changed, 38 insertions(+), 4 deletions(-)
>
> diff --git a/qapi/ui.json b/qapi/ui.json
> index 1052ca9c38..d775c29534 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -1035,13 +1035,17 @@
>  #               assuming the guest will resize the display to match
>  #               the window size then.  Otherwise it defaults to "off".
>  #               Since 3.1
> -#
> +# @full-screen-on-monitor: Monitor number to display QEMU in full screen.
> +#                          Monitor number started from index 1. If total 
> number
> +#                          of monitors is 3, possible values for this option 
> are
> +#                          1, 2 or 3.

This is silently ignored unless 'full-screen': true.  Correct?

>  # Since: 2.12
>  #
>  ##
>  { 'struct'  : 'DisplayGTK',
> -  'data'    : { '*grab-on-hover' : 'bool',
> -                '*zoom-to-fit'   : 'bool'  } }
> +  'data'    : { '*grab-on-hover'          : 'bool',
> +                '*zoom-to-fit'            : 'bool',
> +                '*full-screen-on-monitor' : 'int' } }
>  
>  ##
>  # @DisplayEGLHeadless:
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 14258784b3..29836db663 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][,full-screen-on-monitor=<value>]\n"

Suggest full-screen-on-monitor=<number>.

>  #endif
>  #if defined(CONFIG_VNC)
>      "-display vnc=<display>[,<optargs>]\n"
> diff --git a/ui/gtk.c b/ui/gtk.c
> index 98046f577b..4da904a024 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -2189,6 +2189,8 @@ static void gtk_display_init(DisplayState *ds, 
> DisplayOptions *opts)
>      GdkDisplay *window_display;
>      GtkIconTheme *theme;
>      char *dir;
> +    int monitor_n;
> +    bool monitor_status = false;
>  
>      if (!gtkinit) {
>          fprintf(stderr, "gtk initialization failed\n");
> @@ -2268,6 +2270,34 @@ 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_full_screen_on_monitor) {
> +        monitor_n = gdk_display_get_n_monitors(window_display);
> +
> +        if (opts->u.gtk.full_screen_on_monitor <= monitor_n &&
> +            opts->u.gtk.full_screen_on_monitor > 0) {
> +            gtk_window_fullscreen_on_monitor(GTK_WINDOW(s->window),
> +                gdk_display_get_default_screen(window_display),
> +                opts->u.gtk.full_screen_on_monitor - 1);
> +
> +            if (gdk_display_get_monitor(window_display,
> +                                        opts->u.gtk.full_screen_on_monitor
> +                                        - 1) != NULL) {
> +                monitor_status = true;
> +            }
> +        }
> +        if (monitor_status == false) {
> +            /*
> +             * Calling GDK API to read the number of monitors again in case
> +             * monitor added or removed since last read.
> +             */
> +            monitor_n = gdk_display_get_n_monitors(window_display);
> +            warn_report("Failed to enable full screen on monitor %" PRId64 
> ". "
> +                        "Current total number of monitors is %d, "
> +                        "please verify full-screen-on-monitor option value.",
> +                        opts->u.gtk.full_screen_on_monitor, monitor_n);

I wonder whether the warning should be an error, but that's for the
maintainer to decide.

> +        }
> +    }
>  }
>  
>  static void early_gtk_display_init(DisplayOptions *opts)

Ignorant question: are monitors renumbered when a monitor goes away?

Example: we have three monitors A, B, C, numbered 0, 1, 2 (GTK monitor
numbers start with 0).  Now monitor B goes away.  I figure monitor A
remains number 0.  What about C?  Does its number change from 2 to 1?

I still believe numbering monitors 1, 2, 3 instead of 0, 1, 2 in QMP is
a bad idea.  The unnecessary difference to GTK's monitor numbers is
bound to confuse.  But I'm not the maintainer here.

The code to guard against and detect errors looks highly questionable to
me.  Let me explain.

This is the actual work:

               gtk_window_fullscreen_on_monitor(GTK_WINDOW(s->window),
                   gdk_display_get_default_screen(window_display),
                   opts->u.gtk.full_screen_on_monitor - 1);

The function's documentation advises:

    Note that you shouldn't assume the window is definitely full screen
    afterward.

    You can track the fullscreen state via the "window-state-event"
    signal on GtkWidget.

https://developer.gnome.org/gtk3/stable/GtkWindow.html#gtk-window-fullscreen-on-monitor

You don't follow this advice.  Instead you do:

1. Guard the actual work with "the monitor with the requested number
   exists".  This guard is unreliable, because monitors can go away
   between the guard and the actual work.

2. Check whether the monitor with the requested number still exists
   after the actual work.  This is also unreliable, because the monitor
   can go away between the guard and the actual work, and come back
   between the actual work and the check.

The code to handle the error looks questionable, too.  The number of
monitors reported in the error message can be misleading.  Say the user
asked for monitor 2, and the guard found only 1.  A second monitor
appears between the guard and the error reporting.  Now the current
number of monitors reported in the error has nothing to do with the
actual error.

Is there any particular reason not to use the function the way its
documentation advises?