Merge pull request #224 from HalfWhitt/fix-early-impl-access

Deprecate style.copy(applicator); rework style/applicator setting mechanism

changes/224.bugfix.rst

@@ -0,0 +1 @@
+Assigning a new style object to a node that already has an applicator assigned now properly maintains an association between the applicator and the new style, and triggers a style reapplication.

changes/224.misc.rst

@@ -0,0 +1 @@
+An applicator, once assigned to a node, now receives a reference to its node (as self.node).

changes/224.removal.1.rst

@@ -0,0 +1 @@
+Supplying an applicator to BaseStyle.copy() has been deprecated. If you need to manually assign an applicator to a style, do it separately, after the copy.

changes/224.removal.2.rst

@@ -0,0 +1 @@
+The mechanisms for assigning styles and applicators to nodes, and applying styles, have been reworked. A node will now attempt to apply its style as soon as it is assigned an applicator; this means you should not assign an applicator to a node until the node is sufficiently initialized to apply its style. To accomodate uses that currently do not follow this order, any exceptions resulting from a failed style application are caught, and a runtime warning is issued. In a future version, this will be an exception.

src/travertino/declaration.py

@@ -297,6 +297,22 @@ def _applicator(self):
     def _applicator(self, value):
         self._assigned_applicator = value
 
+        if value is not None:
+            try:
+                self.reapply()
+            # This is backwards compatibility for Toga, which (at least as of
+            # 0.4.8), assigns style and applicator before the widget's
+            # implementation is available.
+            except Exception:
+                warn(
+                    "Failed to apply style when assigning applicator, or when "
+                    "assigning a new style once applicator is present. Node should be "
+                    "sufficiently initialized to apply its style before it is assigned "
+                    "an applicator. This will be an exception in a future version.",
+                    RuntimeWarning,
+                    stacklevel=2,
+                )
+
     ######################################################################
     # Interface that style declarations must define
     ######################################################################
@@ -326,8 +342,17 @@ def update(self, **styles):
     def copy(self, applicator=None):
         "Create a duplicate of this style declaration."
         dup = self.__class__()
-        dup._applicator = applicator
         dup.update(**self)
+
+        if applicator is not None:
+            warn(
+                "Providing an applicator to BaseStyle.copy() is deprecated. Set "
+                "applicator afterward on the returned copy.",
+                DeprecationWarning,
+                stacklevel=2,
+            )
+            dup._applicator = applicator
+
         return dup
 
     def __getitem__(self, name):

src/travertino/node.py

@@ -1,9 +1,11 @@
 class Node:
     def __init__(self, style, applicator=None, children=None):
+        # Explicitly set the internal attribute first, since the setter for style will
+        # access the applicator property.
+        self._applicator = None
+
+        self.style = style
         self.applicator = applicator
-        self.style = style.copy(applicator)
-        self.intrinsic = self.style.IntrinsicSize()
-        self.layout = self.style.Box(self)
 
         self._parent = None
         self._root = None
@@ -14,6 +16,40 @@ def __init__(self, style, applicator=None, children=None):
             for child in children:
                 self.add(child)
 
+    @property
+    def style(self):
+        """The node's style.
+
+        Assigning a style triggers an application of that style if an applicator has
+        already been assigned.
+        """
+        return self._style
+
+    @style.setter
+    def style(self, style):
+        self._style = style.copy()
+        self.intrinsic = self.style.IntrinsicSize()
+        self.layout = self.style.Box(self)
+
+        if self.applicator:
+            self.style._applicator = self.applicator
+
+    @property
+    def applicator(self):
+        """This node's applicator, which handles applying the style.
+
+        Assigning an applicator triggers an application of the node's style.
+        """
+        return self._applicator
+
+    @applicator.setter
+    def applicator(self, applicator):
+        self._applicator = applicator
+        self.style._applicator = applicator
+
+        if applicator:
+            applicator.node = self
+
     @property
     def root(self):
         """The root of the tree containing this node.

tests/test_choices.py

@@ -1,38 +1,14 @@
 from __future__ import annotations
 
-import sys
-from dataclasses import dataclass
-from unittest.mock import Mock
 from warnings import catch_warnings, filterwarnings
 
 import pytest
 
+from tests.utils import mock_attr, prep_style_class
 from travertino.colors import NAMED_COLOR, rgb
 from travertino.constants import GOLDENROD, NONE, REBECCAPURPLE, TOP
 from travertino.declaration import BaseStyle, Choices, validated_property
 
-if sys.version_info < (3, 10):
-    _DATACLASS_KWARGS = {"init": False}
-else:
-    _DATACLASS_KWARGS = {"kw_only": True}
-
-
-def prep_style_class(cls):
-    """Decorator to apply dataclass and mock a style class's apply method."""
-    return mock_apply(dataclass(**_DATACLASS_KWARGS)(cls))
-
-
-def mock_apply(cls):
-    """Only mock apply, without applying dataclass."""
-    orig_init = cls.__init__
-
-    def __init__(self, *args, **kwargs):
-        self.apply = Mock()
-        orig_init(self, *args, **kwargs)
-
-    cls.__init__ = __init__
-    return cls
-
 
 @prep_style_class
 class Style(BaseStyle):
@@ -56,7 +32,7 @@ class Style(BaseStyle):
 with catch_warnings():
     filterwarnings("ignore", category=DeprecationWarning)
 
-    @mock_apply
+    @mock_attr("apply")
     class DeprecatedStyle(BaseStyle):
         pass
 

tests/test_declaration.py

@@ -5,7 +5,7 @@
 
 import pytest
 
-from tests.test_choices import mock_apply, prep_style_class
+from tests.utils import mock_attr, prep_style_class
 from travertino.declaration import (
     BaseStyle,
     Choices,
@@ -51,7 +51,7 @@ class Style(BaseStyle):
 with catch_warnings():
     filterwarnings("ignore", category=DeprecationWarning)
 
-    @mock_apply
+    @mock_attr("apply")
     class DeprecatedStyle(BaseStyle):
         pass
 
@@ -90,6 +90,12 @@ class Sibling(BaseStyle):
     pass
 
 
+@prep_style_class
+@mock_attr("reapply")
+class MockedReapplyStyle(BaseStyle):
+    pass
+
+
 def test_invalid_style():
     with pytest.raises(ValueError):
         # Define an invalid initial value on a validated property
@@ -120,6 +126,15 @@ def test_create_and_copy(StyleClass):
     assert dup.implicit == VALUE3
 
 
+def test_deprecated_copy():
+    style = MockedReapplyStyle()
+
+    with pytest.warns(DeprecationWarning):
+        style_copy = style.copy(applicator=object())
+
+    style_copy.reapply.assert_called_once()
+
+
 @pytest.mark.parametrize("StyleClass", [Style, DeprecatedStyle])
 def test_reapply(StyleClass):
     style = StyleClass(explicit_const=VALUE2, implicit=VALUE3)

tests/test_node.py

@@ -1,12 +1,36 @@
+from unittest.mock import Mock
+
 import pytest
 
-from travertino.declaration import BaseStyle
+from tests.utils import mock_attr, prep_style_class
+from travertino.declaration import BaseStyle, Choices, validated_property
 from travertino.layout import BaseBox, Viewport
 from travertino.node import Node
 from travertino.size import BaseIntrinsicSize
 
 
+@prep_style_class
+@mock_attr("reapply")
 class Style(BaseStyle):
+    int_prop: int = validated_property(Choices(integer=True))
+
+    class IntrinsicSize(BaseIntrinsicSize):
+        pass
+
+    class Box(BaseBox):
+        pass
+
+    def layout(self, root, viewport):
+        # A simple layout scheme that allocates twice the viewport size.
+        root.layout.content_width = viewport.width * 2
+        root.layout.content_height = viewport.height * 2
+
+
+@prep_style_class
+class BrokenStyle(BaseStyle):
+    def reapply(self):
+        raise AttributeError("Missing attribute, node not ready for style application")
+
     class IntrinsicSize(BaseIntrinsicSize):
         pass
 
@@ -219,3 +243,131 @@ def test_clear():
         assert child.root == child
 
     assert node.children == []
+
+
+def test_create_with_no_applicator():
+    style = Style(int_prop=5)
+    node = Node(style=style)
+
+    # Style copies on assignment.
+    assert isinstance(node.style, Style)
+    assert node.style == style
+    assert node.style is not style
+
+    # Since no applicator has been assigned, style wasn't applied.
+    node.style.reapply.assert_not_called()
+
+
+def test_create_with_applicator():
+    style = Style(int_prop=5)
+    applicator = Mock()
+    node = Node(style=style, applicator=applicator)
+
+    # Style copies on assignment.
+    assert isinstance(node.style, Style)
+    assert node.style == style
+    assert node.style is not style
+
+    # Applicator assignment does *not* copy.
+    assert node.applicator is applicator
+    # Applicator gets a reference back to its node and to the style.
+    assert applicator.node is node
+    assert node.style._applicator is applicator
+
+    # Assigning a non-None applicator should always apply style.
+    node.style.reapply.assert_called_once()
+
+
+@pytest.mark.parametrize(
+    "node",
+    [
+        Node(style=Style()),
+        Node(style=Style(), applicator=Mock()),
+    ],
+)
+def test_assign_applicator(node):
+    node.style.reapply.reset_mock()
+
+    applicator = Mock()
+    node.applicator = applicator
+
+    # Applicator assignment does *not* copy.
+    assert node.applicator is applicator
+    # Applicator gets a reference back to its node and to the style.
+    assert applicator.node is node
+    assert node.style._applicator is applicator
+
+    # Assigning a non-None applicator should always apply style.
+    node.style.reapply.assert_called_once()
+
+
+@pytest.mark.parametrize(
+    "node",
+    [
+        Node(style=Style()),
+        Node(style=Style(), applicator=Mock()),
+    ],
+)
+def test_assign_applicator_none(node):
+    node.style.reapply.reset_mock()
+
+    node.applicator = None
+    assert node.applicator is None
+
+    # Should be updated on style as well
+    assert node.style._applicator is None
+    # Assigning None to applicator does not trigger reapply.
+    node.style.reapply.assert_not_called()
+
+
+def test_assign_style_with_applicator():
+    style_1 = Style(int_prop=5)
+    node = Node(style=style_1, applicator=Mock())
+
+    node.style.reapply.reset_mock()
+    style_2 = Style(int_prop=10)
+    node.style = style_2
+
+    # Style copies on assignment.
+    assert isinstance(node.style, Style)
+    assert node.style == style_2
+    assert node.style is not style_2
+
+    assert node.style != style_1
+
+    # Since an applicator has already been assigned, assigning style applies the style.
+    node.style.reapply.assert_called_once()
+
+
+def test_assign_style_with_no_applicator():
+    style_1 = Style(int_prop=5)
+    node = Node(style=style_1)
+
+    node.style.reapply.reset_mock()
+    style_2 = Style(int_prop=10)
+    node.style = style_2
+
+    # Style copies on assignment.
+    assert isinstance(node.style, Style)
+    assert node.style == style_2
+    assert node.style is not style_2
+
+    assert node.style != style_1
+
+    # Since no applicator was present, style should not be applied.
+    node.style.reapply.assert_not_called()
+
+
+def test_apply_before_node_is_ready():
+    style = BrokenStyle()
+    applicator = Mock()
+
+    with pytest.warns(RuntimeWarning):
+        node = Node(style=style)
+        node.applicator = applicator
+
+    with pytest.warns(RuntimeWarning):
+        node.style = BrokenStyle()
+
+    with pytest.warns(RuntimeWarning):
+        Node(style=style, applicator=applicator)