Merge lp:~alan-griffiths/mir/even-NullWindowManager-configures-surface into lp:mir

[Alan Griffiths]

shell: add surface->configure() call to NullWindowManager::set_surface_attribute()

shell: add surface->configure() call to NullWindowManager::set_surface_attribute()

back in -c2391 AbstractShell stopped configuring the surface to give the window manager more control. But that silently changed NullWindowManager from allowing everything to preventing everything. This reverts that unintended effect.

Yes, as downstreams can NullWindowManager there ought to be some tests about. I've added it to my list but wanted this fix to have a chance to land early.

[Robert Carr (robertcarr)]

Raises some questions about the name ;) but ok.

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2424
http://jenkins.qa.ubuntu.com/job/mir-ci/3368/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1825
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1824
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1775
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1365
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1365/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1775
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1775/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4736
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/19165

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

It's not the first time the difference between Null, Stub and Fake has become murky.

Although I suspect if this class is being used in libmirserver in production then a name other than "Null..." is called for.

review: Needs Information

[Daniel van Vugt (vanvugt)]

In fact if "NullSomething" is in production use, the base class "Something" should become the null implementation. Instead of a pure virtual interface.

[Alan Griffiths (alan-griffiths)]

Hopefully a name with raising fewer questions.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2426
http://jenkins.qa.ubuntu.com/job/mir-ci/3384/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1849
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1848
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1799
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1381
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1381/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1799
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1799/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4755
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/19203

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

review: Abstain

[Daniel van Vugt (vanvugt)]

It's slightly better, but I think removing the words {Skelton,Null} (and avoiding the pure virtual base class) would be much better.

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-autolanding/1271/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1864/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1863/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1814/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-autolanding/451/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1814/console

review: Needs Fixing (continuous-integration)

[lp:~alan-griffiths/mir/even-NullWindowManager-configures-surface]

2427. By Alan Griffiths on 2015-03-27

merge lp:mir

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2427
http://jenkins.qa.ubuntu.com/job/mir-ci/3397/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1867
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1866
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1817
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1394
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1394/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1817
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1817/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4771
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/19237

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Come to think of it, establishing new precedent is important. Because people often refer to precedent and consistency in their reasoning. Even as a reason to reject more simplistic and maintainable designs. So we need to break the mold ASAP.

In cases like this where a class used in production is kind of like Null, kind of like Stub, and very much useful in its bare form, I think this is where those prefixes (including Skeleton) should not be used. So rename SkeletonWindowManager -> WindowManager and remove the existing pure virtual one.

This not only results in less typing, but fewer classes and less maintenance. As "Null" and "Stub" forms need never exist (they do implicitly). You can still add a Mock form later if required.

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

> Come to think of it, establishing new precedent is important. Because people
> often refer to precedent and consistency in their reasoning. Even as a reason
> to reject more simplistic and maintainable designs.

I agree that precedents are important. However, "new precedents" should be avoided when they break consistency and introduce simplistic designs.

> So we need to break the
> mold ASAP.

I do not see any need (and certainly not an urgent one) to break from the well established industry practice of having pure virtual interfaces. (This has been widely followed for at least 20 years. See, for example, Barton & Nackman from 1994.)

> In cases like this where a class used in production is kind of like Null, kind
> of like Stub, and very much useful in its bare form, I think this is where
> those prefixes (including Skeleton) should not be used. So rename
> SkeletonWindowManager -> WindowManager and remove the existing pure virtual
> one.

I can remember when I was naive enough to "know better" and entertained thought like this, but experience has taught me that what appear at first to be exceptions are often not as the system evolves over time.

> This not only results in less typing, but fewer classes and less maintenance.
> As "Null" and "Stub" forms need never exist (they do implicitly). You can
> still add a Mock form later if required.

Much more important than reducing typing is reducing reading. Avoiding the need to go look at the implementation of what looks like an interface to discover whether it actually has an implementation is going to save far more time over the life of a project than saving eight keystrokes.

Introducing a precedent like the one you propose will add that cost to every interface in the system - as the reader will never be certain which classes are exceptions to the general rule.

[Alexandros Frantzis (afrantzis)]

In addition to being consistent with our existing practices, implementing a pure abstract/interface is more straightforward for users since they can be sure it comes without any hidden baggage.

I do have some concerns about having a Null/SkeletonWindowManager in the first place (at least as a public production interface), though. Allowing users to inherit from such a class, creates a coupling with the implementation, which I am not convinced is worth it for what this class has to offer (at the moment at least). Why, as things stand, do we want to offer SkeletonWindowManager vs just offering the WindowManager interface?

review: Needs Information

[Alan Griffiths (alan-griffiths)]

> I do have some concerns about having a Null/SkeletonWindowManager in the first
> place (at least as a public production interface), though. Allowing users to
> inherit from such a class, creates a coupling with the implementation, which I
> am not convinced is worth it for what this class has to offer (at the moment
> at least). Why, as things stand, do we want to offer SkeletonWindowManager vs
> just offering the WindowManager interface?

As the window manager has to implement some minimal (as opposed to null) behavior I think we should provide this (as opposed to documenting it).

The use comes in two places:

1. the default window management logic is inconvenient for a number of our acceptance tests (e.g. they don't expect surfaces to be resized or placed). These use SkeletonWindowManager to disable the default.

2. Unity8 also finds the default window management logic is inconvenient and, as it currently uses different (older) customization points[*]. NullWindowManager provides a simple way to disable the default.

[*] It is very plausible that, after 0.13, Unity8 will be able to migrate to using the WindowManager interface for customizing window management (and we will then remove the interfaces it currently employs).

[Alexandros Frantzis (afrantzis)]

> As the window manager has to implement some minimal (as opposed to null) behavior I think we
> should provide this (as opposed to documenting it).
>
> The use comes in two places:
>
> 1. the default window management logic is inconvenient for a number of our acceptance tests
> (e.g. they don't expect surfaces to be resized or placed). These use SkeletonWindowManager
> to disable the default.
>
> 2. Unity8 also finds the default window management logic is inconvenient and, as it currently
> uses different (older) customization points[*]. NullWindowManager provides a simple way to
> disable the default.

I am OK providing an (almost) null implementation for the use cases you mention. I would prefer it if it was not possible to inherit from this class, though.

To use the SkeletonWindowManager class as a base effectively, especially the methods that have a non-null implementation, the user needs to know what that class method do and do not do, which may change between releases, and I think this is a bigger burden for the user than just having to know that, e.g., set_surface_attribute() needs to configure the surface.

How about making SkeletonWindowManager "final", or perhaps a function "shared_ptr<WindowManager> make_skeleton_window_manager()" (and make the class internal)?

[Alan Griffiths (alan-griffiths)]

> I am OK providing an (almost) null implementation for the use cases you
> mention. I would prefer it if it was not possible to inherit from this class,
> though.

Why? It is entirely possible for a responsible user to specialize the class successfully.

> To use the SkeletonWindowManager class as a base effectively, especially the
> methods that have a non-null implementation, the user needs to know what that
> class method do and do not do, which may change between releases, and I think
> this is a bigger burden for the user than just having to know that, e.g.,
> set_surface_attribute() needs to configure the surface.

I don't think that is true. If overriding from a concrete class one should (with rare exceptions) call the overridden function from the override. For the user to assume that it is null omitting the call is a mistake, but not one we should be protecting them from.

> How about making SkeletonWindowManager "final", or perhaps a function
> "shared_ptr<WindowManager> make_skeleton_window_manager()" (and make the class
> internal)?

Personally, I'd only ever declare a class or method final if overriding were automatically an error (or, somehow, violates a design contract). But I could live with it.

[Alexandros Frantzis (afrantzis)]

> Why? It is entirely possible for a responsible user to specialize the class successfully

It is entirely possible, and more straightforward (IMO, read below), for a responsible user to directly use the WindowManager interface directly, too :)

> I don't think that is true. If overriding from a concrete class one should (with rare exceptions)
> call the overridden function from the override. For the user to assume that it is null omitting the
> call is a mistake, but not one we should be protecting them from.

I am more concerned about the order of the call to the overriden function in the new code, rather than whether the function is called or not. In order to write correct code the user needs to know what the overriden function does (e.g. in the set_surface_attribute() case that it calls surface->configure()), to avoid performing the same action again (e.g. call surface->configure() again), and ensure that it's called at the correct point in the new code.

In addition, we are making the implementation details of SkeletonWindowManager part of our effective ABI.

All of the above are general issues with inheriting implementation, and present a trade-off that sometimes is worth it. I am leaning towards "not worth it" in this particular case.

Needs discussion

review: Needs Information

[Alan Griffiths (alan-griffiths)]

I have to correct my earlier answer:

Although I had some tests using [Null|Skeleton]WindowManager at one point there no longer any on any active branch.

OTOH unity-system-compositor /src/window_manager.h derives from mir::shell::NullWindowManager and this use would be broken by making the class final.

The use in Unity8 is as described.

So, given the discussion this has provoked here I'm probably best just moving the code to the downstream projects.

[Daniel van Vugt (vanvugt)]

Equally, my 20+ years of programming experience tells me to do the opposite. :) I can suggest, but not force change. Mir is a democracy.

Unmerged revisions

2427. By Alan Griffiths on 2015-03-27

merge lp:mir

2426. By Alan Griffiths on 2015-03-26

merge lp:mir

2425. By Alan Griffiths on 2015-03-26

NullWindowManager => SkeletonWindowManager

2424. By Alan Griffiths on 2015-03-25

NullWindowManager should configure surface

=== renamed file 'include/server/mir/shell/null_window_manager.h' => 'include/server/mir/shell/skeleton_window_manager.h'
--- include/server/mir/shell/null_window_manager.h	2015-03-25 02:48:40 +0000
+++ include/server/mir/shell/skeleton_window_manager.h	2015-03-27 12:00:42 +0000
@@ -16,8 +16,8 @@
  * Authored By: Alan Griffiths <[e-mail]>
  */
 
-#ifndef MIR_SHELL_NULL_WINDOW_MANAGER_H_
-#define MIR_SHELL_NULL_WINDOW_MANAGER_H_
+#ifndef MIR_SHELL_SKELETON_WINDOW_MANAGER_H_
+#define MIR_SHELL_SKELETON_WINDOW_MANAGER_H_
 
 #include "mir/shell/window_manager.h"
 
@@ -25,7 +25,7 @@
 {
 namespace shell
 {
-class NullWindowManager : public WindowManager
+class SkeletonWindowManager : public WindowManager
 {
 public:
     void add_session(std::shared_ptr<scene::Session> const& session) override;
@@ -60,4 +60,4 @@
 }
 }
 
-#endif /* MIR_SHELL_NULL_WINDOW_MANAGER_H_ */
+#endif /* MIR_SHELL_SKELETON_WINDOW_MANAGER_H_ */
 
=== modified file 'src/server/shell/CMakeLists.txt'
--- src/server/shell/CMakeLists.txt	2015-03-25 02:48:40 +0000
+++ src/server/shell/CMakeLists.txt	2015-03-27 12:00:42 +0000
@@ -2,13 +2,13 @@
   SHELL_SOURCES
 
   abstract_shell.cpp
+  default_configuration.cpp
   default_placement_strategy.cpp
+  default_window_manager.cpp
   frontend_shell.cpp
   graphics_display_layout.cpp
-  default_configuration.cpp
-  default_window_manager.cpp
-  null_window_manager.cpp
   shell_wrapper.cpp
+  skeleton_window_manager.cpp
 )
 
 add_library(
 
=== modified file 'src/server/shell/abstract_shell.cpp'
--- src/server/shell/abstract_shell.cpp	2015-03-25 02:48:40 +0000
+++ src/server/shell/abstract_shell.cpp	2015-03-27 12:00:42 +0000
@@ -18,7 +18,7 @@
 
 #include "mir/shell/abstract_shell.h"
 #include "mir/shell/input_targeter.h"
-#include "mir/shell/null_window_manager.h"
+#include "mir/shell/window_manager.h"
 #include "mir/scene/prompt_session.h"
 #include "mir/scene/prompt_session_manager.h"
 #include "mir/scene/session_coordinator.h"
 
=== renamed file 'src/server/shell/null_window_manager.cpp' => 'src/server/shell/skeleton_window_manager.cpp'
--- src/server/shell/null_window_manager.cpp	2015-03-25 02:48:40 +0000
+++ src/server/shell/skeleton_window_manager.cpp	2015-03-27 12:00:42 +0000
@@ -16,22 +16,23 @@
  * Authored By: Alan Griffiths <[e-mail]>
  */
 
-#include "mir/shell/null_window_manager.h"
+#include "mir/shell/skeleton_window_manager.h"
+#include "mir/scene/surface.h"
 
 namespace mf = mir::frontend;
 namespace ms = mir::scene;
 namespace msh = mir::shell;
 
 
-void msh::NullWindowManager::add_session(std::shared_ptr<scene::Session> const& /*session*/)
-{
-}
-
-void msh::NullWindowManager::remove_session(std::shared_ptr<scene::Session> const& /*session*/)
-{
-}
-
-auto msh::NullWindowManager::add_surface(
+void msh::SkeletonWindowManager::add_session(std::shared_ptr<scene::Session> const& /*session*/)
+{
+}
+
+void msh::SkeletonWindowManager::remove_session(std::shared_ptr<scene::Session> const& /*session*/)
+{
+}
+
+auto msh::SkeletonWindowManager::add_surface(
     std::shared_ptr<scene::Session> const& session,
     scene::SurfaceCreationParameters const& params,
     std::function<frontend::SurfaceId(std::shared_ptr<scene::Session> const& session, scene::SurfaceCreationParameters const& params)> const& build)
@@ -40,40 +41,40 @@
     return build(session, params);
 }
 
-void msh::NullWindowManager::remove_surface(
+void msh::SkeletonWindowManager::remove_surface(
     std::shared_ptr<scene::Session> const& /*session*/,
     std::weak_ptr<scene::Surface> const& /*surface*/)
 {
 }
 
-void msh::NullWindowManager::add_display(geometry::Rectangle const& /*area*/)
-{
-}
-
-void msh::NullWindowManager::remove_display(geometry::Rectangle const& /*area*/)
-{
-}
-
-bool msh::NullWindowManager::handle_key_event(MirKeyboardEvent const* /*event*/)
-{
-    return false;
-}
-
-bool msh::NullWindowManager::handle_touch_event(MirTouchEvent const* /*event*/)
-{
-    return false;
-}
-
-bool msh::NullWindowManager::handle_pointer_event(MirPointerEvent const* /*event*/)
-{
-    return false;
-}
-
-int msh::NullWindowManager::set_surface_attribute(
+void msh::SkeletonWindowManager::add_display(geometry::Rectangle const& /*area*/)
+{
+}
+
+void msh::SkeletonWindowManager::remove_display(geometry::Rectangle const& /*area*/)
+{
+}
+
+bool msh::SkeletonWindowManager::handle_key_event(MirKeyboardEvent const* /*event*/)
+{
+    return false;
+}
+
+bool msh::SkeletonWindowManager::handle_touch_event(MirTouchEvent const* /*event*/)
+{
+    return false;
+}
+
+bool msh::SkeletonWindowManager::handle_pointer_event(MirPointerEvent const* /*event*/)
+{
+    return false;
+}
+
+int msh::SkeletonWindowManager::set_surface_attribute(
     std::shared_ptr<scene::Session> const& /*session*/,
-    std::shared_ptr<scene::Surface> const& /*surface*/,
-    MirSurfaceAttrib /*attrib*/,
+    std::shared_ptr<scene::Surface> const& surface,
+    MirSurfaceAttrib attrib,
     int value)
 {
-    return value;
+    return surface->configure(attrib, value);
 }
 
=== modified file 'src/server/symbols.map'
--- src/server/symbols.map	2015-03-27 06:58:51 +0000
+++ src/server/symbols.map	2015-03-27 12:00:42 +0000
@@ -222,16 +222,6 @@
     mir::shell::InputTargeter::?InputTargeter*;
     mir::shell::InputTargeter::InputTargeter*;
     mir::shell::InputTargeter::operator*;
-    mir::shell::NullWindowManager::add_display*;
-    mir::shell::NullWindowManager::add_session*;
-    mir::shell::NullWindowManager::add_surface*;
-    mir::shell::NullWindowManager::handle_key_event*;
-    mir::shell::NullWindowManager::handle_pointer_event*;
-    mir::shell::NullWindowManager::handle_touch_event*;
-    mir::shell::NullWindowManager::remove_display*;
-    mir::shell::NullWindowManager::remove_session*;
-    mir::shell::NullWindowManager::remove_surface*;
-    mir::shell::NullWindowManager::set_surface_attribute*;
     mir::shell::ShellWrapper::add_display*;
     mir::shell::ShellWrapper::add_prompt_provider_for*;
     mir::shell::ShellWrapper::close_session*;
@@ -251,6 +241,16 @@
     mir::shell::ShellWrapper::start_prompt_session_for*;
     mir::shell::ShellWrapper::stop_prompt_session*;
     mir::shell::ShellWrapper::surface_at*;
+    mir::shell::SkeletonWindowManager::add_display*;
+    mir::shell::SkeletonWindowManager::add_session*;
+    mir::shell::SkeletonWindowManager::add_surface*;
+    mir::shell::SkeletonWindowManager::handle_key_event*;
+    mir::shell::SkeletonWindowManager::handle_pointer_event*;
+    mir::shell::SkeletonWindowManager::handle_touch_event*;
+    mir::shell::SkeletonWindowManager::remove_display*;
+    mir::shell::SkeletonWindowManager::remove_session*;
+    mir::shell::SkeletonWindowManager::remove_surface*;
+    mir::shell::SkeletonWindowManager::set_surface_attribute*;
     mir::shell::WindowManager::operator*;
     mir::shell::WindowManager::?WindowManager*;
     mir::shell::WindowManager::WindowManager*;
@@ -324,16 +324,6 @@
     non-virtual?thunk?to?mir::shell::FocusController::?FocusController*;
     non-virtual?thunk?to?mir::shell::HostLifecycleEventListener::?HostLifecycleEventListener*;
     non-virtual?thunk?to?mir::shell::InputTargeter::?InputTargeter*;
-    non-virtual?thunk?to?mir::shell::NullWindowManager::add_display*;
-    non-virtual?thunk?to?mir::shell::NullWindowManager::add_session*;
-    non-virtual?thunk?to?mir::shell::NullWindowManager::add_surface*;
-    non-virtual?thunk?to?mir::shell::NullWindowManager::handle_key_event*;
-    non-virtual?thunk?to?mir::shell::NullWindowManager::handle_pointer_event*;
-    non-virtual?thunk?to?mir::shell::NullWindowManager::handle_touch_event*;
-    non-virtual?thunk?to?mir::shell::NullWindowManager::remove_display*;
-    non-virtual?thunk?to?mir::shell::NullWindowManager::remove_session*;
-    non-virtual?thunk?to?mir::shell::NullWindowManager::remove_surface*;
-    non-virtual?thunk?to?mir::shell::NullWindowManager::set_surface_attribute*;
     non-virtual?thunk?to?mir::shell::ShellWrapper::add_display*;
     non-virtual?thunk?to?mir::shell::ShellWrapper::add_prompt_provider_for*;
     non-virtual?thunk?to?mir::shell::ShellWrapper::close_session*;
@@ -352,6 +342,16 @@
     non-virtual?thunk?to?mir::shell::ShellWrapper::start_prompt_session_for*;
     non-virtual?thunk?to?mir::shell::ShellWrapper::stop_prompt_session*;
     non-virtual?thunk?to?mir::shell::ShellWrapper::surface_at*;
+    non-virtual?thunk?to?mir::shell::SkeletonWindowManager::add_display*;
+    non-virtual?thunk?to?mir::shell::SkeletonWindowManager::add_session*;
+    non-virtual?thunk?to?mir::shell::SkeletonWindowManager::add_surface*;
+    non-virtual?thunk?to?mir::shell::SkeletonWindowManager::handle_key_event*;
+    non-virtual?thunk?to?mir::shell::SkeletonWindowManager::handle_pointer_event*;
+    non-virtual?thunk?to?mir::shell::SkeletonWindowManager::handle_touch_event*;
+    non-virtual?thunk?to?mir::shell::SkeletonWindowManager::remove_display*;
+    non-virtual?thunk?to?mir::shell::SkeletonWindowManager::remove_session*;
+    non-virtual?thunk?to?mir::shell::SkeletonWindowManager::remove_surface*;
+    non-virtual?thunk?to?mir::shell::SkeletonWindowManager::set_surface_attribute*;
     non-virtual?thunk?to?mir::shell::WindowManager::?WindowManager*;
     non-virtual?thunk?to?mir::time::Alarm::?Alarm*;
     non-virtual?thunk?to?mir::time::AlarmFactory::?AlarmFactory*;
@@ -405,9 +405,9 @@
     typeinfo?for?mir::shell::FocusController;
     typeinfo?for?mir::shell::HostLifecycleEventListener;
     typeinfo?for?mir::shell::InputTargeter;
-    typeinfo?for?mir::shell::NullWindowManager;
     typeinfo?for?mir::shell::Shell;
     typeinfo?for?mir::shell::ShellWrapper;
+    typeinfo?for?mir::shell::SkeletonWindowManager;
     typeinfo?for?mir::shell::WindowManager;
     typeinfo?for?mir::time::Alarm;
     typeinfo?for?mir::time::AlarmFactory;
@@ -461,9 +461,9 @@
     vtable?for?mir::shell::FocusController;
     vtable?for?mir::shell::HostLifecycleEventListener;
     vtable?for?mir::shell::InputTargeter;
-    vtable?for?mir::shell::NullWindowManager;
     vtable?for?mir::shell::Shell;
     vtable?for?mir::shell::ShellWrapper;
+    vtable?for?mir::shell::SkeletonWindowManager;
     vtable?for?mir::shell::WindowManager;
     vtable?for?mir::time::Alarm;
     vtable?for?mir::time::AlarmFactory;