[spice-gtk] widget: Send all smooth-scroll events to the guest

Submitted by Victor Toso on Sept. 14, 2018, 12:25 p.m.

Details

Message ID 20180914122539.24450-1-victortoso@redhat.com
State New
Headers show
Series "widget: Send all smooth-scroll events to the guest" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Victor Toso Sept. 14, 2018, 12:25 p.m.
From: Victor Toso <me@victortoso.com>

Introduced in 2212f05145c5f1d5, smooth-scroll events were taking
inconsideration that the delta_y value received by GdkEventScroll was
1.0 but that can be different for other input devices.

That can trigger the bug rhbz#1627823 where scroll->delta_y is too
small, not triggering the while(ABS(d->scroll_delta_y) > 1) loop
leading to scroll meetings being missed in the guest.

Instead, let's send every event to the guest besides delta_y of 0.0
value that is sent in my machine after several events at once, related
to usage of 2-fingers on trackpad.

Resolve: https://bugzilla.redhat.com/show_bug.cgi?id=1627823
Signed-off-by: Victor Toso <victortoso@redhat.com>
---
 src/spice-widget-priv.h |  1 -
 src/spice-widget.c      | 16 +++++++---------
 2 files changed, 7 insertions(+), 10 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/spice-widget-priv.h b/src/spice-widget-priv.h
index 96f6c1d..30a59d8 100644
--- a/src/spice-widget-priv.h
+++ b/src/spice-widget-priv.h
@@ -148,7 +148,6 @@  struct _SpiceDisplayPrivate {
         SpiceGlScanout      scanout;
     } egl;
 #endif // HAVE_EGL
-    double scroll_delta_y;
 };
 
 int      spice_cairo_image_create                 (SpiceDisplay *display);
diff --git a/src/spice-widget.c b/src/spice-widget.c
index 853c9df..83d264f 100644
--- a/src/spice-widget.c
+++ b/src/spice-widget.c
@@ -1978,15 +1978,13 @@  static gboolean scroll_event(GtkWidget *widget, GdkEventScroll *scroll)
         press_and_release(display, SPICE_MOUSE_BUTTON_DOWN, button_state);
         break;
     case GDK_SCROLL_SMOOTH:
-        d->scroll_delta_y += scroll->delta_y;
-        while (ABS(d->scroll_delta_y) > 1) {
-            if (d->scroll_delta_y < 0) {
-                press_and_release(display, SPICE_MOUSE_BUTTON_UP, button_state);
-                d->scroll_delta_y += 1;
-            } else {
-                press_and_release(display, SPICE_MOUSE_BUTTON_DOWN, button_state);
-                d->scroll_delta_y -= 1;
-            }
+        if (scroll->delta_y == 0) {
+            /* Ignore zero delta */
+            return true;
+        } else if (scroll->delta_y < 0) {
+            press_and_release(display, SPICE_MOUSE_BUTTON_UP, button_state);
+        } else {
+            press_and_release(display, SPICE_MOUSE_BUTTON_DOWN, button_state);
         }
         break;
     default:

Comments

On Fri, Sep 14, 2018 at 02:25:39PM +0200, Victor Toso wrote:
> From: Victor Toso <me@victortoso.com>
> 
> Introduced in 2212f05145c5f1d5, smooth-scroll events were taking
> inconsideration that the delta_y value received by GdkEventScroll was
> 1.0 but that can be different for other input devices.
> 
> That can trigger the bug rhbz#1627823 where scroll->delta_y is too
> small, not triggering the while(ABS(d->scroll_delta_y) > 1) loop
> leading to scroll meetings being missed in the guest.

s/meetings/events/ I guess.

> 
> Instead, let's send every event to the guest besides delta_y of 0.0
> value that is sent in my machine after several events at once, related
> to usage of 2-fingers on trackpad.

On my laptop with a USB mouse plugged, I get fractional delta_y when I
use the touchpad, but I get a 1/-1 when I use the wheel of the
mouse. Sending a scroll event every time we get a smooth scroll event is
likely to result in too fast scrolling in the VM I would think?

> ---
>  src/spice-widget-priv.h |  1 -
>  src/spice-widget.c      | 16 +++++++---------
>  2 files changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/src/spice-widget.c b/src/spice-widget.c
> index 853c9df..83d264f 100644
> --- a/src/spice-widget.c
> +++ b/src/spice-widget.c
> @@ -1978,15 +1978,13 @@ static gboolean scroll_event(GtkWidget *widget, GdkEventScroll *scroll)
>          press_and_release(display, SPICE_MOUSE_BUTTON_DOWN, button_state);
>          break;
>      case GDK_SCROLL_SMOOTH:
> -        d->scroll_delta_y += scroll->delta_y;
> -        while (ABS(d->scroll_delta_y) > 1) {

Actually, we should be checking for >= 1 here, not >, otherwise we'll
indeed be delaying the emission of the button up/down event.

Christophe
Hi,

On Mon, Sep 17, 2018 at 06:37:08PM +0200, Christophe Fergeau wrote:
> On Fri, Sep 14, 2018 at 02:25:39PM +0200, Victor Toso wrote:
> > From: Victor Toso <me@victortoso.com>
> > 
> > Introduced in 2212f05145c5f1d5, smooth-scroll events were taking
> > inconsideration that the delta_y value received by GdkEventScroll was

s/inconsideration/in consideration/ too

> > 1.0 but that can be different for other input devices.
> > 
> > That can trigger the bug rhbz#1627823 where scroll->delta_y is too
> > small, not triggering the while(ABS(d->scroll_delta_y) > 1) loop
> > leading to scroll meetings being missed in the guest.
> 
> s/meetings/events/ I guess.

Thanks

> > 
> > Instead, let's send every event to the guest besides delta_y of 0.0
> > value that is sent in my machine after several events at once, related
> > to usage of 2-fingers on trackpad.
> 
> On my laptop with a USB mouse plugged, I get fractional delta_y when I
> use the touchpad, but I get a 1/-1 when I use the wheel of the
> mouse.

Same here, yes.

> Sending a scroll event every time we get a smooth scroll event
> is likely to result in too fast scrolling in the VM I would
> think?

Not necessarily, it could slow down the scrolling too.

With this patch, a delta-y of 10+ would send a single button-down
while that, before, we would send 10 button-downs:

current:
    (spicy:6759): GSpice-DEBUG: 06:58:38.028: ../src/spice-widget.c:1984 0:0 scroll:10.792969
    (spicy:6759): GSpice-DEBUG: 06:58:38.028: ../src/spice-widget.c:1989 0:0 BUTTON DOWN

before:
    (spicy:6890): GSpice-DEBUG: 07:02:08.286: ../src/spice-widget.c:1983 0:0 scroll:10.523438
    (spicy:6890): GSpice-DEBUG: 07:02:08.286: ../src/spice-widget.c:1990 0:0 BUTTON DOWN
    (spicy:6890): GSpice-DEBUG: 07:02:08.286: ../src/spice-widget.c:1990 0:0 BUTTON DOWN
    (spicy:6890): GSpice-DEBUG: 07:02:08.287: ../src/spice-widget.c:1990 0:0 BUTTON DOWN
    (spicy:6890): GSpice-DEBUG: 07:02:08.287: ../src/spice-widget.c:1990 0:0 BUTTON DOWN
    (spicy:6890): GSpice-DEBUG: 07:02:08.287: ../src/spice-widget.c:1990 0:0 BUTTON DOWN
    (spicy:6890): GSpice-DEBUG: 07:02:08.287: ../src/spice-widget.c:1990 0:0 BUTTON DOWN
    (spicy:6890): GSpice-DEBUG: 07:02:08.287: ../src/spice-widget.c:1990 0:0 BUTTON DOWN
    (spicy:6890): GSpice-DEBUG: 07:02:08.287: ../src/spice-widget.c:1990 0:0 BUTTON DOWN
    (spicy:6890): GSpice-DEBUG: 07:02:08.287: ../src/spice-widget.c:1990 0:0 BUTTON DOWN
    (spicy:6890): GSpice-DEBUG: 07:02:08.287: ../src/spice-widget.c:1990 0:0 BUTTON DOWN

> > ---
> >  src/spice-widget-priv.h |  1 -
> >  src/spice-widget.c      | 16 +++++++---------
> >  2 files changed, 7 insertions(+), 10 deletions(-)
> > 
> > diff --git a/src/spice-widget.c b/src/spice-widget.c
> > index 853c9df..83d264f 100644
> > --- a/src/spice-widget.c
> > +++ b/src/spice-widget.c
> > @@ -1978,15 +1978,13 @@ static gboolean scroll_event(GtkWidget *widget, GdkEventScroll *scroll)
> >          press_and_release(display, SPICE_MOUSE_BUTTON_DOWN, button_state);
> >          break;
> >      case GDK_SCROLL_SMOOTH:
> > -        d->scroll_delta_y += scroll->delta_y;
> > -        while (ABS(d->scroll_delta_y) > 1) {
> 
> Actually, we should be checking for >= 1 here, not >, otherwise we'll
> indeed be delaying the emission of the button up/down event.

Indeed, that would be enough to fix for a user that uses mouse's
wheel and probably for 2-finger scrolling in the touchpad.

At the time I wrote this patch, I got a bit concern with
scrolling with trackpoint which is very sensitivity with small
delta-y between 0.1 to 0.3 and highest I can see around 3.5.

I'm not sure what is a good solution around this as all of it is
a bit of workaround as we don't send the delta-y to the guest.
I'll send a new patch in the meanwhile to fix the while above.

Thanks for reviewing,
Victor