Mir

examples/server_example_input_device_config.cpp (+30/-11)
examples/server_example_input_device_config.h (+2/-8)
include/client/mir_toolkit/mir_input_device.h (+15/-0)
include/platform/mir/input/pointer_settings.h (+4/-0)
include/server/mir/input/pointer_configuration.h (+10/-4)
src/platforms/evdev/libinput_device.cpp (+16/-9)
src/server/input/default_device.cpp (+4/-2)
tests/include/mir/test/doubles/mock_libinput.h (+2/-0)
tests/mir_test_doubles/mock_libinput.cpp (+12/-0)
tests/mir_test_framework/fake_input_device_impl.cpp (+7/-4)
tests/unit-tests/input/evdev/test_libinput_device.cpp (+16/-7)

To merge this branch:

bzr merge lp://staging/~andreas-pokorny/mir/add-option-to-disable-acceleration

Related bugs:

Bug #1522295: [regression] Motion event smoothness lost with the introduction of libinput	Medium	Triaged
Bug #1524145: [regression] Mouse movement is non-linear (annoyingly so) since introducing libinput	Medium	Triaged
Bug #1528109: [regression] Cursor mostly does not move at all in response to slow mouse movement, but then sometimes jumps	High	Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration	2015-12-15	Approve on 2016-01-21
Daniel van Vugt		2015-12-15	Approve on 2016-01-20
Mir CI Bot	continuous-integration		Approve on 2016-01-19
Kevin DuBois (community)			Approve on 2016-01-13
Review via email: mp+280593@code.staging.launchpad.net

This proposal supersedes a proposal from 2015-12-14.

Commit message

add an option to select pointer acceleration profile

Allow the selection of one out of three porfiles: diabled, flat, adaptive. The last one is adaptive to the poimter movement, and our previous default behavior.

Description of the change

This MP adds a configuration option to select the cursor acceleration profile for pointing devices. Libinput offers three profiles including one that disables acceleration. Flat and disabled are now also simulated by the fake input devices.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-12-15: Posted in a previous version of this proposal

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5847/rebuild

review: Approve (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2015-12-15: Posted in a previous version of this proposal

Thanks for putting the time into this. I suspect this is slightly the wrong approach though:

+ bool enable_cursor_acceleration{true};

would be better replaced by an enum that matches enum libinput_config_accel_profile:

http://wayland.freedesktop.org/libinput/doc/latest/libinput_8h.html#ad63796972347f318b180e322e35cee79

So we could set it as LIBINPUT_CONFIG_ACCEL_PROFILE_NONE (or FLAT or ADAPTIVE) instead of enable_cursor_acceleration=false.

review: Needs Fixing

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2015-12-15: Posted in a previous version of this proposal

And therefore we would end up calling libinput_device_config_accel_set_profile:
http://wayland.freedesktop.org/libinput/doc/latest/group__config.html#ga6e72a6214840d76c8a43d3753c1d549d

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2015-12-15: Posted in a previous version of this proposal

Agreed. I guess that would turn that into a more permanent solution.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-12-15: Posted in a previous version of this proposal

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5850/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-12-15:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5853/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-12-15:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5854/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-12-15: Posted in a previous version of this proposal

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5852/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-12-15:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5856/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2015-12-15:

^ needs a newer libinput.. will land with 0.18...

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2015-12-17:

Looking good except:

(1) Dead code needs removing: bool enable_cursor_acceleration{true};

(2) I wonder if we can use more concise and accurate naming than libinput does?...

  MirPointerAccelerationProfile --> MirPointerAcceleration
  mir_pointer_acceleration_profile_none --> mir_pointer_acceleration_none
  mir_pointer_acceleration_profile_flat --> mir_pointer_acceleration_constant (is that accurate?)
  mir_pointer_acceleration_profile_adaptive --> mir_pointer_acceleration_adaptive

review: Needs Fixing

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-12-17:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5871/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2015-12-18:

"acceleration" is already a noun so we don't need to call it "acceleration_profile". Drop the "profile" methinks.

Asking people to remember how to spell acceleration is bad enough without adding extra words :)

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2015-12-18:

@duflu: in both the enum and commandline option?

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2015-12-18:

I think in both cases we could leave the profile out..

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2015-12-19:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/5900/rebuild

review: Approve (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2015-12-21:

Oh, we need to check and verify that the libinput API is not misusing the word "acceleration" like others do. If they are using the word acceleration accurately, then curiously there is no option to change the pointer speed without also accelerating.

We need to check the libinput source and make sure their terminology is correct before we do the same.

review: Needs Information

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2015-12-21:

Looking at the libinput code and docs, it gives conflicting answers as to what "flat acceleration" means. In the code it kind of looks like "flat acceleration" means "zero acceleration with constant velocity multiplier".

Please check my findings but I think this means LIBINPUT_CONFIG_ACCEL_PROFILE_FLAT is incorrectly named and perhaps in most profiles there is zero acceleration in the physical sense. So more accurate names would be:

  MirPointerAcceleration -> MirPointerSpeed
  mir_pointer_acceleration_none -> mir_pointer_speed_native
  mir_pointer_acceleration_constant -> mir_pointer_speed_constant (with bias 0 same as native?)
  mir_pointer_acceleration_adaptive -> mir_pointer_speed_accelerated

Sorry to be pedantic, but it's libinput that has apparently confused the physics terminology. We should try not to copy their mistakes, if they are mistakes...

review: Needs Fixing

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2015-12-21:

profile flat means that the way the relative movement is interpreted is 'constant' - independent of the current velocity of mouse - or better independent of the most recent measured pointer deltas.

So hmm I think speed_native is ok-ish but mir_pointer_speed_constant seems wrong - since the speed of the cursor will not be constant in that case. And since we used the term acceleration in the other enumeration already I would rather stay in that level of derivative. Or we change both... but what is going on is harder to describe in velocity terms.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2015-12-22:

Looking at the libinput source, I *think* LIBINPUT_CONFIG_ACCEL_PROFILE_FLAT means a constant multiple (which is configurable) of the velocity of the mouse. So constant multiple of velocity means acceleration introduced by the input stack is always exactly zero. As such, I think it's misleading to call that "acceleration_constant". It's actually a constant speed (when moving at all) with an acceleration of precisely zero, I think.

I'd be inclined to fix up the terminology for Mir rather than copy the common misnomer. But I'd also like more people to review the libinput source and check my assertion is correct.

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2015-12-22:

Ok I see your point. Still speed_constant seems not right. From the users perspective the setting will change the velocity of the movement. So the user will observe an acceleration.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2015-12-23:

Yes, the user will always observe instantaneous acceleration when velocity changes from zero to non-zero. However pointer acceleration is not about that first event, it's about the second and subsequent events. In the second and subsequent motion events your acceleration is precisely zero if your velocity is constant/flat. So a constant velocity or flat mode should not have "acceleration" in its name.

That all said, a constant velocity multiplier of anything other that one is arguably useless. Less than one and a lot of code will never move the cursor. More than one and you lose pixel accuracy on screen. I guess that's why the libinput code limits the multiplier for flat mode to 2.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2015-12-23:

(1) Naming needs clarification per above.

Manual testing results:

(2) Surprisingly --mouse-acceleration=none does not fix bug 1528109 but --mouse-acceleration=constant does. Have we misinterpreted the "none" mode? Should it be removed in favour of constant mode with no bias?

(3) The --mouse-acceleration option is missing from some servers (mir_proving_server) so still not helpful to me. I guess it or something like it needs to be promoted up into libmirserver default config at some point.

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2015-12-23:

> (1) Naming needs clarification per above.
>
> Manual testing results:
>
> (2) Surprisingly --mouse-acceleration=none does not fix bug 1528109 but
> --mouse-acceleration=constant does. Have we misinterpreted the "none" mode?
> Should it be removed in favour of constant mode with no bias?

ok will investigate that..

>
> (3) The --mouse-acceleration option is missing from some servers
> (mir_proving_server) so still not helpful to me. I guess it or something like
> it needs to be promoted up into libmirserver default config at some point.

Yeah we should do that. Hmm it would be nicer if we could make the configuration device specific.. The current unique ids are not good enough for that.. thats another story that needs fixing..

Revision history for this message

Kevin DuBois (kdub) wrote on 2016-01-07:

+ settings.handedness = left_handed? mir_pointer_handedness_left : mir_pointer_handedness_right;
nit: spacing

lgtm, pending fixing the issues Daniel brought up

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-01-08:

PASSED: Continuous integration, rev:3196
https://mir-jenkins.ubuntu.com/job/mir-ci/17/
Executed test runs:
None: https://mir-jenkins.ubuntu.com/job/generic-update-mp/17/console

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/17/rebuild

review: Approve (continuous-integration)

Revision history for this message

Kevin DuBois (kdub) wrote on 2016-01-13:

> lgtm, pending fixing the issues Daniel brought up
suppose I'll "pre-approve"

review: Approve

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2016-01-17:

Yes this is a bug in libinput. It does not handle the NONE case yet so it falls through to either adaptive high-dpi or low-dpi acceleration.

Revision history for this message

Andreas Pokorny (andreas-pokorny) wrote on 2016-01-18:

So it seems none was only intended to indicate that there is no acceleraton filter available. We could remap our enum value to flat and 'speed' zero, or document how to acchieve an unaccelerated setting. I would prefer the second option since it would avoid mapping errors when the current status is read back, and we have to interpret a float value.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2016-01-18:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/6038/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2016-01-18:

(1) constant mode means zero acceleration, so the enum should not have "acceleration" in its name. I'm just trying to be physically accurate. I think we need to either remove the word "acceleration" from the enum or rename "mir_pointer_acceleration_constant" to "mir_pointer_acceleration_none".

(2) Nit: Please try to keep code within 80 columns.

(3) Nit: Unwanted blank line and s/separates/separated/

154 + * function based on the current velocity that usually consists of a two linear
155 + *
156 + * inclines separates by a plateau.

review: Needs Fixing

Revision history for this message

Mir CI Bot (mir-ci-bot) wrote on 2016-01-19:

FAILED: Continuous integration, rev:3202
https://mir-jenkins.ubuntu.com/job/mir-ci/80/
Executed test runs:
None: https://mir-jenkins.ubuntu.com/job/generic-update-mp/80/console

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/80/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2016-01-19: