[Spice-devel] [PATCH spice-gtk] widget: handle smooth-scroll events

[Marc-André Lureau]

Hi

On Fri, Jun 8, 2018 at 10:27 AM, Christophe Fergeau <cfergeau at redhat.com> wrote:
> Hey,
>
> On Thu, Jun 07, 2018 at 07:42:24PM +0200, marcandre.lureau at redhat.com wrote:
>> From: Marc-André Lureau <marcandre.lureau at redhat.com>
>>
>> With touchpads and similar devices, scroll events are not emitted on
>> Wayland, but only smooth-scroll events.
>>
>> There is some discussion about handling smooth-scroll events in Spice
>> (see "Add horizontal mouse wheel support" thread), but it will mean
>> support from various components. For compatibility reasons, in the
>> meantime, let's synthetize the smooth-scroll events to regular scroll
>> buttons to fix scrolling.
>
> Indeed, nice idea.. ;)
>
>>
>> Tested on f28, gnome-shell under X and wayland.
>>
>> Related to:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1386602
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau at redhat.com>
>> ---
>>  src/spice-widget-priv.h |  1 +
>>  src/spice-widget.c      | 32 ++++++++++++++++++++++++--------
>>  2 files changed, 25 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/spice-widget-priv.h b/src/spice-widget-priv.h
>> index 1189cbb..ac69b5f 100644
>> --- a/src/spice-widget-priv.h
>> +++ b/src/spice-widget-priv.h
>> @@ -151,6 +151,7 @@ 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 72f5334..a1a2fcd 100644
>> --- a/src/spice-widget.c
>> +++ b/src/spice-widget.c
>> @@ -682,6 +682,8 @@ G_GNUC_END_IGNORE_DEPRECATIONS
>>                            GDK_ENTER_NOTIFY_MASK |
>>                            GDK_LEAVE_NOTIFY_MASK |
>>                            GDK_KEY_PRESS_MASK |
>> +                          /* on Wayland, w get only smooth scroll events */
>
> w -> we
>
>> +                          GDK_SMOOTH_SCROLL_MASK |
>>                            GDK_SCROLL_MASK);
>>      gtk_widget_set_can_focus(widget, true);
>>      gtk_event_box_set_above_child(GTK_EVENT_BOX(widget), true);
>> @@ -2005,9 +2007,17 @@ static gboolean motion_event(GtkWidget *widget, GdkEventMotion *motion)
>>      return true;
>>  }
>>
>> +static void press_and_release(SpiceDisplay *display,
>> +                              gint button, gint button_state)
>> +{
>> +    SpiceDisplayPrivate *d = display->priv;
>> +
>> +    spice_inputs_channel_button_press(d->inputs, button, button_state);
>> +    spice_inputs_channel_button_release(d->inputs, button, button_state);
>> +}
>> +
>>  static gboolean scroll_event(GtkWidget *widget, GdkEventScroll *scroll)
>>  {
>> -    int button;
>>      SpiceDisplay *display = SPICE_DISPLAY(widget);
>>      SpiceDisplayPrivate *d = display->priv;
>>
>> @@ -2019,18 +2029,24 @@ static gboolean scroll_event(GtkWidget *widget, GdkEventScroll *scroll)
>>          return true;
>>
>>      if (scroll->direction == GDK_SCROLL_UP)
>> -        button = SPICE_MOUSE_BUTTON_UP;
>> +        press_and_release(display, SPICE_MOUSE_BUTTON_UP,
>> +                          button_mask_gdk_to_spice(scroll->state));
>>      else if (scroll->direction == GDK_SCROLL_DOWN)
>> -        button = SPICE_MOUSE_BUTTON_DOWN;
>> -    else {
>> +        press_and_release(display, SPICE_MOUSE_BUTTON_DOWN,
>> +                          button_mask_gdk_to_spice(scroll->state));
>> +    else if (scroll->direction == GDK_SCROLL_SMOOTH) {
>> +        d->scroll_delta_y += scroll->delta_y;
>> +        while (ABS(d->scroll_delta_y) > 1) {
>> +            press_and_release(display,
>> +                              d->scroll_delta_y < 0 ? SPICE_MOUSE_BUTTON_UP : SPICE_MOUSE_BUTTON_DOWN,
>> +                              button_mask_gdk_to_spice(scroll->state));
>> +            d->scroll_delta_y += d->scroll_delta_y < 0 ? 1 : -1;
>
> A regular if () would be better for readibility in my opinion.
>

ok

>> +        }
>
> So a change of 1 on the y axis corresponds to 1 press of the button, is
> this arbitrary, or is there some rational for that?

I couldn't find documentation about the unit. But after some testing,
it seems like 1 maps well to a button event...

>
>> +    } else {
>>          DISPLAY_DEBUG(display, "unsupported scroll direction");
>
> We could try to get there on GDK_SCROLL_SMOOTH when delta_y is 0 in the
> event, but it's not really important.
>
>>          return true;
>
> That 'return true' is now redundant with the one just below.

ok, thanks for the review, v2 on the way

>
> Christophe
>
>>      }
>>
>> -    spice_inputs_channel_button_press(d->inputs, button,
>> -                                      button_mask_gdk_to_spice(scroll->state));
>> -    spice_inputs_channel_button_release(d->inputs, button,
>> -                                        button_mask_gdk_to_spice(scroll->state));
>>      return true;
>>  }
>>
>>
>> base-commit: e5ece54aca21ae7c4475a8cfebc74d40b6aea44a
>> --
>> 2.18.0.rc1.1.gae296d1cf5
>>
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>



-- 
Marc-André Lureau