Decoration strategy

[AlanGriffiths]

Wrap all the SSD customization into an interface, and provides a default implementation

[AlanGriffiths]

This doesn't do much to fix the current design (which has far too much "ask" and not enough "tell" - c.f. StaticGeometry) but at least provides a single customization point

[AlanGriffiths]

This doesn't do much to fix the current design

I don't have clear ideas on addressing my dissatisfaction with the current design yet. So I'll propose it "as is" and see where other folks have feedback. Maybe next year all will become clear...

Happy holidays all! 🎄

[AlanGriffiths]

To show some of the customization that can be done by changing the "strategy", this makes the borders transparent and moves the buttons to the left:

$ git diff
diff --git a/src/server/shell/decoration/decoration_strategy.cpp b/src/server/shell/decoration/decoration_strategy.cpp
index b025233a19..447b5da92e 100644
--- a/src/server/shell/decoration/decoration_strategy.cpp
+++ b/src/server/shell/decoration/decoration_strategy.cpp
@@ -44,7 +44,7 @@ using msd::InputState;
 
 namespace
 {
-static constexpr auto color(unsigned char r, unsigned char g, unsigned char b, unsigned char a = 0xFF) -> uint32_t
+static constexpr auto color(unsigned char r, unsigned char g, unsigned char b, unsigned char a = 0x7F) -> uint32_t
 {
 #if __BYTE_ORDER == __LITTLE_ENDIAN
     return ((uint32_t)b <<  0) |
@@ -820,7 +820,7 @@ void RendererStrategy::redraw_titlebar_text(geom::Size const scaled_titlebar_siz
         scaled_titlebar_size,
         name,
         geom::Point{
-            static_geometry->title_font_top_left.x.as_value() * scale,
+            scale * (static_geometry->title_font_top_left.x.as_value() + buttons.size() * (static_geometry->button_width.as_value() + static_geometry->padding_between_buttons.as_value())),
             static_geometry->title_font_top_left.y.as_value() * scale},
         static_geometry->title_font_height * scale,
         current_theme->text_color);
@@ -912,10 +912,9 @@ auto DecorationStrategy::button_placement(unsigned n, const WindowState& ws) con
     auto const geometry = static_geometry();
 
     geom::X x =
-        titlebar.right() -
-        as_delta(ws.side_border_width()) -
-        n * as_delta(geometry->button_width + geometry->padding_between_buttons) -
-        as_delta(geometry->button_width);
+        titlebar.left() +
+        as_delta(ws.side_border_width()) +
+        n * as_delta(geometry->button_width + geometry->padding_between_buttons);
     return geom::Rectangle{
                     {x, titlebar.top()},
                     {geometry->button_width, titlebar.size.height}};

[RAOF]

Hm. I also don't have a great idea for reworking this. I'd say it is an improvement, but it's still not great.

[RAOF]

Ah. I see I should add a mrs::as_write_mappable_buffer() to mirror as_read_mappable_buffer(); this should really just be mg::Buffer.

[AlanGriffiths]

I didn't want to force the decoration strategy to deal with the likes of mrs::alloc_buffer_with_content() which needs a GraphicBufferAllocator. Yes, it is all rendering, but this seemed like a tractable seam to insert in the design

[RAOF]

Yeah, mrs::alloc_buffer_with_content() is the wrong interface here; what we really want from the decorator is a mg::Buffer, as long as it's easy enough for them to render to it.

Then again, I've had broader thoughts about the whole requirements, which I'll need to coalesce and transmit in a different forum.

[tarek-y-ismail]

which has far too much "ask" and not enough "tell" - c.f. StaticGeometry

Not sure I understand. What do you mean by "ask"/"tell" Here?

I don't have clear ideas on addressing my dissatisfaction with the current design yet.

Well, you could start by writing them out.

To give things a bit more direction, I think we can start with what we want the user API to look like. The internal/default implementation doesn't really matter for users unless there something shared between user code and internal code (buffer management or input routing for example), the rest of the internal code can then be simplified or cleaned up once things are clear. Shamelessly plugging #3666 in case that helps inspire you.

[AlanGriffiths]

which has far too much "ask" and not enough "tell" - c.f. StaticGeometry

Not sure I understand. What do you mean by "ask"/"tell" Here?

https://martinfowler.com/bliki/TellDontAsk.html

I don't have clear ideas on addressing my dissatisfaction with the current design yet.

Well, you could start by writing them out.

Well, that's part of my problem

To give things a bit more direction, I think we can start with what we want the user API to look like. The internal/default implementation doesn't really matter for users unless there something shared between user code and internal code (buffer management or input routing for example), the rest of the internal code can then be simplified or cleaned up once things are clear. Shamelessly plugging #3666 in case that helps inspire you.

That includes a lot of other concerns. To aid comparison, which part of that corresponds to decoration_strategy.h?

[tarek-y-ismail]

which part of that corresponds to decoration_strategy.h?

I would say decoration_adapter.h. It's the central piece that connects input, rendering, and other event handling all together. It's a bit more bloated/verbose, but that can be taken care of.

[AlanGriffiths]

which part of that corresponds to decoration_strategy.h?

I would say decoration_adapter.h. It's the central piece that connects input, rendering, and other event handling all together. It's a bit more bloated/verbose, but that can be taken care of.

While I don't like decoration_strategy.h I think it does a better job of separating the concern a decoration implementor should be dealing with. (And that is what this PR is trying for)

[AlanGriffiths]

OK, I've had a chance to reflect, I think this PR achieves what it sets out to do: refactor the existing implementation so that the decoration strategy interface is clearly separated.

The next step is cleaning up that interface, but that could sensibly happen in a follow-up PR: #3712