| Message ID | 20230320160856.364319-1-ernunes@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] ui/gtk: use widget size for cursor motion event | expand |
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 >
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
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 >
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.
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
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) {
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(-)