[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RE: [PATCH v4] ui/gtk: New -display gtk option 'full-screen-on-monitor'.
From: |
Romli, Khairul Anuar |
Subject: |
RE: [PATCH v4] ui/gtk: New -display gtk option 'full-screen-on-monitor'. |
Date: |
Fri, 25 Jun 2021 04:28:26 +0000 |
Hi Swee Aun,
I have some comment on the patch.
> -----Original Message-----
> From: Khor, Swee Aun <swee.aun.khor@intel.com>
> Sent: Thursday, June 24, 2021 4:43 PM
> To: qemu-devel@nongnu.org
> Cc: Khor, Swee Aun <swee.aun.khor@intel.com>; kraxel@redhat.com;
> armbru@redhat.com; eblake@redhat.com; Romli, Khairul Anuar
> <khairul.anuar.romli@intel.com>; Kasireddy, Vivek
> <vivek.kasireddy@intel.com>; Mazlan, Hazwan Arif
> <hazwan.arif.mazlan@intel.com>; Khor
> Subject: [PATCH v4] ui/gtk: New -display gtk option 'full-screen-on-monitor'.
>
> This lets user select monitor number to display QEMU in full screen with -
> display gtk,full-screen-on-monitor=<value>.
>
> v2:
> - Added documentation for new member.
> - Renamed member name from monitor-num to monitor.
>
> v3:
> - Cleaned up commit message subject and 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().
>
> Signed-off-by: Khor, Swee Aun <swee.aun.khor@intel.com>
> ---
> qapi/ui.json | 10 +++++++---
> qemu-options.hx | 2 +-
> ui/gtk.c | 35 +++++++++++++++++++++++++++++++++++
> 3 files changed, 43 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..255f25cabd 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -2189,6 +2189,10 @@ static void gtk_display_init(DisplayState *ds,
> DisplayOptions *opts)
> GdkDisplay *window_display;
> GtkIconTheme *theme;
> char *dir;
> + int monitor_n;
> + GdkScreen *gdk_screen;
> + GdkMonitor *gdk_monitor;
> + bool monitor_status = false;
>
> if (!gtkinit) {
> fprintf(stderr, "gtk initialization failed\n"); @@ -2268,6 +2272,37
> @@
> 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) {
> + gdk_screen = gdk_display_get_default_screen(window_display);
> + gtk_window_fullscreen_on_monitor(GTK_WINDOW(s->window),
> gdk_screen,
> +
> opts->u.gtk.full_screen_on_monitor
> + - 1);
> +
> + gdk_monitor = gdk_display_get_monitor(window_display,
> +
> opts->u.gtk.full_screen_on_monitor
> + - 1);
> + if (gdk_monitor != NULL) {
> + monitor_status = true;
[Romli, Khairul Anuar] Do you think we should use gdk_display_get_monitor
inside the if check against the NULL value rather than using a variable? Indeed
that with cause some code readability difficulty but I don't see gdk_monitor is
being used beyond this check.
> + }
> + }
> +
> + 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
- RE: [PATCH v4] ui/gtk: New -display gtk option 'full-screen-on-monitor'.,
Romli, Khairul Anuar <=