Mir

Merge lp:~mir-team/mir/fix-new-input-dispatcher-and-hang-head-in-shame into lp:mir

Proposed by Robert Carr
Status: Merged
Approved by: Daniel van Vugt
Approved revision: no longer in the source branch.
Merged at revision: 2656
Proposed branch: lp:~mir-team/mir/fix-new-input-dispatcher-and-hang-head-in-shame
Merge into: lp:mir
Diff against target: 93 lines (+17/-7)
4 files modified
src/server/input/default_configuration.cpp (+5/-4)
src/server/input/key_repeat_dispatcher.cpp (+9/-2)
src/server/input/key_repeat_dispatcher.h (+2/-0)
tests/unit-tests/input/test_key_repeat_dispatcher.cpp (+1/-1)
To merge this branch: bzr merge lp:~mir-team/mir/fix-new-input-dispatcher-and-hang-head-in-shame
Reviewer Review Type Date Requested Status
Daniel van Vugt Approve
Alberto Aguirre (community) Approve
PS Jenkins bot (community) continuous-integration Approve
Review via email: mp+261773@code.launchpad.net

Commit message

Fix key repeat dispatcher (touch events not working at all)
(LP: #1464174)

Description of the change

Correct: https://bugs.launchpad.net/mir/+bug/1464174

The error is obvious if you look at the key_repeat_dispatcher.cpp diff...it just wasn't forwarding touch events to the next dispatcher! I really did test this at some point but I guess I reverted to an earlier version of the code when rebasing after --overwrite branch catastrophe in Dallas.

It wasn't caught in the tests because the KeyRepeatDispatcher wasn't installed (due to key repeat being disabled to not make the tests timing dependent). I've changed the way key repeat is configured in a way which would have caught this bug. I think the value of this could be argued about though (as the way repeat_enabled is implemented makes it unlikely this would catch future bugs).

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Approve (continuous-integration)
Revision history for this message
Alberto Aguirre (albaguirre) wrote :

LGTM.

Nit:

69 + bool repeat_enabled,

Not aligned

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

Yeah wrong indentation in a couple of places.

But more importantly, tested on krillin and bug fixed.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/server/input/default_configuration.cpp'
2--- src/server/input/default_configuration.cpp 2015-06-08 17:22:32 +0000
3+++ src/server/input/default_configuration.cpp 2015-06-11 18:30:00 +0000
4@@ -223,10 +223,11 @@
5 std::chrono::milliseconds const key_repeat_timeout{20};
6
7 auto const options = the_options();
8- if (!options->get<bool>(options::enable_key_repeat_opt))
9- return the_event_filter_chain_dispatcher();
10- else
11- return std::make_shared<mi::KeyRepeatDispatcher>(the_event_filter_chain_dispatcher(), the_main_loop(), key_repeat_timeout);
12+ auto enable_repeat = options->get<bool>(options::enable_key_repeat_opt);
13+
14+ return std::make_shared<mi::KeyRepeatDispatcher>(
15+ the_event_filter_chain_dispatcher(), the_main_loop(), enable_repeat,
16+ key_repeat_timeout);
17 }
18
19 std::shared_ptr<droidinput::EventHubInterface>
20
21=== modified file 'src/server/input/key_repeat_dispatcher.cpp'
22--- src/server/input/key_repeat_dispatcher.cpp 2015-06-08 20:18:00 +0000
23+++ src/server/input/key_repeat_dispatcher.cpp 2015-06-11 18:30:00 +0000
24@@ -32,9 +32,11 @@
25 mi::KeyRepeatDispatcher::KeyRepeatDispatcher(
26 std::shared_ptr<mi::InputDispatcher> const& next_dispatcher,
27 std::shared_ptr<mir::time::AlarmFactory> const& factory,
28+ bool repeat_enabled,
29 std::chrono::milliseconds repeat_timeout)
30 : next_dispatcher(next_dispatcher),
31 alarm_factory(factory),
32+ repeat_enabled(repeat_enabled),
33 repeat_timeout(repeat_timeout)
34 {
35 }
36@@ -47,17 +49,22 @@
37
38 bool mi::KeyRepeatDispatcher::dispatch(MirEvent const& event)
39 {
40+ if (!repeat_enabled) // if we made this mutable we'd need a guard
41+ {
42+ return next_dispatcher->dispatch(event);
43+ }
44+
45 if (mir_event_get_type(&event) == mir_event_type_input)
46 {
47 auto iev = mir_event_get_input_event(&event);
48 if (mir_input_event_get_type(iev) != mir_input_event_type_key)
49- return false;
50+ return next_dispatcher->dispatch(event);
51 if (!handle_key_input(mir_input_event_get_device_id(iev), mir_input_event_get_keyboard_event(iev)))
52 return next_dispatcher->dispatch(event);
53 else
54 return true;
55 }
56- return false;
57+ return next_dispatcher->dispatch(event);
58 }
59
60 namespace
61
62=== modified file 'src/server/input/key_repeat_dispatcher.h'
63--- src/server/input/key_repeat_dispatcher.h 2015-06-08 17:28:38 +0000
64+++ src/server/input/key_repeat_dispatcher.h 2015-06-11 18:30:00 +0000
65@@ -41,6 +41,7 @@
66 public:
67 KeyRepeatDispatcher(std::shared_ptr<InputDispatcher> const& next_dispatcher,
68 std::shared_ptr<time::AlarmFactory> const& factory,
69+ bool repeat_enabled,
70 std::chrono::milliseconds repeat_timeout);
71
72 // InputDispatcher
73@@ -53,6 +54,7 @@
74
75 std::shared_ptr<InputDispatcher> const next_dispatcher;
76 std::shared_ptr<time::AlarmFactory> const alarm_factory;
77+ bool const repeat_enabled;
78 std::chrono::milliseconds repeat_timeout;
79
80 struct KeyboardState
81
82=== modified file 'tests/unit-tests/input/test_key_repeat_dispatcher.cpp'
83--- tests/unit-tests/input/test_key_repeat_dispatcher.cpp 2015-06-03 16:34:34 +0000
84+++ tests/unit-tests/input/test_key_repeat_dispatcher.cpp 2015-06-11 18:30:00 +0000
85@@ -61,7 +61,7 @@
86 struct KeyRepeatDispatcher : public testing::Test
87 {
88 KeyRepeatDispatcher()
89- : dispatcher(mock_next_dispatcher, mock_alarm_factory, repeat_time)
90+ : dispatcher(mock_next_dispatcher, mock_alarm_factory, true, repeat_time)
91 {
92 }
93 std::shared_ptr<mtd::MockInputDispatcher> mock_next_dispatcher = std::make_shared<mtd::MockInputDispatcher>();

Subscribers

People subscribed via source and target branches