[PATCH xf86-input-libinput] Up the scroll dist value for touchpads

[Hans de Goede]

Hi,

On 04-03-15 22:46, Peter Hutterer wrote:
> On Wed, Mar 04, 2015 at 01:15:31PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 04-03-15 06:00, Peter Hutterer wrote:
>>> For source FINGER and CONTINUOUS, the axis value is the same as relative
>>> motion - but scrolling in X usually doesn't have the same speed as finger
>>> movement, it's a lot coarser.
>>>
>>> We don't know ahead of time where we'll get the scroll events from. Set a
>>> default scroll distance of 15 and multiply any wheel clicks we get by this
>>> value.
>>>
>>> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
>>> ---
>>> 15 is more-or-less a magic value, it feels just right on my box here
>>
>> Hmm, this is somewhat different from what we discussed in our mail conversation.
>
> indeed, I had that first, then compared it to the evdev driver and decided
> that this one is the better approach after all, explanation below.
>
>> First of all the problem most users are reporting and I'm seeing myself is not
>> that mouse wheel scrolling is too slow (which this patch seems to imply), but
>> rather that finger scrolling is much too fast, see e.g.:
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1198467
>>
>> What I understood from our discussion on this is that the idea for
>> mouse wheel scrolling was to emulate discrete-value number of button
>> 4 / 5 clicks and let X do the translation into scrolling axis, giving
>> us the exact same scroll wheel speed as the evdev driver.
>>
>> That and for finger scroll events actually make things slower by applying
>> a multiplication factor < 1.0 .
>
> X has two ways for a driver to submit scroll events: buttons 4-7 or data in
> a scroll valuator. Because clients may rely on either of those methods
> exclusively, the server emulates the other method.
>
> If set up, a driver defines a scroll distance. A valuator movement of that
> scroll distance is equivalent to one mouse wheel click, and vice versa.
> So if the driver sends a button 5 click, the server emulates a
> <distance> motion event. If the driver sends a <distance> motion event, the
> server emulates a button 4 click. The distance accumulates in the server, so
> if you send <distance/2> twice, the server will only emulate the click event
> on the second event.
>
> This is what the scroll distance does here - a movement of 15 on the
> touchpad is now equivalent to a mouse wheel click. So this patch does slow
> down the touchpad scrolling while leaving the mouse wheel as-is.
>
> This is a better approach (and a smaller diff) than the button click
> approach I suggested initially because it gives us some smooth scrolling on
> the wheel as well. A REL_WHEEL value of 2 will now result in a single smooth
> scroll event that can be used for speed calculation. Posting the button
> events directly would prevent that.
>
> Ideally we could just leave the scroll distance at 1 for devices that only
> have mouse wheels but we don't know this at device init time. Hence the
> default distance optimized for touchpads, then we multiply by that distance
> for wheel clicks. The actual value of the scroll distance is meaningless,
> it's just a reference to know how many "scroll units" a motion event
> represents. IIRC synaptics currently uses a default of 100 and that's in
> device coordinates.
>
> So in short, this patch does what we want, it slows down touchpad scrolling
> while leaving wheel scrolling as-is.

Ah thanks for the explanation. Xorg sometimes has too many levels of
indirection making things confusing...

Given the above this patch is:

Reviewed-by: Hans de Goede <hdegoede at redhat.com>

Regards,

Hans

p.s.

We should probably create an updated Fedora package for this, and push it as
an F-22 update, making it close:
https://bugzilla.redhat.com/show_bug.cgi?id=1198467

I can take care of that if you want me to. Please let me know either way.





>
> Cheers,
>     Peter
>
>
>>>
>>>   src/libinput.c | 16 ++++++++++------
>>>   1 file changed, 10 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/src/libinput.c b/src/libinput.c
>>> index 049c15b..5e616c8 100644
>>> --- a/src/libinput.c
>>> +++ b/src/libinput.c
>>> @@ -756,18 +756,22 @@ xf86libinput_handle_axis(InputInfoPtr pInfo, struct libinput_event_pointer *even
>>>
>>>   	axis = LIBINPUT_POINTER_AXIS_SCROLL_VERTICAL;
>>>   	if (libinput_event_pointer_has_axis(event, axis)) {
>>> -		if (source == LIBINPUT_POINTER_AXIS_SOURCE_WHEEL)
>>> +		if (source == LIBINPUT_POINTER_AXIS_SOURCE_WHEEL) {
>>>   			value = libinput_event_pointer_get_axis_value_discrete(event, axis);
>>> -		else
>>> +			value *=  driver_data->scroll_vdist;
>>> +		} else {
>>>   			value = libinput_event_pointer_get_axis_value(event, axis);
>>> +		}
>>>   		valuator_mask_set_double(mask, 3, value);
>>>   	}
>>>   	axis = LIBINPUT_POINTER_AXIS_SCROLL_HORIZONTAL;
>>>   	if (libinput_event_pointer_has_axis(event, axis)) {
>>> -		if (source == LIBINPUT_POINTER_AXIS_SOURCE_WHEEL)
>>> +		if (source == LIBINPUT_POINTER_AXIS_SOURCE_WHEEL) {
>>>   			value = libinput_event_pointer_get_axis_value_discrete(event, axis);
>>> -		else
>>> +			value *=  driver_data->scroll_hdist;
>>> +		} else {
>>>   			value = libinput_event_pointer_get_axis_value(event, axis);
>>> +		}
>>>   		valuator_mask_set_double(mask, 2, value);
>>>   	}
>>>
>>> @@ -1189,8 +1193,8 @@ xf86libinput_pre_init(InputDriverPtr drv,
>>>   	if (!driver_data->valuators)
>>>   		goto fail;
>>>
>>> -	driver_data->scroll_vdist = 1;
>>> -	driver_data->scroll_hdist = 1;
>>> +	driver_data->scroll_vdist = 15;
>>> +	driver_data->scroll_hdist = 15;
>>>
>>>   	path = xf86SetStrOption(pInfo->options, "Device", NULL);
>>>   	if (!path)
>>>