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 (we are here): 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)

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

include/miral/miral/decoration_manager_builder.h

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

include/miral/miral/decoration_manager_builder.h

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

examples/miral-shell/decoration/CMakeLists.txt

examples/miral-shell/decoration/user_decoration_manager_example.cpp

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>

examples/miral-shell/shell_main.cpp

include/miral/miral/decoration_manager_builder.h

include/miral/miral/decoration_basic_manager.h

manager in miral-shell

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.

Respects floating/maximized/minimized states.

Should make it easier and less messy to edit things, a lot more readable too

Should give a bit more context than just `miral::{WindowState,StaticGeometry}`

Removed the usage of the pimpl pattern as well since it's internal anyway.

`miral::decoration` And move some renderer specific variables into `Renderer` instead of the outer namespace.

`miral::DecorationBasicManager`.

Took the chance to revert some shared -> unique ptr changes done earlier, as well as fixing some lifetime issues.

src/server/symbols.map

`DecorationBasicManager` instead.

tarek-y-ismail · 2024-11-22T08:42:36Z

examples/miral-shell/decoration/decoration.cpp

+    for (auto const& widget : widgets)
+    {
+        if (widget->button)
+            button_rects.push_back(widget);
+    }
+
+    return button_rects;


Would've been nice to do this using the ranges API but it seems not ready yet...

"not ready yet"? There's not much code using ranges in our codebase, but we have used it

Looks like I'm who's not ready. I spent about 20 minutes trying to filter and collect but couldn't figure it out.

Something like:

widgets | std::ranges::views::filter([](auto& widget){ return widget->button; }) | std::ranges::to<std::vector<Widget const>>();

But it always complained about piping widgets into std::ranges::views::filter, I tried looking at cppreference, but couldn't figure it out.

AlanGriffiths

I still think that "expose everything so that a consumer-side implementation works, then try to hide the stuff that shouldn't be exposed" is a long route to the destination.

First separating the stuff that should be exposed and then exposing it would be easier

AlanGriffiths · 2024-11-22T09:55:57Z

include/miral/miral/decoration_adapter.h

+struct InputContext
+{
+    InputContext(
+        std::shared_ptr<msh::Shell> const& shell,
+        std::shared_ptr<ms::Session> const& session,
+        std::shared_ptr<ms::Surface> const& window_surface,
+        std::shared_ptr<MirEvent const> const& latest_event,
+        std::shared_ptr<mir::input::CursorImages> const& cursor_images,
+        std::shared_ptr<ms::Surface> const& decoration_surface);
+
+    void request_move() const;
+
+    void request_resize(MirResizeEdge edge) const;
+
+    void set_cursor(std::string const& cursor_image_name) const;
+
+    void request_close() const;
+
+    void request_toggle_maximize() const;
+
+    void request_minimize() const;
+private:
+    std::shared_ptr<msh::Shell> const shell;
+    std::shared_ptr<ms::Session> const session;
+    std::shared_ptr<ms::Surface> const window_surface;
+    std::shared_ptr<const MirEvent> const latest_event;
+    std::shared_ptr<mir::input::CursorImages> const cursor_images;
+    std::shared_ptr<ms::Surface> const decoration_surface;
+};


This is not a good abstraction to have in the miral API: if we change the implementation then there's an ABI break.

The API should be more like:

Suggested change

struct InputContext

{

InputContext(

std::shared_ptr<msh::Shell> const& shell,

std::shared_ptr<ms::Session> const& session,

std::shared_ptr<ms::Surface> const& window_surface,

std::shared_ptr<MirEvent const> const& latest_event,

std::shared_ptr<mir::input::CursorImages> const& cursor_images,

std::shared_ptr<ms::Surface> const& decoration_surface);

void request_move() const;

void request_resize(MirResizeEdge edge) const;

void set_cursor(std::string const& cursor_image_name) const;

void request_close() const;

void request_toggle_maximize() const;

void request_minimize() const;

private:

std::shared_ptr<msh::Shell> const shell;

std::shared_ptr<ms::Session> const session;

std::shared_ptr<ms::Surface> const window_surface;

std::shared_ptr<const MirEvent> const latest_event;

std::shared_ptr<mir::input::CursorImages> const cursor_images;

std::shared_ptr<ms::Surface> const decoration_surface;

};

struct InputContext

{

void request_move() const;

void request_resize(MirResizeEdge edge) const;

void set_cursor(std::string const& cursor_image_name) const;

void request_close() const;

void request_toggle_maximize() const;

void request_minimize() const;

private:

// TODO there needs to be a constructor usable by the API implementation

struct Self;

std::shared_ptr<Self> self;

};

AlanGriffiths · 2024-11-22T10:00:45Z

include/miral/miral/decoration_adapter.h

+    using Pixel = uint32_t;
+
+    // Can we use even more keywords?
+    static inline auto const buffer_format = mir_pixel_format_argb_8888;
+    static inline auto const bytes_per_pixel = 4;


This looks like we are only supporting argb-8888 s/w rendering? Might the user want more choice?

As far as I know this is the most flexible set up at the moment. It wastes memory if you don't need transparency and performance if you want to draw using the GPU, but those are concessions I'm willing to make while I'm experimenting around

If these restrictions were part of the example, I would be fine with it. If we're telling shell developers "this is the only way" then I'm not. Our objective is an API we are willing to support indefinitely, that's very different to "while I'm experimenting around".

How flexible do we want this to be? If we stick to software rendering, we can allow the user to determine the pixel format, and that would modify buffer allocation and pixel access (for render_row, draw_rect, and any other functions that we might want to add in the future).

If we want things to be even more generic (allowing GPU rendering), then I'm a bit lost.

I think sticking to software rendering at the moment would be a good idea since it still allows for GPU rendering with a bit off inefficiency (render and copy to CPU)

AlanGriffiths · 2024-11-22T10:07:52Z

include/miral/miral/decoration_adapter.h

+
+namespace miral
+{
+namespace decoration


I'm still not convinced a namespace is appropriate. (I would probably nest auxiliary types in CustomDecoration.

AlanGriffiths · 2024-11-22T10:10:18Z

include/miral/miral/decoration.h

+    auto location() const -> mir::geometry::Point;
+    auto pressed() const -> bool;


I think shell authors may want more information:

what modifier keys are pressed

touch count, and individual touches

which mouse buttons are pressed

include/miral/miral/decoration.h

AlanGriffiths · 2024-11-22T10:19:08Z

include/miral/miral/decoration.h

+    std::function<void()> on_redraw;
+};
+
+class Decoration // Placeholder names


"Placeholder names"?

This is more of a note to myself. When I was getting the code to work at the start, I wrote down the first name that came to mind without giving it much thought. So I think giving that some time once everything settles might be good

AlanGriffiths · 2024-11-22T10:20:01Z

include/miral/miral/decoration_adapter.h

+namespace msh = mir::shell;
+namespace msd = msh::decoration;
+namespace ms = mir::scene;
+namespace mc = mir::compositor;
+namespace mg = mir::graphics;
+namespace geometry = mir::geometry;


Pulling far too much implementation detail into the API

AlanGriffiths · 2024-11-22T10:23:36Z

include/miral/miral/decoration_adapter.h

+    void init(
+        std::shared_ptr<ms::Surface> const& window_surface,
+        std::shared_ptr<ms::Surface> const& decoration_surface,
+        std::shared_ptr<mg::GraphicBufferAllocator> const& buffer_allocator,
+        std::shared_ptr<msh::Shell> const& shell,
+        std::shared_ptr<mir::input::CursorImages> const& cursor_images
+    );


This is not used by the example code, and looks like an implementation detail being leaked.

AlanGriffiths · 2024-11-22T10:23:51Z

include/miral/miral/decoration_adapter.h

+
+    void set_custom_geometry(std::shared_ptr<miral::decoration::StaticGeometry> geometry);
+
+    auto to_decoration() -> std::unique_ptr<msd::Decoration>;


This is not used by the example code, and looks like an implementation detail being leaked

set_custom_decoration is, to_decoration isn't.

I thought that to_decoration() was what I was commenting on

AlanGriffiths · 2024-11-22T10:26:16Z

include/miral/miral/decoration_adapter.h

+    DecorationAdapter(
+        std::shared_ptr<DecorationRedrawNotifier> const& redraw_notifier,
+
+        // Called by user code in response to input events
+        Renderer::RenderingCallback render_titlebar,
+        Renderer::RenderingCallback render_left_border,
+        Renderer::RenderingCallback render_right_border,
+        Renderer::RenderingCallback render_bottom_border,
+
+        // Called by Mir to inform user code of input events
+        OnProcessEnter process_enter,
+        OnProcessLeave process_leave,
+        OnProcessDown process_down,
+        OnProcessUp process_up,
+        OnProcessMove process_move,
+        OnProcessDrag process_drag,
+
+        // Called by Mir to inform user code of changes to the window being decorated
+        OnWindowAttribChanged attrib_changed,
+        OnWindowResized window_resized_to,
+        OnWindowRenamed window_renamed,
+
+        // Called by Mir to inform user code that the display scale changed
+        OnScaleChanged scale_changed,
+
+        // Called whenever the internal WindowState is updated
+        OnWindowStateUpdated update_decoration_window_state);


A long list of parameters like this is not maintainable: C.f. discussion about using builders to help

tarek-y-ismail · 2024-11-22T08:50:40Z

examples/miral-shell/decoration/decoration.cpp

+    return std::nullopt;
+}
+
+void UserDecoration::InputManager::process_drag(miral::DeviceEvent& device, InputContext ctx)


Maybe the context could be integrated into the input manager? Would require some sort of interface between the internal machinery and the custom decoration to initialize the input manager. Maybe passing the decoration manager to DecorationAdapter during construction?

(low priority, the main reason behind this suggestion is that most meaningful actions require a context, but passing it isn't bad as well)

tarek-y-ismail · 2024-11-22T09:00:23Z

examples/miral-shell/decoration/decoration.cpp

+                           }
+                           decoration->redraw_notifier()->notify();
+                       },
+                       [decoration](auto...)


This (window_resized_to) might not be very useful for the user as we already take care of everything related to resizing for them.

tarek-y-ismail · 2024-11-22T09:01:38Z

examples/miral-shell/decoration/decoration.h

+    static auto create_manager(mir::Server&)
+        -> std::shared_ptr<miral::DecorationManagerAdapter>;
+
+    struct InputManager


Having the definition of the input manager here is unnecessary

tarek-y-ismail · 2024-11-22T09:05:06Z

include/miral/miral/decoration.h

+{
+public:
+    DeviceEvent(mir::shell::decoration::DeviceEvent);
+    operator mir::shell::decoration::DeviceEvent() const;


Unnecessary, we get inputs as msd::DeviceEvents and only need to convert them to miral::DeviceEvent

tarek-y-ismail · 2024-11-22T09:06:47Z

include/miral/miral/decoration.h

+
+private:
+    struct Impl;
+    std::shared_ptr<Impl> impl;


Maybe use unique_ptr?

Edit: The reason I'm not already using unique_ptr is because it has some issues compiling with incomplete classes. Googling for a bit shows that the spec allows it, but I haven't looked deep enough to make it work.

tarek-y-ismail · 2024-11-22T11:52:37Z

src/miral/decoration_adapter.cpp

+void md::InputContext::request_close() const{
+    window_surface->request_client_surface_close();
+}
+
+void md::InputContext::request_toggle_maximize() const{
+    msh::SurfaceSpecification spec;
+    if (window_surface->state() == mir_window_state_maximized)
+        spec.state = mir_window_state_restored;
+    else
+        spec.state = mir_window_state_maximized;
+    shell->modify_surface(session, window_surface, spec);
+}
+
+void md::InputContext::request_minimize() const{
+    msh::SurfaceSpecification spec;
+    spec.state = mir_window_state_minimized;
+    shell->modify_surface(session, window_surface, spec);
+}


Needs formatting

tarek-y-ismail · 2024-11-22T11:56:14Z

src/miral/decoration_adapter.cpp

+        md::OnWindowAttribChanged attrib_changed,
+        md::OnWindowResized window_resized_to,
+        md::OnWindowRenamed window_renamed) :
+        on_attrib_changed{attrib_changed},


Should use braces to match the style of the rest of the code

tarek-y-ismail · 2024-11-22T11:58:52Z

src/miral/decoration_adapter.cpp

+    auto window_spec = [this]
+    {
+        msh::SurfaceSpecification spec;
+
+        window_surface->set_window_margins(
+            as_delta(window_state->titlebar_height()), geom::DeltaX{}, geom::DeltaY{}, geom::DeltaX{});
+
+        if (window_state->window_size().width.as_value())
+            spec.width = window_state->window_size().width;
+        if (window_state->window_size().height.as_value())
+            spec.height = window_state->window_size().height;
+
+        // Could probably be `decoration->get_input_shape(window_state)`.
+        // This will do for now.
+        spec.input_shape = {
+            window_state->titlebar_rect(),
+            window_state->bottom_border_rect(),
+            window_state->left_border_rect(),
+            window_state->right_border_rect(),
+        };
+
+        return spec;
+    }();


Should be moved closer to the place of usage

tarek-y-ismail · 2024-11-22T12:12:53Z

src/miral/decoration_manager_adapter.h

+    void undecorate(std::shared_ptr<mir::scene::Surface> const& surface) override;
+    void undecorate_all() override;
+
+    virtual ~DecorationManagerAdapter();


I don't remember why I added this virtual keyword. This class isn't meant to be inherited from.

tarek-y-ismail · 2024-11-22T12:21:35Z

src/server/shell/decoration/basic_manager.cpp

+    // TODO: probably only frees one pointer, but not the whole set of things
+    // pointing at the decoration.


Old comment from when this was a shared_ptr

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

Add a way to override the decoration manager

324452d

Expose msd::Manager through miral.

f07b887

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

AlanGriffiths requested changes Nov 18, 2024

View reviewed changes

tarek-y-ismail force-pushed the redo-custom-server-side-decorations branch from bad7442 to 8786b40 Compare November 21, 2024 15:16

tarek-y-ismail and others added 23 commits November 21, 2024 17:17

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

ec20cf0

manager in miral-shell

Link against miral instead of using an explicit include path

b94f0ef

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

Use wrappers instead of a bespoke decoration manager init.

082ab3d

Also, fix symbols.

Move input event listening and resolution to InputResolver

aa64ec6

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

0129261

To allow for forward declares.

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

7722708

Copy over all of the code needed to render the titlebar

cc81a58

Enable custom decorations by default for testing

dced985

Use self instead of this in DecorationBasicManager::to_adapter

1e6b8c2

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

Expose the_buffer_allocator in mir::Server

ddba2e4

Pass mir::Server down to where decorations are created

0d24670

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

5304737

Move most rendering and setup code to DecorationAdapter

e72aab1

Move window_state from Decoration::Self to DecorationAdapter

dff4f93

Move Decoration::update_state to DecorationAdapter

ffeae0e

Add window dragging

dcc78b7

Split the implementation and interface in miral::Decoration

0469164

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

bb5b298

Implement minimize, maximize toggle, and close buttons

6cd489b

Remove CMAKE_CURRENT_BINARY_DIR from decoration example includes

c468110

Change decoration titlebar color on focus/unfocus

de222f6

Draw additional decoration sides.

04c9a32

Respects floating/maximized/minimized states.

UserDecoration colors glow up

b3765bf

tarek-y-ismail added 16 commits November 21, 2024 17:17

Merge Renderer::Buffer(Stream)

125b389

Allow decoration authors to pass custom geometry

ea3c6e0

Remove parameters in DeocrationBasicManager builder lambda.

4d76865

Const correct a bunch of parameters, methods, and data members

41009a4

Alias the rest of the callbacks passed to DecorationAdapter

804b1f3

Should make it easier and less messy to edit things, a lot more readable too

Move WindowState and StaticGeometry to miral::decoration.

8aea366

Should give a bit more context than just `miral::{WindowState,StaticGeometry}`

Move InputContext's constructor to decoration_adapter.cpp

7693ae0

Move InputResolverAdapter to be internal to decoration_adapter.cpp

35ef710

Removed the usage of the pimpl pattern as well since it's internal anyway.

Move Renderer method definitions to decoration_adapter.cpp

5e18aa0

Move DecorationAdapter, Renderer, InputContext to

0731b42

`miral::decoration` And move some renderer specific variables into `Renderer` instead of the outer namespace.

Move DecorationRedrawNotifier impl to decoration_adapter.cpp

fe1a97a

Hide DecorationManagerBuilder's internals behind pimpl

565426d

Remove now unused user_decoration_manager_example.{h,cpp}

d11c2b8

Re-enable display scale observing for msd::BasicManager and

fe1355a

`miral::DecorationBasicManager`.

Fix decorations not destroying with their apps.

73f2693

Took the chance to revert some shared -> unique ptr changes done earlier, as well as fixing some lifetime issues.

Fix application flickering when interacting with decorations.

d5ae982

tarek-y-ismail force-pushed the redo-custom-server-side-decorations branch from 8786b40 to 6d461ef Compare November 21, 2024 15:18

tarek-y-ismail requested a review from AlanGriffiths November 21, 2024 15:25

tarek-y-ismail commented Nov 22, 2024

View reviewed changes

src/server/symbols.map Outdated Show resolved Hide resolved

Remove DecorationManagerBuilder, use DecorationManagerAdapter in

2613f31

`DecorationBasicManager` instead.

tarek-y-ismail force-pushed the redo-custom-server-side-decorations branch from 6d461ef to 2613f31 Compare November 22, 2024 07:49

Groom symbols

6e76cde

tarek-y-ismail commented Nov 22, 2024

View reviewed changes

AlanGriffiths requested changes Nov 22, 2024

View reviewed changes

tarek-y-ismail commented Nov 22, 2024

View reviewed changes

tarek-y-ismail added 2 commits November 22, 2024 15:34

Use pimpl in InputContext

c921385

process_{up,down} now work with all supported mouse buttons

c157f0d

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 left a comment

AlanGriffiths Nov 18, 2024

AlanGriffiths Nov 18, 2024

tarek-y-ismail Nov 22, 2024

AlanGriffiths Nov 22, 2024

tarek-y-ismail Nov 22, 2024 •

edited

Loading

AlanGriffiths left a comment

AlanGriffiths Nov 22, 2024

AlanGriffiths Nov 22, 2024

tarek-y-ismail Nov 22, 2024

AlanGriffiths Nov 22, 2024

tarek-y-ismail Nov 25, 2024

AlanGriffiths Nov 22, 2024

AlanGriffiths Nov 22, 2024

AlanGriffiths Nov 22, 2024

tarek-y-ismail Nov 22, 2024

AlanGriffiths Nov 22, 2024

AlanGriffiths Nov 22, 2024

AlanGriffiths Nov 22, 2024

tarek-y-ismail Nov 22, 2024

AlanGriffiths Nov 22, 2024

AlanGriffiths Nov 22, 2024

tarek-y-ismail Nov 22, 2024 •

edited

Loading

tarek-y-ismail Nov 22, 2024

tarek-y-ismail Nov 22, 2024

tarek-y-ismail Nov 22, 2024

tarek-y-ismail Nov 22, 2024 •

edited

Loading

tarek-y-ismail Nov 22, 2024

tarek-y-ismail Nov 22, 2024

tarek-y-ismail Nov 22, 2024

tarek-y-ismail Nov 22, 2024

tarek-y-ismail Nov 22, 2024

		auto location() const -> mir::geometry::Point;
		auto pressed() const -> bool;


		void set_custom_geometry(std::shared_ptr<miral::decoration::StaticGeometry> geometry);

		auto to_decoration() -> std::unique_ptr<msd::Decoration>;

		// TODO: probably only frees one pointer, but not the whole set of things
		// pointing at the decoration.

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

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 22, 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

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 22, 2024 • edited Loading

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 22, 2024 • edited Loading

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 22, 2024 •

edited

Loading

tarek-y-ismail Nov 22, 2024 •

edited

Loading

tarek-y-ismail Nov 22, 2024 •

edited

Loading