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

[Alan Griffiths]

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?

[Alan Griffiths (alan-griffiths)]

*Discussion requested*

review: Needs Information

[Chris Halse Rogers (raof)]

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

[lp:~alan-griffiths/miral/persistant-surface-store]

276. By Alan Griffiths on 2016-08-10

Fix for lp:1611337 has landed

[Alan Griffiths (alan-griffiths)]

> 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

=== added file 'include/miral/toolkit/persistent_id.h'
--- include/miral/toolkit/persistent_id.h	1970-01-01 00:00:00 +0000
+++ include/miral/toolkit/persistent_id.h	2016-08-10 09:21:16 +0000
@@ -0,0 +1,46 @@
+/*
+ * Copyright © 2016 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Alan Griffiths <[e-mail]>
+ */
+
+#ifndef MIRAL_TOOLKIT_PERSISTENT_ID_H
+#define MIRAL_TOOLKIT_PERSISTENT_ID_H
+
+#include <mir_toolkit/mir_surface.h>
+
+#include <memory>
+
+namespace miral
+{
+namespace toolkit
+{
+/// Handle class for MirPersistentId - provides automatic reference counting
+class PersistentId
+{
+public:
+    explicit PersistentId(MirPersistentId* id) : self{id, deleter} {}
+    explicit PersistentId(MirSurface* surface) : PersistentId{mir_surface_request_persistent_id_sync(surface)} {}
+
+    auto c_str() const -> char const* { return mir_persistent_id_as_string(self.get()); }
+
+private:
+    static void deleter(MirPersistentId* id) { mir_persistent_id_release(id); }
+    std::shared_ptr<MirPersistentId> self;
+};
+}
+}
+
+#endif //MIRAL_TOOLKIT_PERSISTENT_ID_H

=== modified file 'miral/CMakeLists.txt'
--- miral/CMakeLists.txt	2016-08-04 16:26:20 +0000
+++ miral/CMakeLists.txt	2016-08-10 09:21:16 +0000
@@ -8,6 +8,7 @@
 add_library(miral-internal STATIC
     basic_window_manager.cpp            basic_window_manager.h window_manager_tools_implementation.h
     mru_window_list.cpp                 mru_window_list.h
+    persistent_surface_store.cpp        persistent_surface_store.h
 )
 
 add_library(miral SHARED
@@ -30,6 +31,7 @@
     window_manager_tools.cpp            ${CMAKE_SOURCE_DIR}/include/miral/window_manager_tools.h
                                         ${CMAKE_SOURCE_DIR}/include/miral/stream_specification.h
                                         ${CMAKE_SOURCE_DIR}/include/miral/toolkit/surface_spec.h
+                                        ${CMAKE_SOURCE_DIR}/include/miral/toolkit/persistent_id.h
                                         ${CMAKE_SOURCE_DIR}/include/miral/toolkit/connection.h
 )
 

=== added file 'miral/persistent_surface_store.cpp'
--- miral/persistent_surface_store.cpp	1970-01-01 00:00:00 +0000
+++ miral/persistent_surface_store.cpp	2016-08-10 09:21:16 +0000
@@ -0,0 +1,53 @@
+/*
+ * Copyright © 2016 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Alan Griffiths <[e-mail]>
+ */
+
+#include "persistent_surface_store.h"
+
+#include <mir/shell/persistent_surface_store.h>
+#include <mir/server.h>
+#include <mir/version.h>
+
+miral::PersistentSurfaceStore::PersistentSurfaceStore() = default;
+miral::PersistentSurfaceStore::~PersistentSurfaceStore() = default;
+miral::PersistentSurfaceStore::PersistentSurfaceStore(PersistentSurfaceStore const&) = default;
+miral::PersistentSurfaceStore& miral::PersistentSurfaceStore::operator=(PersistentSurfaceStore const&) = default;
+
+void miral::PersistentSurfaceStore::operator()(mir::Server& server)
+{
+#if MIR_SERVER_VERSION >= MIR_VERSION_NUMBER(0, 24, 0)
+    server.add_init_callback[this] { self = server.the_persistent_surface_store(); });
+#else
+    (void)server;
+#endif
+}
+
+auto miral::PersistentSurfaceStore::id_for_surface(std::shared_ptr<mir::scene::Surface> const& surface) -> Id
+{
+    if (auto myself = self.lock())
+        return myself->id_for_surface(surface).serialize_to_string();
+    else
+        throw std::logic_error{"PersistentSurfaceStore not initialized"};
+}
+
+auto miral::PersistentSurfaceStore::surface_for_id(Id const& id) const -> std::shared_ptr<mir::scene::Surface>
+{
+    if (auto myself = self.lock())
+        return myself->surface_for_id(id);
+    else
+        return {};
+}

=== added file 'miral/persistent_surface_store.h'
--- miral/persistent_surface_store.h	1970-01-01 00:00:00 +0000
+++ miral/persistent_surface_store.h	2016-08-10 09:21:16 +0000
@@ -0,0 +1,71 @@
+/*
+ * Copyright © 2016 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Alan Griffiths <[e-mail]>
+ */
+
+#ifndef MIRAL_PERSISTENT_SURFACE_STORE_H
+#define MIRAL_PERSISTENT_SURFACE_STORE_H
+
+#include <memory>
+#include <string>
+
+namespace mir
+{
+class Server;
+namespace scene { class Surface; }
+namespace shell { class PersistentSurfaceStore; }
+}
+
+namespace miral
+{
+class PersistentSurfaceStore
+{
+public:
+    PersistentSurfaceStore();
+    ~PersistentSurfaceStore();
+    PersistentSurfaceStore(PersistentSurfaceStore const&);
+    PersistentSurfaceStore& operator=(PersistentSurfaceStore const&);
+
+    void operator()(mir::Server& server);
+
+    using Id = std::string;
+
+    /**
+     * \brief Acquire ID for a Surface
+     * \param [in] surface Surface to query or generate an ID for
+     * \return             The ID for this surface.
+     * \note If \arg surface has not yet had an ID generated, this generates its ID.
+     * \note This does not extend the lifetime of \arg surface.
+     * \warning Prior to Mir-0.24 the underlying APIs are not available: we throw logic_error.
+     */
+    auto id_for_surface(std::shared_ptr<mir::scene::Surface> const& surface) -> Id;
+
+    /**
+     * \brief Lookup Surface by ID.
+     * \param [in] id    ID of surface to lookup
+     * \return           The surface with ID \arg id. If this surface has been destroyed,
+     *                   but the store retains a reference, returns nullptr.
+     * \throws std::out_of_range if the store has no reference for a surface with \arg id.
+     * \warning Prior to Mir-0.24 the underlying APIs are not available: we return nullptr.
+     */
+    auto surface_for_id(Id const& id) const -> std::shared_ptr<mir::scene::Surface>;
+
+private:
+    std::weak_ptr<mir::shell::PersistentSurfaceStore> self;
+};
+}
+
+#endif //MIRAL_PERSISTENT_SURFACE_STORE_H

=== modified file 'test/CMakeLists.txt'
--- test/CMakeLists.txt	2016-08-02 09:05:46 +0000
+++ test/CMakeLists.txt	2016-08-10 09:21:16 +0000
@@ -9,6 +9,7 @@
 add_executable(miral-test
     mru_window_list.cpp
     active_outputs.cpp
+    persistent_surface_store.cpp
 )
 
 target_link_libraries(miral-test

=== added file 'test/persistent_surface_store.cpp'
--- test/persistent_surface_store.cpp	1970-01-01 00:00:00 +0000
+++ test/persistent_surface_store.cpp	2016-08-10 09:21:16 +0000
@@ -0,0 +1,108 @@
+/*
+ * Copyright © 2016 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Alan Griffiths <[e-mail]>
+ */
+
+#include "../miral/persistent_surface_store.h"
+
+#include <miral/toolkit/persistent_id.h>
+
+#include <mir/test/doubles/wrap_shell_to_track_latest_surface.h>
+#include <mir_test_framework/connected_client_with_a_surface.h>
+
+#include <gtest/gtest.h>
+#include <gmock/gmock.h>
+
+namespace msh = mir::shell;
+namespace ms = mir::scene;
+namespace mt = mir::test;
+namespace mtd = mt::doubles;
+namespace mtf = mir_test_framework;
+
+using namespace mir::geometry;
+using namespace testing;
+
+namespace
+{
+struct PersistentSurfaceStore : mtf::ConnectedClientWithASurface
+{
+    void SetUp() override
+    {
+        server.wrap_shell([this](std::shared_ptr<msh::Shell> const& wrapped)
+            {
+                auto const msc = std::make_shared<mtd::WrapShellToTrackLatestSurface>(wrapped);
+                shell = msc;
+                return msc;
+            });
+
+#if MIR_SERVER_VERSION < MIR_VERSION_NUMBER(0, 25, 0)
+        preset_display({}); // Workaround for lp:1611337
+#endif
+        persistent_surface_store(server);
+        mtf::ConnectedClientWithASurface::SetUp();
+    }
+
+    void TearDown() override
+    {
+        mtf::ConnectedClientWithASurface::TearDown();
+    }
+
+    std::shared_ptr<ms::Surface> latest_shell_surface() const
+    {
+        auto myshell = shell.lock();
+        EXPECT_THAT(myshell, NotNull());
+        auto const result = myshell->latest_surface.lock();
+        EXPECT_THAT(result, NotNull());
+        return result;
+    }
+
+    miral::PersistentSurfaceStore persistent_surface_store;
+
+private:
+    std::weak_ptr<mtd::WrapShellToTrackLatestSurface> shell;
+};
+}
+
+TEST_F(PersistentSurfaceStore, server_and_client_persistent_id_matches)
+{
+    auto const shell_server_surface = latest_shell_surface();
+    ASSERT_THAT(shell_server_surface, NotNull());
+
+    miral::toolkit::PersistentId client_surface_id{surface};
+
+#if MIR_SERVER_VERSION >= MIR_VERSION_NUMBER(0, 24, 0)
+    auto const server_surface_id = persistent_surface_store.id_for_surface(shell_server_surface);
+
+    auto const client_surface_id_string = client_surface_id.c_str();
+
+    ASSERT_THAT(server_surface_id, Eq(client_surface_id_string));
+#else
+    EXPECT_THROW(persistent_surface_store.id_for_surface(shell_server_surface), std::logic_error);
+#endif
+}
+
+TEST_F(PersistentSurfaceStore, server_can_identify_surface_specified_by_client)
+{
+    miral::toolkit::PersistentId client_surface_id{surface};
+
+    auto const server_surface = persistent_surface_store.surface_for_id(client_surface_id.c_str());
+
+#if MIR_SERVER_VERSION >= MIR_VERSION_NUMBER(0, 24, 0)
+    ASSERT_THAT(server_surface, Eq(latest_shell_surface()));
+#else
+    ASSERT_THAT(server_surface, IsNull());
+#endif
+}