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

[Romli, Khairul Anuar]

Hi Gerd,

Could you add comment or review on this patch? 

Thank You.

Best Regards,
Khairul 

> -----Original Message-----
> From: Markus Armbruster <armbru@redhat.com>
> Sent: Thursday, July 8, 2021 11:49 PM
> To: Khor, Swee Aun <swee.aun.khor@intel.com>
> Cc: qemu-devel@nongnu.org; Romli, Khairul Anuar
> <khairul.anuar.romli@intel.com>; Mazlan, Hazwan Arif
> <hazwan.arif.mazlan@intel.com>; Kasireddy, Vivek
> <vivek.kasireddy@intel.com>; kraxel@redhat.com; eblake@redhat.com
> Subject: Re: [PATCH v5] ui/gtk: New -display gtk option 'full-screen-on-
> monitor'.
> 
> "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?