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
1=== modified file 'src/platforms/evdev/libinput_device.cpp'
2--- src/platforms/evdev/libinput_device.cpp 2015-11-30 16:34:46 +0000
3+++ src/platforms/evdev/libinput_device.cpp 2015-12-07 01:20:19 +0000
4@@ -202,14 +202,34 @@
5 auto const action = mir_pointer_action_motion;
6 auto const relative_x_value = 0.0f;
7 auto const relative_y_value = 0.0f;
8- auto const hscroll_value = libinput_event_pointer_has_axis(pointer, LIBINPUT_POINTER_AXIS_SCROLL_HORIZONTAL)
9- ? horizontal_scroll_scale * libinput_event_pointer_get_axis_value(pointer,
10- LIBINPUT_POINTER_AXIS_SCROLL_HORIZONTAL)
11- : 0.0f;
12- auto const vscroll_value = libinput_event_pointer_has_axis(pointer, LIBINPUT_POINTER_AXIS_SCROLL_VERTICAL)
13- ? vertical_scroll_scale * libinput_event_pointer_get_axis_value(pointer,
14- LIBINPUT_POINTER_AXIS_SCROLL_VERTICAL)
15- : 0.0f;
16+
17+ auto hscroll_value = 0.0f;
18+ auto vscroll_value = 0.0f;
19+ if (libinput_event_pointer_get_axis_source(pointer) == LIBINPUT_POINTER_AXIS_SOURCE_WHEEL)
20+ {
21+ if (libinput_event_pointer_has_axis(pointer, LIBINPUT_POINTER_AXIS_SCROLL_HORIZONTAL))
22+ hscroll_value = horizontal_scroll_scale * libinput_event_pointer_get_axis_value_discrete(
23+ pointer, LIBINPUT_POINTER_AXIS_SCROLL_HORIZONTAL);
24+ if (libinput_event_pointer_has_axis(pointer, LIBINPUT_POINTER_AXIS_SCROLL_VERTICAL))
25+ vscroll_value = -vertical_scroll_scale * libinput_event_pointer_get_axis_value_discrete(
26+ pointer, LIBINPUT_POINTER_AXIS_SCROLL_VERTICAL);
27+ }
28+ else
29+ {
30+ // by default libinput assumes that wheel ticks represent a rotation of 15 degrees. This relation
31+ // is used to map wheel rotations to 'scroll units'. To map the immediate scroll units received
32+ // from gesture based scrolling we invert that transformation here.
33+ auto const scroll_units_to_ticks = 15.0f;
34+ if (libinput_event_pointer_has_axis(pointer, LIBINPUT_POINTER_AXIS_SCROLL_HORIZONTAL))
35+ hscroll_value = horizontal_scroll_scale *
36+ libinput_event_pointer_get_axis_value(pointer, LIBINPUT_POINTER_AXIS_SCROLL_HORIZONTAL) /
37+ scroll_units_to_ticks;
38+
39+ if (libinput_event_pointer_has_axis(pointer, LIBINPUT_POINTER_AXIS_SCROLL_VERTICAL))
40+ vscroll_value = -vertical_scroll_scale *
41+ libinput_event_pointer_get_axis_value(pointer, LIBINPUT_POINTER_AXIS_SCROLL_VERTICAL) /
42+ scroll_units_to_ticks;
43+ }
44
45 report->received_event_from_kernel(time.count(), EV_REL, 0, 0);
46 return builder->pointer_event(time, action, button_state, hscroll_value, vscroll_value, relative_x_value,
47
48=== modified file 'tests/include/mir/test/doubles/mock_libinput.h'
49--- tests/include/mir/test/doubles/mock_libinput.h 2015-10-05 02:18:18 +0000
50+++ tests/include/mir/test/doubles/mock_libinput.h 2015-12-07 01:20:19 +0000
51@@ -71,6 +71,8 @@
52 MOCK_METHOD1(libinput_event_pointer_get_seat_button_count, uint32_t(libinput_event_pointer*));
53 MOCK_METHOD1(libinput_event_pointer_get_axis, libinput_pointer_axis(libinput_event_pointer*));
54 MOCK_METHOD2(libinput_event_pointer_get_axis_value, double(libinput_event_pointer*, libinput_pointer_axis));
55+ MOCK_METHOD1(libinput_event_pointer_get_axis_source, libinput_pointer_axis_source(libinput_event_pointer*));
56+ MOCK_METHOD2(libinput_event_pointer_get_axis_value_discrete, double(libinput_event_pointer*, libinput_pointer_axis));
57 MOCK_METHOD2(libinput_event_pointer_has_axis,int(libinput_event_pointer *,libinput_pointer_axis));
58
59 MOCK_METHOD1(libinput_event_touch_get_time, uint32_t(libinput_event_touch*));
60
61=== modified file 'tests/mir_test_doubles/mock_libinput.cpp'
62--- tests/mir_test_doubles/mock_libinput.cpp 2015-10-05 02:18:18 +0000
63+++ tests/mir_test_doubles/mock_libinput.cpp 2015-12-07 01:20:19 +0000
64@@ -197,11 +197,21 @@
65 return global_libinput->libinput_event_pointer_get_axis(event);
66 }
67
68+libinput_pointer_axis_source libinput_event_pointer_get_axis_source(libinput_event_pointer* event)
69+{
70+ return global_libinput->libinput_event_pointer_get_axis_source(event);
71+}
72+
73 double libinput_event_pointer_get_axis_value(libinput_event_pointer* event, libinput_pointer_axis axis)
74 {
75 return global_libinput->libinput_event_pointer_get_axis_value(event, axis);
76 }
77
78+double libinput_event_pointer_get_axis_value_discrete(libinput_event_pointer* event, libinput_pointer_axis axis)
79+{
80+ return global_libinput->libinput_event_pointer_get_axis_value_discrete(event, axis);
81+}
82+
83 int libinput_event_pointer_has_axis(libinput_event_pointer* event, libinput_pointer_axis axis)
84 {
85 return global_libinput->libinput_event_pointer_has_axis(event, axis);
86
87=== modified file 'tests/unit-tests/input/evdev/test_libinput_device.cpp'
88--- tests/unit-tests/input/evdev/test_libinput_device.cpp 2015-11-30 16:34:46 +0000
89+++ tests/unit-tests/input/evdev/test_libinput_device.cpp 2015-12-07 01:20:19 +0000
90@@ -338,6 +338,30 @@
91 .WillByDefault(Return(pointer_event));
92 ON_CALL(mock_libinput, libinput_event_pointer_get_time_usec(pointer_event))
93 .WillByDefault(Return(event_time));
94+ ON_CALL(mock_libinput, libinput_event_pointer_get_axis_source(pointer_event))
95+ .WillByDefault(Return(LIBINPUT_POINTER_AXIS_SOURCE_WHEEL));
96+ ON_CALL(mock_libinput, libinput_event_pointer_has_axis(pointer_event, LIBINPUT_POINTER_AXIS_SCROLL_HORIZONTAL))
97+ .WillByDefault(Return(horizontal!=0.0));
98+ ON_CALL(mock_libinput, libinput_event_pointer_has_axis(pointer_event, LIBINPUT_POINTER_AXIS_SCROLL_VERTICAL))
99+ .WillByDefault(Return(vertical!=0.0));
100+ ON_CALL(mock_libinput, libinput_event_pointer_get_axis_value_discrete(pointer_event, LIBINPUT_POINTER_AXIS_SCROLL_VERTICAL))
101+ .WillByDefault(Return(vertical));
102+ ON_CALL(mock_libinput, libinput_event_pointer_get_axis_value_discrete(pointer_event, LIBINPUT_POINTER_AXIS_SCROLL_HORIZONTAL))
103+ .WillByDefault(Return(horizontal));
104+ }
105+
106+ void setup_finger_axis_event(libinput_event* event, uint64_t event_time, double horizontal, double vertical)
107+ {
108+ auto pointer_event = reinterpret_cast<libinput_event_pointer*>(event);
109+
110+ ON_CALL(mock_libinput, libinput_event_get_type(event))
111+ .WillByDefault(Return(LIBINPUT_EVENT_POINTER_AXIS));
112+ ON_CALL(mock_libinput, libinput_event_get_pointer_event(event))
113+ .WillByDefault(Return(pointer_event));
114+ ON_CALL(mock_libinput, libinput_event_pointer_get_time_usec(pointer_event))
115+ .WillByDefault(Return(event_time));
116+ ON_CALL(mock_libinput, libinput_event_pointer_get_axis_source(pointer_event))
117+ .WillByDefault(Return(LIBINPUT_POINTER_AXIS_SOURCE_FINGER));
118 ON_CALL(mock_libinput, libinput_event_pointer_has_axis(pointer_event, LIBINPUT_POINTER_AXIS_SCROLL_HORIZONTAL))
119 .WillByDefault(Return(horizontal!=0.0));
120 ON_CALL(mock_libinput, libinput_event_pointer_has_axis(pointer_event, LIBINPUT_POINTER_AXIS_SCROLL_VERTICAL))
121@@ -616,8 +640,8 @@
122 InSequence seq;
123 // expect two scroll events..
124 EXPECT_CALL(mock_builder,
125- pointer_event(time_stamp_1, mir_pointer_action_motion, 0, 0.0f, 20.0f, 0.0f, 0.0f));
126- EXPECT_CALL(mock_sink, handle_input(mt::PointerAxisChange(mir_pointer_axis_vscroll, 20.0f)));
127+ pointer_event(time_stamp_1, mir_pointer_action_motion, 0, 0.0f, -20.0f, 0.0f, 0.0f));
128+ EXPECT_CALL(mock_sink, handle_input(mt::PointerAxisChange(mir_pointer_axis_vscroll, -20.0f)));
129 EXPECT_CALL(mock_builder,
130 pointer_event(time_stamp_2, mir_pointer_action_motion, 0, 5.0f, 0.0f, 0.0f, 0.0f));
131 EXPECT_CALL(mock_sink, handle_input(mt::PointerAxisChange(mir_pointer_axis_hscroll, 5.0f)));
132@@ -807,7 +831,7 @@
133 setup_axis_event(fake_event_2, event_time_2, -2.0, 0.0);
134
135 // expect two scroll events..
136- EXPECT_CALL(mock_sink, handle_input(mt::PointerAxisChange(mir_pointer_axis_vscroll, -3.0f)));
137+ EXPECT_CALL(mock_sink, handle_input(mt::PointerAxisChange(mir_pointer_axis_vscroll, 3.0f)));
138 EXPECT_CALL(mock_sink, handle_input(mt::PointerAxisChange(mir_pointer_axis_hscroll, -10.0f)));
139
140 setup_pointer_configuration(mouse.device(), 1, mir_pointer_handedness_right);
141@@ -829,6 +853,26 @@
142 EXPECT_THAT(val.is_set(), Eq(false));
143 }
144
145+TEST_F(LibInputDeviceOnTouchpad, process_event_handles_scroll)
146+{
147+ setup_finger_axis_event(fake_event_1, event_time_1, 0.0, 150.0);
148+ setup_finger_axis_event(fake_event_2, event_time_2, 15.0, 0.0);
149+
150+ InSequence seq;
151+ // expect two scroll events..
152+ EXPECT_CALL(mock_builder,
153+ pointer_event(time_stamp_1, mir_pointer_action_motion, 0, 0.0f, -10.0f, 0.0f, 0.0f));
154+ EXPECT_CALL(mock_sink, handle_input(mt::PointerAxisChange(mir_pointer_axis_vscroll, -10.0f)));
155+ EXPECT_CALL(mock_builder,
156+ pointer_event(time_stamp_2, mir_pointer_action_motion, 0, 1.0f, 0.0f, 0.0f, 0.0f));
157+ EXPECT_CALL(mock_sink, handle_input(mt::PointerAxisChange(mir_pointer_axis_hscroll, 1.0f)));
158+
159+ touchpad.start(&mock_sink, &mock_builder);
160+ touchpad.process_event(fake_event_1);
161+ touchpad.process_event(fake_event_2);
162+
163+}
164+
165 TEST_F(LibInputDeviceOnTouchpad, reads_touchpad_settings_from_libinput)
166 {
167 setup_touchpad_configuration(fake_device, mir_touchpad_click_mode_finger_count,

Subscribers

People subscribed via source and target branches