[PATCH 1/2] ui/gtk: use widget size for cursor motion event

[PATCH 1/2] ui/gtk: use widget size for cursor motion event

[Erico Nunes]

The gd_motion_event size has some calculations for the cursor position,
which also take into account things like different size of the
framebuffer compared to the window size.
The use of window size makes things more difficult though, as at least
in the case of Wayland includes the size of ui elements like a menu bar
at the top of the window. This leads to a wrong position calculation by
a few pixels.
Fix it by using the size of the widget, which already returns the size
of the actual space to render the framebuffer.

Signed-off-by: Erico Nunes <ernunes@redhat.com>
---
 ui/gtk.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/ui/gtk.c b/ui/gtk.c
index fd82e9b1ca..d1b2a80c2b 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -868,7 +868,6 @@ static gboolean gd_motion_event(GtkWidget *widget, GdkEventMotion *motion,
 {
     VirtualConsole *vc = opaque;
     GtkDisplayState *s = vc->s;
-    GdkWindow *window;
     int x, y;
     int mx, my;
     int fbh, fbw;
@@ -881,10 +880,9 @@ static gboolean gd_motion_event(GtkWidget *widget, GdkEventMotion *motion,
     fbw = surface_width(vc->gfx.ds) * vc->gfx.scale_x;
     fbh = surface_height(vc->gfx.ds) * vc->gfx.scale_y;
 
-    window = gtk_widget_get_window(vc->gfx.drawing_area);
-    ww = gdk_window_get_width(window);
-    wh = gdk_window_get_height(window);
-    ws = gdk_window_get_scale_factor(window);
+    ww = gtk_widget_get_allocated_width(widget);
+    wh = gtk_widget_get_allocated_height(widget);
+    ws = gtk_widget_get_scale_factor(widget);
 
     mx = my = 0;
     if (ww > fbw) {
-- 
2.39.2

[PATCH 2/2] ui/gtk-egl: fix scaling for cursor position in scanout mode

[Erico Nunes]

vc->gfx.w and vc->gfx.h are not updated appropriately in this code path,
which leads to a different scaling factor for rendering the cursor on
some edge cases (e.g. the focus has left and re-entered the gtk window).
This can be reproduced using vhost-user-gpu with the gtk ui on the x11
backend.
Use the surface dimensions which are already updated accordingly.

Signed-off-by: Erico Nunes <ernunes@redhat.com>
---
 ui/gtk-egl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c
index e84431790c..a4422be837 100644
--- a/ui/gtk-egl.c
+++ b/ui/gtk-egl.c
@@ -88,8 +88,8 @@ void gd_egl_draw(VirtualConsole *vc)
 #endif
         gd_egl_scanout_flush(&vc->gfx.dcl, 0, 0, vc->gfx.w, vc->gfx.h);
 
-        vc->gfx.scale_x = (double)ww / vc->gfx.w;
-        vc->gfx.scale_y = (double)wh / vc->gfx.h;
+        vc->gfx.scale_x = (double)ww / surface_width(vc->gfx.ds);
+        vc->gfx.scale_y = (double)wh / surface_height(vc->gfx.ds);
 
         glFlush();
 #ifdef CONFIG_GBM
-- 
2.39.2

RE: [PATCH 1/2] ui/gtk: use widget size for cursor motion event

[Kasireddy, Vivek]

Hi Erico,

> 
> The gd_motion_event size has some calculations for the cursor position,
> which also take into account things like different size of the
> framebuffer compared to the window size.
> The use of window size makes things more difficult though, as at least
> in the case of Wayland includes the size of ui elements like a menu bar
> at the top of the window. This leads to a wrong position calculation by
> a few pixels.
> Fix it by using the size of the widget, which already returns the size
> of the actual space to render the framebuffer.
> 
> Signed-off-by: Erico Nunes <ernunes@redhat.com>
> ---
>  ui/gtk.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/ui/gtk.c b/ui/gtk.c
> index fd82e9b1ca..d1b2a80c2b 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -868,7 +868,6 @@ static gboolean gd_motion_event(GtkWidget *widget,
> GdkEventMotion *motion,
>  {
>      VirtualConsole *vc = opaque;
>      GtkDisplayState *s = vc->s;
> -    GdkWindow *window;
>      int x, y;
>      int mx, my;
>      int fbh, fbw;
> @@ -881,10 +880,9 @@ static gboolean gd_motion_event(GtkWidget *widget,
> GdkEventMotion *motion,
>      fbw = surface_width(vc->gfx.ds) * vc->gfx.scale_x;
>      fbh = surface_height(vc->gfx.ds) * vc->gfx.scale_y;
> 
> -    window = gtk_widget_get_window(vc->gfx.drawing_area);
> -    ww = gdk_window_get_width(window);
> -    wh = gdk_window_get_height(window);
> -    ws = gdk_window_get_scale_factor(window);
> +    ww = gtk_widget_get_allocated_width(widget);
> +    wh = gtk_widget_get_allocated_height(widget);
[Kasireddy, Vivek] Could you please confirm if this works in X-based compositor
environments as well? Last time I checked (with Fedora 36 and Gnome + X), the
get_allocated_xxx APIs were not accurate in X-based environments. Therefore,
I restricted the above change to Wayland-based environments only:
https://lists.nongnu.org/archive/html/qemu-devel/2022-11/msg03100.html


Thanks,
Vivek

> +    ws = gtk_widget_get_scale_factor(widget);
> 
>      mx = my = 0;
>      if (ww > fbw) {
> --
> 2.39.2
> 

Re: [PATCH 1/2] ui/gtk: use widget size for cursor motion event

[Erico Nunes]

Hi Vivek,

On 21/03/2023 04:29, Kasireddy, Vivek wrote:
> Hi Erico,
> 
>>
>> The gd_motion_event size has some calculations for the cursor position,
>> which also take into account things like different size of the
>> framebuffer compared to the window size.
>> The use of window size makes things more difficult though, as at least
>> in the case of Wayland includes the size of ui elements like a menu bar
>> at the top of the window. This leads to a wrong position calculation by
>> a few pixels.
>> Fix it by using the size of the widget, which already returns the size
>> of the actual space to render the framebuffer.
>>
>> Signed-off-by: Erico Nunes <ernunes@redhat.com>
>> ---
>>  ui/gtk.c | 8 +++-----
>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/ui/gtk.c b/ui/gtk.c
>> index fd82e9b1ca..d1b2a80c2b 100644
>> --- a/ui/gtk.c
>> +++ b/ui/gtk.c
>> @@ -868,7 +868,6 @@ static gboolean gd_motion_event(GtkWidget *widget,
>> GdkEventMotion *motion,
>>  {
>>      VirtualConsole *vc = opaque;
>>      GtkDisplayState *s = vc->s;
>> -    GdkWindow *window;
>>      int x, y;
>>      int mx, my;
>>      int fbh, fbw;
>> @@ -881,10 +880,9 @@ static gboolean gd_motion_event(GtkWidget *widget,
>> GdkEventMotion *motion,
>>      fbw = surface_width(vc->gfx.ds) * vc->gfx.scale_x;
>>      fbh = surface_height(vc->gfx.ds) * vc->gfx.scale_y;
>>
>> -    window = gtk_widget_get_window(vc->gfx.drawing_area);
>> -    ww = gdk_window_get_width(window);
>> -    wh = gdk_window_get_height(window);
>> -    ws = gdk_window_get_scale_factor(window);
>> +    ww = gtk_widget_get_allocated_width(widget);
>> +    wh = gtk_widget_get_allocated_height(widget);
> [Kasireddy, Vivek] Could you please confirm if this works in X-based compositor
> environments as well? Last time I checked (with Fedora 36 and Gnome + X), the
> get_allocated_xxx APIs were not accurate in X-based environments. Therefore,
> I restricted the above change to Wayland-based environments only:
> https://lists.nongnu.org/archive/html/qemu-devel/2022-11/msg03100.html

Yes, I tested again and it seems to work fine for me even with the gtk
ui running on X. I'm using Fedora 37.

I was not aware of that patch series though and just spent some time
debugging these ui issues. It looks like your series was missed?

I'm still debugging additional issues with cursor position calculation,
especially in wayland environments (and in particular with
vhost-user-gpu now). Do those patches address more cursor issues?

Thank you

Erico

RE: [PATCH 1/2] ui/gtk: use widget size for cursor motion event

[Kasireddy, Vivek]

Hi Erico,

> >
> >>
> >> The gd_motion_event size has some calculations for the cursor position,
> >> which also take into account things like different size of the
> >> framebuffer compared to the window size.
> >> The use of window size makes things more difficult though, as at least
> >> in the case of Wayland includes the size of ui elements like a menu bar
> >> at the top of the window. This leads to a wrong position calculation by
> >> a few pixels.
> >> Fix it by using the size of the widget, which already returns the size
> >> of the actual space to render the framebuffer.
> >>
> >> Signed-off-by: Erico Nunes <ernunes@redhat.com>
> >> ---
> >>  ui/gtk.c | 8 +++-----
> >>  1 file changed, 3 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/ui/gtk.c b/ui/gtk.c
> >> index fd82e9b1ca..d1b2a80c2b 100644
> >> --- a/ui/gtk.c
> >> +++ b/ui/gtk.c
> >> @@ -868,7 +868,6 @@ static gboolean gd_motion_event(GtkWidget *widget,
> >> GdkEventMotion *motion,
> >>  {
> >>      VirtualConsole *vc = opaque;
> >>      GtkDisplayState *s = vc->s;
> >> -    GdkWindow *window;
> >>      int x, y;
> >>      int mx, my;
> >>      int fbh, fbw;
> >> @@ -881,10 +880,9 @@ static gboolean gd_motion_event(GtkWidget
> *widget,
> >> GdkEventMotion *motion,
> >>      fbw = surface_width(vc->gfx.ds) * vc->gfx.scale_x;
> >>      fbh = surface_height(vc->gfx.ds) * vc->gfx.scale_y;
> >>
> >> -    window = gtk_widget_get_window(vc->gfx.drawing_area);
> >> -    ww = gdk_window_get_width(window);
> >> -    wh = gdk_window_get_height(window);
> >> -    ws = gdk_window_get_scale_factor(window);
> >> +    ww = gtk_widget_get_allocated_width(widget);
> >> +    wh = gtk_widget_get_allocated_height(widget);
> > [Kasireddy, Vivek] Could you please confirm if this works in X-based compositor
> > environments as well? Last time I checked (with Fedora 36 and Gnome + X), the
> > get_allocated_xxx APIs were not accurate in X-based environments. Therefore,
> > I restricted the above change to Wayland-based environments only:
> > https://lists.nongnu.org/archive/html/qemu-devel/2022-11/msg03100.html
> 
> Yes, I tested again and it seems to work fine for me even with the gtk
> ui running on X. I'm using Fedora 37.
[Kasireddy, Vivek] Ok, in that case, this patch is 
Acked-by: Vivek Kasireddy <vivek.kasireddy@intel.com>

> 
> I was not aware of that patch series though and just spent some time
> debugging these ui issues. It looks like your series was missed?
[Kasireddy, Vivek] Yeah, not sure why my series was missed but in 
retrospect, I probably should have separated out bug fix patches
from new feature enablement patches.

> 
> I'm still debugging additional issues with cursor position calculation,
> especially in wayland environments (and in particular with
> vhost-user-gpu now). Do those patches address more cursor issues?
[Kasireddy, Vivek] They do address more cursor issues but not sure how
helpful they would be for you as most of them deal with relative mode +
Wayland environment. However, there is another one that deals with
cursor/pointer in absolute mode + multiple monitors:
https://lists.nongnu.org/archive/html/qemu-devel/2022-11/msg03097.html

Thanks,
Vivek
> 
> Thank you
> 
> Erico
> 

Re: [PATCH 1/2] ui/gtk: use widget size for cursor motion event

[Marc-André Lureau]

Hi Erico

On Thu, Mar 23, 2023 at 9:02 AM Kasireddy, Vivek
<vivek.kasireddy@intel.com> wrote:
>
> Hi Erico,
>
> > >
> > >>
> > >> The gd_motion_event size has some calculations for the cursor position,
> > >> which also take into account things like different size of the
> > >> framebuffer compared to the window size.
> > >> The use of window size makes things more difficult though, as at least
> > >> in the case of Wayland includes the size of ui elements like a menu bar
> > >> at the top of the window. This leads to a wrong position calculation by
> > >> a few pixels.
> > >> Fix it by using the size of the widget, which already returns the size
> > >> of the actual space to render the framebuffer.
> > >>
> > >> Signed-off-by: Erico Nunes <ernunes@redhat.com>
> > >> ---
> > >>  ui/gtk.c | 8 +++-----
> > >>  1 file changed, 3 insertions(+), 5 deletions(-)
> > >>
> > >> diff --git a/ui/gtk.c b/ui/gtk.c
> > >> index fd82e9b1ca..d1b2a80c2b 100644
> > >> --- a/ui/gtk.c
> > >> +++ b/ui/gtk.c
> > >> @@ -868,7 +868,6 @@ static gboolean gd_motion_event(GtkWidget *widget,
> > >> GdkEventMotion *motion,
> > >>  {
> > >>      VirtualConsole *vc = opaque;
> > >>      GtkDisplayState *s = vc->s;
> > >> -    GdkWindow *window;
> > >>      int x, y;
> > >>      int mx, my;
> > >>      int fbh, fbw;
> > >> @@ -881,10 +880,9 @@ static gboolean gd_motion_event(GtkWidget
> > *widget,
> > >> GdkEventMotion *motion,
> > >>      fbw = surface_width(vc->gfx.ds) * vc->gfx.scale_x;
> > >>      fbh = surface_height(vc->gfx.ds) * vc->gfx.scale_y;
> > >>
> > >> -    window = gtk_widget_get_window(vc->gfx.drawing_area);
> > >> -    ww = gdk_window_get_width(window);
> > >> -    wh = gdk_window_get_height(window);
> > >> -    ws = gdk_window_get_scale_factor(window);
> > >> +    ww = gtk_widget_get_allocated_width(widget);
> > >> +    wh = gtk_widget_get_allocated_height(widget);
> > > [Kasireddy, Vivek] Could you please confirm if this works in X-based compositor
> > > environments as well? Last time I checked (with Fedora 36 and Gnome + X), the
> > > get_allocated_xxx APIs were not accurate in X-based environments. Therefore,
> > > I restricted the above change to Wayland-based environments only:
> > > https://lists.nongnu.org/archive/html/qemu-devel/2022-11/msg03100.html
> >
> > Yes, I tested again and it seems to work fine for me even with the gtk
> > ui running on X. I'm using Fedora 37.
> [Kasireddy, Vivek] Ok, in that case, this patch is
> Acked-by: Vivek Kasireddy <vivek.kasireddy@intel.com>
>
> >
> > I was not aware of that patch series though and just spent some time
> > debugging these ui issues. It looks like your series was missed?
> [Kasireddy, Vivek] Yeah, not sure why my series was missed but in
> retrospect, I probably should have separated out bug fix patches
> from new feature enablement patches.
>
> >
> > I'm still debugging additional issues with cursor position calculation,
> > especially in wayland environments (and in particular with
> > vhost-user-gpu now). Do those patches address more cursor issues?
> [Kasireddy, Vivek] They do address more cursor issues but not sure how
> helpful they would be for you as most of them deal with relative mode +
> Wayland environment. However, there is another one that deals with
> cursor/pointer in absolute mode + multiple monitors:
> https://lists.nongnu.org/archive/html/qemu-devel/2022-11/msg03097.html
>

Should we queue the 2 patches from this series? (note that they were
not correctly handled by patchew, probably because you dropped the
cover letter).

For me -display gtk is unusable on hidpi & wayland anyway, because the
cursor position given to the guest does not match the dimensions given
for the monitor.

Also relative mouse support is broken as well (mouse wrapping and
confinement/grab is not supported by gdk/gtk on wayland).

I am not actively looking at these problems, they are "solved" with
spice (use -display spice-app). And I am also regularly working on a
gtk4/rust widget, using -display dbus
(https://gitlab.gnome.org/malureau/rdw). There is also
https://gitlab.gnome.org/chergert/libmks as a gtk4/C alternative. I am
not sure we should keep maintaining the gtk3 backend going forward.
And as a Gtk4/-display dbus client mature, I hope we can offer a
better alternative than ui/sdl or ui/cocoa on other platforms as well.


-- 
Marc-André Lureau

Re: [PATCH 1/2] ui/gtk: use widget size for cursor motion event

[Erico Nunes]

On 23/03/2023 15:41, Marc-André Lureau wrote:
> Should we queue the 2 patches from this series? (note that they were
> not correctly handled by patchew, probably because you dropped the
> cover letter).
> 
> For me -display gtk is unusable on hidpi & wayland anyway, because the
> cursor position given to the guest does not match the dimensions given
> for the monitor.
> 
> Also relative mouse support is broken as well (mouse wrapping and
> confinement/grab is not supported by gdk/gtk on wayland).
> 
> I am not actively looking at these problems, they are "solved" with
> spice (use -display spice-app). And I am also regularly working on a
> gtk4/rust widget, using -display dbus
> (https://gitlab.gnome.org/malureau/rdw). There is also
> https://gitlab.gnome.org/chergert/libmks as a gtk4/C alternative. I am
> not sure we should keep maintaining the gtk3 backend going forward.
> And as a Gtk4/-display dbus client mature, I hope we can offer a
> better alternative than ui/sdl or ui/cocoa on other platforms as well.

To answer this: well since we already have the fixes, if it gets reviews
I think we could merge them anyway.

But realizing that the built-in gtk UI is in this best-effort state, I
probably won't be spending much time on it anymore either.
It seems to be more productive to focus the effort on improving the
experience with the dbus backend and its UIs going forward, then.

Thanks

Erico