[RFC,weston] libinput: support high-resolution scroll wheels

Submitted by Peter Hutterer on Jan. 29, 2019, 6:57 a.m.

Details

Message ID 20190129065734.GA11070@jelly
State New
Series "libinput: support high-resolution scroll wheels"
Headers show

Commit Message

Peter Hutterer Jan. 29, 2019, 6:57 a.m.
The new API returns scroll wheels as fractions of 120.

Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
---
Turns out it's not the most complicated patch...

This is an RFC only because libinput hasn't been released yet, and it's
waiting on the kernel release anyway. Given the expected delay, I hope
autotools is removed by the time this becomes a mergeable.

 libweston/libinput-device.c | 6 ++++++
 meson.build                 | 9 +++++++++
 2 files changed, 15 insertions(+)

Patch hide | download patch | download mbox

diff --git a/libweston/libinput-device.c b/libweston/libinput-device.c
index e25df144..e028d246 100644
--- a/libweston/libinput-device.c
+++ b/libweston/libinput-device.c
@@ -190,9 +190,15 @@  normalize_scroll(struct libinput_event_pointer *pointer_event,
 	 */
 	switch (source) {
 	case LIBINPUT_POINTER_AXIS_SOURCE_WHEEL:
+#if HAVE_LIBINPUT_AXIS_V120
+		value = 10 * libinput_event_pointer_get_axis_value_v120(
+								   pointer_event,
+								   axis)/120.0;
+#else
 		value = 10 * libinput_event_pointer_get_axis_value_discrete(
 								   pointer_event,
 								   axis);
+#endif
 		break;
 	case LIBINPUT_POINTER_AXIS_SOURCE_FINGER:
 	case LIBINPUT_POINTER_AXIS_SOURCE_CONTINUOUS:
diff --git a/meson.build b/meson.build
index 7826dbb0..dfb10ce5 100644
--- a/meson.build
+++ b/meson.build
@@ -143,6 +143,15 @@  dep_wayland_server = dependency('wayland-server', version: '>= 1.12.0')
 dep_wayland_client = dependency('wayland-client', version: '>= 1.12.0')
 dep_pixman = dependency('pixman-1', version: '>= 0.25.2')
 dep_libinput = dependency('libinput', version: '>= 0.8.0')
+have_libinput_axis_v120_code = '''
+#include <libinput.h>
+int main(void) { libinput_event_pointer_get_axis_value_v120(NULL, 0); }
+'''
+have_libinput_axis_v120 = cc.links(have_libinput_axis_v120_code,
+				   name: 'libinput_event_pointer_get_axis_value_v120',
+				   dependencies: dep_libinput)
+config_h.set10('HAVE_LIBINPUT_AXIS_V120', have_libinput_axis_v120)
+
 dep_libm = cc.find_library('m')
 dep_libdl = cc.find_library('dl')
 dep_libdrm = dependency('libdrm', version: '>= 2.4.68')

Comments

Pekka Paalanen Jan. 29, 2019, 1:36 p.m.
On Tue, 29 Jan 2019 16:57:34 +1000
Peter Hutterer <peter.hutterer@who-t.net> wrote:

> The new API returns scroll wheels as fractions of 120.
> 
> Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
> ---
> Turns out it's not the most complicated patch...
> 
> This is an RFC only because libinput hasn't been released yet, and it's
> waiting on the kernel release anyway. Given the expected delay, I hope
> autotools is removed by the time this becomes a mergeable.
> 
>  libweston/libinput-device.c | 6 ++++++
>  meson.build                 | 9 +++++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/libweston/libinput-device.c b/libweston/libinput-device.c
> index e25df144..e028d246 100644
> --- a/libweston/libinput-device.c
> +++ b/libweston/libinput-device.c
> @@ -190,9 +190,15 @@ normalize_scroll(struct libinput_event_pointer *pointer_event,
>  	 */
>  	switch (source) {
>  	case LIBINPUT_POINTER_AXIS_SOURCE_WHEEL:
> +#if HAVE_LIBINPUT_AXIS_V120
> +		value = 10 * libinput_event_pointer_get_axis_value_v120(
> +								   pointer_event,
> +								   axis)/120.0;

Hi Peter,

spaces needed around the operator.

> +#else
>  		value = 10 * libinput_event_pointer_get_axis_value_discrete(
>  								   pointer_event,
>  								   axis);
> +#endif
>  		break;
>  	case LIBINPUT_POINTER_AXIS_SOURCE_FINGER:
>  	case LIBINPUT_POINTER_AXIS_SOURCE_CONTINUOUS:
> diff --git a/meson.build b/meson.build
> index 7826dbb0..dfb10ce5 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -143,6 +143,15 @@ dep_wayland_server = dependency('wayland-server', version: '>= 1.12.0')
>  dep_wayland_client = dependency('wayland-client', version: '>= 1.12.0')
>  dep_pixman = dependency('pixman-1', version: '>= 0.25.2')
>  dep_libinput = dependency('libinput', version: '>= 0.8.0')
> +have_libinput_axis_v120_code = '''
> +#include <libinput.h>
> +int main(void) { libinput_event_pointer_get_axis_value_v120(NULL, 0); }
> +'''
> +have_libinput_axis_v120 = cc.links(have_libinput_axis_v120_code,
> +				   name: 'libinput_event_pointer_get_axis_value_v120',
> +				   dependencies: dep_libinput)

I guess the above gets replaced with a version check after libinput is
released?

> +config_h.set10('HAVE_LIBINPUT_AXIS_V120', have_libinput_axis_v120)
> +
>  dep_libm = cc.find_library('m')
>  dep_libdl = cc.find_library('dl')
>  dep_libdrm = dependency('libdrm', version: '>= 2.4.68')

Fine by me, but I can't claim to have actually verified the correctness
of the semantics. A Weston MR would be cool, if you prefix the title
with "WIP:" Gitlab makes it unmergeable until the prefix is removed. :-)


Thanks,
pq
Simon Ser Jan. 29, 2019, 2:19 p.m.
On Tuesday, January 29, 2019 7:57 AM, Peter Hutterer <peter.hutterer@who-t.net> wrote:
>  	switch (source) {
>  	case LIBINPUT_POINTER_AXIS_SOURCE_WHEEL:
> +#if HAVE_LIBINPUT_AXIS_V120
> +		value = 10 * libinput_event_pointer_get_axis_value_v120(
> +								   pointer_event,
> +								   axis)/120.0;
> +#else
>  		value = 10 * libinput_event_pointer_get_axis_value_discrete(
>  								   pointer_event,
>  								   axis);
> +#endif

Just a general libinput question: in wlroots, we just use
libinput_event_pointer_get_axis_value regardless of the source. Is the
multiply-by-10 workaround supposed to be implemented in all
compositors?
Peter Hutterer Jan. 29, 2019, 9:13 p.m.
On Tue, Jan 29, 2019 at 02:19:08PM +0000, Simon Ser wrote:
> On Tuesday, January 29, 2019 7:57 AM, Peter Hutterer <peter.hutterer@who-t.net> wrote:
> >  	switch (source) {
> >  	case LIBINPUT_POINTER_AXIS_SOURCE_WHEEL:
> > +#if HAVE_LIBINPUT_AXIS_V120
> > +		value = 10 * libinput_event_pointer_get_axis_value_v120(
> > +								   pointer_event,
> > +								   axis)/120.0;
> > +#else
> >  		value = 10 * libinput_event_pointer_get_axis_value_discrete(
> >  								   pointer_event,
> >  								   axis);
> > +#endif
> 
> Just a general libinput question: in wlroots, we just use
> libinput_event_pointer_get_axis_value regardless of the source. Is the
> multiply-by-10 workaround supposed to be implemented in all
> compositors?

it's a backwards-compatibility thing Weston has (and mutter too). The
wayland protocol says that pointer axis x/y are the same dimension as
pointer movement. Early during Weston's development and before libinput was
split out a mouse wheel was set to 10. libinput later changed the API in
favour of physical angles (that's what we use virtually everywhere in the
API) but the 10 stayed in place.

if you don't have the factor 10 here and you just pass libinput values on,
it means your mouse is going to scoll a hardware-dependent amount of motion 
for every wheel movement. That's 15 for 90+% of all mice, 20 for some [1],
others are quite rare.

Whether 10 is the right value or 12, 15, whatever are better I can't really
comment on, I simply never looked into it.

Cheers,
   Peter

[1] those 20 degree click mice are sold as "fine-grained scrolling", so
there's an argument to be made to normalize into a smaller range to deliver
on what the cardboard box says.
Peter Hutterer Jan. 29, 2019, 11:47 p.m.
On Tue, Jan 29, 2019 at 03:36:41PM +0200, Pekka Paalanen wrote:
> On Tue, 29 Jan 2019 16:57:34 +1000
> Peter Hutterer <peter.hutterer@who-t.net> wrote:
> 
> > The new API returns scroll wheels as fractions of 120.
> > 
> > Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
> > ---
> > Turns out it's not the most complicated patch...
> > 
> > This is an RFC only because libinput hasn't been released yet, and it's
> > waiting on the kernel release anyway. Given the expected delay, I hope
> > autotools is removed by the time this becomes a mergeable.
> > 
> >  libweston/libinput-device.c | 6 ++++++
> >  meson.build                 | 9 +++++++++
> >  2 files changed, 15 insertions(+)
> > 
> > diff --git a/libweston/libinput-device.c b/libweston/libinput-device.c
> > index e25df144..e028d246 100644
> > --- a/libweston/libinput-device.c
> > +++ b/libweston/libinput-device.c
> > @@ -190,9 +190,15 @@ normalize_scroll(struct libinput_event_pointer *pointer_event,
> >  	 */
> >  	switch (source) {
> >  	case LIBINPUT_POINTER_AXIS_SOURCE_WHEEL:
> > +#if HAVE_LIBINPUT_AXIS_V120
> > +		value = 10 * libinput_event_pointer_get_axis_value_v120(
> > +								   pointer_event,
> > +								   axis)/120.0;
> 
> Hi Peter,
> 
> spaces needed around the operator.

thx, and I will submit a MR proper anyway, this was just a FYI patch to
illustrate the world probably won't end if we add hi-res scrolling.

> > +#else
> >  		value = 10 * libinput_event_pointer_get_axis_value_discrete(
> >  								   pointer_event,
> >  								   axis);
> > +#endif
> >  		break;
> >  	case LIBINPUT_POINTER_AXIS_SOURCE_FINGER:
> >  	case LIBINPUT_POINTER_AXIS_SOURCE_CONTINUOUS:
> > diff --git a/meson.build b/meson.build
> > index 7826dbb0..dfb10ce5 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -143,6 +143,15 @@ dep_wayland_server = dependency('wayland-server', version: '>= 1.12.0')
> >  dep_wayland_client = dependency('wayland-client', version: '>= 1.12.0')
> >  dep_pixman = dependency('pixman-1', version: '>= 0.25.2')
> >  dep_libinput = dependency('libinput', version: '>= 0.8.0')
> > +have_libinput_axis_v120_code = '''
> > +#include <libinput.h>
> > +int main(void) { libinput_event_pointer_get_axis_value_v120(NULL, 0); }
> > +'''
> > +have_libinput_axis_v120 = cc.links(have_libinput_axis_v120_code,
> > +				   name: 'libinput_event_pointer_get_axis_value_v120',
> > +				   dependencies: dep_libinput)
> 
> I guess the above gets replaced with a version check after libinput is
> released?

I found the direct function check works just as well and has the benefit of
working with pre-releases (or patched versions) where needed. Esp. because
here we just look for a single API call here. Do you have a preference?

> > +config_h.set10('HAVE_LIBINPUT_AXIS_V120', have_libinput_axis_v120)
> > +
> >  dep_libm = cc.find_library('m')
> >  dep_libdl = cc.find_library('dl')
> >  dep_libdrm = dependency('libdrm', version: '>= 2.4.68')
> 
> Fine by me, but I can't claim to have actually verified the correctness
> of the semantics. A Weston MR would be cool, if you prefix the title
> with "WIP:" Gitlab makes it unmergeable until the prefix is removed. :-)

Testing a bit more yesterday, there's some issue with Firefox (double
scrolling) that I haven't identified yet, but expect the MR to show up once
I know what it is.

Cheers,
   Peter
Pekka Paalanen Jan. 30, 2019, 9:31 a.m.
On Wed, 30 Jan 2019 09:47:39 +1000
Peter Hutterer <peter.hutterer@who-t.net> wrote:

> On Tue, Jan 29, 2019 at 03:36:41PM +0200, Pekka Paalanen wrote:
> > On Tue, 29 Jan 2019 16:57:34 +1000
> > Peter Hutterer <peter.hutterer@who-t.net> wrote:
> >   
> > > The new API returns scroll wheels as fractions of 120.
> > > 
> > > Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
> > > ---
> > > Turns out it's not the most complicated patch...
> > > 
> > > This is an RFC only because libinput hasn't been released yet, and it's
> > > waiting on the kernel release anyway. Given the expected delay, I hope
> > > autotools is removed by the time this becomes a mergeable.
> > > 
> > >  libweston/libinput-device.c | 6 ++++++
> > >  meson.build                 | 9 +++++++++
> > >  2 files changed, 15 insertions(+)
> > > 
> > > diff --git a/libweston/libinput-device.c b/libweston/libinput-device.c
> > > index e25df144..e028d246 100644
> > > --- a/libweston/libinput-device.c
> > > +++ b/libweston/libinput-device.c
> > > @@ -190,9 +190,15 @@ normalize_scroll(struct libinput_event_pointer *pointer_event,
> > >  	 */
> > >  	switch (source) {
> > >  	case LIBINPUT_POINTER_AXIS_SOURCE_WHEEL:
> > > +#if HAVE_LIBINPUT_AXIS_V120
> > > +		value = 10 * libinput_event_pointer_get_axis_value_v120(
> > > +								   pointer_event,
> > > +								   axis)/120.0;  
> > 
> > Hi Peter,
> > 
> > spaces needed around the operator.  
> 
> thx, and I will submit a MR proper anyway, this was just a FYI patch to
> illustrate the world probably won't end if we add hi-res scrolling.
> 
> > > +#else
> > >  		value = 10 * libinput_event_pointer_get_axis_value_discrete(
> > >  								   pointer_event,
> > >  								   axis);
> > > +#endif
> > >  		break;
> > >  	case LIBINPUT_POINTER_AXIS_SOURCE_FINGER:
> > >  	case LIBINPUT_POINTER_AXIS_SOURCE_CONTINUOUS:
> > > diff --git a/meson.build b/meson.build
> > > index 7826dbb0..dfb10ce5 100644
> > > --- a/meson.build
> > > +++ b/meson.build
> > > @@ -143,6 +143,15 @@ dep_wayland_server = dependency('wayland-server', version: '>= 1.12.0')
> > >  dep_wayland_client = dependency('wayland-client', version: '>= 1.12.0')
> > >  dep_pixman = dependency('pixman-1', version: '>= 0.25.2')
> > >  dep_libinput = dependency('libinput', version: '>= 0.8.0')
> > > +have_libinput_axis_v120_code = '''
> > > +#include <libinput.h>
> > > +int main(void) { libinput_event_pointer_get_axis_value_v120(NULL, 0); }
> > > +'''
> > > +have_libinput_axis_v120 = cc.links(have_libinput_axis_v120_code,
> > > +				   name: 'libinput_event_pointer_get_axis_value_v120',
> > > +				   dependencies: dep_libinput)  
> > 
> > I guess the above gets replaced with a version check after libinput is
> > released?  
> 
> I found the direct function check works just as well and has the benefit of
> working with pre-releases (or patched versions) where needed. Esp. because
> here we just look for a single API call here. Do you have a preference?

Hi Peter,

maybe simply hard-require the new libinput release and don't even
bother with the #define below? But I suppose that could be inconvenient
for some.

Since it's just one function, I suppose I don't mind that much. The
version check would be much simpler in meson though.

Oh, you should probably use cc.has_function() instead of that
hand-written thing, that would help.


Thanks,
pq

> > > +config_h.set10('HAVE_LIBINPUT_AXIS_V120', have_libinput_axis_v120)
> > > +