Merge lp:~alan-griffiths/miral/persistant-surface-store into lp:miral

Proposed by Alan Griffiths on 2016-08-09
Status: Rejected
Rejected by: Alan Griffiths on 2016-08-10
Proposed branch: lp:~alan-griffiths/miral/persistant-surface-store
Merge into: lp:miral
Diff against target: 329 lines (+281/-0)
6 files modified
include/miral/toolkit/persistent_id.h (+46/-0)
miral/CMakeLists.txt (+2/-0)
miral/persistent_surface_store.cpp (+53/-0)
miral/persistent_surface_store.h (+71/-0)
test/CMakeLists.txt (+1/-0)
test/persistent_surface_store.cpp (+108/-0)
To merge this branch: bzr merge lp:~alan-griffiths/miral/persistant-surface-store
Reviewer Review Type Date Requested Status
Chris Halse Rogers Disapprove on 2016-08-10
Alan Griffiths Needs Information on 2016-08-09
Review via email: mp+302408@code.launchpad.net

Description of the change

I'm not sure how best to expose this functionality. I suspect that servers will only ever want to retrieve the Window/WindowInfo corresponding to a persistent ID.

If that's the case, then adding WindowManagerTools::window_for_id() is probably more useful than exposing the whole class implemented here.

Thoughts/suggestions?

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

*Discussion requested*

review: Needs Information
Chris Halse Rogers (raof) wrote :

I think you're correct; the shell is likely only interested in WindowManagerTools::window_for_id() in the short term.

In the longer term, we'll need to provide a way to *persist* the PersistentSurfaceStore; I was expecting that could be done by the shell providing an implementation of it that wrote to disc, but on second thoughts I think we might actually want shells to provide us with a key/value store implementation.

Either way, that's something that this miral::PersistentSurfaceStore doesn't help with.

review: Disapprove
276. By Alan Griffiths on 2016-08-10

Fix for lp:1611337 has landed

Alan Griffiths (alan-griffiths) wrote :

> I think you're correct; the shell is likely only interested in
> WindowManagerTools::window_for_id() in the short term.

I caught up with <dednick> on IRC, he concurs.

Unmerged revisions

276. By Alan Griffiths on 2016-08-10

Fix for lp:1611337 has landed

275. By Alan Griffiths on 2016-08-09

Cleanup CMakeLists.txt

274. By Alan Griffiths on 2016-08-09

Split implementation code out of test

273. By Alan Griffiths on 2016-08-09

merge lp:miral

272. By Alan Griffiths on 2016-08-09

Simpler workaround

271. By Alan Griffiths on 2016-08-09

Workaround lp:1611337

270. By Alan Griffiths on 2016-08-09

merge :push

269. By Alan Griffiths on 2016-08-09

Tidy code

268. By Alan Griffiths on 2016-08-09

Split out miral/toolkit/persistent_id.h

267. By Alan Griffiths on 2016-08-09

PersistentSurfaceStore.server_can_identify_surface_specified_by_client

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== added file 'include/miral/toolkit/persistent_id.h'
2--- include/miral/toolkit/persistent_id.h 1970-01-01 00:00:00 +0000
3+++ include/miral/toolkit/persistent_id.h 2016-08-10 09:21:16 +0000
4@@ -0,0 +1,46 @@
5+/*
6+ * Copyright © 2016 Canonical Ltd.
7+ *
8+ * This program is free software: you can redistribute it and/or modify it
9+ * under the terms of the GNU General Public License version 3,
10+ * as published by the Free Software Foundation.
11+ *
12+ * This program is distributed in the hope that it will be useful,
13+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
14+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
15+ * GNU General Public License for more details.
16+ *
17+ * You should have received a copy of the GNU General Public License
18+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
19+ *
20+ * Authored by: Alan Griffiths <alan@octopull.co.uk>
21+ */
22+
23+#ifndef MIRAL_TOOLKIT_PERSISTENT_ID_H
24+#define MIRAL_TOOLKIT_PERSISTENT_ID_H
25+
26+#include <mir_toolkit/mir_surface.h>
27+
28+#include <memory>
29+
30+namespace miral
31+{
32+namespace toolkit
33+{
34+/// Handle class for MirPersistentId - provides automatic reference counting
35+class PersistentId
36+{
37+public:
38+ explicit PersistentId(MirPersistentId* id) : self{id, deleter} {}
39+ explicit PersistentId(MirSurface* surface) : PersistentId{mir_surface_request_persistent_id_sync(surface)} {}
40+
41+ auto c_str() const -> char const* { return mir_persistent_id_as_string(self.get()); }
42+
43+private:
44+ static void deleter(MirPersistentId* id) { mir_persistent_id_release(id); }
45+ std::shared_ptr<MirPersistentId> self;
46+};
47+}
48+}
49+
50+#endif //MIRAL_TOOLKIT_PERSISTENT_ID_H
51
52=== modified file 'miral/CMakeLists.txt'
53--- miral/CMakeLists.txt 2016-08-04 16:26:20 +0000
54+++ miral/CMakeLists.txt 2016-08-10 09:21:16 +0000
55@@ -8,6 +8,7 @@
56 add_library(miral-internal STATIC
57 basic_window_manager.cpp basic_window_manager.h window_manager_tools_implementation.h
58 mru_window_list.cpp mru_window_list.h
59+ persistent_surface_store.cpp persistent_surface_store.h
60 )
61
62 add_library(miral SHARED
63@@ -30,6 +31,7 @@
64 window_manager_tools.cpp ${CMAKE_SOURCE_DIR}/include/miral/window_manager_tools.h
65 ${CMAKE_SOURCE_DIR}/include/miral/stream_specification.h
66 ${CMAKE_SOURCE_DIR}/include/miral/toolkit/surface_spec.h
67+ ${CMAKE_SOURCE_DIR}/include/miral/toolkit/persistent_id.h
68 ${CMAKE_SOURCE_DIR}/include/miral/toolkit/connection.h
69 )
70
71
72=== added file 'miral/persistent_surface_store.cpp'
73--- miral/persistent_surface_store.cpp 1970-01-01 00:00:00 +0000
74+++ miral/persistent_surface_store.cpp 2016-08-10 09:21:16 +0000
75@@ -0,0 +1,53 @@
76+/*
77+ * Copyright © 2016 Canonical Ltd.
78+ *
79+ * This program is free software: you can redistribute it and/or modify it
80+ * under the terms of the GNU General Public License version 3,
81+ * as published by the Free Software Foundation.
82+ *
83+ * This program is distributed in the hope that it will be useful,
84+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
85+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
86+ * GNU General Public License for more details.
87+ *
88+ * You should have received a copy of the GNU General Public License
89+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
90+ *
91+ * Authored by: Alan Griffiths <alan@octopull.co.uk>
92+ */
93+
94+#include "persistent_surface_store.h"
95+
96+#include <mir/shell/persistent_surface_store.h>
97+#include <mir/server.h>
98+#include <mir/version.h>
99+
100+miral::PersistentSurfaceStore::PersistentSurfaceStore() = default;
101+miral::PersistentSurfaceStore::~PersistentSurfaceStore() = default;
102+miral::PersistentSurfaceStore::PersistentSurfaceStore(PersistentSurfaceStore const&) = default;
103+miral::PersistentSurfaceStore& miral::PersistentSurfaceStore::operator=(PersistentSurfaceStore const&) = default;
104+
105+void miral::PersistentSurfaceStore::operator()(mir::Server& server)
106+{
107+#if MIR_SERVER_VERSION >= MIR_VERSION_NUMBER(0, 24, 0)
108+ server.add_init_callback[this] { self = server.the_persistent_surface_store(); });
109+#else
110+ (void)server;
111+#endif
112+}
113+
114+auto miral::PersistentSurfaceStore::id_for_surface(std::shared_ptr<mir::scene::Surface> const& surface) -> Id
115+{
116+ if (auto myself = self.lock())
117+ return myself->id_for_surface(surface).serialize_to_string();
118+ else
119+ throw std::logic_error{"PersistentSurfaceStore not initialized"};
120+}
121+
122+auto miral::PersistentSurfaceStore::surface_for_id(Id const& id) const -> std::shared_ptr<mir::scene::Surface>
123+{
124+ if (auto myself = self.lock())
125+ return myself->surface_for_id(id);
126+ else
127+ return {};
128+}
129
130=== added file 'miral/persistent_surface_store.h'
131--- miral/persistent_surface_store.h 1970-01-01 00:00:00 +0000
132+++ miral/persistent_surface_store.h 2016-08-10 09:21:16 +0000
133@@ -0,0 +1,71 @@
134+/*
135+ * Copyright © 2016 Canonical Ltd.
136+ *
137+ * This program is free software: you can redistribute it and/or modify it
138+ * under the terms of the GNU General Public License version 3,
139+ * as published by the Free Software Foundation.
140+ *
141+ * This program is distributed in the hope that it will be useful,
142+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
143+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
144+ * GNU General Public License for more details.
145+ *
146+ * You should have received a copy of the GNU General Public License
147+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
148+ *
149+ * Authored by: Alan Griffiths <alan@octopull.co.uk>
150+ */
151+
152+#ifndef MIRAL_PERSISTENT_SURFACE_STORE_H
153+#define MIRAL_PERSISTENT_SURFACE_STORE_H
154+
155+#include <memory>
156+#include <string>
157+
158+namespace mir
159+{
160+class Server;
161+namespace scene { class Surface; }
162+namespace shell { class PersistentSurfaceStore; }
163+}
164+
165+namespace miral
166+{
167+class PersistentSurfaceStore
168+{
169+public:
170+ PersistentSurfaceStore();
171+ ~PersistentSurfaceStore();
172+ PersistentSurfaceStore(PersistentSurfaceStore const&);
173+ PersistentSurfaceStore& operator=(PersistentSurfaceStore const&);
174+
175+ void operator()(mir::Server& server);
176+
177+ using Id = std::string;
178+
179+ /**
180+ * \brief Acquire ID for a Surface
181+ * \param [in] surface Surface to query or generate an ID for
182+ * \return The ID for this surface.
183+ * \note If \arg surface has not yet had an ID generated, this generates its ID.
184+ * \note This does not extend the lifetime of \arg surface.
185+ * \warning Prior to Mir-0.24 the underlying APIs are not available: we throw logic_error.
186+ */
187+ auto id_for_surface(std::shared_ptr<mir::scene::Surface> const& surface) -> Id;
188+
189+ /**
190+ * \brief Lookup Surface by ID.
191+ * \param [in] id ID of surface to lookup
192+ * \return The surface with ID \arg id. If this surface has been destroyed,
193+ * but the store retains a reference, returns nullptr.
194+ * \throws std::out_of_range if the store has no reference for a surface with \arg id.
195+ * \warning Prior to Mir-0.24 the underlying APIs are not available: we return nullptr.
196+ */
197+ auto surface_for_id(Id const& id) const -> std::shared_ptr<mir::scene::Surface>;
198+
199+private:
200+ std::weak_ptr<mir::shell::PersistentSurfaceStore> self;
201+};
202+}
203+
204+#endif //MIRAL_PERSISTENT_SURFACE_STORE_H
205
206=== modified file 'test/CMakeLists.txt'
207--- test/CMakeLists.txt 2016-08-02 09:05:46 +0000
208+++ test/CMakeLists.txt 2016-08-10 09:21:16 +0000
209@@ -9,6 +9,7 @@
210 add_executable(miral-test
211 mru_window_list.cpp
212 active_outputs.cpp
213+ persistent_surface_store.cpp
214 )
215
216 target_link_libraries(miral-test
217
218=== added file 'test/persistent_surface_store.cpp'
219--- test/persistent_surface_store.cpp 1970-01-01 00:00:00 +0000
220+++ test/persistent_surface_store.cpp 2016-08-10 09:21:16 +0000
221@@ -0,0 +1,108 @@
222+/*
223+ * Copyright © 2016 Canonical Ltd.
224+ *
225+ * This program is free software: you can redistribute it and/or modify it
226+ * under the terms of the GNU General Public License version 3,
227+ * as published by the Free Software Foundation.
228+ *
229+ * This program is distributed in the hope that it will be useful,
230+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
231+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
232+ * GNU General Public License for more details.
233+ *
234+ * You should have received a copy of the GNU General Public License
235+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
236+ *
237+ * Authored by: Alan Griffiths <alan@octopull.co.uk>
238+ */
239+
240+#include "../miral/persistent_surface_store.h"
241+
242+#include <miral/toolkit/persistent_id.h>
243+
244+#include <mir/test/doubles/wrap_shell_to_track_latest_surface.h>
245+#include <mir_test_framework/connected_client_with_a_surface.h>
246+
247+#include <gtest/gtest.h>
248+#include <gmock/gmock.h>
249+
250+namespace msh = mir::shell;
251+namespace ms = mir::scene;
252+namespace mt = mir::test;
253+namespace mtd = mt::doubles;
254+namespace mtf = mir_test_framework;
255+
256+using namespace mir::geometry;
257+using namespace testing;
258+
259+namespace
260+{
261+struct PersistentSurfaceStore : mtf::ConnectedClientWithASurface
262+{
263+ void SetUp() override
264+ {
265+ server.wrap_shell([this](std::shared_ptr<msh::Shell> const& wrapped)
266+ {
267+ auto const msc = std::make_shared<mtd::WrapShellToTrackLatestSurface>(wrapped);
268+ shell = msc;
269+ return msc;
270+ });
271+
272+#if MIR_SERVER_VERSION < MIR_VERSION_NUMBER(0, 25, 0)
273+ preset_display({}); // Workaround for lp:1611337
274+#endif
275+ persistent_surface_store(server);
276+ mtf::ConnectedClientWithASurface::SetUp();
277+ }
278+
279+ void TearDown() override
280+ {
281+ mtf::ConnectedClientWithASurface::TearDown();
282+ }
283+
284+ std::shared_ptr<ms::Surface> latest_shell_surface() const
285+ {
286+ auto myshell = shell.lock();
287+ EXPECT_THAT(myshell, NotNull());
288+ auto const result = myshell->latest_surface.lock();
289+ EXPECT_THAT(result, NotNull());
290+ return result;
291+ }
292+
293+ miral::PersistentSurfaceStore persistent_surface_store;
294+
295+private:
296+ std::weak_ptr<mtd::WrapShellToTrackLatestSurface> shell;
297+};
298+}
299+
300+TEST_F(PersistentSurfaceStore, server_and_client_persistent_id_matches)
301+{
302+ auto const shell_server_surface = latest_shell_surface();
303+ ASSERT_THAT(shell_server_surface, NotNull());
304+
305+ miral::toolkit::PersistentId client_surface_id{surface};
306+
307+#if MIR_SERVER_VERSION >= MIR_VERSION_NUMBER(0, 24, 0)
308+ auto const server_surface_id = persistent_surface_store.id_for_surface(shell_server_surface);
309+
310+ auto const client_surface_id_string = client_surface_id.c_str();
311+
312+ ASSERT_THAT(server_surface_id, Eq(client_surface_id_string));
313+#else
314+ EXPECT_THROW(persistent_surface_store.id_for_surface(shell_server_surface), std::logic_error);
315+#endif
316+}
317+
318+TEST_F(PersistentSurfaceStore, server_can_identify_surface_specified_by_client)
319+{
320+ miral::toolkit::PersistentId client_surface_id{surface};
321+
322+ auto const server_surface = persistent_surface_store.surface_for_id(client_surface_id.c_str());
323+
324+#if MIR_SERVER_VERSION >= MIR_VERSION_NUMBER(0, 24, 0)
325+ ASSERT_THAT(server_surface, Eq(latest_shell_surface()));
326+#else
327+ ASSERT_THAT(server_surface, IsNull());
328+#endif
329+}

Subscribers

People subscribed via source and target branches