Repository: Textualize/textual
Pull request: Margin spacing is now invisible

Problem statement:
Don't render margin
Currently a widget renders margin as solid color (if present).

It would be more correct to allow the background to be visible. I think this will require modifying the size of a widget to compensate for margin prior to rendering

Hints from issue discussion:

Commit message: Margin space is now not rendered as solid colour

Patch to review:
diff --git a/src/textual/css/_styles_builder.py b/src/textual/css/_styles_builder.py
index 17349a04f7..871d21f39b 100644
--- a/src/textual/css/_styles_builder.py
+++ b/src/textual/css/_styles_builder.py
@@ -128,7 +128,7 @@ def _process_space(self, name: str, tokens: list[Token]) -> None:
         append = space.append
         for token in tokens:
             (token_name, value, _, _, location) = token
-            if token_name == "scalar":
+            if token_name in ("number", "scalar"):
                 try:
                     append(int(value))
                 except ValueError:
diff --git a/src/textual/geometry.py b/src/textual/geometry.py
index b9fcb2f527..d89075397a 100644
--- a/src/textual/geometry.py
+++ b/src/textual/geometry.py
@@ -415,6 +415,24 @@ def clip(self, width: int, height: int) -> Region:
         )
         return new_region
 
+    def shrink(self, margin: Spacing) -> Region:
+        """Shrink a region by pushing each edge inwards.
+
+        Args:
+            margin (Spacing): Defines how many cells to shrink the Region by at each edge.
+
+        Returns:
+            Region: The new, smaller region.
+        """
+        _clamp = clamp
+        top, right, bottom, left = margin
+        return Region(
+            x=_clamp(self.x + left, 0, self.width),
+            y=_clamp(self.y + top, 0, self.height),
+            width=_clamp(self.width - left - right, 0, self.width),
+            height=_clamp(self.height - top - bottom, 0, self.height),
+        )
+
     def intersection(self, region: Region) -> Region:
         """Get that covers both regions.
 
diff --git a/src/textual/view.py b/src/textual/view.py
index d6cd03784f..2eb5893526 100644
--- a/src/textual/view.py
+++ b/src/textual/view.py
@@ -114,7 +114,24 @@ def get_arrangement(self, size: Size, scroll: Offset) -> Iterable[WidgetPlacemen
         cached_size, cached_scroll, arrangement = self._cached_arrangement
         if cached_size == size and cached_scroll == scroll:
             return arrangement
-        arrangement = list(self._layout.arrange(self, size, scroll))
+
+        arrangement = []
+        placements = self._layout.arrange(self, size, scroll)
+        for placement in placements:
+            region, widget, order = placement
+            styles = widget.styles
+            if styles.has_margin:
+                margin = styles.margin
+                arrangement.append(
+                    WidgetPlacement(
+                        region=region.shrink(margin),
+                        widget=widget,
+                        order=order,
+                    )
+                )
+            else:
+                arrangement.append(placement)
+
         self._cached_arrangement = (size, scroll, arrangement)
         return arrangement
 
diff --git a/src/textual/widget.py b/src/textual/widget.py
index 51c7e97ab5..0f81a7d150 100644
--- a/src/textual/widget.py
+++ b/src/textual/widget.py
@@ -181,9 +181,6 @@ def render_styled(self) -> RenderableType:
                     renderable, styles.border, style=renderable_text_style
                 )
 
-            if styles.has_margin:
-                renderable = Padding(renderable, styles.margin, style=parent_text_style)
-
             if styles.has_outline:
                 renderable = Border(
                     renderable,
diff --git a/tests/test_geometry.py b/tests/test_geometry.py
index ffaded07b9..2bfbc695dd 100644
--- a/tests/test_geometry.py
+++ b/tests/test_geometry.py
@@ -1,6 +1,6 @@
 import pytest
 
-from textual.geometry import clamp, Offset, Size, Region
+from textual.geometry import clamp, Offset, Size, Region, Spacing
 
 
 def test_dimensions_region():
@@ -173,6 +173,12 @@ def test_clip():
     assert Region(10, 10, 20, 30).clip(20, 25) == Region(10, 10, 10, 15)
 
 
+def test_region_shrink():
+    margin = Spacing(top=1, right=2, bottom=3, left=4)
+    region = Region(x=10, y=10, width=50, height=50)
+    assert region.shrink(margin) == Region(x=14, y=11, width=44, height=46)
+
+
 def test_region_intersection():
     assert Region(0, 0, 100, 50).intersection(Region(10, 10, 10, 10)) == Region(
         10, 10, 10, 10


Code review comments:
diff_hunk: @@ -114,7 +114,24 @@ def get_arrangement(self, size: Size, scroll: Offset) -> Iterable[WidgetPlacemen
         cached_size, cached_scroll, arrangement = self._cached_arrangement
         if cached_size == size and cached_scroll == scroll:
             return arrangement
-        arrangement = list(self._layout.arrange(self, size, scroll))
+
+        arrangement = []
+        placements = self._layout.arrange(self, size, scroll)
+        for placement in placements:
+            region, widget, order = placement
path: src/textual/view.py
text: @user1:
Could you do this within the loop, i.e. `for region, widget, order in placements`.

diff_hunk: @@ -114,7 +114,24 @@ def get_arrangement(self, size: Size, scroll: Offset) -> Iterable[WidgetPlacemen
         cached_size, cached_scroll, arrangement = self._cached_arrangement
         if cached_size == size and cached_scroll == scroll:
             return arrangement
-        arrangement = list(self._layout.arrange(self, size, scroll))
+
+        arrangement = []
path: src/textual/view.py
text: @user1:
I'm thinking this might simplify if we move some of the funcationality to the `WidgetPlacement` object itself.

Let's say we implement a `apply_margin` method on `WidgetPlacement` that returned either a new object or self, dependant on if there is margin or not.

Then this code could be:

```python
placements = [
    placement.apply_margin()
    for placement in self._layout.arrange(self, size, scroll)
]
```

I wouldn't be surprised if we grow other operations on the WidgetPlacement objects, and this may keep it simple.

What do you think?

@author:
Sounds good to me. I've implemented this change now.