Re: [PATCH] display/gtk: get proper refreshrate - Philippe Mathieu-Daudé

From: "Philippe Mathieu-Daudé" <philmd@redhat.com>
To: Nikola Pavlica <pavlica.nikola@gmail.com>,
	qemu-devel@nongnu.org, Gerd Hoffmann <kraxel@redhat.com>
Subject: Re: [PATCH] display/gtk: get proper refreshrate
Date: Mon, 30 Dec 2019 23:59:20 +0100	[thread overview]
Message-ID: <dc998ea3-4fbe-8ecb-65f5-0af59ddd0d6f@redhat.com> (raw)
In-Reply-To: <8531e41f28df7f4bebe4db257ff95e92f5145afc.camel@gmail.com>

Hi Nikola,

Thanks for your patch!

On 12/30/19 6:28 PM, Nikola Pavlica wrote:
>  From 70c95b18fa056b2dd0ecc202ab517bc775b986da Mon Sep 17 00:00:00 2001
> From: Nikola Pavlica <pavlica.nikola@gmail.com>
> Date: Mon, 30 Dec 2019 18:17:35 +0100
> Subject: [PATCH] display/gtk: get proper refreshrate

Can you describe here the problem you encountered, and how your patch 
fixes it?

> 
> Signed-off-by: Nikola Pavlica <pavlica.nikola@gmail.com>
> ---
>   ui/gtk.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/ui/gtk.c b/ui/gtk.c
> index 692ccc7bbb..7a041457f2 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -2259,6 +2259,11 @@ static void gtk_display_init(DisplayState *ds,
> DisplayOptions *opts)
>           opts->u.gtk.grab_on_hover) {
>           gtk_menu_item_activate(GTK_MENU_ITEM(s->grab_on_hover_item));
>       }
> +
> +    GdkDisplay *display = gdk_display_get_default();

Can we use window_display declared earlier instead?

     window_display = gtk_widget_get_display(s->window);

If you look at the CODING_STYLE.rst file referenced here:
https://wiki.qemu.org/Contribute/SubmitAPatch#Use_the_QEMU_coding_style
It states:

   Declarations
   ============

   Mixed declarations (interleaving statements and declarations
   within blocks) are generally not allowed; declarations should
   be at the beginning of blocks.

So you should move the declaration of both display/monitor variables 
earlier, around line 2192.

> +    GdkMonitor *monitor = gdk_display_get_primary_monitor(display);
> +    vc->gfx.dcl.update_interval = 1000000 /
> +        gdk_monitor_get_refresh_rate(monitor);

Now looking at this line, I think this should be done in the 
gd_vc_gfx_init() function (line 2029, before the 
register_displaychangelistener() call).

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

As suggested on IRC, your patch have more chances to get reviewed if you 
Cc its maintainers. See this help here:
https://wiki.qemu.org/Contribute/SubmitAPatch#CC_the_relevant_maintainer

In this case we get:

$ ./scripts/get_maintainer.pl -f ui/gtk.c
Gerd Hoffmann <kraxel@redhat.com> (odd fixer:Graphics)
qemu-devel@nongnu.org (open list:All patches CC here)

I'm Cc'ing Gerd for you.

Regards,

Phil.