Created attachment 210115 [details] [review]
st: Add rudimentary support for CLUTTER_SCROLL_SMOOTH events

Currently compilation fails with -Werror, as we don't handle the
(newly introduced) smooth scroll events in switch statements; add
some basic support, which should make the compiler happy.

Review of attachment 210115 [details] [review]:

::: src/st/st-scroll-bar.c
@@ +466,3 @@
+                                      &delta_x, &delta_y);
+      if (value == lower || value == upper)
+        return FALSE;

I know it matches what was there before but this has no point and seems quite wrong to me that we'd mark the event handled until the scroll region hits the end then stop handling the event and let the parent handle it.

The only thing I can think of is that the code is trying to handle the case of a not-scrollable scrolled area and let the event pass through to a parent scrollable area, but that seems really fiddly to me. upper isn't even right since what the range of an adjustment is:

 MAX (priv->lower, priv->upper - priv->page_size)

so in one direction the current code is just busted. Just rely on the clamping in st_adjustment_set_value() I think.

@@ +467,3 @@
+      if (value == lower || value == upper)
+        return FALSE;
+      else if (delta_x > delta_y)

Need to be comparing fabs(delta) right?

::: src/st/st-scroll-view.c
@@ +699,3 @@
+      clutter_event_get_scroll_delta ((ClutterEvent *)event,
+                                      &delta_x, &delta_y);
+      g_object_get (delta_x > delta_y ? priv->hadjustment : priv->vadjustment,

also missing a fabs. 

You could possible do 'adjustment = ; delta = ;' to avoid the duplicated calls to g_object_get() and st_adjustment_set_value(), etc, but maybe it would be better to scroll the hadjustment the delta_x and the vadjusment by the delta_y? Is only one supposed to be non-zero?

Created attachment 210123 [details] [review]
st: Add rudimentary support for CLUTTER_SCROLL_SMOOTH events

(In reply to comment #2)
> I know it matches what was there before but this has no point and seems quite
> wrong to me that we'd mark the event handled until the scroll region hits the
> end then stop handling the event and let the parent handle it.
> 
> The only thing I can think of is that the code is trying to handle the case of
> a not-scrollable scrolled area and let the event pass through to a parent
> scrollable area, but that seems really fiddly to me. upper isn't even right
> since what the range of an adjustment is:
> 
>  MAX (priv->lower, priv->upper - priv->page_size)

Ugh, you are of course right.


> Just rely on the clamping in st_adjustment_set_value() I think.

Makes sense to me.


> @@ +467,3 @@
> +      if (value == lower || value == upper)
> +        return FALSE;
> +      else if (delta_x > delta_y)
> 
> Need to be comparing fabs(delta) right?

Right, fixed.


> ::: src/st/st-scroll-view.c
> @@ +699,3 @@
> +      clutter_event_get_scroll_delta ((ClutterEvent *)event,
> +                                      &delta_x, &delta_y);
> +      g_object_get (delta_x > delta_y ? priv->hadjustment : priv->vadjustment,
> 
> also missing a fabs. 
> [...] but maybe it would be better to scroll the hadjustment the delta_x
> and the vadjusment by the delta_y? Is only one supposed to be non-zero?

That sounds like the nicer solution to me. I would assume that both values may be non-zero, but I don't see how it would hurt if that's not the case ...

Review of attachment 210123 [details] [review]:

Looks good, one whitespace issue

::: src/st/st-scroll-bar.c
@@ +466,3 @@
+      clutter_event_get_scroll_delta ((ClutterEvent *)event,
+                                      &delta_x, &delta_y);
+      if (fabs (delta_x) > fabs(delta_y))

missing space in fabs(delta_y)

Created attachment 210125 [details] [review]
st: Clean up scroll event code

Currently the scroll event code only handles scroll events if the
adjustment's value is within the "lower" and "upper" limits. The
likely intent was to pass events to a parent scroll view when
reaching the bounds (uh, nested scroll views!), but apparently
we never made use of this, as the upper bound is actually wrong
(an adjustment's maximum value is upper - page_size, not upper).
Just handle all scroll events unconditionally and rely on the
bound checks in StAdjustment.

Comment on attachment 210123 [details] [review]
st: Add rudimentary support for CLUTTER_SCROLL_SMOOTH events

Attachment 210123 [details] pushed as a7d4c7d - st: Add rudimentary support for CLUTTER_SCROLL_SMOOTH events

Review of attachment 210125 [details] [review]:

Is this still necessary?

(In reply to comment #7)
> Is this still necessary?

It is a cleanup, so not strictly necessary. Still nice to have IMHO though (e.g. nothing has changed here since the patch was written).

Review of attachment 210125 [details] [review]:

Go for it, then.

Attachment 210125 [details] pushed as 700c060 - st: Clean up scroll event code