* [PATCH v5] ui/gtk: New -display gtk option 'full-screen-on-monitor'.
@ 2021-06-25 8:24 Khor, Swee Aun
2021-07-08 15:48 ` Markus Armbruster
0 siblings, 1 reply; 3+ messages in thread
From: Khor, Swee Aun @ 2021-06-25 8:24 UTC (permalink / raw)
To: qemu-devel
Cc: swee.aun.khor, khairul.anuar.romli, Hazwan.Arif.Mazlan,
vivek.kasireddy, armbru, kraxel, eblake
This lets user select monitor number to display QEMU in full screen
with -display gtk,full-screen-on-monitor=<value>.
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/.
Signed-off-by: Khor Swee Aun <swee.aun.khor@intel.com>
---
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.
# 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"
#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);
+ }
+ }
}
static void early_gtk_display_init(DisplayOptions *opts)
--
2.24.3
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v5] ui/gtk: New -display gtk option 'full-screen-on-monitor'.
2021-06-25 8:24 [PATCH v5] ui/gtk: New -display gtk option 'full-screen-on-monitor' Khor, Swee Aun
@ 2021-07-08 15:48 ` Markus Armbruster
2021-07-28 3:19 ` Romli, Khairul Anuar
0 siblings, 1 reply; 3+ messages in thread
From: Markus Armbruster @ 2021-07-08 15:48 UTC (permalink / raw)
To: Khor, Swee Aun
Cc: khairul.anuar.romli, Hazwan.Arif.Mazlan, qemu-devel,
vivek.kasireddy, kraxel, eblake
"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?
^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: [PATCH v5] ui/gtk: New -display gtk option 'full-screen-on-monitor'.
2021-07-08 15:48 ` Markus Armbruster
@ 2021-07-28 3:19 ` Romli, Khairul Anuar
0 siblings, 0 replies; 3+ messages in thread
From: Romli, Khairul Anuar @ 2021-07-28 3:19 UTC (permalink / raw)
To: Markus Armbruster, Khor, Swee Aun, Gerd Hoffmann
Cc: Mazlan, Hazwan Arif, eblake, qemu-devel, Kasireddy, Vivek
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?
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-07-28 3:21 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-25 8:24 [PATCH v5] ui/gtk: New -display gtk option 'full-screen-on-monitor' Khor, Swee Aun
2021-07-08 15:48 ` Markus Armbruster
2021-07-28 3:19 ` Romli, Khairul Anuar
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.