Mir

Merge lp:~mir-team/mir/touch-validating-client into lp:mir

  1. touch-validating-client
  2. Merge into development-branch
Proposed by Robert Carr on 2015-05-18
Status: Merged
Approved by: Robert Carr on 2015-06-04
Approved revision: 2580
Merged at revision: 2626
Proposed branch: lp:~mir-team/mir/touch-validating-client
Merge into: lp:mir
Diff against target: 222 lines (+207/-0)
2 files modified
examples/CMakeLists.txt (+7/-0)
examples/client_touch_validator.cpp (+200/-0)
To merge this branch: bzr merge lp:~mir-team/mir/touch-validating-client
Reviewer Review Type Date Requested Status
PS Jenkins bot continuous-integration Approve on 2015-06-05
Kevin DuBois (community) Approve on 2015-06-02
Alexandros Frantzis (community) Needs Fixing on 2015-05-21
Alan Griffiths 2015-05-18 Approve on 2015-05-21
Review via email: mp+259436@code.launchpad.net

Commit Message

Add a client which may be used for validating touch input events.

Description of the Change

Add a client which may be used for validating touch input events.

To post a comment you must log in.
Alan Griffiths (alan-griffiths) wrote :

66 +bool validate_events(MirTouchEvent const* last_event, MirTouchEvent const* event)

"last_event" doesn't seem the clearest name as "last" has several meanings. How about "previous" and "current"?

~~~~

134 + last_event = mir_event_ref(reinterpret_cast<MirEvent const*>(event));
...
137 + if (!validate_events(reinterpret_cast<MirTouchEvent const*>(last_event),
...
141 + last_event = mir_event_ref(reinterpret_cast<MirEvent const*>(event));

Are these reinterpret_cast<>s really the cleanest code?

review: Needs Information
Kevin DuBois (kdub) wrote :

some nits:
155 + void on_event(MirSurface * /*surface*/, const MirEvent *event, void *context)
indentation
157 + TouchState *state = (TouchState*)context;
c-casts
137 + if (!validate_events(reinterpret_cast<MirTouchEvent const*>(last_event), event))
We could just abort in the function instead of returning a bool

192 + Color red = {opacity, 0.0f, 0.0f, opacity};
193 + Color green = {0.0f, opacity, 0.0f, opacity};
194 + Color blue = {0.0f, 0.0f, opacity, opacity};
If this was an array, we could loop over the colors in the render loop:
std::array<Color, 3> colors {
    Color{opacity, 0.0f, 0.0f, opacity},
    Color{0.0f, opacity, 0.0f, opacity},
    Color{0.0f, 0.0f, opacity, opacity}
};

review: Needs Fixing
Kevin DuBois (kdub) wrote :

Also, needs-info on: Could this be a test?

Robert Carr (robertcarr) wrote :

>>> "last_event" doesn't seem the clearest name as "last" has several meanings. How about
>> "previous" and "current"?

Ok

>> Are these reinterpret_cast<>s really the cleanest code?

I guess I thought the version without casts was cleaner due to retaining shorter methods but I've updated it.

Robert Carr (robertcarr) wrote :

>> 155 + void on_event(MirSurface * /*surface*/, const MirEvent *event, void *context)
indentation
>> 157 + TouchState *state = (TouchState*)context;
>> c-casts

Fixed

>> 137 + if (!validate_events(reinterpret_cast<MirTouchEvent const*>(last_event), event))
>> We could just abort in the function instead of returning a bool
It seems basically equivalent except if someone wanted to copy paste the function somewhere else
they would probably be thankful for the bool so I've left it that way.

>> 192 + Color red = {opacity, 0.0f, 0.0f, opacity};
>> 193 + Color green = {0.0f, opacity, 0.0f, opacity};
>> 194 + Color blue = {0.0f, 0.0f, opacity, opacity};
>> If this was an array, we could loop over the colors in the render loop:

Fixed

PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:2577
http://jenkins.qa.ubuntu.com/job/mir-ci/3864/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2516
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/26
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2464
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/20
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/20/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2464
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2464/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5341
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20589

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

review: Approve (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Robert Carr (robertcarr) wrote :

>> Also, needs-info on: Could this be a test?

Sort of but I don't yet already have any expected input that produces invalid input this is more of an exploratory tool so far.

Alan Griffiths (alan-griffiths) wrote :

1 === modified file '3rd_party/android-input/android/frameworks/base/services/input/InputDispatcher.cpp'

Spurious changes?

review: Needs Fixing
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:2583
http://jenkins.qa.ubuntu.com/job/mir-ci/3887/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2553
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/63
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2501
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/43
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/43/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2501
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2501/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5371
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20666

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

review: Approve (continuous-integration)
Alan Griffiths (alan-griffiths) wrote :

While there should be a test of how the system responds to input (and badly formed input) this is potentially useful for experimentation and device bring up.

review: Approve
Alexandros Frantzis (afrantzis) wrote :

132 + void record_event(MirEvent const* event)
133 + {
...

Code has tabs mixed with spaces and inconsistent indentation.

62 +// Validates:
63 +// 1. All touches which were down stay down unless they were coming up
64 +// 2. All touches which were released do not appear
65 +// 3. No touches appear before a down
66 +// 4. Only one touch comes up or down per event

It's not obvious how/if the code validates these conditions. Perhaps reworking the code a bit (e.g. having a function for each validation condition) will help make the logic more clear?

review: Needs Fixing
Kevin DuBois (kdub) wrote :

alright, lgtm then

review: Approve
Robert Carr (robertcarr) wrote :

I've added some comments which I think is more clear here than splitting the functions.

Robert Carr (robertcarr) wrote :

Alexandros is away at a workshop, and I think a fourth reviewer on such a low risk/impact change would be a waste of someones time so going to TA.

PS Jenkins bot (ps-jenkins) wrote :

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-autolanding/1507/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2724
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/237/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2672
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-autolanding/62
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-autolanding/62/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2672
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2672/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5516
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20977

review: Needs Fixing (continuous-integration)
Robert Carr (robertcarr) wrote :

Jenkins failure is network error.

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 'examples/CMakeLists.txt'
2--- examples/CMakeLists.txt 2015-05-29 12:53:18 +0000
3+++ examples/CMakeLists.txt 2015-06-04 15:53:38 +0000
4@@ -89,6 +89,13 @@
5 )
6 target_link_libraries(mir_demo_client_tooltip
7 eglapp
8+ )
9+
10+mir_add_wrapped_executable(mir_demo_client_touch_validator
11+ client_touch_validator.cpp
12+)
13+target_link_libraries(mir_demo_client_touch_validator
14+ eglapp
15 )
16
17 mir_add_wrapped_executable(mir_demo_client_basic
18
19=== added file 'examples/client_touch_validator.cpp'
20--- examples/client_touch_validator.cpp 1970-01-01 00:00:00 +0000
21+++ examples/client_touch_validator.cpp 2015-06-04 15:53:38 +0000
22@@ -0,0 +1,200 @@
23+/*
24+ * Client which validates incoming touch events.
25+ *
26+ * Copyright © 2015 Canonical Ltd.
27+ *
28+ * This program is free software: you can redistribute it and/or modify
29+ * it under the terms of the GNU General Public License version 3 as
30+ * published by the Free Software Foundation.
31+ *
32+ * This program is distributed in the hope that it will be useful,
33+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
34+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
35+ * GNU General Public License for more details.
36+ *
37+ * You should have received a copy of the GNU General Public License
38+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
39+ *
40+ * Author: Robert Carr <robert.carr@canonical.com>
41+ */
42+
43+#include "eglapp.h"
44+
45+#include <mir_toolkit/mir_client_library.h>
46+
47+#include <GLES2/gl2.h>
48+
49+#include <set>
50+
51+#include <stdio.h>
52+#include <stdlib.h>
53+#include <unistd.h>
54+
55+namespace
56+{
57+// Assumes all touch input comes from a single device.
58+// Validates:
59+// 1. All touches which were down stay down unless they were coming up
60+// 2. All touches which were released do not appear
61+// 3. Only one touch comes up or down at a time.
62+
63+bool validate_events(MirTouchEvent const* last_event, MirTouchEvent const* event)
64+{
65+ std::set<MirTouchId> must_be_present;
66+ std::set<MirTouchId> may_not_be_present;
67+
68+ // Here, from the last event, we build a list of points which must be
69+ // and which must not be present in the next event.
70+ for (size_t i = 0; i < mir_touch_event_point_count(last_event); i++)
71+ {
72+ auto id = mir_touch_event_id(last_event, i);
73+ auto action = mir_touch_event_action(last_event, i);
74+ if (action == mir_touch_action_change)
75+ must_be_present.insert(id);
76+ else if (action == mir_touch_action_down)
77+ must_be_present.insert(id);
78+ else
79+ may_not_be_present.insert(id);
80+ }
81+ if (may_not_be_present.size() > 1)
82+ {
83+ printf("More than one touch came up\n");
84+ return false;
85+ }
86+
87+ bool found_a_up_down = false;
88+ for (size_t i = 0; i < mir_touch_event_point_count(event); i++)
89+ {
90+ auto id = mir_touch_event_id(event, i);
91+ auto it = may_not_be_present.find(id);
92+ // Here we validate condition 2, where released touches may not reappear
93+ if (it != may_not_be_present.end())
94+ {
95+ printf("We repeated a touch which was already lifted (%d)\n", static_cast<int>(id));
96+ return false;
97+ }
98+ it = must_be_present.find(id);
99+ if (it != must_be_present.end())
100+ {
101+ must_be_present.erase(it);
102+ }
103+ // Here we validate condition 4
104+ else if (mir_touch_event_action(event, i) == mir_touch_action_down)
105+ {
106+ if (found_a_up_down)
107+ printf("Found too many downs in one event\n");
108+ found_a_up_down = true;
109+ }
110+ if (mir_touch_event_action(event, i) == mir_touch_action_up)
111+ {
112+ if (found_a_up_down)
113+ printf("Found too many ups in one event\n");
114+ found_a_up_down = true;
115+ }
116+ }
117+
118+ // Here we validate condition 1
119+ if (must_be_present.size())
120+ {
121+ printf("We received a touch which did not contain all required IDs\n");
122+ return false;
123+ }
124+
125+ return true;
126+}
127+
128+class TouchState
129+{
130+public:
131+ TouchState() : last_event(nullptr) {}
132+ ~TouchState() { if (last_event) mir_event_unref(last_event); }
133+
134+ void record_event(MirTouchEvent const* event)
135+ {
136+ if (!last_event)
137+ {
138+ last_event = mir_event_ref(reinterpret_cast<MirEvent const*>(event));
139+ return;
140+ }
141+ if (!validate_events(reinterpret_cast<MirTouchEvent const*>(last_event), event))
142+ abort();
143+
144+ mir_event_unref(last_event);
145+ last_event = mir_event_ref(reinterpret_cast<MirEvent const*>(event));
146+ }
147+private:
148+ MirEvent const* last_event;
149+};
150+
151+void on_input_event(TouchState *state, MirInputEvent const *event)
152+{
153+ if (mir_input_event_get_type(event) != mir_input_event_type_touch)
154+ return;
155+ auto tev = mir_input_event_get_touch_event(event);
156+ state->record_event(tev);
157+}
158+
159+void on_event(MirSurface * /*surface*/, const MirEvent *event, void *context)
160+{
161+ TouchState *state = (TouchState*)context;
162+
163+ switch (mir_event_get_type(event))
164+ {
165+ case mir_event_type_input:
166+ on_input_event(state, mir_event_get_input_event(event));
167+ break;
168+ case mir_event_type_close_surface:
169+ abort();
170+ break;
171+ default:
172+ break;
173+ }
174+}
175+}
176+
177+
178+typedef struct Color
179+{
180+ GLfloat r, g, b, a;
181+} Color;
182+
183+int main(int argc, char *argv[])
184+{
185+ unsigned int width = 0, height = 0;
186+
187+ if (!mir_eglapp_init(argc, argv, &width, &height))
188+ return 1;
189+
190+ TouchState state;
191+
192+ MirSurface *surface = mir_eglapp_native_surface();
193+ mir_surface_set_event_handler(surface, on_event, &state);
194+
195+ float const opacity = mir_eglapp_background_opacity;
196+ Color red = {opacity, 0.0f, 0.0f, opacity};
197+ Color green = {0.0f, opacity, 0.0f, opacity};
198+ Color blue = {0.0f, 0.0f, opacity, opacity};
199+
200+ /* This is probably the simplest GL you can do */
201+ while (mir_eglapp_running())
202+ {
203+ glClearColor(red.r, red.g, red.b, red.a);
204+ glClear(GL_COLOR_BUFFER_BIT);
205+ mir_eglapp_swap_buffers();
206+ sleep(1);
207+
208+ glClearColor(green.r, green.g, green.b, green.a);
209+ glClear(GL_COLOR_BUFFER_BIT);
210+ mir_eglapp_swap_buffers();
211+ sleep(1);
212+
213+ glClearColor(blue.r, blue.g, blue.b, blue.a);
214+ glClear(GL_COLOR_BUFFER_BIT);
215+ mir_eglapp_swap_buffers();
216+ sleep(1);
217+ }
218+
219+ mir_eglapp_shutdown();
220+
221+ return 0;
222+}

Subscribers

People subscribed via source and target branches