Mir

Merge lp:~mir-team/mir/fix-1528109 into lp:mir

Proposed by Daniel van Vugt
Status: Merged
Approved by: Alan Griffiths
Approved revision: no longer in the source branch.
Merged at revision: 3464
Proposed branch: lp:~mir-team/mir/fix-1528109
Merge into: lp:mir
Diff against target: 153 lines (+58/-18)
6 files modified
include/test/mir/test/event_matchers.h (+8/-3)
src/platforms/evdev/libinput_device.cpp (+4/-5)
src/server/input/seat_input_device_tracker.cpp (+11/-9)
src/server/input/seat_input_device_tracker.h (+4/-1)
tests/unit-tests/input/evdev/test_libinput_device.cpp (+14/-0)
tests/unit-tests/input/test_seat_input_device_tracker.cpp (+17/-0)
To merge this branch: bzr merge lp:~mir-team/mir/fix-1528109
Reviewer Review Type Date Requested Status
Andreas Pokorny (community) Approve
Alan Griffiths Approve
Mir CI Bot continuous-integration Approve
Review via email: mp+291984@code.launchpad.net

Commit message

Fix unresponsive cursor to slow mouse movement (LP: #1528109)

Since we started using libinput and its acceleration curve, that actually
reduces the amount of motion we receive for slow mouse movements. A device
increment of 1 actually results in us receiving a dx or dy from libinput
of less than 1.0. So this fix ensures that such small movements no longer
get truncated to zero.

To post a comment you must log in.
Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

FAILED: Continuous integration, rev:3468
https://mir-jenkins.ubuntu.com/job/mir-ci/846/
Executed test runs:
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-mir/857/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/894
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/885
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/885
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/867/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/867
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/867/artifact/output/*zip*/output.zip
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/867/console
    FAILURE: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/867/console
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/867
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/867/artifact/output/*zip*/output.zip

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

review: Needs Fixing (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

+ auto confined{old};

See https://herbsutter.com/2013/06/05/gotw-92-auto-variables-part-1/

(part 4 applies)

review: Needs Fixing
Revision history for this message
Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:3469
https://mir-jenkins.ubuntu.com/job/mir-ci/848/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/862
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/899
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/890
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/890
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/872
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/872/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/872
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/872/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/872
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/872/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/872
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/872/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/872
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/872/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)
Revision history for this message
Alan Griffiths (alan-griffiths) wrote :

> + auto confined{old};
>
> See https://herbsutter.com/2013/06/05/gotw-92-auto-variables-part-1/
>
> (part 4 applies)

Fixed

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

yes .. lgtm

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/test/mir/test/event_matchers.h'
2--- include/test/mir/test/event_matchers.h 2016-03-23 06:39:56 +0000
3+++ include/test/mir/test/event_matchers.h 2016-04-15 15:11:30 +0000
4@@ -383,16 +383,21 @@
5 return false;
6 }
7
8-MATCHER_P2(PointerEventWithDiff, dx, dy, "")
9+MATCHER_P2(PointerEventWithDiff, expect_dx, expect_dy, "")
10 {
11 auto pev = maybe_pointer_event(to_address(arg));
12 if (pev == nullptr)
13 return false;
14 if (mir_pointer_event_action(pev) != mir_pointer_action_motion)
15 return false;
16- if (mir_pointer_event_axis_value(pev, mir_pointer_axis_relative_x) != dx)
17+ auto const error = 0.00001f;
18+ auto const actual_dx = mir_pointer_event_axis_value(pev,
19+ mir_pointer_axis_relative_x);
20+ if (std::abs(expect_dx - actual_dx) > error)
21 return false;
22- if (mir_pointer_event_axis_value(pev, mir_pointer_axis_relative_y) != dy)
23+ auto const actual_dy = mir_pointer_event_axis_value(pev,
24+ mir_pointer_axis_relative_y);
25+ if (std::abs(expect_dy - actual_dy) > error)
26 return false;
27 return true;
28 }
29
30=== modified file 'src/platforms/evdev/libinput_device.cpp'
31--- src/platforms/evdev/libinput_device.cpp 2016-03-23 06:39:56 +0000
32+++ src/platforms/evdev/libinput_device.cpp 2016-04-15 15:11:30 +0000
33@@ -171,11 +171,10 @@
34
35 report->received_event_from_kernel(time.count(), EV_REL, 0, 0);
36
37- mir::geometry::Displacement const movement{libinput_event_pointer_get_dx(pointer),
38- libinput_event_pointer_get_dy(pointer)};
39-
40- return builder->pointer_event(time, action, button_state, hscroll_value, vscroll_value, movement.dx.as_float(),
41- movement.dy.as_float());
42+ return builder->pointer_event(time, action, button_state,
43+ hscroll_value, vscroll_value,
44+ libinput_event_pointer_get_dx(pointer),
45+ libinput_event_pointer_get_dy(pointer));
46 }
47
48 mir::EventUPtr mie::LibInputDevice::convert_absolute_motion_event(libinput_event_pointer* pointer)
49
50=== modified file 'src/server/input/seat_input_device_tracker.cpp'
51--- src/server/input/seat_input_device_tracker.cpp 2016-03-23 06:39:56 +0000
52+++ src/server/input/seat_input_device_tracker.cpp 2016-04-15 15:11:30 +0000
53@@ -200,7 +200,7 @@
54
55 mir::geometry::Point mi::SeatInputDeviceTracker::cursor_position() const
56 {
57- return cursor_pos;
58+ return {cursor_x, cursor_y};
59 }
60
61 MirPointerButtons mi::SeatInputDeviceTracker::button_state() const
62@@ -210,12 +210,14 @@
63
64 void mi::SeatInputDeviceTracker::update_cursor(MirPointerEvent const* event)
65 {
66- mir::geometry::Displacement movement{
67- mir_pointer_event_axis_value(event, mir_pointer_axis_relative_x),
68- mir_pointer_event_axis_value(event, mir_pointer_axis_relative_y),
69- };
70- cursor_pos = cursor_pos + movement;
71- input_region->confine(cursor_pos);
72-
73- cursor_listener->cursor_moved_to(cursor_pos.x.as_float(), cursor_pos.y.as_float());
74+ cursor_x += mir_pointer_event_axis_value(event, mir_pointer_axis_relative_x);
75+ cursor_y += mir_pointer_event_axis_value(event, mir_pointer_axis_relative_y);
76+
77+ mir::geometry::Point const old{cursor_x, cursor_y};
78+ auto confined = old;
79+ input_region->confine(confined);
80+ if (confined.x != old.x) cursor_x = confined.x.as_int();
81+ if (confined.y != old.y) cursor_y = confined.y.as_int();
82+
83+ cursor_listener->cursor_moved_to(cursor_x, cursor_y);
84 }
85
86=== modified file 'src/server/input/seat_input_device_tracker.h'
87--- src/server/input/seat_input_device_tracker.h 2016-03-23 06:39:56 +0000
88+++ src/server/input/seat_input_device_tracker.h 2016-04-15 15:11:30 +0000
89@@ -80,7 +80,10 @@
90 std::vector<TouchVisualizer::Spot> spots;
91 };
92
93- mir::geometry::Point cursor_pos;
94+ // Libinput's acceleration curve means the cursor moves by non-integer
95+ // increments, and often less than 1.0, so float is required...
96+ float cursor_x = 0.0f, cursor_y = 0.0f;
97+
98 MirInputEventModifiers modifier;
99 MirPointerButtons buttons;
100 std::unordered_map<MirInputDeviceId, DeviceData> device_data;
101
102=== modified file 'tests/unit-tests/input/evdev/test_libinput_device.cpp'
103--- tests/unit-tests/input/evdev/test_libinput_device.cpp 2016-03-23 06:39:56 +0000
104+++ tests/unit-tests/input/evdev/test_libinput_device.cpp 2016-04-15 15:11:30 +0000
105@@ -407,6 +407,20 @@
106 process_events(mouse);
107 }
108
109+TEST_F(LibInputDeviceOnMouse, notices_slow_relative_movements)
110+{ // Regression test for LP: #1528109
111+ float x1 = 0.5f, x2 = 0.6f;
112+ float y1 = 1.7f, y2 = 1.8f;
113+
114+ EXPECT_CALL(mock_sink, handle_input(mt::PointerEventWithDiff(x1,y1)));
115+ EXPECT_CALL(mock_sink, handle_input(mt::PointerEventWithDiff(x2,y2)));
116+
117+ mouse.start(&mock_sink, &mock_builder);
118+ env.mock_libinput.setup_pointer_event(fake_device, event_time_1, x1, y1);
119+ env.mock_libinput.setup_pointer_event(fake_device, event_time_2, x2, y2);
120+ process_events(mouse);
121+}
122+
123 TEST_F(LibInputDeviceOnMouse, process_event_handles_press_and_release)
124 {
125 float const x = 0;
126
127=== modified file 'tests/unit-tests/input/test_seat_input_device_tracker.cpp'
128--- tests/unit-tests/input/test_seat_input_device_tracker.cpp 2016-02-05 10:19:39 +0000
129+++ tests/unit-tests/input/test_seat_input_device_tracker.cpp 2016-04-15 15:11:30 +0000
130@@ -149,6 +149,23 @@
131 move_x, move_y));
132 }
133
134+TEST_F(SeatInputDeviceTracker, slow_pointer_movement_updates_cursor)
135+{ // Regression test for LP: #1528109
136+ float const step = 0.25f;
137+ float const target = 3.0f;
138+
139+ for (float x = step; x <= target; x += step)
140+ EXPECT_CALL(mock_cursor_listener,
141+ cursor_moved_to(FloatEq(x), FloatEq(x))).Times(1);
142+
143+ tracker.add_device(some_device);
144+
145+ for (float x = step; x <= target; x += step)
146+ tracker.dispatch(*some_device_builder.pointer_event(
147+ arbitrary_timestamp, mir_pointer_action_motion, 0, 0.0f, 0.0f,
148+ step, step));
149+}
150+
151 TEST_F(SeatInputDeviceTracker, key_strokes_of_modifier_key_update_modifier)
152 {
153 const MirInputEventModifiers shift_left = mir_input_event_modifier_shift_left | mir_input_event_modifier_shift;

Subscribers

People subscribed via source and target branches