Add APIs to override decoration managers and customize decorations #3604

tarek-y-ismail · 2024-09-20T08:12:30Z

No description provided.

src/include/server/mir/server_configuration.h

src/include/server/mir/shell/decoration.h

src/include/server/mir/shell/decoration_notifier.h

src/server/shell/decoration/basic_decoration.cpp

src/server/symbols.map

tests/unit-tests/shell/test_decoration_basic_manager.cpp

src/server/symbols.map

src/server/shell/decoration/basic_decoration.cpp

src/server/shell/decoration/basic_decoration.h

AlanGriffiths · 2024-10-18T12:59:13Z

src/server/shell/decoration/basic_manager.h

+protected:
+    std::unordered_map<scene::Surface*, std::unique_ptr<Decoration>> decorations;
+


I'm not enthusiastic about making member data public (protected is public in disguise).

In this case the only use is in tests, which manipulate decorations directly. That seems fragile and suggests there's an abstraction missing.

I'll try to come up with a concrete suggestion.

AlanGriffiths

This looks mostly sensible, but I'd like to see an example using this API. Can we add something to miral-shell?

tarek-y-ismail · 2024-10-18T13:24:16Z

This looks mostly sensible, but I'd like to see an example using this API. Can we add something to miral-shell?

Sure thing. Anything quick in mind? The first thing that comes to mind for me is a variant of BasicDecoration that assigns a random color to each new decoration.

AlanGriffiths · 2024-10-18T13:46:19Z

This looks mostly sensible, but I'd like to see an example using this API. Can we add something to miral-shell?

Sure thing. Anything quick in mind? The first thing that comes to mind for me is a variant of BasicDecoration that assigns a random color to each new decoration.

That sounds bizarre. How about "light mode"?

tarek-y-ismail · 2024-10-18T13:51:42Z

That sounds bizarre. How about "light mode"?

Hmm, that should do too. But first things first, let's make decorations private. Would add_decoration, override_decoration, and num_decorations be a good interface? I think BasicManager is a good base for authors to build atop so I don't want to close it up entirely

AlanGriffiths · 2024-10-18T13:56:59Z

I think BasicManager is a good base for authors to build atop so I don't want to close it up entirely

Here's your first problem then: basic_manager.h is internal to mirserver. Authors cannot use it

tarek-y-ismail · 2024-10-18T14:00:48Z

basic_manager.h is internal to mirserver.

Ok, should be easy to fix. I propose include/common/mir/decoration/ as a location, what do you think?

AlanGriffiths · 2024-10-18T14:11:06Z

Ok, should be easy to fix. I propose include/common/mir/decoration/ as a location, what do you think?

I think that's for APIs supported by mircommon

tarek-y-ismail · 2024-10-18T14:16:07Z

What about include/server/mir/decoration?

AlanGriffiths · 2024-10-18T14:28:50Z

What about include/server/mir/decoration?

src/include/server/mir/decoration

tarek-y-ismail · 2024-10-21T15:19:54Z

I went around and implemented "light mode" in the laziest/easiest way possible (i.e. just paramterizing BasicDecoration on the different needed colors) . Nitpicks welcome!

PS: At some point when I was hooking up miral::decoration::LightMode, I introduced a bug that causes multiple "Mir on X" windows to open whenever Mir is launched. I have no idea what's the cause of this bug but I'll investigate it next.

AlanGriffiths

We are trying to provide a way for users to customise decorations themselves.

Instead, miral::LigtMode provides an option to select a customisation we wrote, not the capability to write it as a user.

AlanGriffiths

This needs cleaning up:

Examples the user can adapt (UserDecorationManagerExample et alia) do not belong in the miral namespace nor library.
The example files user_decoration_example/... belong in examples not include/miral and src/miral
The example code should be used in an example shell
There is no need for miral::decoration::LightMode, nor for the changes introduced to support it.

tarek-y-ismail · 2024-11-07T12:18:49Z

Examples the user can adapt (UserDecorationManagerExample et alia) do not belong in the miral namespace nor library.
The example files user_decoration_example/... belong in examples not include/miral and src/miral
The example code should be used in an example shell
(half done) There is no need for miral::decoration::LightMode, nor for the changes introduced to support it.
Still need to go through the commits and figure out what needs to be reverted.

Remove `msd::Decoration::redraw`. Since it's only really an internal implementation detail, it's up to the decoration author to do it in whichever way they see fit. `BasicDecoration` can be used as a template/example. Remove `WindowSurfaceObserverManager`, move its observer to `Decoration` Fix `DecorationBasicManager` tests (again...) Move `msd::Decoration` notification logic to `msd::DecorationNotifier`

Inititalizing the decoration manager in the pre_init callbacks caused extra windows to be opened due to `the_display` being called (which creates "Mir on X" windows) during the decoration manager creation, but not cached, which causes later calls during initialization to launch extra windows.

src/server/shell/decoration/decoration_notifier.cpp

src/miral/decoration_light_mode.cpp

src/include/server/mir/shell/decoration_notifier.h

tarek-y-ismail · 2024-11-11T15:28:48Z

src/server/shell/decoration/renderer.cpp

+auto init_button_drawing_functions(msd::ButtonTheme button_icons)
+{
+    using namespace msd;
+
+    std::map<ButtonFunction, std::pair<msd::Icon const, msd::Icon::DrawingFunction const>> buttons;
+
+    for(auto const& [button_function, icon]: button_icons)
+    {
+        // A little lookup table, more legible than a switch case?
+        static auto const drawing_functions = std::map<ButtonFunction, msd::Icon::DrawingFunction>{
+            {ButtonFunction::Close, render_close_icon},
+            {ButtonFunction::Maximize, render_maximize_icon},
+            {ButtonFunction::Minimize, render_minimize_icon},
+        };
+
+        buttons.emplace(button_function, std::make_pair(icon, drawing_functions.at(button_function)));
+    }
+
+    return buttons;
+}


I see no reason to have the icon, button function, and icon drawing function split. A list of tuples should do?

tarek-y-ismail · 2024-11-11T15:40:02Z

include/miral/miral/basic_decoration/basic_decoration.h

This (46ec605) and its friends should probably be split into their own commit

Update mirserver symbols Bump mirserver symbol stanza to 2.19 since we're breaking ABI Fix header ordering in `decoration_notifier.h` Move `BasicDecoration::redraw` to be private. Wrap the contents of `Decoration::set_scale` with `ThreadSafeAccess::spawn`

`src/include/server/mir/decoration`

button theme.

These are meant to isolate miral client code from unstable/internal mir code so that we can change stuff without breaking client code. Move `decoration_wrapper` (now `decoration_notifier`) to `DecorationAdapter` Additionally, expose `window_surface` and `decoration_surface` through getters to pass them to the adapter. Co-authored-by: Alan Griffiths <[email protected]>

Move custom decorations to their own examples. Remove custom decoration code from the `miral::decoration` namespace Make `UserDecorationExample` light by default to help differentiate it from Mir's built-in decorations.

`src/server/shell/decoration`

Added `InputResolverAdapter` to expose a more stable(?) interface. It allows users to point to their implementation of `process_{enter,leave,up,down,motion, drag}`. Remove ununsed `DeviceEvent&` parameter from `process_{leave,up,down}`.

tarek-y-ismail · 2024-11-11T17:22:01Z

examples/miral-custom-decorations/user_decoration_manager_example.cpp

+#include <mir/graphics/display_configuration.h>
+#include <mir/graphics/null_display_configuration_observer.h>
+/* #include <bffoost/throw_exception.hpp> */
+#include "mirserver-internal/mir/observer_registrar.h"


Is this okay, or should observer_registrar.h be exposed / I should access it some other way?

AlanGriffiths

This iteration of the API is leaking implementation details

AlanGriffiths · 2024-11-11T17:36:04Z

include/miral/miral/decoration/decoration_adapter.h

+#ifndef MIRAL_DECORATION_DECORATION_ADAPTER_H
+#define MIRAL_DECORATION_DECORATION_ADAPTER_H
+
+#include "mir/shell/decoration_notifier.h"


This should not be used by a public libmiral header.

So, like, create a miral::* version of the class, with the header file completely isolating the internal symbols from the user, and the source file using/including the header?

Does this also apply to similar cases in this review? (input_resolver.h, adapters, etc...)?

No, it should just be internal and unavailable to the shell author

AlanGriffiths · 2024-11-11T17:36:58Z

include/miral/miral/decoration/decoration_adapter.h

+#include <memory>
+
+
+namespace miral::decoration


I doubt we need a nested decoration namespace

AlanGriffiths · 2024-11-11T17:39:58Z

include/miral/miral/decoration/decoration_input_resolver_adapter.h

+#ifndef MIRAL_DECORATION_DECORATION_INPUT_MANAGER_ADAPTER_H
+#define MIRAL_DECORATION_DECORATION_INPUT_MANAGER_ADAPTER_H
+
+#include "mir/shell/input_resolver.h"


This should not be used by a public libmiral header.

AlanGriffiths · 2024-11-11T17:41:32Z

include/miral/miral/decoration/decoration_input_resolver_adapter.h

+namespace miral::decoration
+{
+class InputResolverBuilder;
+class InputResolverAdapter : public mir::shell::decoration::InputResolver
+{
+protected:
+    void process_enter(DeviceEvent& device) override;
+    void process_leave() override;
+    void process_down() override;
+    void process_up() override;
+    void process_move(DeviceEvent& device) override;
+    void process_drag(DeviceEvent& device) override;
+
+private:
+    friend InputResolverBuilder;
+    InputResolverAdapter();
+
+    std::function<void(DeviceEvent& device)> on_process_enter;
+    std::function<void()> on_process_leave;
+    std::function<void()> on_process_down;
+    std::function<void()> on_process_up;
+    std::function<void(DeviceEvent& device)> on_process_move;
+    std::function<void(DeviceEvent& device)> on_process_drag;
+};


The Builder exists because this class doesn't belong in a public header. Users of libmiral should be isolated from such details.

AlanGriffiths · 2024-11-11T17:41:41Z

include/miral/miral/decoration/decoration_adapter.h

+class DecorationAdapter : public mir::shell::decoration::Decoration
+{
+public:
+
+    void handle_input_event(std::shared_ptr<MirEvent const> const& /*event*/) final override;
+    void set_scale(float new_scale) final override;
+    void attrib_changed(mir::scene::Surface const* window_surface, MirWindowAttrib attrib, int value) final override;
+    void window_resized_to(
+        mir::scene::Surface const* window_surface, mir::geometry::Size const& window_size) final override;
+    void window_renamed(mir::scene::Surface const* window_surface, std::string const& name) final override;
+
+private:
+    friend DecorationBuilder;
+    DecorationAdapter(
+        std::shared_ptr<mir::scene::Surface> decoration_surface, std::shared_ptr<mir::scene::Surface> window_surface);
+
+    std::function<void(std::shared_ptr<MirEvent const> const&)> on_handle_input_event;
+    std::function<void(float new_scale)> on_set_scale;
+    std::function<void(mir::scene::Surface const* window_surface, MirWindowAttrib attrib, int value)> on_attrib_changed;
+    std::function<void(mir::scene::Surface const* window_surface, mir::geometry::Size const& window_size)>
+        on_window_resized_to;
+    std::function<void(mir::scene::Surface const* window_surface, std::string const& name)> on_window_renamed;
+
+    mir::shell::decoration::DecorationNotifier decoration_notifier;
+};


The Builder exists because this class doesn't belong in a public header. Users of libmiral should be isolated from such details.

AlanGriffiths · 2024-11-11T17:43:36Z

include/miral/miral/decoration/decoration_manager_adapter.h

+class DecorationManagerAdapter : public mir::shell::decoration::Manager
+{
+public:
+    void init(std::weak_ptr<mir::shell::Shell> const& shell) override;
+    void decorate(std::shared_ptr<mir::scene::Surface> const& surface) override;
+    void undecorate(std::shared_ptr<mir::scene::Surface> const& surface) override;
+    void undecorate_all() override;
+
+private:
+    friend DecorationManagerBuilder;
+    DecorationManagerAdapter();
+
+    std::function<void(std::weak_ptr<mir::shell::Shell> const& shell)> on_init;
+    std::function<void(std::shared_ptr<mir::scene::Surface> const& surface)> on_decorate;
+    std::function<void(std::shared_ptr<mir::scene::Surface> const& surface)> on_undecorate;
+    std::function<void()> on_undecorate_all;
+};


The Builder exists because this class doesn't belong in a public header. Users of libmiral should be isolated from such details.

AlanGriffiths · 2024-11-11T17:45:06Z

examples/CMakeLists.txt

@@ -5,5 +5,6 @@ add_subdirectory(miral-kiosk)
 add_subdirectory(miral-system-compositor)
 add_subdirectory(mir_demo_server)
 add_subdirectory(mir-x11-kiosk)
+add_subdirectory(miral-custom-decorations)


I don't think custom decorations need their own example. Adding them as a miral-shell option would be fine

AlanGriffiths · 2024-11-11T17:45:52Z

examples/miral-custom-decorations/miral_custom_decorations.cpp

+
+
+#include "mir/server.h"
+#include "mir/main_loop.h" // Needed for the conversion of `the_main_loop` to an executor


If this is needed, then something is wrong with the proposed API

AlanGriffiths · 2024-11-11T17:46:58Z

examples/miral-custom-decorations/miral_custom_decorations.cpp

+
+    void operator()(mir::Server& s) const
+    {
+        s.add_pre_init_callback(


User code should not be accessing the Server in this way (it isn't a public header)

AlanGriffiths · 2024-11-11T17:47:58Z

examples/miral-custom-decorations/miral_custom_decorations.cpp

+        /* [&external_client_launcher](mir::Server& s) */
+        /* { */
+        /*     s.add_init_callback( */
+        /*         [&external_client_launcher] */
+        /*         { */
+        /*             external_client_launcher.launch("xeyes"); */
+        /*         }); */
+        /* }, */


Suggested change

/* [&external_client_launcher](mir::Server& s) */

/* { */

/* s.add_init_callback( */

/* [&external_client_launcher] */

/* { */

/* external_client_launcher.launch("xeyes"); */

/* }); */

/* }, */

AlanGriffiths · 2024-11-11T17:49:36Z

I think the first goal (starting from main) should be to isolate the policy elements of the decoration code from the generic. As you've seen, pulling all the code out of libmirserver introduces difficulties with dependencies on internal APIs. A slimmed down "policy" would help avoid that, and I think you've learnt enough to make that separation work.

Worked on my machine™

AlanGriffiths · 2024-11-12T09:56:59Z

examples/miral-custom-decorations/CMakeLists.txt

  ${PROJECT_SOURCE_DIR}/src/include/server
  ${PROJECT_SOURCE_DIR}/include/platform
+  ${PROJECT_SOURCE_DIR}/include/wayland


Example code should not depend on any of these include paths

tarek-y-ismail self-assigned this Sep 20, 2024

tarek-y-ismail force-pushed the custom-server-side-decorations branch from 4072e84 to 8ec208b Compare September 20, 2024 11:32

tarek-y-ismail marked this pull request as ready for review September 20, 2024 11:32

tarek-y-ismail requested a review from a team as a code owner September 20, 2024 11:32

AlanGriffiths reviewed Sep 23, 2024

View reviewed changes

src/include/server/mir/server_configuration.h Outdated Show resolved Hide resolved

tarek-y-ismail requested a review from AlanGriffiths September 24, 2024 08:52

AlanGriffiths requested changes Sep 24, 2024

View reviewed changes

src/include/server/mir/shell/decoration.h Outdated Show resolved Hide resolved

src/include/server/mir/shell/decoration.h Outdated Show resolved Hide resolved

src/include/server/mir/shell/decoration.h Outdated Show resolved Hide resolved

tarek-y-ismail requested a review from AlanGriffiths September 24, 2024 11:38

AlanGriffiths requested changes Oct 10, 2024

View reviewed changes

tarek-y-ismail requested a review from AlanGriffiths October 11, 2024 14:52

AlanGriffiths requested changes Oct 11, 2024

View reviewed changes

tarek-y-ismail force-pushed the custom-server-side-decorations branch from 4e519ba to 21e5c3e Compare October 14, 2024 10:05

tarek-y-ismail requested a review from AlanGriffiths October 14, 2024 10:06

tarek-y-ismail force-pushed the custom-server-side-decorations branch from a8c7835 to a23ecff Compare October 18, 2024 10:36

AlanGriffiths reviewed Oct 18, 2024

View reviewed changes

AlanGriffiths requested changes Oct 18, 2024

View reviewed changes

tarek-y-ismail requested a review from AlanGriffiths October 21, 2024 15:19

AlanGriffiths requested changes Oct 23, 2024

View reviewed changes

tarek-y-ismail requested a review from AlanGriffiths November 1, 2024 10:48

AlanGriffiths requested changes Nov 6, 2024

View reviewed changes

tarek-y-ismail added 2 commits November 11, 2024 12:32

tarek-y-ismail force-pushed the custom-server-side-decorations branch from 58c44e0 to 977ef38 Compare November 11, 2024 13:14

tarek-y-ismail commented Nov 11, 2024

View reviewed changes

src/server/shell/decoration/decoration_notifier.cpp Outdated Show resolved Hide resolved

tarek-y-ismail commented Nov 11, 2024

View reviewed changes

src/miral/decoration_light_mode.cpp Show resolved Hide resolved

tarek-y-ismail commented Nov 11, 2024

View reviewed changes

src/include/server/mir/shell/decoration_notifier.h Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

tarek-y-ismail commented Nov 11, 2024

View reviewed changes

tarek-y-ismail and others added 10 commits November 11, 2024 17:50

Minor fixes & cleanups

bed8491

Update mirserver symbols Bump mirserver symbol stanza to 2.19 since we're breaking ABI Fix header ordering in `decoration_notifier.h` Move `BasicDecoration::redraw` to be private. Wrap the contents of `Decoration::set_scale` with `ThreadSafeAccess::spawn`

Make BasicManager::decorations private. Add accessor methods for tests

a0250bd

Move basic_{manager, decoration}.h and input.h under

e2222c0

`src/include/server/mir/decoration`

Parametrize msd::{Renderer, BasicDecoration} on theme and

d40bd9e

button theme.

Copy over BasicDecoration and rename it to UserDecorationExample.

884e3ce

Move custom decorations to their own examples. Remove custom decoration code from the `miral::decoration` namespace Make `UserDecorationExample` light by default to help differentiate it from Mir's built-in decorations.

Move basic_decoration(_manager).h and friends back to

c2c337f

`src/server/shell/decoration`

Groom symbols

2870bd1

Minor cleanups & fixes

d96e5f9

tarek-y-ismail force-pushed the custom-server-side-decorations branch from 977ef38 to d96e5f9 Compare November 11, 2024 17:20

tarek-y-ismail commented Nov 11, 2024

View reviewed changes

tarek-y-ismail force-pushed the custom-server-side-decorations branch from 5f6a0ff to 9548497 Compare November 11, 2024 17:38

AlanGriffiths requested changes Nov 11, 2024

View reviewed changes

Fix includes to use local project includes.

38b971a

Worked on my machine™

tarek-y-ismail force-pushed the custom-server-side-decorations branch from 9548497 to 38b971a Compare November 11, 2024 17:56

Fix includes to compile on CI

9365a8e

AlanGriffiths reviewed Nov 12, 2024

View reviewed changes

tarek-y-ismail closed this Nov 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add APIs to override decoration managers and customize decorations #3604

Add APIs to override decoration managers and customize decorations #3604

tarek-y-ismail commented Sep 20, 2024

AlanGriffiths Oct 18, 2024

AlanGriffiths left a comment

tarek-y-ismail commented Oct 18, 2024

AlanGriffiths commented Oct 18, 2024

tarek-y-ismail commented Oct 18, 2024

AlanGriffiths commented Oct 18, 2024

tarek-y-ismail commented Oct 18, 2024 •

edited

Loading

AlanGriffiths commented Oct 18, 2024

tarek-y-ismail commented Oct 18, 2024

AlanGriffiths commented Oct 18, 2024

tarek-y-ismail commented Oct 21, 2024 •

edited

Loading

AlanGriffiths left a comment

AlanGriffiths left a comment

tarek-y-ismail commented Nov 7, 2024

This comment was marked as resolved.

This comment was marked as resolved.

tarek-y-ismail Nov 11, 2024

tarek-y-ismail Nov 11, 2024 •

edited

Loading

tarek-y-ismail Nov 11, 2024 •

edited

Loading

AlanGriffiths left a comment

AlanGriffiths Nov 11, 2024

tarek-y-ismail Nov 11, 2024

AlanGriffiths Nov 12, 2024

AlanGriffiths Nov 11, 2024

AlanGriffiths Nov 11, 2024

AlanGriffiths Nov 11, 2024

AlanGriffiths Nov 11, 2024

AlanGriffiths Nov 11, 2024

AlanGriffiths Nov 11, 2024

AlanGriffiths Nov 11, 2024

AlanGriffiths Nov 11, 2024

AlanGriffiths Nov 11, 2024

AlanGriffiths commented Nov 11, 2024

AlanGriffiths Nov 12, 2024

		protected:
		std::unordered_map<scene::Surface*, std::unique_ptr<Decoration>> decorations;



		#include "mir/server.h"
		#include "mir/main_loop.h" // Needed for the conversion of `the_main_loop` to an executor

Add APIs to override decoration managers and customize decorations #3604

Add APIs to override decoration managers and customize decorations #3604

Conversation

tarek-y-ismail commented Sep 20, 2024

Choose a reason for hiding this comment

AlanGriffiths left a comment

Choose a reason for hiding this comment

tarek-y-ismail commented Oct 18, 2024

AlanGriffiths commented Oct 18, 2024

tarek-y-ismail commented Oct 18, 2024

AlanGriffiths commented Oct 18, 2024

tarek-y-ismail commented Oct 18, 2024 • edited Loading

AlanGriffiths commented Oct 18, 2024

tarek-y-ismail commented Oct 18, 2024

AlanGriffiths commented Oct 18, 2024

tarek-y-ismail commented Oct 21, 2024 • edited Loading

AlanGriffiths left a comment

Choose a reason for hiding this comment

AlanGriffiths left a comment

Choose a reason for hiding this comment

tarek-y-ismail commented Nov 7, 2024

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

tarek-y-ismail Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

tarek-y-ismail Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

AlanGriffiths left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlanGriffiths commented Nov 11, 2024

Choose a reason for hiding this comment

tarek-y-ismail commented Oct 18, 2024 •

edited

Loading

tarek-y-ismail commented Oct 21, 2024 •

edited

Loading

tarek-y-ismail Nov 11, 2024 •

edited

Loading

tarek-y-ismail Nov 11, 2024 •

edited

Loading