Hi Gerd, Could you add comment or review on this patch?
Thank You. Best Regards, Khairul > -----Original Message----- > From: Markus Armbruster <arm...@redhat.com> > Sent: Thursday, July 8, 2021 11:49 PM > To: Khor, Swee Aun <swee.aun.k...@intel.com> > Cc: qemu-devel@nongnu.org; Romli, Khairul Anuar > <khairul.anuar.ro...@intel.com>; Mazlan, Hazwan Arif > <hazwan.arif.maz...@intel.com>; Kasireddy, Vivek > <vivek.kasire...@intel.com>; kra...@redhat.com; ebl...@redhat.com > Subject: Re: [PATCH v5] ui/gtk: New -display gtk option 'full-screen-on- > monitor'. > > "Khor, Swee Aun" <swee.aun.k...@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.k...@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.k...@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?
