Merge lp:~robertcarr/mir/focus-tell-dont-ask into lp:~mir-team/mir/trunk
- focus-tell-dont-ask
- Merge into trunk
Status: | Superseded |
---|---|
Proposed branch: | lp:~robertcarr/mir/focus-tell-dont-ask |
Merge into: | lp:~mir-team/mir/trunk |
Diff against target: |
478 lines (+160/-105) 14 files modified
include/server/mir/frontend/communicator_report.h (+0/-1) include/server/mir/shell/application_session.h (+6/-0) include/server/mir/shell/default_focus_mechanism.h (+2/-2) include/server/mir/shell/session.h (+5/-0) include/test/mir_test_doubles/mock_shell_session.h (+3/-0) include/test/mir_test_doubles/stub_shell_session.h (+8/-0) src/server/frontend/protobuf_socket_communicator.cpp (+1/-1) src/server/frontend/socket_messenger.cpp (+13/-3) src/server/frontend/socket_messenger.h (+6/-1) src/server/shell/application_session.cpp (+38/-3) src/server/shell/default_focus_mechanism.cpp (+10/-18) tests/unit-tests/frontend/CMakeLists.txt (+1/-0) tests/unit-tests/frontend/test_socket_messenger.cpp (+37/-0) tests/unit-tests/shell/test_default_focus_mechanism.cpp (+30/-76) |
To merge this branch: | bzr merge lp:~robertcarr/mir/focus-tell-dont-ask |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Alan Griffiths | Pending | ||
Daniel van Vugt | Pending | ||
PS Jenkins bot | continuous-integration | Pending | |
Alexandros Frantzis | Pending | ||
Review via email: mp+186655@code.launchpad.net |
This proposal supersedes a proposal from 2013-09-06.
This proposal has been superseded by a proposal from 2013-09-23.
Commit message
Description of the change
== Old Description ==
Fix the races around focus. Essentially these stem from that holding std::shared_
So this branch changes ApplicationSession to not destroy the surface explicitly and instead rely on ~msh::Surface, which means that you really can hold a safe pointer. I think this as a reasonable choice, as the shell might want to do similar things, i.e. continue to animate a surface after a session has closed it.
==
New approach relies on ApplicationSession to hold the last focused ptr...WIP
Robert Carr (robertcarr) wrote : Posted in a previous version of this proposal | # |
Robert Carr (robertcarr) wrote : Posted in a previous version of this proposal | # |
The unsafely held surface pointer in this case is the previously focused app.
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal | # |
PASSED: Continuous integration, rev:1054
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
Click here to trigger a rebuild:
http://
Alexandros Frantzis (afrantzis) wrote : Posted in a previous version of this proposal | # |
There is an inconsistency with this approach: when a session is closed its surface resources are destroyed immediately, but when a surface is explicitly destroyed the resources are not destroyed until there are no more users.
Would it (make sense|be possible) for msh::Surface to own (hold a strong pointer to) ms::Surface and rely on ~msh::Surface() in both the cases above, thus removing the msh::Surface -> ms::Surface weak_ptr dance?
Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal | # |
> There is an inconsistency with this approach: when a session is closed its
> surface resources are destroyed immediately, but when a surface is explicitly
> destroyed the resources are not destroyed until there are no more users.
>
> Would it (make sense|be possible) for msh::Surface to own (hold a strong
> pointer to) ms::Surface and rely on ~msh::Surface() in both the cases above,
> thus removing the msh::Surface -> ms::Surface weak_ptr dance?
I've not yet looked at the MP, but I can contribute the origin of the "weak_ptr dance".
Once a surface has been "destroyed" it is no longer in the scenegraph (ok, SurfaceStack) and therefore not composited. If the ms::Surface continued to exist it could reach a state where it has client buffers meaning threads that require them will wait forever.
It may well be that subsequent changes (particularly the introduction of force_requests_
Alan Griffiths (alan-griffiths) wrote : Posted in a previous version of this proposal | # |
I don't think this is the right evolution of the code, but I don't think it makes things worse.
The right solution would be more complex and should probably come as part of fixing the data model.
Robert Carr (robertcarr) wrote : Posted in a previous version of this proposal | # |
I think it makes sense to not destroy the surfaces explicitly from the session (as in the last committ). It may be possible to fix the weak_ptr dance (it almost surely is possible). It's not really clear what direction this code should go in right now though (except probably not further down this one), so I am trying to just fix the crashes non invasively
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal | # |
PASSED: Continuous integration, rev:1055
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
SUCCESS: http://
deb: http://
Click here to trigger a rebuild:
http://
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Bug 1216727 is still happening :(
terminate called after throwing an instance of 'boost:
what(): Requesting handle for an unregistered channel
Should we just unlink it from the branch?
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
P.S. Before declaring bug 1216727 fixed, please find a machine on which you can reproduce it (using trunk). To be sure the changes are actually having an effect.
Robert Carr (robertcarr) wrote : Posted in a previous version of this proposal | # |
Can someone review this again?
Daniel van Vugt (vanvugt) : Posted in a previous version of this proposal | # |
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal | # |
FAILED: Continuous integration, rev:1057
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
Click here to trigger a rebuild:
http://
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Weirdly, merging this branch now causes an almost immediate hang:
Running tests...
Test project /home/dan/
Start 1: LGPL-required
1/145 Test #1: LGPL-required .......
Start 2: GPL-required
2/145 Test #2: GPL-required .......
Start 3: acceptance-
PS Jenkins bot (ps-jenkins) wrote : Posted in a previous version of this proposal | # |
FAILED: Continuous integration, rev:1058
http://
Executed test runs:
SUCCESS: http://
SUCCESS: http://
FAILURE: http://
SUCCESS: http://
deb: http://
Click here to trigger a rebuild:
http://
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal | # |
Another bug with this branch is that on Alt+Tab in demo_server_shell, the client rendering freezes. Permanently.
- 1085. By Robert Carr
-
Typo
- 1086. By Robert Carr
-
Merge development-branch
- 1087. By Robert Carr
-
Merge socket-
messenger- reporitng - 1088. By Robert Carr
-
Fix error in relinquish focus
Unmerged revisions
- 1088. By Robert Carr
-
Fix error in relinquish focus
- 1087. By Robert Carr
-
Merge socket-
messenger- reporitng - 1086. By Robert Carr
-
Merge development-branch
- 1085. By Robert Carr
-
Typo
- 1084. By Robert Carr
-
application_
session: Introduce default_ surface_ locked to dedupe some code - 1083. By Robert Carr
-
Merge fix-1226139
- 1082. By Robert Carr
-
More cleanup
- 1081. By Robert Carr
-
Merge trunk
- 1080. By Robert Carr
-
Cleanup
- 1079. By Robert Carr
-
Mutex for surface
Preview Diff
1 | === modified file 'include/server/mir/frontend/communicator_report.h' | |||
2 | --- include/server/mir/frontend/communicator_report.h 2013-06-19 16:14:40 +0000 | |||
3 | +++ include/server/mir/frontend/communicator_report.h 2013-09-23 14:32:39 +0000 | |||
4 | @@ -42,7 +42,6 @@ | |||
5 | 42 | class NullCommunicatorReport : public CommunicatorReport | 42 | class NullCommunicatorReport : public CommunicatorReport |
6 | 43 | { | 43 | { |
7 | 44 | public: | 44 | public: |
8 | 45 | |||
9 | 46 | void error(std::exception const& error); | 45 | void error(std::exception const& error); |
10 | 47 | }; | 46 | }; |
11 | 48 | } | 47 | } |
12 | 49 | 48 | ||
13 | === modified file 'include/server/mir/shell/application_session.h' | |||
14 | --- include/server/mir/shell/application_session.h 2013-08-28 03:41:48 +0000 | |||
15 | +++ include/server/mir/shell/application_session.h 2013-09-23 14:32:39 +0000 | |||
16 | @@ -67,6 +67,9 @@ | |||
17 | 67 | 67 | ||
18 | 68 | void set_lifecycle_state(MirLifecycleState state); | 68 | void set_lifecycle_state(MirLifecycleState state); |
19 | 69 | 69 | ||
20 | 70 | void receive_focus(std::shared_ptr<InputTargeter> const& targeter, std::shared_ptr<SurfaceController> const& controller); | ||
21 | 71 | void relinquish_focus(); | ||
22 | 72 | |||
23 | 70 | protected: | 73 | protected: |
24 | 71 | ApplicationSession(ApplicationSession const&) = delete; | 74 | ApplicationSession(ApplicationSession const&) = delete; |
25 | 72 | ApplicationSession& operator=(ApplicationSession const&) = delete; | 75 | ApplicationSession& operator=(ApplicationSession const&) = delete; |
26 | @@ -79,6 +82,7 @@ | |||
27 | 79 | std::shared_ptr<frontend::EventSink> const event_sink; | 82 | std::shared_ptr<frontend::EventSink> const event_sink; |
28 | 80 | 83 | ||
29 | 81 | frontend::SurfaceId next_id(); | 84 | frontend::SurfaceId next_id(); |
30 | 85 | std::shared_ptr<Surface> default_surface_locked(std::unique_lock<std::mutex> const& lock) const; | ||
31 | 82 | 86 | ||
32 | 83 | std::atomic<int> next_surface_id; | 87 | std::atomic<int> next_surface_id; |
33 | 84 | 88 | ||
34 | @@ -86,6 +90,8 @@ | |||
35 | 86 | Surfaces::const_iterator checked_find(frontend::SurfaceId id) const; | 90 | Surfaces::const_iterator checked_find(frontend::SurfaceId id) const; |
36 | 87 | std::mutex mutable surfaces_mutex; | 91 | std::mutex mutable surfaces_mutex; |
37 | 88 | Surfaces surfaces; | 92 | Surfaces surfaces; |
38 | 93 | |||
39 | 94 | std::weak_ptr<Surface> last_focused_surface; | ||
40 | 89 | }; | 95 | }; |
41 | 90 | 96 | ||
42 | 91 | } | 97 | } |
43 | 92 | 98 | ||
44 | === modified file 'include/server/mir/shell/default_focus_mechanism.h' | |||
45 | --- include/server/mir/shell/default_focus_mechanism.h 2013-08-28 03:41:48 +0000 | |||
46 | +++ include/server/mir/shell/default_focus_mechanism.h 2013-09-23 14:32:39 +0000 | |||
47 | @@ -50,8 +50,8 @@ | |||
48 | 50 | std::shared_ptr<InputTargeter> const input_targeter; | 50 | std::shared_ptr<InputTargeter> const input_targeter; |
49 | 51 | std::shared_ptr<SurfaceController> const surface_controller; | 51 | std::shared_ptr<SurfaceController> const surface_controller; |
50 | 52 | 52 | ||
53 | 53 | std::mutex surface_focus_lock; | 53 | std::mutex focus_lock; |
54 | 54 | std::weak_ptr<Surface> currently_focused_surface; | 54 | std::weak_ptr<Session> currently_focused_session; |
55 | 55 | }; | 55 | }; |
56 | 56 | 56 | ||
57 | 57 | } | 57 | } |
58 | 58 | 58 | ||
59 | === modified file 'include/server/mir/shell/session.h' | |||
60 | --- include/server/mir/shell/session.h 2013-08-28 03:41:48 +0000 | |||
61 | +++ include/server/mir/shell/session.h 2013-09-23 14:32:39 +0000 | |||
62 | @@ -28,6 +28,8 @@ | |||
63 | 28 | namespace shell | 28 | namespace shell |
64 | 29 | { | 29 | { |
65 | 30 | class Surface; | 30 | class Surface; |
66 | 31 | class InputTargeter; | ||
67 | 32 | class SurfaceController; | ||
68 | 31 | 33 | ||
69 | 32 | class Session : public frontend::Session | 34 | class Session : public frontend::Session |
70 | 33 | { | 35 | { |
71 | @@ -37,6 +39,9 @@ | |||
72 | 37 | 39 | ||
73 | 38 | virtual void take_snapshot(SnapshotCallback const& snapshot_taken) = 0; | 40 | virtual void take_snapshot(SnapshotCallback const& snapshot_taken) = 0; |
74 | 39 | virtual std::shared_ptr<Surface> default_surface() const = 0; | 41 | virtual std::shared_ptr<Surface> default_surface() const = 0; |
75 | 42 | |||
76 | 43 | virtual void receive_focus(std::shared_ptr<InputTargeter> const& targeter, std::shared_ptr<SurfaceController> const& controller) = 0; | ||
77 | 44 | virtual void relinquish_focus() = 0; | ||
78 | 40 | }; | 45 | }; |
79 | 41 | 46 | ||
80 | 42 | } | 47 | } |
81 | 43 | 48 | ||
82 | === modified file 'include/test/mir_test_doubles/mock_shell_session.h' | |||
83 | --- include/test/mir_test_doubles/mock_shell_session.h 2013-08-28 03:41:48 +0000 | |||
84 | +++ include/test/mir_test_doubles/mock_shell_session.h 2013-09-23 14:32:39 +0000 | |||
85 | @@ -49,6 +49,9 @@ | |||
86 | 49 | 49 | ||
87 | 50 | MOCK_METHOD1(send_display_config, void(graphics::DisplayConfiguration const&)); | 50 | MOCK_METHOD1(send_display_config, void(graphics::DisplayConfiguration const&)); |
88 | 51 | MOCK_METHOD3(configure_surface, int(frontend::SurfaceId, MirSurfaceAttrib, int)); | 51 | MOCK_METHOD3(configure_surface, int(frontend::SurfaceId, MirSurfaceAttrib, int)); |
89 | 52 | |||
90 | 53 | MOCK_METHOD2(receive_focus, void(std::shared_ptr<shell::InputTargeter> const&, std::shared_ptr<shell::SurfaceController> const&)); | ||
91 | 54 | MOCK_METHOD0(relinquish_focus, void()); | ||
92 | 52 | }; | 55 | }; |
93 | 53 | 56 | ||
94 | 54 | } | 57 | } |
95 | 55 | 58 | ||
96 | === modified file 'include/test/mir_test_doubles/stub_shell_session.h' | |||
97 | --- include/test/mir_test_doubles/stub_shell_session.h 2013-08-13 22:20:37 +0000 | |||
98 | +++ include/test/mir_test_doubles/stub_shell_session.h 2013-09-23 14:32:39 +0000 | |||
99 | @@ -71,6 +71,14 @@ | |||
100 | 71 | { | 71 | { |
101 | 72 | return std::shared_ptr<shell::Surface>(); | 72 | return std::shared_ptr<shell::Surface>(); |
102 | 73 | } | 73 | } |
103 | 74 | |||
104 | 75 | void receive_focus(std::shared_ptr<shell::InputTargeter> const&, | ||
105 | 76 | std::shared_ptr<shell::SurfaceController> const&) | ||
106 | 77 | { | ||
107 | 78 | } | ||
108 | 79 | void relinquish_focus() | ||
109 | 80 | { | ||
110 | 81 | } | ||
111 | 74 | }; | 82 | }; |
112 | 75 | 83 | ||
113 | 76 | } | 84 | } |
114 | 77 | 85 | ||
115 | === modified file 'src/server/frontend/protobuf_socket_communicator.cpp' | |||
116 | --- src/server/frontend/protobuf_socket_communicator.cpp 2013-08-28 03:41:48 +0000 | |||
117 | +++ src/server/frontend/protobuf_socket_communicator.cpp 2013-09-23 14:32:39 +0000 | |||
118 | @@ -135,7 +135,7 @@ | |||
119 | 135 | { | 135 | { |
120 | 136 | if (!ec) | 136 | if (!ec) |
121 | 137 | { | 137 | { |
123 | 138 | auto messenger = std::make_shared<detail::SocketMessenger>(socket); | 138 | auto messenger = std::make_shared<detail::SocketMessenger>(socket, report); |
124 | 139 | auto client_pid = messenger->client_pid(); | 139 | auto client_pid = messenger->client_pid(); |
125 | 140 | if (session_authorizer->connection_is_allowed(client_pid)) | 140 | if (session_authorizer->connection_is_allowed(client_pid)) |
126 | 141 | { | 141 | { |
127 | 142 | 142 | ||
128 | === modified file 'src/server/frontend/socket_messenger.cpp' | |||
129 | --- src/server/frontend/socket_messenger.cpp 2013-08-28 03:41:48 +0000 | |||
130 | +++ src/server/frontend/socket_messenger.cpp 2013-09-23 14:32:39 +0000 | |||
131 | @@ -18,13 +18,16 @@ | |||
132 | 18 | 18 | ||
133 | 19 | #include "socket_messenger.h" | 19 | #include "socket_messenger.h" |
134 | 20 | #include "mir/frontend/client_constants.h" | 20 | #include "mir/frontend/client_constants.h" |
135 | 21 | #include "mir/frontend/communicator_report.h" | ||
136 | 21 | 22 | ||
137 | 22 | namespace mfd = mir::frontend::detail; | 23 | namespace mfd = mir::frontend::detail; |
138 | 23 | namespace bs = boost::system; | 24 | namespace bs = boost::system; |
139 | 24 | namespace ba = boost::asio; | 25 | namespace ba = boost::asio; |
140 | 25 | 26 | ||
143 | 26 | mfd::SocketMessenger::SocketMessenger(std::shared_ptr<ba::local::stream_protocol::socket> const& socket) | 27 | mfd::SocketMessenger::SocketMessenger(std::shared_ptr<ba::local::stream_protocol::socket> const& socket, |
144 | 27 | : socket(socket) | 28 | std::shared_ptr<CommunicatorReport> const& report) |
145 | 29 | : socket(socket), | ||
146 | 30 | report(report) | ||
147 | 28 | { | 31 | { |
148 | 29 | whole_message.reserve(serialization_buffer_size); | 32 | whole_message.reserve(serialization_buffer_size); |
149 | 30 | } | 33 | } |
150 | @@ -59,7 +62,14 @@ | |||
151 | 59 | // function has completed (if it would be executed asynchronously. | 62 | // function has completed (if it would be executed asynchronously. |
152 | 60 | // NOTE: we rely on this synchronous behavior as per the comment in | 63 | // NOTE: we rely on this synchronous behavior as per the comment in |
153 | 61 | // mf::SessionMediator::create_surface | 64 | // mf::SessionMediator::create_surface |
155 | 62 | ba::write(*socket, ba::buffer(whole_message)); | 65 | try |
156 | 66 | { | ||
157 | 67 | ba::write(*socket, ba::buffer(whole_message)); | ||
158 | 68 | } | ||
159 | 69 | catch (std::exception &ex) | ||
160 | 70 | { | ||
161 | 71 | report->error(ex); | ||
162 | 72 | } | ||
163 | 63 | } | 73 | } |
164 | 64 | 74 | ||
165 | 65 | void mfd::SocketMessenger::send_fds(std::vector<int32_t> const& fds) | 75 | void mfd::SocketMessenger::send_fds(std::vector<int32_t> const& fds) |
166 | 66 | 76 | ||
167 | === modified file 'src/server/frontend/socket_messenger.h' | |||
168 | --- src/server/frontend/socket_messenger.h 2013-08-28 03:41:48 +0000 | |||
169 | +++ src/server/frontend/socket_messenger.h 2013-09-23 14:32:39 +0000 | |||
170 | @@ -26,13 +26,16 @@ | |||
171 | 26 | { | 26 | { |
172 | 27 | namespace frontend | 27 | namespace frontend |
173 | 28 | { | 28 | { |
174 | 29 | class CommunicatorReport; | ||
175 | 30 | |||
176 | 29 | namespace detail | 31 | namespace detail |
177 | 30 | { | 32 | { |
178 | 31 | class SocketMessenger : public MessageSender, | 33 | class SocketMessenger : public MessageSender, |
179 | 32 | public MessageReceiver | 34 | public MessageReceiver |
180 | 33 | { | 35 | { |
181 | 34 | public: | 36 | public: |
183 | 35 | SocketMessenger(std::shared_ptr<boost::asio::local::stream_protocol::socket> const& socket); | 37 | SocketMessenger(std::shared_ptr<boost::asio::local::stream_protocol::socket> const& socket, |
184 | 38 | std::shared_ptr<frontend::CommunicatorReport> const& report); | ||
185 | 36 | 39 | ||
186 | 37 | void send(std::string const& body); | 40 | void send(std::string const& body); |
187 | 38 | void send_fds(std::vector<int32_t> const& fds); | 41 | void send_fds(std::vector<int32_t> const& fds); |
188 | @@ -42,6 +45,8 @@ | |||
189 | 42 | 45 | ||
190 | 43 | private: | 46 | private: |
191 | 44 | std::shared_ptr<boost::asio::local::stream_protocol::socket> socket; | 47 | std::shared_ptr<boost::asio::local::stream_protocol::socket> socket; |
192 | 48 | std::shared_ptr<CommunicatorReport> const& report; | ||
193 | 49 | |||
194 | 45 | std::vector<char> whole_message; | 50 | std::vector<char> whole_message; |
195 | 46 | }; | 51 | }; |
196 | 47 | } | 52 | } |
197 | 48 | 53 | ||
198 | === modified file 'src/server/shell/application_session.cpp' | |||
199 | --- src/server/shell/application_session.cpp 2013-08-28 03:41:48 +0000 | |||
200 | +++ src/server/shell/application_session.cpp 2013-09-23 14:32:39 +0000 | |||
201 | @@ -21,6 +21,7 @@ | |||
202 | 21 | #include "mir/shell/surface_factory.h" | 21 | #include "mir/shell/surface_factory.h" |
203 | 22 | #include "mir/shell/snapshot_strategy.h" | 22 | #include "mir/shell/snapshot_strategy.h" |
204 | 23 | #include "mir/shell/session_listener.h" | 23 | #include "mir/shell/session_listener.h" |
205 | 24 | #include "mir/shell/input_targeter.h" | ||
206 | 24 | #include "mir/frontend/event_sink.h" | 25 | #include "mir/frontend/event_sink.h" |
207 | 25 | 26 | ||
208 | 26 | #include <boost/throw_exception.hpp> | 27 | #include <boost/throw_exception.hpp> |
209 | @@ -99,16 +100,21 @@ | |||
210 | 99 | snapshot_strategy->take_snapshot_of(default_surface(), snapshot_taken); | 100 | snapshot_strategy->take_snapshot_of(default_surface(), snapshot_taken); |
211 | 100 | } | 101 | } |
212 | 101 | 102 | ||
214 | 102 | std::shared_ptr<msh::Surface> msh::ApplicationSession::default_surface() const | 103 | std::shared_ptr<msh::Surface> msh::ApplicationSession::default_surface_locked(std::unique_lock<std::mutex> const& /* lock */) const |
215 | 103 | { | 104 | { |
216 | 104 | std::unique_lock<std::mutex> lock(surfaces_mutex); | ||
217 | 105 | |||
218 | 106 | if (surfaces.size()) | 105 | if (surfaces.size()) |
219 | 107 | return surfaces.begin()->second; | 106 | return surfaces.begin()->second; |
220 | 108 | else | 107 | else |
221 | 109 | return std::shared_ptr<msh::Surface>(); | 108 | return std::shared_ptr<msh::Surface>(); |
222 | 110 | } | 109 | } |
223 | 111 | 110 | ||
224 | 111 | std::shared_ptr<msh::Surface> msh::ApplicationSession::default_surface() const | ||
225 | 112 | { | ||
226 | 113 | std::unique_lock<std::mutex> lock(surfaces_mutex); | ||
227 | 114 | |||
228 | 115 | return default_surface_locked(lock); | ||
229 | 116 | } | ||
230 | 117 | |||
231 | 112 | void msh::ApplicationSession::destroy_surface(mf::SurfaceId id) | 118 | void msh::ApplicationSession::destroy_surface(mf::SurfaceId id) |
232 | 113 | { | 119 | { |
233 | 114 | std::unique_lock<std::mutex> lock(surfaces_mutex); | 120 | std::unique_lock<std::mutex> lock(surfaces_mutex); |
234 | @@ -171,3 +177,32 @@ | |||
235 | 171 | { | 177 | { |
236 | 172 | event_sink->handle_lifecycle_event(state); | 178 | event_sink->handle_lifecycle_event(state); |
237 | 173 | } | 179 | } |
238 | 180 | |||
239 | 181 | void msh::ApplicationSession::receive_focus(std::shared_ptr<msh::InputTargeter> const& targeter, | ||
240 | 182 | std::shared_ptr<SurfaceController> const& controller) | ||
241 | 183 | { | ||
242 | 184 | std::unique_lock<std::mutex> lock(surfaces_mutex); | ||
243 | 185 | if (surfaces.size() == 0) | ||
244 | 186 | { | ||
245 | 187 | targeter->focus_cleared(); | ||
246 | 188 | return; | ||
247 | 189 | } | ||
248 | 190 | // Use default_surface_locked idiom instead | ||
249 | 191 | auto focus_surface = surfaces.begin()->second; | ||
250 | 192 | |||
251 | 193 | focus_surface->raise(controller); | ||
252 | 194 | focus_surface->take_input_focus(targeter); | ||
253 | 195 | focus_surface->configure(mir_surface_attrib_focus, mir_surface_focused); | ||
254 | 196 | |||
255 | 197 | last_focused_surface = focus_surface; | ||
256 | 198 | } | ||
257 | 199 | |||
258 | 200 | void msh::ApplicationSession::relinquish_focus() | ||
259 | 201 | { | ||
260 | 202 | std::unique_lock<std::mutex> lock(surfaces_mutex); | ||
261 | 203 | auto surf = default_surface_locked(lock); | ||
262 | 204 | if (surf) | ||
263 | 205 | { | ||
264 | 206 | surf->configure(mir_surface_attrib_focus, mir_surface_unfocused); | ||
265 | 207 | } | ||
266 | 208 | } | ||
267 | 174 | 209 | ||
268 | === modified file 'src/server/shell/default_focus_mechanism.cpp' | |||
269 | --- src/server/shell/default_focus_mechanism.cpp 2013-08-28 03:41:48 +0000 | |||
270 | +++ src/server/shell/default_focus_mechanism.cpp 2013-09-23 14:32:39 +0000 | |||
271 | @@ -35,28 +35,20 @@ | |||
272 | 35 | 35 | ||
273 | 36 | void msh::DefaultFocusMechanism::set_focus_to(std::shared_ptr<Session> const& focus_session) | 36 | void msh::DefaultFocusMechanism::set_focus_to(std::shared_ptr<Session> const& focus_session) |
274 | 37 | { | 37 | { |
275 | 38 | std::lock_guard<std::mutex> lg(focus_lock); | ||
276 | 39 | |||
277 | 40 | auto last_session = currently_focused_session.lock(); | ||
278 | 41 | if (last_session) | ||
279 | 42 | { | ||
280 | 43 | last_session->relinquish_focus(); | ||
281 | 44 | } | ||
282 | 45 | |||
283 | 38 | // TODO: This path should be encapsulated in a seperate clear_focus message | 46 | // TODO: This path should be encapsulated in a seperate clear_focus message |
284 | 39 | if (!focus_session) | 47 | if (!focus_session) |
285 | 40 | { | 48 | { |
286 | 41 | input_targeter->focus_cleared(); | 49 | input_targeter->focus_cleared(); |
287 | 42 | return; | 50 | return; |
288 | 43 | } | 51 | } |
307 | 44 | 52 | focus_session->receive_focus(input_targeter, surface_controller); | |
308 | 45 | auto surface = focus_session->default_surface(); | 53 | currently_focused_session = focus_session; |
291 | 46 | if (surface) | ||
292 | 47 | { | ||
293 | 48 | std::lock_guard<std::mutex> lg(surface_focus_lock); | ||
294 | 49 | auto current_focus = currently_focused_surface.lock(); | ||
295 | 50 | if (current_focus) | ||
296 | 51 | current_focus->configure(mir_surface_attrib_focus, mir_surface_unfocused); | ||
297 | 52 | surface->configure(mir_surface_attrib_focus, mir_surface_focused); | ||
298 | 53 | currently_focused_surface = surface; | ||
299 | 54 | |||
300 | 55 | surface->raise(surface_controller); | ||
301 | 56 | surface->take_input_focus(input_targeter); | ||
302 | 57 | } | ||
303 | 58 | else | ||
304 | 59 | { | ||
305 | 60 | input_targeter->focus_cleared(); | ||
306 | 61 | } | ||
309 | 62 | } | 54 | } |
310 | 63 | 55 | ||
311 | === modified file 'tests/unit-tests/frontend/CMakeLists.txt' | |||
312 | --- tests/unit-tests/frontend/CMakeLists.txt 2013-08-28 03:41:48 +0000 | |||
313 | +++ tests/unit-tests/frontend/CMakeLists.txt 2013-09-23 14:32:39 +0000 | |||
314 | @@ -8,6 +8,7 @@ | |||
315 | 8 | ${CMAKE_CURRENT_SOURCE_DIR}/test_resource_cache.cpp | 8 | ${CMAKE_CURRENT_SOURCE_DIR}/test_resource_cache.cpp |
316 | 9 | ${CMAKE_CURRENT_SOURCE_DIR}/test_session_mediator.cpp | 9 | ${CMAKE_CURRENT_SOURCE_DIR}/test_session_mediator.cpp |
317 | 10 | ${CMAKE_CURRENT_SOURCE_DIR}/test_socket_session.cpp | 10 | ${CMAKE_CURRENT_SOURCE_DIR}/test_socket_session.cpp |
318 | 11 | ${CMAKE_CURRENT_SOURCE_DIR}/test_socket_messenger.cpp | ||
319 | 11 | ${CMAKE_CURRENT_SOURCE_DIR}/test_event_sender.cpp | 12 | ${CMAKE_CURRENT_SOURCE_DIR}/test_event_sender.cpp |
320 | 12 | ${CMAKE_CURRENT_SOURCE_DIR}/test_global_event_sender.cpp | 13 | ${CMAKE_CURRENT_SOURCE_DIR}/test_global_event_sender.cpp |
321 | 13 | ) | 14 | ) |
322 | 14 | 15 | ||
323 | === added file 'tests/unit-tests/frontend/test_socket_messenger.cpp' | |||
324 | --- tests/unit-tests/frontend/test_socket_messenger.cpp 1970-01-01 00:00:00 +0000 | |||
325 | +++ tests/unit-tests/frontend/test_socket_messenger.cpp 2013-09-23 14:32:39 +0000 | |||
326 | @@ -0,0 +1,37 @@ | |||
327 | 1 | /* | ||
328 | 2 | * Copyright © 2013 Canonical Ltd. | ||
329 | 3 | * | ||
330 | 4 | * This program is free software: you can redistribute it and/or modify | ||
331 | 5 | * it under the terms of the GNU General Public License version 3 as | ||
332 | 6 | * published by the Free Software Foundation. | ||
333 | 7 | * | ||
334 | 8 | * This program is distributed in the hope that it will be useful, | ||
335 | 9 | * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
336 | 10 | * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
337 | 11 | * GNU General Public License for more details. | ||
338 | 12 | * | ||
339 | 13 | * You should have received a copy of the GNU General Public License | ||
340 | 14 | * along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
341 | 15 | * | ||
342 | 16 | * Authored by: Daniel van Vugt <daniel.van.vugt@canonical.com> | ||
343 | 17 | */ | ||
344 | 18 | |||
345 | 19 | #include "mir/frontend/communicator_report.h" | ||
346 | 20 | #include "src/server/frontend/socket_messenger.h" | ||
347 | 21 | |||
348 | 22 | #include <gmock/gmock.h> | ||
349 | 23 | #include <gtest/gtest.h> | ||
350 | 24 | |||
351 | 25 | namespace mf = mir::frontend; | ||
352 | 26 | |||
353 | 27 | using namespace mf::detail; | ||
354 | 28 | using namespace boost::asio; | ||
355 | 29 | |||
356 | 30 | TEST(SocketMessengerTest, write_failures_never_throw) | ||
357 | 31 | { | ||
358 | 32 | io_service svc; | ||
359 | 33 | auto sock = std::make_shared<local::stream_protocol::socket>(svc); | ||
360 | 34 | SocketMessenger mess(sock, std::make_shared<mf::NullCommunicatorReport>()); | ||
361 | 35 | |||
362 | 36 | EXPECT_NO_THROW( mess.send("foo") ); | ||
363 | 37 | } | ||
364 | 0 | 38 | ||
365 | === modified file 'tests/unit-tests/shell/test_default_focus_mechanism.cpp' | |||
366 | --- tests/unit-tests/shell/test_default_focus_mechanism.cpp 2013-08-28 03:41:48 +0000 | |||
367 | +++ tests/unit-tests/shell/test_default_focus_mechanism.cpp 2013-09-23 14:32:39 +0000 | |||
368 | @@ -47,80 +47,34 @@ | |||
369 | 47 | namespace mt = mir::test; | 47 | namespace mt = mir::test; |
370 | 48 | namespace mtd = mir::test::doubles; | 48 | namespace mtd = mir::test::doubles; |
371 | 49 | 49 | ||
448 | 50 | TEST(DefaultFocusMechanism, raises_default_surface) | 50 | TEST(DefaultFocusMechanism, hands_focus_to_session) |
449 | 51 | { | 51 | { |
450 | 52 | using namespace ::testing; | 52 | using namespace ::testing; |
451 | 53 | 53 | ||
452 | 54 | NiceMock<mtd::MockShellSession> app1; | 54 | mtd::MockShellSession app1; |
453 | 55 | mtd::MockSurface mock_surface(&app1, std::make_shared<mtd::StubSurfaceBuilder>()); | 55 | EXPECT_CALL(app1, receive_focus(_, _)).Times(1); |
454 | 56 | { | 56 | |
455 | 57 | InSequence seq; | 57 | msh::DefaultFocusMechanism focus_mechanism(std::make_shared<mtd::StubInputTargeter>(), std::make_shared<mtd::StubSurfaceController>()); |
456 | 58 | EXPECT_CALL(app1, default_surface()).Times(1) | 58 | |
457 | 59 | .WillOnce(Return(mt::fake_shared(mock_surface))); | 59 | focus_mechanism.set_focus_to(mt::fake_shared(app1)); |
458 | 60 | } | 60 | } |
459 | 61 | 61 | ||
460 | 62 | auto controller = std::make_shared<mtd::StubSurfaceController>(); | 62 | TEST(DefaultFocusMechanism, relinquishes_focus_from_last_session) |
461 | 63 | EXPECT_CALL(mock_surface, raise(Eq(controller))).Times(1); | 63 | { |
462 | 64 | mtd::StubInputTargeter targeter; | 64 | using namespace ::testing; |
463 | 65 | msh::DefaultFocusMechanism focus_mechanism(mt::fake_shared(targeter), controller); | 65 | |
464 | 66 | 66 | auto app1 = std::make_shared<mtd::MockShellSession>(); | |
465 | 67 | focus_mechanism.set_focus_to(mt::fake_shared(app1)); | 67 | auto app2 = std::make_shared<mtd::MockShellSession>(); |
466 | 68 | } | 68 | |
467 | 69 | 69 | { | |
468 | 70 | TEST(DefaultFocusMechanism, mechanism_notifies_default_surface_of_focus_changes) | 70 | InSequence seq; |
469 | 71 | { | 71 | EXPECT_CALL(*app1, receive_focus(_, _)).Times(1); |
470 | 72 | using namespace ::testing; | 72 | EXPECT_CALL(*app1, relinquish_focus()).Times(1); |
471 | 73 | 73 | EXPECT_CALL(*app2, receive_focus(_, _)).Times(1); | |
472 | 74 | NiceMock<mtd::MockShellSession> app1, app2; | 74 | } |
473 | 75 | mtd::MockSurface mock_surface1(&app1, std::make_shared<mtd::StubSurfaceBuilder>()); | 75 | |
474 | 76 | mtd::MockSurface mock_surface2(&app2, std::make_shared<mtd::StubSurfaceBuilder>()); | 76 | msh::DefaultFocusMechanism focus_mechanism(std::make_shared<mtd::StubInputTargeter>(), std::make_shared<mtd::StubSurfaceController>()); |
475 | 77 | 77 | ||
476 | 78 | ON_CALL(app1, default_surface()).WillByDefault(Return(mt::fake_shared(mock_surface1))); | 78 | focus_mechanism.set_focus_to(app1); |
477 | 79 | ON_CALL(app2, default_surface()).WillByDefault(Return(mt::fake_shared(mock_surface2))); | 79 | focus_mechanism.set_focus_to(app2); |
402 | 80 | |||
403 | 81 | |||
404 | 82 | { | ||
405 | 83 | InSequence seq; | ||
406 | 84 | EXPECT_CALL(mock_surface1, configure(mir_surface_attrib_focus, mir_surface_focused)).Times(1); | ||
407 | 85 | EXPECT_CALL(mock_surface1, configure(mir_surface_attrib_focus, mir_surface_unfocused)).Times(1); | ||
408 | 86 | EXPECT_CALL(mock_surface2, configure(mir_surface_attrib_focus, mir_surface_focused)).Times(1); | ||
409 | 87 | } | ||
410 | 88 | |||
411 | 89 | msh::DefaultFocusMechanism focus_mechanism(std::make_shared<mtd::StubInputTargeter>(), | ||
412 | 90 | std::make_shared<mtd::StubSurfaceController>()); | ||
413 | 91 | |||
414 | 92 | focus_mechanism.set_focus_to(mt::fake_shared(app1)); | ||
415 | 93 | focus_mechanism.set_focus_to(mt::fake_shared(app2)); | ||
416 | 94 | } | ||
417 | 95 | |||
418 | 96 | TEST(DefaultFocusMechanism, sets_input_focus) | ||
419 | 97 | { | ||
420 | 98 | using namespace ::testing; | ||
421 | 99 | |||
422 | 100 | NiceMock<mtd::MockShellSession> app1; | ||
423 | 101 | mtd::MockSurface mock_surface(&app1, std::make_shared<mtd::StubSurfaceBuilder>()); | ||
424 | 102 | { | ||
425 | 103 | InSequence seq; | ||
426 | 104 | EXPECT_CALL(app1, default_surface()).Times(1) | ||
427 | 105 | .WillOnce(Return(mt::fake_shared(mock_surface))); | ||
428 | 106 | EXPECT_CALL(app1, default_surface()).Times(1) | ||
429 | 107 | .WillOnce(Return(std::shared_ptr<msh::Surface>())); | ||
430 | 108 | } | ||
431 | 109 | |||
432 | 110 | mtd::MockInputTargeter targeter; | ||
433 | 111 | |||
434 | 112 | msh::DefaultFocusMechanism focus_mechanism(mt::fake_shared(targeter), std::make_shared<mtd::StubSurfaceController>()); | ||
435 | 113 | |||
436 | 114 | { | ||
437 | 115 | InSequence seq; | ||
438 | 116 | EXPECT_CALL(mock_surface, take_input_focus(_)).Times(1); | ||
439 | 117 | // When we have no default surface. | ||
440 | 118 | EXPECT_CALL(targeter, focus_cleared()).Times(1); | ||
441 | 119 | // When we have no session. | ||
442 | 120 | EXPECT_CALL(targeter, focus_cleared()).Times(1); | ||
443 | 121 | } | ||
444 | 122 | |||
445 | 123 | focus_mechanism.set_focus_to(mt::fake_shared(app1)); | ||
446 | 124 | focus_mechanism.set_focus_to(mt::fake_shared(app1)); | ||
447 | 125 | focus_mechanism.set_focus_to(std::shared_ptr<msh::Session>()); | ||
478 | 126 | } | 80 | } |
I was able to run the stress test with continuous cursor input for the full ten minute run. There is also good reason to believe this fixes: https:/ /bugs.launchpad .net/mir/ +bug/1220845
The exception in ~SessionMediator can be triggered if session_ manager- >close_ session throws (due to this focus race), causing SessionMediator ::disconnect to fail to reset it's session member-variable pointer. This exception is caught in frontend. However now in ~SessionMediator the pointer has not been reset so it closes the session again throwing an Invalid Session exception. causing the session to be closed again in ~