Add APIs to override decoration managers and customize decorations (Take 2) #3666

tarek-y-ismail · 2024-11-12T16:24:51Z

Attempt 1: Naive. Trying to expose an internal (an unstable) mir API to users.
Attempt 2: Bottom up. Trying to bring out the API from msd::BasicDecoration and expose it via adapters.
Attempt 3: Top down. Trying to extract the minimal API needed for users to implement at least something similar to msd::BasicDecoration (The code is nasty on purpose)

manager in miral-shell

AlanGriffiths

Proposing the interface for review without "dragging implementation code along" is good.

However, I would rather have focused first on separating out the "policy" part of decorations as otherwise the miral::*Decoration* interface is somewhat speculative. (In particular, there's no way to "hook up" input in the current draft.)

examples/miral-shell/decoration/CMakeLists.txt

AlanGriffiths · 2024-11-12T16:49:58Z

include/miral/miral/decoration_manager_builder.h

+{
+public:
+    static auto build(
+        std::function<void(std::weak_ptr<mir::shell::Shell> const& shell)> on_init,


User code shouldn't be using Shell so there's no point to this functor.

Looking at BasicDecoration, it uses the shell for a bunch of things: resizing itself, moving, maximizing, minimizing, etc..

Is there another way to access this functionality?

Maybe it would be a good thing to work on isolating the decoration "strategy" code before designing the API to customise it?

src/include/server/mir/default_server_configuration.h

src/miral/custom_decorations.cpp

src/server/shell/default_configuration.cpp

src/server/symbols.map

examples/miral-shell/shell_main.cpp

AlanGriffiths · 2024-11-12T17:17:39Z

examples/miral-shell/shell_main.cpp

@@ -135,6 +138,7 @@ int main(int argc, char const* argv[])

    return runner.run_with(
        {
+            CustomDecorations{UserDecorationManagerExample::get_adapter()},


Is the adaptor created once on program startup and used for all decorations? Or might it need to maintain state associated with each window (and therefore be created once per window?)

Is the adaptor created once on program startup and used for all decorations?

Yep.

Or might it need to maintain state associated with each window (and therefore be created once per window?)

As it stands with BasicDecoration, the manager keeps track of decorations, and each decoration has it's own window-specific state.

AlanGriffiths · 2024-11-12T17:23:58Z

include/miral/miral/decoration_manager_builder.h

+class DecorationManagerBuilder
+{
+public:
+    static auto build(
+        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) -> DecorationManagerBuilder;
+
+    auto done() -> std::shared_ptr<DecorationManagerAdapter>;
+
+private:
+    DecorationManagerBuilder();
+    std::shared_ptr<DecorationManagerAdapter> adapter;
+};
+}


I think the implementation type should be opaque (e.g. DecorationManagerBuilder::Self) for future proofing. (It could currently be a trivial subclass of DecorationManagerAdapter)

There needs to be a way to attach functors for input handling in addition to the decorate handling.

Co-authored-by: Alan Griffiths <[email protected]>

Also, fix symbols.

To allow for forward declares.

This should guarantee that the lifetime is extended since `self` is a shared ptr

The implementation is now called `UserDecoration` and is implemented as an external class in miral-shell. The interface does book keeping of the necessary data members.

AlanGriffiths

OK, you said this was "ready to review", but I now see it change whilst reviewing. I'll leave it for now and just post what I've seen so far

AlanGriffiths · 2024-11-18T16:27:23Z

examples/miral-shell/decoration/CMakeLists.txt

+include_directories(
+  ${CMAKE_CURRENT_BINARY_DIR}
+)


AlanGriffiths · 2024-11-18T16:28:04Z

examples/miral-shell/decoration/user_decoration_manager_example.cpp

AlanGriffiths · 2024-11-18T16:28:24Z

examples/miral-shell/decoration/user_decoration_manager_example.h

AlanGriffiths · 2024-11-18T16:30:57Z

examples/miral-shell/shell_main.cpp

@@ -14,6 +14,8 @@
 * along with this program.  If not, see <http://www.gnu.org/licenses/>.
 */

+#include "decoration/decoration.h"
+#include "miral/decoration.h"


Suggested change

#include "miral/decoration.h"

AlanGriffiths · 2024-11-18T16:31:09Z

examples/miral-shell/shell_main.cpp

+#include <string>
+#include <vector>


Suggested change

#include <string>

#include <vector>

AlanGriffiths · 2024-11-18T16:32:57Z

examples/miral-shell/shell_main.cpp

+    return runner.run_with({
+        CustomDecorations{UserDecoration::create_manager},
+        CursorTheme{"default:DMZ-White"},
+        WaylandExtensions{},
+        X11Support{},
+        ConfigureDecorations{},
+        window_managers,
+        display_configuration_options,
+        external_client_launcher,
+        launcher,
+        config_keymap,
+        AppendEventFilter{quit_on_ctrl_alt_bksp},
+        StartupInternalClient{spinner},
+        ConfigurationOption{run_startup_apps, "startup-apps", "Colon separated list of startup apps", ""},
+        pre_init(ConfigurationOption{
+            [&](std::string const& typeface)
+            {
+                ::wallpaper::font_file(typeface);
+            },
+            "shell-wallpaper-font",
+            "font file to use for wallpaper",
+            ::wallpaper::font_file()}),
+        ConfigurationOption{
+            [&](std::string const& cmd)
+            {
+                terminal_cmd = cmd;
+            },
+            "shell-terminal-emulator",
+            "terminal emulator to use",
+            terminal_cmd},
+    });


I think the only substantive change is adding CustomDecorations{UserDecoration::create_manager},. The rest is unnecessary, and distracting, whitespace adjustment

AlanGriffiths · 2024-11-18T16:39:36Z

include/miral/miral/decoration_manager_builder.h

Not used by the example: shouldn't be part of the API

AlanGriffiths · 2024-11-18T16:45:14Z

include/miral/miral/decoration_basic_manager.h

+class DecorationBasicManager
+{
+public:
+    DecorationBasicManager(mir::Server&, Decoration::DecorationBuilder&&);

+    auto to_adapter() -> std::shared_ptr<DecorationManagerAdapter>;
 private:
-    Decoration(Decoration const&) = delete;
-    Decoration& operator=(Decoration const&) = delete;
+    struct Self;
+    std::shared_ptr<Self> self;
 };


There doesn't seem to be much purpose to this class. It takes "stuff" in its constructor and returns "other stuff" from to_adapter() - that's pretty much a function in disguise.

Respects floating/maximized/minimized states.

tarek-y-ismail added 3 commits November 12, 2024 14:06

Add a way to override the decoration manager

324452d

Expose msd::Manager through miral.

f07b887

Add option to use and enable/disable a custom (external) decoration

3065115

manager in miral-shell

tarek-y-ismail requested a review from AlanGriffiths November 12, 2024 16:24

tarek-y-ismail self-assigned this Nov 12, 2024

AlanGriffiths requested changes Nov 12, 2024

View reviewed changes

tarek-y-ismail and others added 18 commits November 13, 2024 11:30

Link against miral instead of using an explicit include path

640de3e

Co-authored-by: Alan Griffiths <[email protected]>

Use wrappers instead of a bespoke decoration manager init.

86720fa

Also, fix symbols.

Move input event listening and resolution to InputResolver

bc4c48d

Move InputResolver::DeviceEvent out to mir::shell::decoration

7aeeb65

To allow for forward declares.

Expose msd::{Decoration,BasicManager} through miral interfaces

266f9d5

Copy over all of the code needed to render the titlebar

5b065de

Enable custom decorations by default for testing

e446979

Use self instead of this in DecorationBasicManager::to_adapter

9d98118

This should guarantee that the lifetime is extended since `self` is a shared ptr

Expose the_buffer_allocator in mir::Server

98fbb29

Pass mir::Server down to where decorations are created

6966bba

Hook up the code needed to get input events and render a titlebar

4d36c36

Move most rendering and setup code to DecorationAdapter

6f70331

Move window_state from Decoration::Self to DecorationAdapter

5a36226

Move Decoration::update_state to DecorationAdapter

c0faac6

Add window dragging

323efed

Split the implementation and interface in miral::Decoration

a3f4c35

The implementation is now called `UserDecoration` and is implemented as an external class in miral-shell. The interface does book keeping of the necessary data members.

Implement window resizing and cursor image changing

33703f0

Implement minimize, maximize toggle, and close buttons

851709a

AlanGriffiths requested changes Nov 18, 2024

View reviewed changes

tarek-y-ismail added 5 commits November 19, 2024 11:50

Remove CMAKE_CURRENT_BINARY_DIR from decoration example includes

cb0a912

Change decoration titlebar color on focus/unfocus

d127352

Draw additional decoration sides.

099239f

Respects floating/maximized/minimized states.

UserDecoration colors glow up

a140ce0

Merge Renderer::Buffer(Stream)

550966a

Allow decoration authors to pass custom geometry

fa2811d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add APIs to override decoration managers and customize decorations (Take 2) #3666

Add APIs to override decoration managers and customize decorations (Take 2) #3666

tarek-y-ismail commented Nov 12, 2024 •

edited

Loading

AlanGriffiths left a comment

AlanGriffiths Nov 12, 2024

tarek-y-ismail Nov 13, 2024

AlanGriffiths Nov 13, 2024

AlanGriffiths Nov 12, 2024

tarek-y-ismail Nov 13, 2024 •

edited

Loading

AlanGriffiths Nov 12, 2024

AlanGriffiths left a comment

AlanGriffiths Nov 18, 2024

AlanGriffiths Nov 18, 2024

AlanGriffiths Nov 18, 2024

AlanGriffiths Nov 18, 2024

AlanGriffiths Nov 18, 2024

AlanGriffiths Nov 18, 2024

AlanGriffiths Nov 18, 2024

AlanGriffiths Nov 18, 2024

Add APIs to override decoration managers and customize decorations (Take 2) #3666

Are you sure you want to change the base?

Add APIs to override decoration managers and customize decorations (Take 2) #3666

Conversation

tarek-y-ismail commented Nov 12, 2024 • edited Loading

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

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

Choose a reason for hiding this comment

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

tarek-y-ismail commented Nov 12, 2024 •

edited

Loading

tarek-y-ismail Nov 13, 2024 •

edited

Loading