Mir

Merge lp:~alan-griffiths/mir/start-tidy-up-of-InputManager into lp:mir

  1. start-tidy-up-of-InputManager
  2. Merge into development-branch
Proposed by Alan Griffiths on 2016-03-02
Status: Merged
Approved by: Cemil Azizoglu on 2016-03-08
Approved revision: 3360
Merged at revision: 3371
Proposed branch: lp:~alan-griffiths/mir/start-tidy-up-of-InputManager
Merge into: lp:mir
Prerequisite: lp:~alan-griffiths/mir/fix-1528110
Diff against target: 179 lines (+25/-22)
5 files modified
include/server/mir/input/input_manager.h (+3/-1)
src/server/input/default_configuration.cpp (+2/-6)
src/server/input/default_input_manager.cpp (+8/-3)
src/server/input/default_input_manager.h (+10/-3)
tests/unit-tests/input/test_default_input_manager.cpp (+2/-9)
To merge this branch: bzr merge lp:~alan-griffiths/mir/start-tidy-up-of-InputManager
Reviewer Review Type Date Requested Status
Andreas Pokorny (community) Abstain on 2016-03-04
Mir CI Bot continuous-integration Approve on 2016-03-03
Cemil Azizoglu (community) Approve on 2016-03-02
Kevin DuBois (community) 2016-03-02 Approve on 2016-03-02
Review via email: mp+287810@code.launchpad.net

Commit Message

input: Make the input platform a dependency of DefaultInputManager

Description of the Change

input: Make the input platform a dependency of DefaultInputManager

the input platform should be a simple constructor dependency of InputManager, and there's no use case for adding platforms. There's no immediate need to break the mirserver API/ABI for this, so I've just deprecated InputManager::add_platform() instead of removing it already.

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

PASSED: Continuous integration, rev:3359
https://mir-jenkins.ubuntu.com/job/mir-ci/472/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/298
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/323
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/315
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/315
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/306
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/306/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/306
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/306/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/306
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/306/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/306
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/306/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/306
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/306/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)
Kevin DuBois (kdub) wrote :

lgtm, better without 2step initialization.

review: Approve
Cemil Azizoglu (cemil-azizoglu) wrote :

LGTM

review: Approve
Daniel van Vugt (vanvugt) wrote :

I have concerns about the prerequisite branch tho...

Andreas Pokorny (andreas-pokorny) wrote :

* Needs discussion *
I thought you would only remove probing of multiple possible platforms for now... avoiding the issues that we experience right now in specific startup cases. Issues that we have because we allow our probing to see very little of the server setup. I think this is still also a part that will change.

review: Needs Information
lp:~alan-griffiths/mir/start-tidy-up-of-InputManager updated on 2016-03-03
3360. By Alan Griffiths on 2016-03-03

merge lp:~alan-griffiths/mir/fix-1528110

Alan Griffiths (alan-griffiths) wrote :

> * Needs discussion *
> I thought you would only remove probing of multiple possible platforms for
> now... avoiding the issues that we experience right now in specific startup
> cases. Issues that we have because we allow our probing to see very little of
> the server setup. I think this is still also a part that will change.

We can change things as the need arrives. Meanwhile, I suggest having the simplest codebase that does the job.

Mir CI Bot (mir-ci-bot) wrote :

PASSED: Continuous integration, rev:3360
https://mir-jenkins.ubuntu.com/job/mir-ci/480/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/307
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/332
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/324
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/324
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/316
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/316/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/316
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/316/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/316
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/316/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/316
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/316/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/316
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/316/artifact/output/*zip*/output.zip

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

review: Approve (continuous-integration)
Andreas Pokorny (andreas-pokorny) wrote :

> > * Needs discussion *
> > I thought you would only remove probing of multiple possible platforms for
> > now... avoiding the issues that we experience right now in specific startup
> > cases. Issues that we have because we allow our probing to see very little
> of
> > the server setup. I think this is still also a part that will change.
>
> We can change things as the need arrives. Meanwhile, I suggest having the
> simplest codebase that does the job.

I do agree to that point of view in many cases. In this case it does not seem to be a big simplification win. The main concern for me is that the sub-text of the MP reads like a decision to cut off those design options from the decision tree. I.e marking the method as deprecated..

Alan Griffiths (alan-griffiths) wrote :

> I do agree to that point of view in many cases. In this case it does not seem
> to be a big simplification win. The main concern for me is that the sub-text
> of the MP reads like a decision to cut off those design options from the
> decision tree. I.e marking the method as deprecated..

We're not cutting off a design option, we are /not committing/ to a design that works by "add_platform()".

When we know more about what needs to work we can use a different design, or reinstate the old one.

Andreas Pokorny (andreas-pokorny) :
review: Abstain

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/server/mir/input/input_manager.h'
2--- include/server/mir/input/input_manager.h 2015-06-17 05:20:42 +0000
3+++ include/server/mir/input/input_manager.h 2016-03-03 09:58:55 +0000
4@@ -1,5 +1,5 @@
5 /*
6- * Copyright © 2012 Canonical Ltd.
7+ * Copyright © 2012, 2016 Canonical Ltd.
8 *
9 * This program is free software: you can redistribute it and/or modify it
10 * under the terms of the GNU General Public License version 3,
11@@ -32,6 +32,8 @@
12 class InputManager
13 {
14 public:
15+ // TODO Remove add_platform() when we next break mirserver ABI
16+ __attribute__ ((deprecated))
17 virtual void add_platform(std::shared_ptr<Platform> const& platform) = 0;
18 virtual void start() = 0;
19 virtual void stop() = 0;
20
21=== modified file 'src/server/input/default_configuration.cpp'
22--- src/server/input/default_configuration.cpp 2016-03-03 09:58:55 +0000
23+++ src/server/input/default_configuration.cpp 2016-03-03 09:58:55 +0000
24@@ -1,5 +1,5 @@
25 /*
26- * Copyright © 2013-2014 Canonical Ltd.
27+ * Copyright © 2013-2016 Canonical Ltd.
28 *
29 * This program is free software: you can redistribute it and/or modify it
30 * under the terms of the GNU General Public License version 3,
31@@ -243,11 +243,7 @@
32 auto platform = probe_input_platforms(*options, the_emergency_cleanup(), the_input_device_registry(),
33 the_input_report(), *the_shared_library_prober_report());
34
35- auto const ret = std::make_shared<mi::DefaultInputManager>(the_input_reading_multiplexer());
36-
37- ret->add_platform(std::move(platform));
38-
39- return ret;
40+ return std::make_shared<mi::DefaultInputManager>(the_input_reading_multiplexer(), std::move(platform));
41 }
42 }
43 );
44
45=== modified file 'src/server/input/default_input_manager.cpp'
46--- src/server/input/default_input_manager.cpp 2016-02-24 03:55:11 +0000
47+++ src/server/input/default_input_manager.cpp 2016-03-03 09:58:55 +0000
48@@ -1,5 +1,5 @@
49 /*
50- * Copyright © 2015 Canonical Ltd.
51+ * Copyright © 2015-2016 Canonical Ltd.
52 *
53 * This program is free software: you can redistribute it and/or modify it
54 * under the terms of the GNU General Public License version 3,
55@@ -32,8 +32,13 @@
56
57 namespace mi = mir::input;
58
59-mi::DefaultInputManager::DefaultInputManager(std::shared_ptr<dispatch::MultiplexingDispatchable> const& multiplexer)
60- : multiplexer{multiplexer}, queue{std::make_shared<mir::dispatch::ActionQueue>()}, state{State::stopped}
61+mi::DefaultInputManager::DefaultInputManager(
62+ std::shared_ptr<dispatch::MultiplexingDispatchable> const& multiplexer,
63+ std::shared_ptr<Platform> const& platform) :
64+ platforms{platform},
65+ multiplexer{multiplexer},
66+ queue{std::make_shared<mir::dispatch::ActionQueue>()},
67+ state{State::stopped}
68 {
69 }
70
71
72=== modified file 'src/server/input/default_input_manager.h'
73--- src/server/input/default_input_manager.h 2016-01-29 08:18:22 +0000
74+++ src/server/input/default_input_manager.h 2016-03-03 09:58:55 +0000
75@@ -1,5 +1,5 @@
76 /*
77- * Copyright © 2015 Canonical Ltd.
78+ * Copyright © 2015-2016 Canonical Ltd.
79 *
80 * This program is free software: you can redistribute it and/or modify it
81 * under the terms of the GNU General Public License version 3,
82@@ -42,12 +42,19 @@
83 class DefaultInputManager : public InputManager
84 {
85 public:
86- DefaultInputManager(std::shared_ptr<dispatch::MultiplexingDispatchable> const& multiplexer);
87+ DefaultInputManager(
88+ std::shared_ptr<dispatch::MultiplexingDispatchable> const& multiplexer,
89+ std::shared_ptr<Platform> const& platform);
90 ~DefaultInputManager();
91- void add_platform(std::shared_ptr<Platform> const& platform) override;
92+
93 void start() override;
94 void stop() override;
95 private:
96+ // TODO Remove add_platform() when we next break mirserver ABI
97+ // when we do that we can also convert platforms to a std::shared_ptr<Platform> const
98+ // and simplify start_platforms() & stop_platforms()
99+ void add_platform(std::shared_ptr<Platform> const& platform) override;
100+
101 void start_platforms();
102 void stop_platforms();
103 std::vector<std::shared_ptr<Platform>> platforms;
104
105=== modified file 'tests/unit-tests/input/test_default_input_manager.cpp'
106--- tests/unit-tests/input/test_default_input_manager.cpp 2016-01-29 08:18:22 +0000
107+++ tests/unit-tests/input/test_default_input_manager.cpp 2016-03-03 09:58:55 +0000
108@@ -1,5 +1,5 @@
109 /*
110- * Copyright © 2015 Canonical Ltd.
111+ * Copyright © 2015-2016 Canonical Ltd.
112 *
113 * This program is free software: you can redistribute it and/or modify
114 * it under the terms of the GNU General Public License version 3 as
115@@ -47,7 +47,7 @@
116 md::ActionQueue platform_dispatchable;
117 NiceMock<mtd::MockInputPlatform> platform;
118 mir::Fd event_hub_fd{eventfd(0, EFD_CLOEXEC|EFD_NONBLOCK)};
119- mir::input::DefaultInputManager input_manager{mt::fake_shared(multiplexer)};
120+ mir::input::DefaultInputManager input_manager{mt::fake_shared(multiplexer), mt::fake_shared(platform)};
121
122 DefaultInputManagerTest()
123 {
124@@ -76,7 +76,6 @@
125 EXPECT_CALL(platform, start()).Times(1);
126 EXPECT_CALL(platform, dispatchable()).Times(1);
127
128- input_manager.add_platform(mt::fake_shared(platform));
129 input_manager.start();
130
131 // start() is synchronous, all start-up operations should be finished at this point
132@@ -92,7 +91,6 @@
133 EXPECT_CALL(platform, stop()).Times(1);
134
135 input_manager.start();
136- input_manager.add_platform(mt::fake_shared(platform));
137
138 EXPECT_TRUE(wait_for_multiplexer_dispatch());
139 }
140@@ -102,7 +100,6 @@
141 EXPECT_CALL(platform, stop()).Times(1);
142
143 input_manager.start();
144- input_manager.add_platform(mt::fake_shared(platform));
145 input_manager.stop();
146 }
147
148@@ -111,7 +108,6 @@
149 EXPECT_CALL(platform, start()).Times(1);
150
151 input_manager.start();
152- input_manager.add_platform(mt::fake_shared(platform));
153 input_manager.start();
154
155 EXPECT_TRUE(wait_for_multiplexer_dispatch());
156@@ -123,7 +119,6 @@
157 EXPECT_CALL(platform, stop()).Times(1);
158
159 input_manager.start();
160- input_manager.add_platform(mt::fake_shared(platform));
161 input_manager.stop();
162 input_manager.stop();
163 }
164@@ -132,7 +127,6 @@
165 EXPECT_CALL(platform, start()).Times(1);
166 const int more_than_one_thread = 10;
167
168- input_manager.add_platform(mt::fake_shared(platform));
169 std::list<std::thread> threads;
170 for (int i = 0; i != more_than_one_thread; ++i)
171 {
172@@ -154,7 +148,6 @@
173 EXPECT_CALL(platform, stop()).Times(1);
174 const int more_than_one_thread = 10;
175
176- input_manager.add_platform(mt::fake_shared(platform));
177 input_manager.start();
178 std::list<std::thread> threads;
179 for (int i = 0; i != more_than_one_thread; ++i)

Subscribers

People subscribed via source and target branches