run libinput-debug-events and check what the output values are when you move the wheel. Are they still 15?

Note that MOUSE_WHEEL_CLICK_ANGLE is not a configuration item, it's supposed a property describing the physical hardware on your device, specifically the angle your mouse wheel moves per click. There is no guarantee that the behaviour stays the same across libinput versions.

Setting it to -1 means the wheel is mounted the wrong way round, and only moves 1 degree per click. That's... unlikely.

If you want to increase the scroll speed of the wheel, you should do this in the respective toolkit settings.

(In reply to Peter Hutterer from comment #1)
> run libinput-debug-events and check what the output values are when you move
> the wheel. Are they still 15?

No, that works, the correct value is applied.

> Setting it to -1 means the wheel is mounted the wrong way round, and only
> moves 1 degree per click. That's... unlikely.

"-1" was just an example, as I said:

"I tried multiple postive and negative values, with no noticeable effect."

> If you want to increase the scroll speed of the wheel, you should do this in
> the respective toolkit settings.

Even without changing it for every toolkit (which is also annoying, obviously there should be a central place to set this), one should be able to notice a difference. Which I do for dolphin (it's always slow, but that seems to be the case because qt5/kde5 doesn't support libinput at the moment). For chrome or evince I see no difference.

The problem with chrome seems to be an upstream bug:

https://code.google.com/p/chromium/issues/detail?id=22638

(In reply to peter.eszlari from comment #2)
> Even without changing it for every toolkit (which is also annoying,
> obviously there should be a central place to set this), 

Unfortunately, "one central place to keep all config options" is a pipe-dream, requirements for different desktops are quite different. The very fact that we have a multitude of similar but slightly different desktops is the proof for that.

> Which I do for dolphin (it's always slow, but that
> seems to be the case because qt5/kde5 doesn't support libinput at the
> moment). For chrome or evince I see no difference.

So here's the basic problem: if you're using libinput under X, events are read in by the xorg libinput driver and passed to the server. For mouse wheel events like yours, the driver reads the discrete events (i.e. click events) and passes them to the server.

The property you're changing changes how many degrees are expected per wheel click, but this has no effect on the actual event data. Regardless of the value, the xorg driver still sees one event per click and passes that on.

So while the value is read and handled correctly in libinput, and passed on to the xorg driver, the driver doesn't use it. Most of X isn't really set up to change the scroll distances or use custom distances for wheels but your starting point would be to modify scroll.vdist in xf86libinput.c of the xf86-input-libinput driver.

I'm changing this over to the xorg libinput driver, but there's no plans on fixing this.

actually, now that I think of this: the MOUSE_WHEEL_CLICK_ANGLE is a descriptive property that describes the HW, not any of the software stack. I would accept a patch to xf86-input-libinput that checks that udev property for a value and adjusts the scroll.vdist/hdist accordingly.

(In reply to Peter Hutterer from comment #4)
> Unfortunately, "one central place to keep all config options" is a
> pipe-dream, requirements for different desktops are quite different.

Is $XDG_CONFIG_HOME a "pipe-dream"? Maybe I'm missing something, but wouldn't it be enough to just make this some kind of XDG standard? Like ".config/xdg-input/settings.conf" or something?

> The very fact that we have a multitude of similar but slightly different
> desktops is the proof for that.

Well, maybe this is not the place to analyze the evolution of the Linux desktop, but imho this fact has more to do with history and politics than with technical necessities.

(In reply to peter.eszlari from comment #6)
> (In reply to Peter Hutterer from comment #4)
> > Unfortunately, "one central place to keep all config options" is a
> > pipe-dream, requirements for different desktops are quite different.
> 
> Is $XDG_CONFIG_HOME a "pipe-dream"? Maybe I'm missing something, but
> wouldn't it be enough to just make this some kind of XDG standard? Like
> ".config/xdg-input/settings.conf" or something?

"or something" is the issue :)
which filename? is it in json, xml, ini format? does it cover just libinput, or is it a generic list of settings that will apply to any future implementation in 5 years time? how detailed does it get? It sounds simple in theory, but once you get to the details it gets complex enough, and not for technical reasons.

and of course you'd have to convince everyone to make use of these settings too, otherwise you'll have the X situation, where there are multiple different levels of configurations (commandline tools overwriting gnome overwriting properties overwriting xorg.conf settings)

I was just looking into this bug, and I think I have found a simple solution (it works for me). xf86-input-libinput handles scroll wheels differently than trackpads, ignoring the MOUSE_WHEEL_CLICK_ANGLE configured in libinput. If you simply remove that special handling, MOUSE_WHEEL_CLICK_ANGLE is handled by libinput and xorg simply receives a distance to scroll, just like with a trackpad.

In my case, I have a Logitech mouse with a high resolution scroll wheel (I think its ~2 degrees per tick), and applying this patch makes it work just as well as it did with evdev.

A possible issue I can see with the patch in its current state is it interprets 1 degree of mouse movement to be the same as 1 unit of finger scrolling, but it would be easy to implement a multiplier (hardcoded or configurable) to equate the two unit systems. The current setup feels fine to me, but that may be because I have my MOUSE_WHEEL_CLICK_ANGLE set too small.

Let me know what you think.

Created attachment 127323 [details] [review]
Possibly fix that removes special handling for wheel events

Comment on attachment 127323 [details] [review]
Possibly fix that removes special handling for wheel events

Ignore the last part of the patch, its a mistake.

Created attachment 127324 [details] [review]
(CORRECT VERSION) Possible fix that removes special handling for wheel events

Ignore the first patch, it had a mistake.

Comment on attachment 127323 [details] [review]
Possibly fix that removes special handling for wheel events

Marking as obsolete based on comment 11

The problem here is that since XI2.1 with smooth scrolling, we have a specification for the scroll distance used by smooth scrolling (the vdist value). For mouse wheels, the data has to be a multiple of that, otherwise a client may wait until a full scroll click equivalent has accumulated.

The problem is that X requires all information about the device to be available ahead of time, libinput only provides it on events. so we need to set vdist to some hardcoded value and then use it.

the only way around this now is to use the udev property in the driver and init vdist based on that. that's a bit iffy because libinput doesn't guarantee that this value actually represents the croll, but it would do for now (and for 95% of the cases).

now that I think about this more, I'm going to counter some of my points above, sorry about that.

(In reply to Ben Wolsieffer from comment #8)
> I was just looking into this bug, and I think I have found a simple solution
> (it works for me). xf86-input-libinput handles scroll wheels differently
> than trackpads, ignoring the MOUSE_WHEEL_CLICK_ANGLE configured in libinput.

this isn't true. xf86-input-libinput doesn't care about the click angle and unlike in libinput where it signals degrees, the vdist is just an arbitrary number that signals "one click". synaptics used to use 100 by default. this is the reason for the special handling of clicks, it's always a multiple of the "scroll distance".

if you set your click angle to 5 degrees, every 5 degrees you'd get a scroll distance of 15 which should be interpreted as 1 unit of scrolling by the client. again - the actual value is meaningless, it only matters in relation to the vdist.

Thank you for explaining how vdist works. I think I have a better understanding of it now. I'm going to write the rest of this according to my current limited understanding of xinput and libinput, so feel free to tell me if I've made a mistake in my analysis. Sorry about the wall of text over such a little bug, but I want to come to a good solution.

My mouse is a Logitech M705, which is somewhat unusual in that its wheel moves 15 degrees per tactile click (but the wheel can also be unlocked so it freewheels), but it actually is capable of sending 8 events every 15 degrees, meaning it should have a MOUSE_WHEEL_CLICK_ANGLE of 1.875 degrees, but decimal values are not supported, so I rounded it to 2. When it is connected to a computer without special software (Solaar), it behaves like a normal mouse (15 degree click angle). Solaar activates a smooth scrolling mode, which changes the behavior so the wheel has a much higher resolution. A number of other Logitech mice also have this capability. From now on, when I refer to the M705, the same also applies to any Logitech mouse with the same feature.

Right now vdist is hardcoded as 15, for both finger and wheel scroll events. The problem with the current implementation is that libinput_event_pointer_get_axis_value_discrete() returns the number of clicks my mouse wheel moves. This means that if, for example, my mouse were to move 4 (3.75 in reality) degrees, (get_axis_value_discrete() * vdist) would equal 30, causing the screen to scroll two full units, when it only should scroll a little more 0.5 units.

With my patch, it seems like everything would work fine for clients that support smooth scrolling, because in that case vdist serves mainly to convert device units into screen units. Smooth scrolling clients work fine even if each click does not scroll a distance that is an exact multiple of vdist.

As I understand it, the problem arises with clients that do not support smooth scrolling/XInput 2.1. Assuming a fixed vdist of 15, and a MOUSE_WHEEL_CLICK_ANGLE of 2, the axis value is rarely a multiple of vdist, so scrolling will happen unpredictably (sometimes scrolling twice when the wheel is only clicked once). With most mice the axis value will be a multiple of 15, because most mice have a click angle of 15. Looking through the systemd hwdb* I noticed that the Logitech M325 has a click angle of 20 which causes similar problems as a small click angle.

A couple of miscellaneous points:

Based on what you wrote, it sounds like it is not possible to dymanically adjust vdist. It seems like it is possible to work around this, using some math: scroll_units = vdist * (axis_value / new_vdist). Below, when I refer to changing vdist, this could be how I implement it.

It is also possible to calculate the mouse wheel click angle without parsing the hwdb by dividing get_axis_value() by get_axis_value_discrete(). This is guaranteed to be the angle per click by the libinput documentation.

Some possible options:

1. Use the patch in its current state. In my opinion, this isn't that bad of a solution. Most mice, which have a click angle of 15, will behave the same way they always have because vdist = 15. All mice will work correctly with clients that support smooth scrolling. A few wierd mice (M325 and M705) will have slightly wierd scrolling behavior on old clients. The M325 will scroll a tiny bit faster than it does now.

2. Use my patch, but introduce a configuration option for vdist (I'm not sure what it should be called). It could be set to a multiple of the click distance by the user. By default, it should be 15. For the M705, it should be set to 16 (2 degrees * 8 clicks). For the M325, it should be set to 20. Smooth scrolling would work correctly for the M705 and discrete scrolling clients would scroll 1 unit for every tactile click of the wheel. All other mice would work like they already do in all clients. The problem with this is that it introduces an obscure and somewhat difficult to understand configuration option. If the option was left at its default and the user was using an M705 or M325, then it would behave like option 1.

3. A compromise between the first two options that preserves the existing behavior for all mice except for those with high resolution scroll wheels. If the click angle is greater that 15 degrees, vdist is adjusted to be equal to the click angle. If the click angle is less than 15, vdist is set to 15 and the axis value is passed straight to XInput. This means that the M705 will have the same problems as I described in option 1 with clients that do not support smooth scrolling, but all other mice will retain their existing behavior in all clients.

4. Keep using the current implementation. When high resolution scrolling is enabled on the M705 (or other mice with high resolution wheels), scrolling is way to fast. Smooth scrolling does not work for mouse wheels. All other mice will work correctly in all clients, independent of whether the clients support smooth scrolling. I would rather not do this, unless you feel there is no better solution.

I personally believe option 1 is the best, because it is simple and every mouse will scroll at the same speed, at the expense of some wierdness if a client does not support smooth scrolling. Please let me know what you think.

* https://github.com/systemd/systemd/blob/dd8352659c9428b196706d04399eec106a8917ed/hwdb/70-mouse.hwdb

(In reply to Ben Wolsieffer from comment #15)
> My mouse is a Logitech M705, which is somewhat unusual in that its wheel
> moves 15 degrees per tactile click (but the wheel can also be unlocked so it
> freewheels), but it actually is capable of sending 8 events every 15
> degrees, meaning it should have a MOUSE_WHEEL_CLICK_ANGLE of 1.875 degrees,
> but decimal values are not supported, so I rounded it to 2. 

this is something I'll have to fix separately anyway. It's not the first mouse where this has become a problem, so I'm planning to introduce MOUSE_WHEEL_CLICK_COUNT instead with the number of clicks per 360 rotation. That at least should take care of the rounding error.

> When it is
> connected to a computer without special software (Solaar), it behaves like a
> normal mouse (15 degree click angle). Solaar activates a smooth scrolling
> mode, which changes the behavior so the wheel has a much higher resolution.
> A number of other Logitech mice also have this capability. From now on, when
> I refer to the M705, the same also applies to any Logitech mouse with the
> same feature.

oh? I didn't know this, I'll have to try. I have a few L mice floating around here, I just never noticed you can change the click count.

> Right now vdist is hardcoded as 15, for both finger and wheel scroll events.

important: the actual *value* simply doesn't matter the way we use the vdist in X. we could set vdist to 38274 and then multiply the click count by that number and the behaviour would be exactly the same. The only thing that matters right now is that vdist is always the same within the driver so we don't set it up as X and then multiply the click count by Y.

I think this is where some misunderstanding comes from, you read vdist as 15 degrees, when it's really closer to "base 15". Re-read your comment with 15 replaced by 12345 and you'll see how much sense that would make ;)

fwiw, the reason it's 15 is because that was it before we introduced the click count in libinput and we never changed it. note how touchpads also have a vdist of 15 and a touchpad doesn't really have a "15 degree" value :)

> The problem with the current implementation is that
> libinput_event_pointer_get_axis_value_discrete() returns the number of
> clicks my mouse wheel moves. This means that if, for example, my mouse were
> to move 4 (3.75 in reality) degrees, (get_axis_value_discrete() * vdist)
> would equal 30, causing the screen to scroll two full units, when it only
> should scroll a little more 0.5 units.

From what I understand, the issue with your device is that we have a hardware-descriptive udev property (CLICK_ANGLE) but it's set to the wrong value once you used solaar to change the hardware. That's to be expected and we have a similar case with the DPI settings when the out-of-the-box configuration changes. 

What you need is a local fix that changes the click angle to 2. Once that's set, everything else should mostly fall into place.

> It is also possible to calculate the mouse wheel click angle without parsing
> the hwdb by dividing get_axis_value() by get_axis_value_discrete(). This is
> guaranteed to be the angle per click by the libinput documentation.

just to re-iterate, the angle is never used anywhere in the X driver. it just happens to be 15 right now, but vdist could be 12345 and be equally valid.

> 4. Keep using the current implementation. When high resolution scrolling is
> enabled on the M705 (or other mice with high resolution wheels), scrolling
> is way to fast. Smooth scrolling does not work for mouse wheels. All other
> mice will work correctly in all clients, independent of whether the clients
> support smooth scrolling. I would rather not do this, unless you feel there
> is no better solution.

I still think that having a local udev hwdb entry to override the click angle is the correct solution here. once your click wheel angle says 2, each actual click on the device will be a multiple of vdist and the scrolling will work as before, albeit at a higher resolution.

now, the real issue is of course that you have to put that udev hwdb in and that we can't detect this otherwise. But that's a separate issue and I don't have any good solutions for that yet (libratbag, in the future, could help here but it's not there yet and besides, that's a fairly heavy-handed approach).

(In reply to Peter Hutterer from comment #16)
> From what I understand, the issue with your device is that we have a
> hardware-descriptive udev property (CLICK_ANGLE) but it's set to the wrong
> value once you used solaar to change the hardware. That's to be expected and
> we have a similar case with the DPI settings when the out-of-the-box
> configuration changes. 
> 
> What you need is a local fix that changes the click angle to 2. Once that's
> set, everything else should mostly fall into place.

> I still think that having a local udev hwdb entry to override the click
> angle is the correct solution here. once your click wheel angle says 2, each
> actual click on the device will be a multiple of vdist and the scrolling
> will work as before, albeit at a higher resolution.

I have already done that, and it doesn't fix the problem, because with the current implementation, MOUSE_WHEEL_CLICK_ANGLE is ignored completely. My patch allows the driver to take it into account.

(In reply to Ben Wolsieffer from comment #17)
> I have already done that, and it doesn't fix the problem, because with the
> current implementation, MOUSE_WHEEL_CLICK_ANGLE is ignored completely. My
> patch allows the driver to take it into account.

because the actual angle doesn't matter. when the property takes effect libinput should send one click per 2 deg of scrolling (or whatever the wheel sends). the driver should convert this one click into a vdist of 15, converted by the server into a single scroll button event (and a smooth scroll event of value 15). To the driver and the server, the whole thing is completely transparent.

(In reply to Peter Hutterer from comment #18)
> because the actual angle doesn't matter. when the property takes effect
> libinput should send one click per 2 deg of scrolling (or whatever the wheel
> sends). the driver should convert this one click into a vdist of 15,
> converted by the server into a single scroll button event (and a smooth
> scroll event of value 15). To the driver and the server, the whole thing is
> completely transparent.

Your description is correct, but this is exactly what causes the problem. Every time my mouse wheel moves 2 degrees, the screen scrolls the same amount it would when a normal mouse wheel moves 15 degrees. This means everything scrolls 7.5 times faster than it should.

It doesn't matter whether MOUSE_WHEEL_CLICK_ANGLE is 2 or 15, or 1000, the same exact behavior occurs (because the value is ignored, which is the premise of this bug).

I discovered this bug when I tried changing MOUSE_WHEEL_CLICK_ANGLE and it had no effect on scrolling behavior, but I know that the setting is being read correctly by libinput/udev because it is printed by libinput-debug-events.

Without making any more changes other than applying the patch, scrolling begins working as expected (with the caveats I explained before).

ok, I think I'm finally getting it. the solution here isn't to mess around (or even use) the click angle property but rather have a driver-specific option that tells us what value should constitute a scroll event. That can be in degrees, for ease of use, but it cannot be a property because we can't change vdist/hdist after DEVICE_INIT. so it'll have to be an xorg.conf option.

the default for that will be 15 (or, more sensibly, reading the CLICK_ANGLE property) property. then we just forward the scroll value (i.e. your patch) and let the server take care of the rest.

If you change the new option to something higher than the click angle, you'll get smooth scrolling on your mouse wheel. legacy apps that don't do XI2 smooth scrolling will just see a button event every X wheel clicks. everyone else sees smooth scrolling  events like a touchpad does.

for your case with the 2 degree wheel that would mean that you have to change the CLICK_ANGLE property to match the hardware *and* change the option to reset it to 15, or whatever.

I'm a bit worried because libinput doesn't guarantee that the scroll output matches the CLICK_ANGLE property. but I guess we can cross that bridge when we get to it.

This is pretty much what I was thinking with my "option 2" in comment #15. I agree that this is the most correct solution if you are okay with adding a new option.

It is possible to implement this as a driver-specific option, even though we can't actually change vdist/hdist after initialization. You can just use the formula I described before. For example, you could set vdist to a fixed value of 100 (it doesn't really matter), and then transform the axis values using (100 * (axis_value / the_option_value)). The result of this calculation will be the same as if you were actually able to change vdist.

Also, like I described before, it is possible to calculate the click angle based on a libinput event, by dividing get_axis_value() by get_axis_value_discrete(). This is guaranteed by the libinput documentation to be the click angle, but it does not depend on the fact that libinput is using MOUSE_WHEEL_CLICK_ANGLE. The issue with this is that it is not possible to calculate to this calculation until the first scroll event is received.

systemd PR to add a new property for non-integer click angles:
https://github.com/systemd/systemd/pull/4440

I have some time now, so I'm working on implementing this functionality, but I have a few questions.

A lot of the properties have a separate default property, but some don't ("Horizontal Scroll Enabled", for example). Should this new property have a default property?

Do you think it would be better to combine the horizontal and vertical values into one property with two values or create two separate properties? Right now I'm using a single property with two values.

Do you care what the property is called? I'm thinking "libinput Scroll Distance".

Thanks.

the driver has two classes of properties, the ones that mirror libinput configuration and xorg-driver specific ones (e.g. the Horizonal scroll property). The former are the ones with default/available but since this one won't be mirroring libinput configuration you don't need those extra properties.

Definitely use one property with two values, you're already on the right approach here. 

As for the name - yeah, that's fine for now, I can't think of anything better but I think we'll have to change it away from "distance" to something ... better :) but that's an easy change anyway.

I've been talking to Carlos (cc-d now) about this given that it also has implications in Wayland and for the clients. We're still missing a few bits of the big picture. Essentially what we need to do is decouple the current definition of scroll click from the definition of a scroll unit so that you can multiple clicks per unit. How to do that exactly is still a bit blurry (read: not written down yet)

(In reply to Peter Hutterer from comment #25)
> the driver has two classes of properties, the ones that mirror libinput
> configuration and xorg-driver specific ones (e.g. the Horizonal scroll
> property). The former are the ones with default/available but since this one
> won't be mirroring libinput configuration you don't need those extra
> properties.

Ok, that makes sense.

> I've been talking to Carlos (cc-d now) about this given that it also has
> implications in Wayland and for the clients. We're still missing a few bits
> of the big picture. Essentially what we need to do is decouple the current
> definition of scroll click from the definition of a scroll unit so that you
> can multiple clicks per unit. How to do that exactly is still a bit blurry
> (read: not written down yet)

I've actually looked at the GNOME on Wayland situation a bit before I started working on this. As you probably know, scrolling in Wayland is handled by the compositor, so in GNOME's case this is Mutter. Mutter has the same behavior as xf86-input-libinput, caused by this code: https://github.com/GNOME/clutter/blob/master/clutter/evdev/clutter-device-manager-evdev.c#L1598

(In reply to Ben Wolsieffer from comment #26)
> I've actually looked at the GNOME on Wayland situation a bit before I
> started working on this. As you probably know, scrolling in Wayland is
> handled by the compositor, so in GNOME's case this is Mutter. Mutter has the
> same behavior as xf86-input-libinput, caused by this code:
> https://github.com/GNOME/clutter/blob/master/clutter/evdev/clutter-device-
> manager-evdev.c#L1598

yeah, the wayland protocol for scrolling was directly influenced by libinput's API and thus is almost identical. fwiw, scrolling isn't really handled by the compositor it's merely converted from libinput API to wayland events. the actual scrolling is handled by the client and that suffers from the same issue - we defined angle and click, but we don't have a definition of a scroll unit (or rather: one click is one unit, with no finer granularity)

Created attachment 128174 [details] [review]
Patch that adds a new "ScrollDistance" option with a static default value of 15

This patch implements a new xorg.conf option called "ScrollDistance" and a corresponding xinput property called "libinput Scroll Distance".

It adjusts allows for the adjustment of the amount of scroll wheel movement that corresponds to one unit of on-screen scrolling.

I have written some documentation for it, but I'm not sure how good it is.

The patch works for me, and I tested it using xorg.conf and "xinput set-prop".

Right now, the default value for both the horizontal and vertical properties is 15. I can create a new patch that uses the value of MOUSE_WHEEL_CLICK_ANGLE as the default if you want.

Comment on attachment 128174 [details] [review]
Patch that adds a new "ScrollDistance" option with a static default value of 15

Review of attachment 128174 [details] [review]:
-----------------------------------------------------------------

::: man/libinput.man
@@ +155,5 @@
> +Sets horizontal and vertical amount (respectively) of scroll wheel movement
> +that should result in one full scroll tick. The vertical value should be in
> +degrees, but the horizontal value may have an arbitrary unit unless the device
> +has a horizontal scroll wheel.
> +.TP 7

Just make both of them degrees. No need to care about devices without horiz scroll wheels here, they can't scroll anyway. Maybe something like "if he device does not have a horizontal scroll wheel, use 0".

@@ +253,5 @@
> +Sets horizontal and vertical amount (respectively) of scroll wheel movement
> +that should result in one full scroll tick. The vertical value should be in
> +degrees, but the horizontal value may have an arbitrary unit unless the device
> +has a horizontal scroll wheel.
> +.TP 7

same here. I'd add a proper man page section to explain what scroll ticks are etc. and just use a "see section ..." instead. That way we can be descriptive enough to not be confusing.

::: src/xf86libinput.c
@@ +2611,5 @@
> +	int num_distance;
> +
> +	str = xf86CheckStrOption(pInfo->options,
> +				 "ScrollDistance",
> +				 "15 15");

fits on one line, doesn't it?

same with the scanf below

@@ +2618,5 @@
> +
> +	num_distance = sscanf(str, "%f %f ",
> +				 scroll_hdist, scroll_vdist);
> +	if (num_distance != 2 ||
> +		*scroll_hdist == 0.0 || *scroll_vdist == 0.0) {

please align with the n from num_distance

@@ +2622,5 @@
> +		*scroll_hdist == 0.0 || *scroll_vdist == 0.0) {
> +		xf86IDrvMsg(pInfo, X_ERROR,
> +			    "Invalid scroll distance: %s, using default\n",  str);
> +		*scroll_hdist = 15;
> +		*scroll_vdist = 15;

we have 15 hardcoded here and then again below. let's use a #define here

@@ +2950,5 @@
>  
>  	pInfo->private = driver_data;
>  	driver_data->pInfo = pInfo;
> +	driver_data->scroll.vdist = 100;
> +	driver_data->scroll.hdist = 100;

nope, please leave this at the 15 default.

@@ +4780,5 @@
> +LibinputInitScrollDistanceProperty(DeviceIntPtr dev,
> +				struct xf86libinput *driver_data)
> +{
> +	float scroll_distance[2];
> +	scroll_distance[0] = driver_data->options.scroll_hdist;

empty line between declarations and the first line of implementation please

(In reply to Peter Hutterer from comment #29)
> Just make both of them degrees. No need to care about devices without horiz
> scroll wheels here, they can't scroll anyway. Maybe something like "if he
> device does not have a horizontal scroll wheel, use 0".

A lot of devices without horizontal scroll wheels can still scroll horizontally, by tilting the scroll wheel. This causes libinput to report a scroll distance equal to MOUSE_WHEEL_CLICK_ANGLE (default 15) or MOUSE_WHEEL_CLICK_ANGLE_HORIZONTAL if it is defined. Therefore, the horizontal component of the scroll distance must be set to 15 (which is the default) to make horizontal scrolling work correctly for normal mice.

> @@ +2950,5 @@
> >  
> >  	pInfo->private = driver_data;
> >  	driver_data->pInfo = pInfo;
> > +	driver_data->scroll.vdist = 100;
> > +	driver_data->scroll.hdist = 100;
> 
> nope, please leave this at the 15 default.

I can change it back if you want, but I think it is better to leave it as 100 because its value has no effect on the scrolling behavior. By changing its value to something other than 15, this prevents others reading the code from thinking its value is related to the default scroll distance.

(In reply to Ben Wolsieffer from comment #30)
> A lot of devices without horizontal scroll wheels can still scroll
> horizontally, by tilting the scroll wheel. This causes libinput to report a
> scroll distance equal to MOUSE_WHEEL_CLICK_ANGLE (default 15) or
> MOUSE_WHEEL_CLICK_ANGLE_HORIZONTAL if it is defined. Therefore, the
> horizontal component of the scroll distance must be set to 15 (which is the
> default) to make horizontal scrolling work correctly for normal mice.

I'd say this is a bug. if the scrolling isn't triggered by a tilt wheel we should accommodate for this with a new, more correct, scroll sources. Please file a separate bug for this and make it block this one.

> I can change it back if you want, but I think it is better to leave it as
> 100 because its value has no effect on the scrolling behavior. By changing
> its value to something other than 15, this prevents others reading the code
> from thinking its value is related to the default scroll distance.

unrelated: what happens if the option isn't set? don't the option values end up undefined (or 0?). sorry, I don't have the brain space right now to get past this bit, will try a re-review tomorrow

> driver_data->scroll.vdist = 100;
> driver_data->scroll.hdist = 100;

These two lines set the vdist and hdist values that are passed to XInput. This value remains constant no matter what the values of the "Scroll Distance" options are. If I replaced 100 with any other non-zero number, the scrolling behavior would not change.

These two lines in "xf86libinput_handle_axis" are the reason this is true:
> line 1356: value = (value / driver_data->options.scroll_vdist) * driver_data->scroll.vdist;
> line 1366: value = (value / driver_data->options.scroll_hdist) * driver_data->scroll.hdist;

This is the math that converts libinput scroll units to XInput scroll units. "value" is the libinput event axis value, "driver_data->options.scroll_vdist/hdist" is the value of the scroll distance option and "driver_data->scroll.vdist/hdist" equals 100.

(In reply to Peter Hutterer from comment #31)
> I'd say this is a bug. if the scrolling isn't triggered by a tilt wheel we
> should accommodate for this with a new, more correct, scroll sources. Please
> file a separate bug for this and make it block this one.

Scrolling is triggered by tilting the wheel, the issue is that libinput reports the scroll events as wheel events, because there i

Oops, I didn't mean to post that last comment.

Created attachment 128233 [details] [review]
Improved "Scroll Distance" option patch

This patch fixes the code style issues of the original patch and contains a second version of the documentation. The new documentation is written as if bug #98881 has been implemented.

The only change to the behavior of the code is that the "libinput Scroll Distance" option is not initialized (so it doesn't show up in xinput list-props) if the device is not a pointing device.

Created attachment 128234 [details] [review]
0001-Calculate-the-required-scroll-distance-based-on-the-.patch

Your patch is pretty much there. thanks. But while I was just rewriting a few man pages sections I finally had something of an epiphany. Specifically: libinput gives us all the information we need, we're just not using it. There is no need for a config option, we can do this automatically.

This patch uses your approach by setting an arbitrary scroll dist (100). When the first event comes in, it calculates the portion we need to advance by for each discrete click. In your case, 7 events would add up to one full distance. Then it sends out the events for that and everything should be hunky-dory.

Bonus: no user configuration needed, it'll just work out of the box. We rely on libinput to be configured correctly, but that's IMO better than having some config in libinput and then overriding it again in the xorg driver.

Give it a try please, thanks.

I tried the patch, and works for me (with click angle = 2), but I think it might work better with certain click angles if you changed the math that calculates the scroll factor.

Here is a graph of the scroll factor using the equation in your patch: https://s15.postimg.org/otmbmi923/patch_graph.png

The graph shows that your implementation works well for click angles >= 2, but every click angle between 1 and 2 results in a scroll factor of 14. This is about right for 1, but as you get closer to 2, this will result in scroll factors that are nearly twice as large as they should be. This problem is not just theoretical, because my mouse will have a click angle of 1.875 once MOUSE_WHEEL_CLICK_COUNT becomes available. With your implementation, this will cause my mouse to have a scroll factor of 14 rather than the correct value of 8. Also, if the click angle is less than 1, this will cause a divide by zero.

https://s15.postimg.org/u6b60mwyj/patch_full_click_angle.png
This graph provides further illustration of the problem. It shows the "full click angle": the number of degrees of movement required to scroll one full scroll unit. As the click angle approaches 1, the full click angle approaches ~28, nearly twice the optimum value of 15.

My solution to this is to change line 1367 to "f = round(15 / angle);". This produces this scroll factor graph:
https://s15.postimg.org/ow679ccpn/new_graph.png

Small click angles produce better scroll factors, while larger ones behave similarly to your implementation. A click angle of 1.875 will produce the correct scroll factor of 8.

Here is a graph of the "full click angle" for my implementation:
https://s15.postimg.org/okoqwkw9n/new_full_click_angle.png

Please let me know what you think and if you see any errors in my analysis.

yes, this looks a lot better. I'm not sure about the 6->12 mapping, I think for that case it'd be better to map to 18 mapping but that's something I can live with. We can tweak this once we get more feedback from those affected. Thanks heaps, that's great feedback.

What software did you use for those graphs btw? They look nicer than a gnuplot :)

Created attachment 128256 [details]
graph to illustrate click angle -> full click angle mapping

Attaching your graph here for the archives. Graph shows the physical click angle (x) vs the click angle needed to trigger one legacy scroll button press (y). For example, a physical click angle of 2 degrees maps to 15 degrees before the scroll distance is filled and a button event is generated.

(In reply to Peter Hutterer from comment #38)
> What software did you use for those graphs btw? They look nicer than a
> gnuplot :)

Mathematica. I'm a college student, so I get it for free through my school.

ah, unfair. I don't have access to fancy stuff like that ;) guess I'll stick with gnuplot then. for the archives: gnuplot commands:

gnuplot> round(a) = floor(a + 0.5)
gnuplot> f(a) = round(15/a)
gnuplot> set xrange [0:10]
gnuplot> plot x * f(x)


Patches are on the list: https://patchwork.freedesktop.org/series/16060/
Have a look at 1/3 in this series, it explains why I changed the default vdist back to 15. Still works the same since motion axes are floating point anyway but the main takeaway is: for wheels the value doesn't matter but for touchpads it does. And since we cannot know whether a device is a wheel mouse until we get the first event, this was the easiest approach to fix it.

Thanks for the help and patience

commit 0dad7408fac3b69c4b6ab7705f39f790d7ba20c2
Author: Peter Hutterer <peter.hutterer@who-t.net>
Date:   Mon Nov 28 14:09:06 2016 +1000

    Calculate the required scroll distance based on the angle

The correct solution to this, would be to stop reading discrete values for scroll-wheel, but find a way to pass the angle-per-step to the xorg driver and report that correctly in the scroll valuator, so client apps can calculate the right distance.

Btw, the default scroll step is either 1 or 120.. 15 is nonsense. Typically non-smooth wheels are recognized by having a scroll-step of 1. The alternative 120 is the old standard because 120px is moved per step.

Created attachment 128980 [details] [review]
simple-scrolldist-patch.diff

I wrote this much simpler patch to just fix smooth scrolling until libinput has the necessary API to fix the issue correctly.