GdkEventButton seems to be one double larger than GdkEventScroll
GdkEventClient seems to be even larger than GdkEventButton

Is it okay to extend the structs at the end as long as the GdkEvent union doesn't grow? I think so.

shall i get this done? scroll behaviour is killing us in ardour right now, because every single scroll event causes a redraw ...

AFAIK, the multiple scroll events should be gone with GTK+ from trunk already. So the problems should be gone.

But as you'd still want to behave "properly" (read: "like a native application"), this still has to be implemented; go ahead and develop a patch for this issue.

from gdk/quartz/gdkevents-quartz.c:

        /* The delta is how much the mouse wheel has moved. Since there's no such thing in GTK+
         * we accomodate by sending a different number of scroll wheel events.
         */

so i think the issue is very much alive in the quartz branch, and by implication, in any backend where the underlying windowing system can send scroll wheel event with a "distance moved" value.

i'll work on a patch.

Sven means that sending multiple scroll events to accomodate for the delta was disabled while ago (I did that a month ago or so). We only scroll once now, for any delta.

the code in gdkevents-quartz.c that i checked out about 4 days ago still sends multiple scroll events:

 while (dy > 0.0)
          {
            event = create_scroll_event (window, nsevent, direction);
            append_event (event);
            dy--;

are you sure you committed those changes?

ah. missed the "break;" and comment you added right after it.sorry.

patch compiling now.

Created attachment 107296 [details] [review]
Patch to add distance/delta info to GdkEventScroll

tested on X11 and Quartz backends. Note that as stated in the bug, GdkEventScroll was already 8 bytes smaller than GdkEventButton so this doesn't affect the GdkEvent union size.

Created attachment 107298 [details] [review]
2nd version of scroll delta patch

removed linux-fb component, moved event struct change to end of structure.

That patch looks good to me.

Created attachment 107305 [details] [review]
small followup patch to make GtkRange use scroll delta

this makes GtkRange use the new delta information. it works very nicely.

Comment on attachment 107298 [details] [review]
2nd version of scroll delta patch

>Index: gdk/gdkevents.c
>===================================================================
>--- gdk/gdkevents.c	(revision 19860)
>+++ gdk/gdkevents.c	(working copy)
>@@ -316,6 +318,7 @@
>       new_event->scroll.y = 0.;
>       new_event->scroll.x_root = 0.;
>       new_event->scroll.y_root = 0.;
>+      new_event->scroll.delta = 0;
>       break;
>     case GDK_ENTER_NOTIFY:
>     case GDK_LEAVE_NOTIFY:

Shouldn't that be "1"? This way non-supporting backends would get the right value for free.

Also I'd prefer to write "1.0" explicitly so everyone reading the code knows that it's a floating point number. (And from looking at the context, the current style seems to ask for "1." at least.)

Great work.

sven, i was unsure about that but now that you raise the issue, i agree with you.

I agree with Svens comment. 
Other than that, the gtkrange patch doesn't really make intuitive sense to me. 
Why multiply ?

Paul, could you fix these 3 things?

1. 0 => 1
2. properly formatted floats and doubles
3. behave a little more intuitive in GtkRange

(In reply to comment #15)
> Other than that, the gtkrange patch doesn't really make intuitive sense to me. 
> Why multiply ?

I tested the patch. It reports values from 0.1 to 1.0 for slow to medium movements and 1.0 up to large values (maximum I saw was 15) for medium to fast scrolling.

So adjusting the default scroll width by multiplication absolutely feels correct here (tested with the scrollbars in gtk-demo).

But somehow you're right; if it's a scaling factor, it shouldn't be called delta but scale. Do you agree, Matthias?

Um, I don't follow...

delta is a     -> scroll by b
delta is 2 * a -> scroll by 2 * b
delta is n * a -> scroll by n * b

I merely see a linear dependency between the delta
and the amount we scroll, of course that delta is
*multiplied* with something, but it still is the
delta the wheel has moved.

Still, in that patch the function _gtk_range_get_wheel_delta()
should be changed to take a GdkEventScroll and do the multiplication
with event->delta internally.

I also think the generic code in gdk/gdkevents.c should initialize the
delta to 1.0 instead of 0.0 (which never makes sense).

Mitch, jajaja, I'm still working on my cleaned up patch. All your comments have been considered before you wrote them :-)

Created attachment 174695 [details] [review]
proposed patch v3 (patches GDK, GtkRange and GtkScrolledWindow)

Here's the patch, updated to a quite recent version of master.

Looks ok to me.

Review of attachment 174695 [details] [review]:

::: docs/reference/gdk/tmpl/event_structs.sgml
@@ +197,3 @@
 @y_root: the y coordinate of the pointer relative to the root of the screen.
+@delta: the distance scrolled by the wheel (1.0 by default and on backends
+  that don't report scrolling speeds).

The docs were moved to gdk/gdkevents.h

Created attachment 196376 [details] [review]
Proof-of-contept patch that adds smooth scrolling

The old patch didn't apply cleanly any longer, so I updated it
for HEAD of the gtk-2-24 branch and added support for smooth
scrolling when an input device supporting it is used (like
the magic mouse).

Problems:
- Code processing scroll events needs to be aware of the
  new delta field, or scrolling will occur at insanely
  high rates and be unusable
- This is actually impossible to implement without adding
  more state to GdkEventScroll, because there are now two
  types of scroll events: classic one with discrete logical
  scroll steps, and new ones where delta represents device
  units as delivered by the device
- The patch contains an ugly "/ 40.0" when creating scroll
  events from smooth values in order to convert them to fractions
  of logical steps. This magic value is pure trial-and-error
  and probably depends on the input device's resolution and
  my screen resolution

Created attachment 197616 [details] [review]
Updated smooth scrolling patch that compiles for earlier versions of Mac OS

Here is an updated version of Mitch's patch that compiles targetting versions of Mac OS before the scrollingDeltaX/Y methods were introduced to NSEvent (which was 10.7, I believe).

Very nice, thanks for taking care of that, patch builds nicely and still
works fine on Lion. I'm still thinking about the right way to do this in
master, because as-is, the patch isn't suitable for upstream yet (but can
of course be used in app bundles which ship their own GTK+).

Created attachment 200174 [details] [review]
Next patch iteration

First, this is still against gtk-2-24 because I need it there, of course
we can't add new features to 2.x, but it's good for testing against
existing applications, and trivial to port to master.

Changes:

- add gdk_event_get_scroll_deltas() which returns TRUE and the
  deltas only if the event has precise scroll info
- add delta_x *and* delta_y at the same time to allow scrolling freely
- change range and scrolled window to use the new information if it
  is available
- the ugly make-it-work magic factor in the quartz backend is gone
  because logical-step and smooth scrolling are now distinguishable

Would be good to verify that any new api here can work with

http://who-t.blogspot.com/2011/09/whats-new-in-xi-21-smooth-scrolling.html

Review of attachment 200174 [details] [review]:

After talking to Peter about smooth scrolling a bit, adding the deltas in our scroll event should work fine.

Created attachment 201008 [details] [review]
Raw patch, gtk3 ready

This patch is a raw port to gtk3, implementing smooth scrolling in X via XI2.1 (note this isn't guarded by #ifdefs yet)

We should probably add some flag to have widgets opt in this new behavior though, I've only seen quirks in non-native scrolledwindow children, such as ephy and devhelp

Review of attachment 201008 [details] [review]:

::: gdk/x11/gdkdevice-xi2.c
@@ +840,3 @@
+
+          delta = valuator_value - scroll->last_value;
+          scroll->last_value = valuator_value;

Something is missing here. From what I've read in Peters blog, we should look at the 'increment' of the scroll valuator to translate the raw value change into 'scroll units'

::: gdk/x11/gdkdevicemanager-xi2.c
@@ +1180,3 @@
           case 6:
           case 7:
+#if 0

From what Peter told me, we need to ignore events which have the XIPointerEmulated flag set, and not just all button events for buttons 4-7.

FYI, the unit used in the smooth events on OSX seems to be pixels. The
docs say device units, but interpreting them as display device units
makes GTK+ scroll exactly the same way as native widgets. This should
also be used for X11, because it's the unly unit that actually makes
sense IMO.

Also the scroll direction should be determined from the biggest delta, so

if (abs (dx) > abs (dy))
  direction = dx < 0 ? LEFT : RIGHT;
else
  etc...

And do normal scroll events still arrive in intervals? We need them because
smooth scrolling must become an op-in feature at the widget level.

with 'normal scroll events' you mean button 4-7 presses ? 
yes, xi2 generates those in addition to the new-style events, and markes them as emulated with the flag that I've mentioned above. So, we get duplicated events and need to ignore the one set or the other. 

Why does smooth scrolling need to be an explicit-opt-in feature at the widget level ? I need to get an actual xi2.1 xserver to play with it, I guess...

Because only as much as touching the apple magic mouse generates several
scroll events. If we deliver them all to unported scroll_event()
handlers, things scroll at insane speed, that's totally b0rk.

The way apple does it is they deliver *all* events, where most events
have a "delta" of 0.0, and *some* have a delta of > 1.0. These are the
equivalent of old logical scroll steps that GTK+ has now.

Then there is hasPreviceScrollingDelta() which returns TRUE if the
event comes from a precise device. This is the equivalent of the getter
I added in the patch.

Since GTK+ doesn't have a legacy concept of "delta", and there is
no NONE scroll movement that applications would use traditionally,
we somehow need to shield old widgets from the event flood.

I propose gtk_widget_set_handles_smooth_scrooll_event() or something.

I see. But we can certainly make all scroll event consumers inside gtk itself deal with deltas, so the opt-in would only be for third-party scroll event consumers.

Yep, already done, there are only range and scrolled window.

Nonsense, there are more. At least the opt-in will allow us to port
them one-by-one.

(In reply to comment #40)
> Nonsense, there are more. At least the opt-in will allow us to port
> them one-by-one.

And there are some widgets where handling smooth scrolling doesn't even make much sense, say spinbuttons, calendars...

(In reply to comment #37)
> I propose gtk_widget_set_handles_smooth_scrooll_event() or something.

How about a new GDK_SMOOTH_SCROLL_MASK GdkEventMask value? That could be used in _gdk_windowing_got_event() to filter one or the other kind of events, so we don't need to push both to the upper layers

(In reply to comment #41)

> How about a new GDK_SMOOTH_SCROLL_MASK GdkEventMask value? That could be used
> in _gdk_windowing_got_event() to filter one or the other kind of events, so we
> don't need to push both to the upper layers

Sounds interesting. So we'd just let emulated and smooth scrolling events come up to windowing_got_event and filter them out there ? Would be good to see a patch, to see how that works in practice.

Something like that. Carlos and I had a long discussion yesterday, and
we seem to know the direction, but it's hard to describe, especially
*why* it is needed like we think it is. A patch will describe it
by itself. Stay tuned :)

still tuned here...

Just an FYI, but the patch from comment #29 breaks the build on OSX older than 10.7

gdkevents-quartz.c: In function 'gdk_event_translate':
gdkevents-quartz.c:1347: warning: 'NSEvent' may not respond to '-hasPreciseScrollingDeltas'
gdkevents-quartz.c:1347: warning: (Messages without a matching method signature
gdkevents-quartz.c:1347: warning: will be assumed to return 'id' and accept
gdkevents-quartz.c:1347: warning: '...' as arguments.)
gdkevents-quartz.c:1349: warning: 'NSEvent' may not respond to '-scrollingDeltaX'
gdkevents-quartz.c:1349: error: incompatible types in assignment
gdkevents-quartz.c:1350: warning: 'NSEvent' may not respond to '-scrollingDeltaY'
gdkevents-quartz.c:1350: error: incompatible types in assignment
make[4]: *** [gdkevents-quartz.lo] Error 1

Created attachment 201916 [details] [review]
New gtk-2-24 patch, builds on < 10.7

Hey,

I made a patch for GTK 3 to support XI2 smooth scrolling over the summer.
It's not finished yet, but it works with my touchpad on a Thinkpad X220.

I also worked on implementing the corresponding part for the Trackpoint driver, but that doesn't work yet.

Note that I made a new GdkSmoothScroll event for this.
I had some chat in #xorg, but I haven't figured out yet why they deliver the smooth scrolling information as absolute floating point values instead of relative ones.

Have a look.
Niklas

(The patch should apply on gtk-3.2.1)

Created attachment 202520 [details] [review]
Smooth scrolling draft implementation for GTK3

We have gdk_event_get_scroll_deltas now