Mir

Merge lp:~robertcarr/mir/weakify-event-filters into lp:~mir-team/mir/trunk

  1. weakify-event-filters
  2. Merge into trunk
Proposed by Robert Carr
Status: Merged
Approved by: Robert Carr
Approved revision: no longer in the source branch.
Merged at revision: 631
Proposed branch: lp:~robertcarr/mir/weakify-event-filters
Merge into: lp:~mir-team/mir/trunk
Diff against target: 208 lines (+44/-18)
7 files modified
src/server/input/event_filter_chain.cpp (+9/-2)
src/server/input/event_filter_chain.h (+2/-2)
tests/integration-tests/input/android/test_android_cursor_listener.cpp (+4/-3)
tests/integration-tests/input/android/test_android_input_manager.cpp (+12/-10)
tests/unit-tests/input/CMakeLists.txt (+4/-0)
tests/unit-tests/input/android/CMakeLists.txt (+0/-1)
tests/unit-tests/input/test_event_filter_chain.cpp (+13/-0)
To merge this branch: bzr merge lp:~robertcarr/mir/weakify-event-filters
Reviewer Review Type Date Requested Status
Alan Griffiths Approve
Daniel van Vugt Approve
PS Jenkins bot (community) continuous-integration Approve
Review via email: mp+160497@code.launchpad.net

Commit message

EventFilterChain should hold references to the EventFilter by weak_ptr.

Description of the change

EventFilterChain should hold references to the EventFilter by weak_ptr. This aimed at solving the circular dependency described here: https://code.launchpad.net/~robertcarr/mir/demo-shell/+merge/159253. I think it makes sense, as any EventFilter is likely to hold references to server components (otherwise what is it going to do with events!).

The iteration in ::handles is a little strange, considered using std::remove_if pattern but it seemed inappropriate as removal is not the primary purpose of the iteration.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:594
http://jenkins.qa.ubuntu.com/job/mir-ci/484/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/520
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/403
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/489
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/489/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=precise,flavor=amd64/237

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

review: Approve (continuous-integration)
Revision history for this message
Robert Ancell (robert-ancell) wrote :

You have an indentation error in src/server/input/event_filter_chain.cpp

Revision history for this message
Robert Carr (robertcarr) wrote :

Updated! thanks

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

PASSED: Continuous integration, rev:595
http://jenkins.qa.ubuntu.com/job/mir-ci/487/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/523
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/406
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/492
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/492/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)
Revision history for this message
Kevin DuBois (kdub) wrote :

13 + auto filter = (*it).lock();
could be
it->lock();

are event filters created at the start, and constant throughout the program? if whoever owns them now (I think DefaultInputConfiguration) were to change them, we'd have to keep in sync. i'm sort of in a more unfamiliar part of the system to me, so forgive me if its a silly question :)

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

Yep, (*it).lock should be it->lock, but not really an issue.

This code scares me... Its width makes it unreadable. But really it's a pre-existing issue:

80 - configuration = std::make_shared<mtd::FakeEventHubInputConfiguration>(std::initializer_list<std::shared_ptr<mi::EventFilter> const>{mt::fake_shared(event_filter)}, mt::fake_shared(viewable_area), null_cursor_listener);
81 + event_filter = std::make_shared<MockEventFilter>();
82 + configuration = std::make_shared<mtd::FakeEventHubInputConfiguration>(std::initializer_list<std::shared_ptr<mi::EventFilter> const>{event_filter}, mt::fake_shared(viewable_area), null_cursor_listener);

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

I agree, writing the loop to do two things makes it more complicated.

I'd be tempted to use the erase idiom first, then the loop:

    filters.erase(
        std::remove_if(filters.begin(), filters.end(), [](EventFilterVector::value_type const& f) {return !f.lock();}),
        filters.end());

    for (auto const& f : filters)
    {
       if (auto const filter = f.lock())
           if (filter->handles(event)) return true;
    }

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

On 24/04/13 11:49, Alan Griffiths wrote:
> Review: Approve
>
> I agree, writing the loop to do two things makes it more complicated.
>
> I'd be tempted to use the erase idiom first, then the loop:
>
> filters.erase(
> std::remove_if(filters.begin(), filters.end(), [](EventFilterVector::value_type const& f) {return

Or even "EventFilterVector::const_reference f"

--
Alan Griffiths +44 (0)798 9938 758
Octopull Ltd http://www.octopull.co.uk/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/server/input/event_filter_chain.cpp'
2--- src/server/input/event_filter_chain.cpp 2013-04-16 09:08:29 +0000
3+++ src/server/input/event_filter_chain.cpp 2013-04-23 23:38:29 +0000
4@@ -27,10 +27,17 @@
5
6 bool mi::EventFilterChain::handles(const MirEvent &event)
7 {
8- for (auto it = filters.begin(); it != filters.end(); it++)
9+ auto it = filters.begin();
10+ while (it != filters.end())
11 {
12- auto filter = *it;
13+ auto filter = (*it).lock();
14+ if (!filter)
15+ {
16+ it = filters.erase(it);
17+ continue;
18+ }
19 if (filter->handles(event)) return true;
20+ ++it;
21 }
22 return false;
23 }
24
25=== modified file 'src/server/input/event_filter_chain.h'
26--- src/server/input/event_filter_chain.h 2013-04-16 09:08:29 +0000
27+++ src/server/input/event_filter_chain.h 2013-04-23 23:38:29 +0000
28@@ -44,8 +44,8 @@
29 virtual bool handles(const MirEvent &event);
30
31 private:
32- typedef std::vector<std::shared_ptr<EventFilter>> EventFilterVector;
33- EventFilterVector const filters;
34+ typedef std::vector<std::weak_ptr<EventFilter>> EventFilterVector;
35+ EventFilterVector filters;
36 };
37
38 }
39
40=== modified file 'tests/integration-tests/input/android/test_android_cursor_listener.cpp'
41--- tests/integration-tests/input/android/test_android_cursor_listener.cpp 2013-04-01 20:17:11 +0000
42+++ tests/integration-tests/input/android/test_android_cursor_listener.cpp 2013-04-23 23:38:29 +0000
43@@ -64,8 +64,9 @@
44 geom::Size{geom::Width(1024), geom::Height(1024)}
45 };
46
47+ event_filter = std::make_shared<MockEventFilter>();
48 configuration = std::make_shared<mtd::FakeEventHubInputConfiguration>(
49- std::initializer_list<std::shared_ptr<mi::EventFilter> const>{mt::fake_shared(event_filter)},
50+ std::initializer_list<std::shared_ptr<mi::EventFilter> const>{event_filter},
51 mt::fake_shared(viewable_area),
52 mt::fake_shared(cursor_listener));
53
54@@ -86,7 +87,7 @@
55
56 std::shared_ptr<mtd::FakeEventHubInputConfiguration> configuration;
57 mia::FakeEventHub* fake_event_hub;
58- MockEventFilter event_filter;
59+ std::shared_ptr<MockEventFilter> event_filter;
60 NiceMock<mtd::MockViewableArea> viewable_area;
61 std::shared_ptr<mia::InputManager> input_manager;
62 MockCursorListener cursor_listener;
63@@ -107,7 +108,7 @@
64 EXPECT_CALL(cursor_listener, cursor_moved_to(x, y)).Times(1);
65
66 // The stack doesn't like shutting down while events are still moving through
67- EXPECT_CALL(event_filter, handles(_))
68+ EXPECT_CALL(*event_filter, handles(_))
69 .WillOnce(mt::ReturnFalseAndWakeUp(wait_condition));
70
71 fake_event_hub->synthesize_builtin_cursor_added();
72
73=== modified file 'tests/integration-tests/input/android/test_android_input_manager.cpp'
74--- tests/integration-tests/input/android/test_android_input_manager.cpp 2013-04-16 17:46:35 +0000
75+++ tests/integration-tests/input/android/test_android_input_manager.cpp 2013-04-23 23:38:29 +0000
76@@ -71,7 +71,8 @@
77 public:
78 AndroidInputManagerAndEventFilterDispatcherSetup()
79 {
80- configuration = std::make_shared<mtd::FakeEventHubInputConfiguration>(std::initializer_list<std::shared_ptr<mi::EventFilter> const>{mt::fake_shared(event_filter)}, mt::fake_shared(viewable_area), null_cursor_listener);
81+ event_filter = std::make_shared<MockEventFilter>();
82+ configuration = std::make_shared<mtd::FakeEventHubInputConfiguration>(std::initializer_list<std::shared_ptr<mi::EventFilter> const>{event_filter}, mt::fake_shared(viewable_area), null_cursor_listener);
83 ON_CALL(viewable_area, view_area())
84 .WillByDefault(Return(default_view_area));
85
86@@ -91,7 +92,7 @@
87 std::shared_ptr<mtd::FakeEventHubInputConfiguration> configuration;
88 mia::FakeEventHub* fake_event_hub;
89 std::shared_ptr<mia::InputManager> input_manager;
90- MockEventFilter event_filter;
91+ std::shared_ptr<MockEventFilter> event_filter;
92 NiceMock<mtd::MockViewableArea> viewable_area;
93 };
94
95@@ -104,7 +105,7 @@
96 mt::WaitCondition wait_condition;
97
98 EXPECT_CALL(
99- event_filter,
100+ *event_filter,
101 handles(mt::KeyDownEvent()))
102 .Times(1)
103 .WillOnce(mt::ReturnFalseAndWakeUp(&wait_condition));
104@@ -125,7 +126,7 @@
105 mt::WaitCondition wait_condition;
106
107 EXPECT_CALL(
108- event_filter,
109+ *event_filter,
110 handles(mt::ButtonDownEvent()))
111 .Times(1)
112 .WillOnce(mt::ReturnFalseAndWakeUp(&wait_condition));
113@@ -148,10 +149,10 @@
114 {
115 InSequence seq;
116
117- EXPECT_CALL(event_filter,
118+ EXPECT_CALL(*event_filter,
119 handles(mt::MotionEvent(100, 100)))
120 .WillOnce(Return(false));
121- EXPECT_CALL(event_filter,
122+ EXPECT_CALL(*event_filter,
123 handles(mt::MotionEvent(200, 100)))
124 .WillOnce(mt::ReturnFalseAndWakeUp(&wait_condition));
125 }
126@@ -203,8 +204,9 @@
127 {
128 AndroidInputManagerDispatcherInterceptSetup()
129 {
130+ event_filter = std::make_shared<MockEventFilter>();
131 configuration = std::make_shared<TestingInputConfiguration>(
132- mt::fake_shared(event_filter),
133+ event_filter,
134 mt::fake_shared(viewable_area), null_cursor_listener);
135 fake_event_hub = configuration->the_fake_event_hub();
136
137@@ -238,7 +240,7 @@
138 return fds[0];
139 }
140
141- MockEventFilter event_filter;
142+ std::shared_ptr<MockEventFilter> event_filter;
143 NiceMock<mtd::MockViewableArea> viewable_area;
144 std::shared_ptr<TestingInputConfiguration> configuration;
145 mia::FakeEventHub* fake_event_hub;
146@@ -268,7 +270,7 @@
147 auto input_fd = test_fd();
148 mtd::StubSurfaceTarget surface(input_fd);
149
150- EXPECT_CALL(event_filter, handles(_)).Times(1).WillOnce(Return(false));
151+ EXPECT_CALL(*event_filter, handles(_)).Times(1).WillOnce(Return(false));
152 // We return -1 here to skip publishing of the event (to an unconnected test socket!).
153 EXPECT_CALL(*dispatcher_policy, interceptKeyBeforeDispatching(WindowHandleWithInputFd(input_fd), _, _))
154 .Times(1).WillOnce(DoAll(mt::WakeUp(&wait_condition), Return(-1)));
155@@ -302,7 +304,7 @@
156 input_target_listener->input_surface_opened(mt::fake_shared(session), mt::fake_shared(surface1));
157 input_target_listener->input_surface_opened(mt::fake_shared(session), mt::fake_shared(surface2));
158
159- EXPECT_CALL(event_filter, handles(_)).Times(3).WillRepeatedly(Return(false));
160+ EXPECT_CALL(*event_filter, handles(_)).Times(3).WillRepeatedly(Return(false));
161
162 {
163 InSequence seq;
164
165=== modified file 'tests/unit-tests/input/CMakeLists.txt'
166--- tests/unit-tests/input/CMakeLists.txt 2013-03-13 04:54:15 +0000
167+++ tests/unit-tests/input/CMakeLists.txt 2013-04-23 23:38:29 +0000
168@@ -1,5 +1,9 @@
169 add_subdirectory(android)
170
171+list(APPEND UNIT_TEST_SOURCES
172+ ${CMAKE_CURRENT_SOURCE_DIR}/test_event_filter_chain.cpp
173+)
174+
175 set(
176 UNIT_TEST_SOURCES
177 ${UNIT_TEST_SOURCES}
178
179=== modified file 'tests/unit-tests/input/android/CMakeLists.txt'
180--- tests/unit-tests/input/android/CMakeLists.txt 2013-04-10 20:34:38 +0000
181+++ tests/unit-tests/input/android/CMakeLists.txt 2013-04-23 23:38:29 +0000
182@@ -1,5 +1,4 @@
183 list(APPEND UNIT_TEST_SOURCES
184- ${CMAKE_CURRENT_SOURCE_DIR}/test_event_filter_chain.cpp
185 ${CMAKE_CURRENT_SOURCE_DIR}/test_event_filter_input_dispatcher_policy.cpp
186 ${CMAKE_CURRENT_SOURCE_DIR}/test_android_pointer_controller.cpp
187 ${CMAKE_CURRENT_SOURCE_DIR}/test_android_input_lexicon.cpp
188
189=== renamed file 'tests/unit-tests/input/android/test_event_filter_chain.cpp' => 'tests/unit-tests/input/test_event_filter_chain.cpp'
190--- tests/unit-tests/input/android/test_event_filter_chain.cpp 2013-03-13 08:09:52 +0000
191+++ tests/unit-tests/input/test_event_filter_chain.cpp 2013-04-23 23:38:29 +0000
192@@ -58,3 +58,16 @@
193 EXPECT_TRUE(filter_chain.handles(ev));
194 }
195
196+TEST(EventFilterChain, does_not_own_event_filters)
197+{
198+ using namespace ::testing;
199+
200+ auto filter = std::make_shared<mtd::MockEventFilter>();
201+ MirEvent ev;
202+
203+ mi::EventFilterChain filter_chain{filter};
204+ EXPECT_CALL(*filter, handles(_)).Times(1).WillOnce(Return(true));
205+ EXPECT_TRUE(filter_chain.handles(ev));
206+ filter.reset();
207+ EXPECT_FALSE(filter_chain.handles(ev));
208+}

Subscribers

People subscribed via source and target branches