Mir

Merge lp:~andreas-pokorny/mir/alternative-fix-1522673 into lp:mir

  1. alternative-fix-1522673
  2. Merge into development-branch
Proposed by Andreas Pokorny
Status: Merged
Approved by: Andreas Pokorny
Approved revision: no longer in the source branch.
Merged at revision: 3179
Proposed branch: lp:~andreas-pokorny/mir/alternative-fix-1522673
Merge into: lp:mir
Diff against target: 167 lines (+87/-11)
4 files modified
src/platforms/evdev/libinput_device.cpp (+28/-8)
tests/include/mir/test/doubles/mock_libinput.h (+2/-0)
tests/mir_test_doubles/mock_libinput.cpp (+10/-0)
tests/unit-tests/input/evdev/test_libinput_device.cpp (+47/-3)
To merge this branch: bzr merge lp:~andreas-pokorny/mir/alternative-fix-1522673
Reviewer Review Type Date Requested Status
PS Jenkins bot (community) continuous-integration Approve
Alan Griffiths Approve
Daniel van Vugt Approve
Review via email: mp+279713@code.launchpad.net

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

Commit message

input-evdev: use ticks where possible, or scale scroll units and invert vertical scroll axis

Description of the change

This is an alternative fix, described here
https://code.launchpad.net/~vanvugt/mir/fix-1522673/+merge/279551/comments/708263

This changes input-evdev platform to read ticks instead of scroll units when possible, or scale the scroll units to assumed ticks..

To post a comment you must log in.
Revision history for this message
Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal

Changing the meaning of mir_pointer_event_axis_value() is surely an API break?

Do we really know the effect on all downstreams?

review: Needs Information
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal

PASSED: Continuous integration, rev:3177
http://jenkins.qa.ubuntu.com/job/mir-ci/5777/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5208
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4114
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5156
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-xenial-touch/103/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/106
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/106/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/106
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/106/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5156
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5156/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7686
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/25822
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/101/console

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

review: Approve (continuous-integration)
Revision history for this message
Andreas Pokorny (andreas-pokorny) wrote : Posted in a previous version of this proposal

Agreed going through the different downstreams. This is probably not the right way to do. We should instead come up with a separate axis.

Revision history for this message
Andreas Pokorny (andreas-pokorny) wrote : Posted in a previous version of this proposal

meh .. i will resubmit this mp

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:3178
http://jenkins.qa.ubuntu.com/job/mir-ci/5781/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/5212
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/4118
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/5160
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-xenial-touch/108/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/110
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-amd64-ci/110/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/110
        deb: http://jenkins.qa.ubuntu.com/job/mir-xenial-i386-ci/110/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5160
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/5160/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-touch/7690
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/25830
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-xenial-armhf/106/console

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

review: Approve (continuous-integration)
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

I don't fully understand but manual testing shows it works without modifying the existing mir_proving_server code. So sanity check passed :)

Theoretically I think treating the wheel as different to non-wheel scroll axes is a very good idea. Because a mouse wheel is always (at least not yet proven otherwise) just two buttons emitting discrete events for buttons 4/5. It has no scale; it's just two booleans, so the input library has no right to impose a scale on that. On that note, I will be proposing another related branch...

review: Approve
Revision history for this message
Daniel van Vugt (vanvugt) wrote :

I would also be happy to abolish the scroll axis for mouse wheels (just a passing comment albeit an API break). Because the scroll axis thing is much more ambiguous than simply responding to button 4/5 events. Plus I have to map these back into button 4/5 events in Xmir anyway :P

A second reason for emitting the real wheel button events is for consistency with horizontal scrolling. I've just got a Microsoft Comfort Mouse that also has "horizontal" wheel scrolling, but that's just button 6/7 events when the wheel sways left and right.

Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

Seems sensible

review: Approve
Revision history for this message
Andreas Pokorny (andreas-pokorny) wrote :

Yes keeping them separate might be a good idea.. Kinetic scrolling triggered by 'scroll' events would be a toolkit feature that makes very little sense to be triggered by mouse wheels ticks, but reasonable for touchpad gestures...

Revision history for this message
PS Jenkins bot (ps-jenkins) :
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'src/platforms/evdev/libinput_device.cpp'
--- src/platforms/evdev/libinput_device.cpp 2015-11-30 16:34:46 +0000
+++ src/platforms/evdev/libinput_device.cpp 2015-12-07 01:20:19 +0000
@@ -202,14 +202,34 @@
202 auto const action = mir_pointer_action_motion;202 auto const action = mir_pointer_action_motion;
203 auto const relative_x_value = 0.0f;203 auto const relative_x_value = 0.0f;
204 auto const relative_y_value = 0.0f;204 auto const relative_y_value = 0.0f;
205 auto const hscroll_value = libinput_event_pointer_has_axis(pointer, LIBINPUT_POINTER_AXIS_SCROLL_HORIZONTAL)205
206 ? horizontal_scroll_scale * libinput_event_pointer_get_axis_value(pointer,206 auto hscroll_value = 0.0f;
207 LIBINPUT_POINTER_AXIS_SCROLL_HORIZONTAL)207 auto vscroll_value = 0.0f;
208 : 0.0f;208 if (libinput_event_pointer_get_axis_source(pointer) == LIBINPUT_POINTER_AXIS_SOURCE_WHEEL)
209 auto const vscroll_value = libinput_event_pointer_has_axis(pointer, LIBINPUT_POINTER_AXIS_SCROLL_VERTICAL)209 {
210 ? vertical_scroll_scale * libinput_event_pointer_get_axis_value(pointer,210 if (libinput_event_pointer_has_axis(pointer, LIBINPUT_POINTER_AXIS_SCROLL_HORIZONTAL))
211 LIBINPUT_POINTER_AXIS_SCROLL_VERTICAL)211 hscroll_value = horizontal_scroll_scale * libinput_event_pointer_get_axis_value_discrete(
212 : 0.0f;212 pointer, LIBINPUT_POINTER_AXIS_SCROLL_HORIZONTAL);
213 if (libinput_event_pointer_has_axis(pointer, LIBINPUT_POINTER_AXIS_SCROLL_VERTICAL))
214 vscroll_value = -vertical_scroll_scale * libinput_event_pointer_get_axis_value_discrete(
215 pointer, LIBINPUT_POINTER_AXIS_SCROLL_VERTICAL);
216 }
217 else
218 {
219 // by default libinput assumes that wheel ticks represent a rotation of 15 degrees. This relation
220 // is used to map wheel rotations to 'scroll units'. To map the immediate scroll units received
221 // from gesture based scrolling we invert that transformation here.
222 auto const scroll_units_to_ticks = 15.0f;
223 if (libinput_event_pointer_has_axis(pointer, LIBINPUT_POINTER_AXIS_SCROLL_HORIZONTAL))
224 hscroll_value = horizontal_scroll_scale *
225 libinput_event_pointer_get_axis_value(pointer, LIBINPUT_POINTER_AXIS_SCROLL_HORIZONTAL) /
226 scroll_units_to_ticks;
227
228 if (libinput_event_pointer_has_axis(pointer, LIBINPUT_POINTER_AXIS_SCROLL_VERTICAL))
229 vscroll_value = -vertical_scroll_scale *
230 libinput_event_pointer_get_axis_value(pointer, LIBINPUT_POINTER_AXIS_SCROLL_VERTICAL) /
231 scroll_units_to_ticks;
232 }
213233
214 report->received_event_from_kernel(time.count(), EV_REL, 0, 0);234 report->received_event_from_kernel(time.count(), EV_REL, 0, 0);
215 return builder->pointer_event(time, action, button_state, hscroll_value, vscroll_value, relative_x_value,235 return builder->pointer_event(time, action, button_state, hscroll_value, vscroll_value, relative_x_value,
216236
=== modified file 'tests/include/mir/test/doubles/mock_libinput.h'
--- tests/include/mir/test/doubles/mock_libinput.h 2015-10-05 02:18:18 +0000
+++ tests/include/mir/test/doubles/mock_libinput.h 2015-12-07 01:20:19 +0000
@@ -71,6 +71,8 @@
71 MOCK_METHOD1(libinput_event_pointer_get_seat_button_count, uint32_t(libinput_event_pointer*));71 MOCK_METHOD1(libinput_event_pointer_get_seat_button_count, uint32_t(libinput_event_pointer*));
72 MOCK_METHOD1(libinput_event_pointer_get_axis, libinput_pointer_axis(libinput_event_pointer*));72 MOCK_METHOD1(libinput_event_pointer_get_axis, libinput_pointer_axis(libinput_event_pointer*));
73 MOCK_METHOD2(libinput_event_pointer_get_axis_value, double(libinput_event_pointer*, libinput_pointer_axis));73 MOCK_METHOD2(libinput_event_pointer_get_axis_value, double(libinput_event_pointer*, libinput_pointer_axis));
74 MOCK_METHOD1(libinput_event_pointer_get_axis_source, libinput_pointer_axis_source(libinput_event_pointer*));
75 MOCK_METHOD2(libinput_event_pointer_get_axis_value_discrete, double(libinput_event_pointer*, libinput_pointer_axis));
74 MOCK_METHOD2(libinput_event_pointer_has_axis,int(libinput_event_pointer *,libinput_pointer_axis));76 MOCK_METHOD2(libinput_event_pointer_has_axis,int(libinput_event_pointer *,libinput_pointer_axis));
7577
76 MOCK_METHOD1(libinput_event_touch_get_time, uint32_t(libinput_event_touch*));78 MOCK_METHOD1(libinput_event_touch_get_time, uint32_t(libinput_event_touch*));
7779
=== modified file 'tests/mir_test_doubles/mock_libinput.cpp'
--- tests/mir_test_doubles/mock_libinput.cpp 2015-10-05 02:18:18 +0000
+++ tests/mir_test_doubles/mock_libinput.cpp 2015-12-07 01:20:19 +0000
@@ -197,11 +197,21 @@
197 return global_libinput->libinput_event_pointer_get_axis(event);197 return global_libinput->libinput_event_pointer_get_axis(event);
198}198}
199199
200libinput_pointer_axis_source libinput_event_pointer_get_axis_source(libinput_event_pointer* event)
201{
202 return global_libinput->libinput_event_pointer_get_axis_source(event);
203}
204
200double libinput_event_pointer_get_axis_value(libinput_event_pointer* event, libinput_pointer_axis axis)205double libinput_event_pointer_get_axis_value(libinput_event_pointer* event, libinput_pointer_axis axis)
201{206{
202 return global_libinput->libinput_event_pointer_get_axis_value(event, axis);207 return global_libinput->libinput_event_pointer_get_axis_value(event, axis);
203}208}
204209
210double libinput_event_pointer_get_axis_value_discrete(libinput_event_pointer* event, libinput_pointer_axis axis)
211{
212 return global_libinput->libinput_event_pointer_get_axis_value_discrete(event, axis);
213}
214
205int libinput_event_pointer_has_axis(libinput_event_pointer* event, libinput_pointer_axis axis)215int libinput_event_pointer_has_axis(libinput_event_pointer* event, libinput_pointer_axis axis)
206{216{
207 return global_libinput->libinput_event_pointer_has_axis(event, axis);217 return global_libinput->libinput_event_pointer_has_axis(event, axis);
208218
=== modified file 'tests/unit-tests/input/evdev/test_libinput_device.cpp'
--- tests/unit-tests/input/evdev/test_libinput_device.cpp 2015-11-30 16:34:46 +0000
+++ tests/unit-tests/input/evdev/test_libinput_device.cpp 2015-12-07 01:20:19 +0000
@@ -338,6 +338,30 @@
338 .WillByDefault(Return(pointer_event));338 .WillByDefault(Return(pointer_event));
339 ON_CALL(mock_libinput, libinput_event_pointer_get_time_usec(pointer_event))339 ON_CALL(mock_libinput, libinput_event_pointer_get_time_usec(pointer_event))
340 .WillByDefault(Return(event_time));340 .WillByDefault(Return(event_time));
341 ON_CALL(mock_libinput, libinput_event_pointer_get_axis_source(pointer_event))
342 .WillByDefault(Return(LIBINPUT_POINTER_AXIS_SOURCE_WHEEL));
343 ON_CALL(mock_libinput, libinput_event_pointer_has_axis(pointer_event, LIBINPUT_POINTER_AXIS_SCROLL_HORIZONTAL))
344 .WillByDefault(Return(horizontal!=0.0));
345 ON_CALL(mock_libinput, libinput_event_pointer_has_axis(pointer_event, LIBINPUT_POINTER_AXIS_SCROLL_VERTICAL))
346 .WillByDefault(Return(vertical!=0.0));
347 ON_CALL(mock_libinput, libinput_event_pointer_get_axis_value_discrete(pointer_event, LIBINPUT_POINTER_AXIS_SCROLL_VERTICAL))
348 .WillByDefault(Return(vertical));
349 ON_CALL(mock_libinput, libinput_event_pointer_get_axis_value_discrete(pointer_event, LIBINPUT_POINTER_AXIS_SCROLL_HORIZONTAL))
350 .WillByDefault(Return(horizontal));
351 }
352
353 void setup_finger_axis_event(libinput_event* event, uint64_t event_time, double horizontal, double vertical)
354 {
355 auto pointer_event = reinterpret_cast<libinput_event_pointer*>(event);
356
357 ON_CALL(mock_libinput, libinput_event_get_type(event))
358 .WillByDefault(Return(LIBINPUT_EVENT_POINTER_AXIS));
359 ON_CALL(mock_libinput, libinput_event_get_pointer_event(event))
360 .WillByDefault(Return(pointer_event));
361 ON_CALL(mock_libinput, libinput_event_pointer_get_time_usec(pointer_event))
362 .WillByDefault(Return(event_time));
363 ON_CALL(mock_libinput, libinput_event_pointer_get_axis_source(pointer_event))
364 .WillByDefault(Return(LIBINPUT_POINTER_AXIS_SOURCE_FINGER));
341 ON_CALL(mock_libinput, libinput_event_pointer_has_axis(pointer_event, LIBINPUT_POINTER_AXIS_SCROLL_HORIZONTAL))365 ON_CALL(mock_libinput, libinput_event_pointer_has_axis(pointer_event, LIBINPUT_POINTER_AXIS_SCROLL_HORIZONTAL))
342 .WillByDefault(Return(horizontal!=0.0));366 .WillByDefault(Return(horizontal!=0.0));
343 ON_CALL(mock_libinput, libinput_event_pointer_has_axis(pointer_event, LIBINPUT_POINTER_AXIS_SCROLL_VERTICAL))367 ON_CALL(mock_libinput, libinput_event_pointer_has_axis(pointer_event, LIBINPUT_POINTER_AXIS_SCROLL_VERTICAL))
@@ -616,8 +640,8 @@
616 InSequence seq;640 InSequence seq;
617 // expect two scroll events..641 // expect two scroll events..
618 EXPECT_CALL(mock_builder,642 EXPECT_CALL(mock_builder,
619 pointer_event(time_stamp_1, mir_pointer_action_motion, 0, 0.0f, 20.0f, 0.0f, 0.0f));643 pointer_event(time_stamp_1, mir_pointer_action_motion, 0, 0.0f, -20.0f, 0.0f, 0.0f));
620 EXPECT_CALL(mock_sink, handle_input(mt::PointerAxisChange(mir_pointer_axis_vscroll, 20.0f)));644 EXPECT_CALL(mock_sink, handle_input(mt::PointerAxisChange(mir_pointer_axis_vscroll, -20.0f)));
621 EXPECT_CALL(mock_builder,645 EXPECT_CALL(mock_builder,
622 pointer_event(time_stamp_2, mir_pointer_action_motion, 0, 5.0f, 0.0f, 0.0f, 0.0f));646 pointer_event(time_stamp_2, mir_pointer_action_motion, 0, 5.0f, 0.0f, 0.0f, 0.0f));
623 EXPECT_CALL(mock_sink, handle_input(mt::PointerAxisChange(mir_pointer_axis_hscroll, 5.0f)));647 EXPECT_CALL(mock_sink, handle_input(mt::PointerAxisChange(mir_pointer_axis_hscroll, 5.0f)));
@@ -807,7 +831,7 @@
807 setup_axis_event(fake_event_2, event_time_2, -2.0, 0.0);831 setup_axis_event(fake_event_2, event_time_2, -2.0, 0.0);
808832
809 // expect two scroll events..833 // expect two scroll events..
810 EXPECT_CALL(mock_sink, handle_input(mt::PointerAxisChange(mir_pointer_axis_vscroll, -3.0f)));834 EXPECT_CALL(mock_sink, handle_input(mt::PointerAxisChange(mir_pointer_axis_vscroll, 3.0f)));
811 EXPECT_CALL(mock_sink, handle_input(mt::PointerAxisChange(mir_pointer_axis_hscroll, -10.0f)));835 EXPECT_CALL(mock_sink, handle_input(mt::PointerAxisChange(mir_pointer_axis_hscroll, -10.0f)));
812836
813 setup_pointer_configuration(mouse.device(), 1, mir_pointer_handedness_right);837 setup_pointer_configuration(mouse.device(), 1, mir_pointer_handedness_right);
@@ -829,6 +853,26 @@
829 EXPECT_THAT(val.is_set(), Eq(false));853 EXPECT_THAT(val.is_set(), Eq(false));
830}854}
831855
856TEST_F(LibInputDeviceOnTouchpad, process_event_handles_scroll)
857{
858 setup_finger_axis_event(fake_event_1, event_time_1, 0.0, 150.0);
859 setup_finger_axis_event(fake_event_2, event_time_2, 15.0, 0.0);
860
861 InSequence seq;
862 // expect two scroll events..
863 EXPECT_CALL(mock_builder,
864 pointer_event(time_stamp_1, mir_pointer_action_motion, 0, 0.0f, -10.0f, 0.0f, 0.0f));
865 EXPECT_CALL(mock_sink, handle_input(mt::PointerAxisChange(mir_pointer_axis_vscroll, -10.0f)));
866 EXPECT_CALL(mock_builder,
867 pointer_event(time_stamp_2, mir_pointer_action_motion, 0, 1.0f, 0.0f, 0.0f, 0.0f));
868 EXPECT_CALL(mock_sink, handle_input(mt::PointerAxisChange(mir_pointer_axis_hscroll, 1.0f)));
869
870 touchpad.start(&mock_sink, &mock_builder);
871 touchpad.process_event(fake_event_1);
872 touchpad.process_event(fake_event_2);
873
874}
875
832TEST_F(LibInputDeviceOnTouchpad, reads_touchpad_settings_from_libinput)876TEST_F(LibInputDeviceOnTouchpad, reads_touchpad_settings_from_libinput)
833{877{
834 setup_touchpad_configuration(fake_device, mir_touchpad_click_mode_finger_count,878 setup_touchpad_configuration(fake_device, mir_touchpad_click_mode_finger_count,

Subscribers

People subscribed via source and target branches