From 412e059c66bf3d668b49a6ce9351dd552e04c9b4 Mon Sep 17 00:00:00 2001 From: Charles Whittington Date: Fri, 18 Oct 2024 11:20:39 -0400 Subject: [PATCH 01/20] Delay assigning applicator until after setting properties on copied style --- src/travertino/declaration.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/travertino/declaration.py b/src/travertino/declaration.py index c5b4642..a06d65d 100644 --- a/src/travertino/declaration.py +++ b/src/travertino/declaration.py @@ -326,8 +326,8 @@ 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) + dup._applicator = applicator return dup def __getitem__(self, name): From e45ff863038350c848f966fdf2afc91c66525042 Mon Sep 17 00:00:00 2001 From: Charles Whittington Date: Fri, 18 Oct 2024 11:31:56 -0400 Subject: [PATCH 02/20] Added changenote --- changes/224.bugfix.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 changes/224.bugfix.rst diff --git a/changes/224.bugfix.rst b/changes/224.bugfix.rst new file mode 100644 index 0000000..6b676b9 --- /dev/null +++ b/changes/224.bugfix.rst @@ -0,0 +1 @@ +Fixed a bug that could cause nodes/widgets to attempt to apply their styles before their implementation is available. From 922667300545e0931c187a972236a5efbfd78d4f Mon Sep 17 00:00:00 2001 From: Charles Whittington Date: Mon, 28 Oct 2024 22:44:01 -0400 Subject: [PATCH 03/20] Deprecated copy(applicator), style and applicator are now properties --- src/travertino/declaration.py | 11 +++++- src/travertino/node.py | 72 ++++++++++++++++++++++++++++++++++- tests/test_choices.py | 30 ++------------- tests/test_declaration.py | 6 +-- tests/test_node.py | 52 ++++++++++++++++++++++++- tests/utils.py | 29 ++++++++++++++ 6 files changed, 167 insertions(+), 33 deletions(-) create mode 100644 tests/utils.py diff --git a/src/travertino/declaration.py b/src/travertino/declaration.py index a06d65d..afe2e1c 100644 --- a/src/travertino/declaration.py +++ b/src/travertino/declaration.py @@ -327,7 +327,16 @@ def copy(self, applicator=None): "Create a duplicate of this style declaration." dup = self.__class__() dup.update(**self) - dup._applicator = applicator + + 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): diff --git a/src/travertino/node.py b/src/travertino/node.py index 73ab0c9..f52ea8d 100644 --- a/src/travertino/node.py +++ b/src/travertino/node.py @@ -1,7 +1,14 @@ +from warnings import filterwarnings, warn + +# Make sure deprecation warnings are shown by default +filterwarnings("default", category=DeprecationWarning) + + class Node: def __init__(self, style, applicator=None, children=None): self.applicator = applicator - self.style = style.copy(applicator) + self.style = style + self.intrinsic = self.style.IntrinsicSize() self.layout = self.style.Box(self) @@ -14,6 +21,69 @@ 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 getattr(self, "_style", None) + + @style.setter + def style(self, style): + if style is None: + self._style = None + return + + self._style = style.copy() + + if self.applicator: + self.style._applicator = self.applicator + + try: + self.style.reapply() + # Backwards compatibility for Toga + except AttributeError: + warn( + "Failed to apply style, when new style assigned and applicator " + "already present. Node should be sufficiently initialized to " + "apply style before it has both a style and an applicator.", + DeprecationWarning, + stacklevel=2, + ) + + @property + def applicator(self): + """This node's applicator, which handles applying the style. + + Assigning an applicator triggers an application of that style if a style has + already been assigned. + """ + return getattr(self, "_applicator", None) + + @applicator.setter + def applicator(self, applicator): + self._applicator = applicator + + if self.style is not None: + self.style._applicator = applicator + + if applicator: + + try: + self.style.reapply() + # Backwards compatibility for Toga + except AttributeError: + warn( + "Failed to apply style, when new applicator assigned and style " + "already present. Node should be sufficiently initialized to " + "apply style before it has both a style and an applicator.", + DeprecationWarning, + stacklevel=2, + ) + @property def root(self): """The root of the tree containing this node. diff --git a/tests/test_choices.py b/tests/test_choices.py index fc40fdf..e3f9bef 100644 --- a/tests/test_choices.py +++ b/tests/test_choices.py @@ -1,40 +1,16 @@ 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 apply_dataclass, mock_attr 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 +@apply_dataclass class Style(BaseStyle): none: str = validated_property(choices=Choices(NONE, REBECCAPURPLE), initial=NONE) allow_string: str = validated_property( @@ -56,7 +32,7 @@ class Style(BaseStyle): with catch_warnings(): filterwarnings("ignore", category=DeprecationWarning) - @mock_apply + @mock_attr("apply") class DeprecatedStyle(BaseStyle): pass diff --git a/tests/test_declaration.py b/tests/test_declaration.py index 62f4254..0f792f0 100644 --- a/tests/test_declaration.py +++ b/tests/test_declaration.py @@ -5,7 +5,7 @@ import pytest -from tests.test_choices import mock_apply, prep_style_class +from tests.utils import apply_dataclass, mock_attr from travertino.declaration import ( BaseStyle, Choices, @@ -22,7 +22,7 @@ DEFAULT_VALUE_CHOICES = Choices(VALUE1, VALUE2, VALUE3, integer=True) -@prep_style_class +@apply_dataclass class Style(BaseStyle): # Some properties with explicit initial values explicit_const: str | int = validated_property( @@ -51,7 +51,7 @@ class Style(BaseStyle): with catch_warnings(): filterwarnings("ignore", category=DeprecationWarning) - @mock_apply + @mock_attr("apply") class DeprecatedStyle(BaseStyle): pass diff --git a/tests/test_node.py b/tests/test_node.py index b9c2c0f..32f2b30 100644 --- a/tests/test_node.py +++ b/tests/test_node.py @@ -1,12 +1,17 @@ import pytest -from travertino.declaration import BaseStyle +from tests.utils import apply_dataclass, mock_attr +from travertino.declaration import BaseStyle, Choices, validated_property from travertino.layout import BaseBox, Viewport from travertino.node import Node from travertino.size import BaseIntrinsicSize +@apply_dataclass +@mock_attr("reapply") class Style(BaseStyle): + int_prop: int = validated_property(Choices(integer=True)) + class IntrinsicSize(BaseIntrinsicSize): pass @@ -219,3 +224,48 @@ def test_clear(): assert child.root == child assert node.children == [] + + +def test_style_and_applicator_assignment(): + style_1 = Style(int_prop=5) + node = Node(style=style_1) + + # Style copies on assignment. + assert isinstance(node.style, Style) + assert node.style == style_1 + assert node.style is not style_1 + + # Since no applicator has been assigned, style wasn't applied. + node.style.reapply.assert_not_called() + + applicator = object() + node.applicator = applicator + + # Applicator assignment does *not* copy. + assert node.applicator is applicator + # Should have also assigned to the style. + assert node.style._applicator is applicator + # Reapply should be called, now that we have both style and applicator. + node.style.reapply.assert_called_once() + + style_2 = Style(int_prop=6) + node.style = style_2 + + assert node.style == style_2 + # Since applicator is already assigned, assigning style triggers reapply. + node.style.reapply.assert_called_once() + + node.style.reapply.reset_mock() + + # Assigning None to applicator does not trigger reapply. + node.applicator = None + assert node.applicator is None + assert node.style._applicator is None + node.style.reapply.assert_not_called() + + node.style = None + assert node.style is None + + # There's no mock to test, but the fact that this doesn't raise an exception means + # it didn't try to call reapply. + node.applicator = object() diff --git a/tests/utils.py b/tests/utils.py new file mode 100644 index 0000000..49c4edf --- /dev/null +++ b/tests/utils.py @@ -0,0 +1,29 @@ +import sys +from dataclasses import dataclass +from unittest.mock import Mock + +if sys.version_info < (3, 10): + _DATACLASS_KWARGS = {"init": False} +else: + _DATACLASS_KWARGS = {"kw_only": True} + + +def apply_dataclass(cls): + """Decorator to apply dataclass and mock apply.""" + return mock_attr("apply")(dataclass(**_DATACLASS_KWARGS)(cls)) + + +def mock_attr(attr): + """Mock an arbitrary attribute of a class.""" + + def returned_decorator(cls): + orig_init = cls.__init__ + + def __init__(self, *args, **kwargs): + setattr(self, attr, Mock()) + orig_init(self, *args, **kwargs) + + cls.__init__ = __init__ + return cls + + return returned_decorator From 6967d4396de9493136fb4ed23f95af6b22e8e7db Mon Sep 17 00:00:00 2001 From: Charles Whittington Date: Mon, 28 Oct 2024 23:17:02 -0400 Subject: [PATCH 04/20] Added tests for deprecations --- tests/test_declaration.py | 7 +++++++ tests/test_node.py | 25 +++++++++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/tests/test_declaration.py b/tests/test_declaration.py index 0f792f0..dd739f7 100644 --- a/tests/test_declaration.py +++ b/tests/test_declaration.py @@ -120,6 +120,13 @@ def test_create_and_copy(StyleClass): assert dup.implicit == VALUE3 +@pytest.mark.parametrize("StyleClass", [Style, DeprecatedStyle]) +def test_deprecated_copy(StyleClass): + style = StyleClass(explicit_const=VALUE2) + with pytest.warns(DeprecationWarning): + style.copy(applicator=object()) + + @pytest.mark.parametrize("StyleClass", [Style, DeprecatedStyle]) def test_reapply(StyleClass): style = StyleClass(explicit_const=VALUE2, implicit=VALUE3) diff --git a/tests/test_node.py b/tests/test_node.py index 32f2b30..a19814c 100644 --- a/tests/test_node.py +++ b/tests/test_node.py @@ -24,6 +24,23 @@ def layout(self, root, viewport): root.layout.content_height = viewport.height * 2 +@apply_dataclass +class BrokenStyle(BaseStyle): + def reapply(self): + raise AttributeError + + 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 + + def test_create_leaf(): """A leaf can be created""" style = Style() @@ -269,3 +286,11 @@ def test_style_and_applicator_assignment(): # There's no mock to test, but the fact that this doesn't raise an exception means # it didn't try to call reapply. node.applicator = object() + + +def test_style_and_applicator_assignment_deprecated(): + style = BrokenStyle() + applicator = object() + + with pytest.warns(DeprecationWarning): + Node(style=style, applicator=applicator) From a628751bbfbf75444c1e05dabe8439b9cbbd73af Mon Sep 17 00:00:00 2001 From: Charles Whittington Date: Mon, 28 Oct 2024 23:22:12 -0400 Subject: [PATCH 05/20] More comprehensive deprecation test --- tests/test_node.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/test_node.py b/tests/test_node.py index a19814c..585c8ff 100644 --- a/tests/test_node.py +++ b/tests/test_node.py @@ -294,3 +294,13 @@ def test_style_and_applicator_assignment_deprecated(): with pytest.warns(DeprecationWarning): Node(style=style, applicator=applicator) + + with pytest.warns(DeprecationWarning): + node = Node(style=style) + node.applicator = applicator + + with pytest.warns(DeprecationWarning): + node = Node(style=style) + node.style = None + node.applicator = applicator + node.style = style From 9951cf779a8f09a23b7ae10bacf733ba67350b26 Mon Sep 17 00:00:00 2001 From: Charles Whittington Date: Mon, 28 Oct 2024 23:23:56 -0400 Subject: [PATCH 06/20] Put intrinsic and layout assignments in style property --- src/travertino/node.py | 5 ++--- tests/test_node.py | 4 +--- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/travertino/node.py b/src/travertino/node.py index f52ea8d..abbeb63 100644 --- a/src/travertino/node.py +++ b/src/travertino/node.py @@ -9,9 +9,6 @@ def __init__(self, style, applicator=None, children=None): self.applicator = applicator self.style = style - self.intrinsic = self.style.IntrinsicSize() - self.layout = self.style.Box(self) - self._parent = None self._root = None if children is None: @@ -38,6 +35,8 @@ def style(self, style): return self._style = style.copy() + self.intrinsic = self.style.IntrinsicSize() + self.layout = self.style.Box(self) if self.applicator: self.style._applicator = self.applicator diff --git a/tests/test_node.py b/tests/test_node.py index 585c8ff..c602288 100644 --- a/tests/test_node.py +++ b/tests/test_node.py @@ -300,7 +300,5 @@ def test_style_and_applicator_assignment_deprecated(): node.applicator = applicator with pytest.warns(DeprecationWarning): - node = Node(style=style) - node.style = None - node.applicator = applicator + node = Node(style=None, applicator=applicator) node.style = style From 68959a99a6dfae335ad1dfa2b524e4df8ccd4127 Mon Sep 17 00:00:00 2001 From: Charles Whittington Date: Mon, 28 Oct 2024 23:30:34 -0400 Subject: [PATCH 07/20] restore decorator name -- from a bit of refactoring I undid --- tests/test_choices.py | 4 ++-- tests/test_declaration.py | 4 ++-- tests/test_node.py | 6 +++--- tests/utils.py | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/test_choices.py b/tests/test_choices.py index e3f9bef..9eea0f8 100644 --- a/tests/test_choices.py +++ b/tests/test_choices.py @@ -4,13 +4,13 @@ import pytest -from tests.utils import apply_dataclass, mock_attr +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 -@apply_dataclass +@prep_style_class class Style(BaseStyle): none: str = validated_property(choices=Choices(NONE, REBECCAPURPLE), initial=NONE) allow_string: str = validated_property( diff --git a/tests/test_declaration.py b/tests/test_declaration.py index dd739f7..36269c8 100644 --- a/tests/test_declaration.py +++ b/tests/test_declaration.py @@ -5,7 +5,7 @@ import pytest -from tests.utils import apply_dataclass, mock_attr +from tests.utils import mock_attr, prep_style_class from travertino.declaration import ( BaseStyle, Choices, @@ -22,7 +22,7 @@ DEFAULT_VALUE_CHOICES = Choices(VALUE1, VALUE2, VALUE3, integer=True) -@apply_dataclass +@prep_style_class class Style(BaseStyle): # Some properties with explicit initial values explicit_const: str | int = validated_property( diff --git a/tests/test_node.py b/tests/test_node.py index c602288..70f598c 100644 --- a/tests/test_node.py +++ b/tests/test_node.py @@ -1,13 +1,13 @@ import pytest -from tests.utils import apply_dataclass, mock_attr +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 -@apply_dataclass +@prep_style_class @mock_attr("reapply") class Style(BaseStyle): int_prop: int = validated_property(Choices(integer=True)) @@ -24,7 +24,7 @@ def layout(self, root, viewport): root.layout.content_height = viewport.height * 2 -@apply_dataclass +@prep_style_class class BrokenStyle(BaseStyle): def reapply(self): raise AttributeError diff --git a/tests/utils.py b/tests/utils.py index 49c4edf..0649494 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -8,7 +8,7 @@ _DATACLASS_KWARGS = {"kw_only": True} -def apply_dataclass(cls): +def prep_style_class(cls): """Decorator to apply dataclass and mock apply.""" return mock_attr("apply")(dataclass(**_DATACLASS_KWARGS)(cls)) From a8b6b6edf1963c997f4cd5ff82d58804327da8e7 Mon Sep 17 00:00:00 2001 From: Charles Whittington Date: Wed, 30 Oct 2024 08:25:01 -0400 Subject: [PATCH 08/20] de-icked warning, put reapply logic in BaseStyle._applicator setter, applicator gets a ref to node, revamped tests --- src/travertino/declaration.py | 16 ++++ src/travertino/node.py | 47 +++--------- tests/test_node.py | 136 +++++++++++++++++++++++++++------- 3 files changed, 135 insertions(+), 64 deletions(-) diff --git a/src/travertino/declaration.py b/src/travertino/declaration.py index afe2e1c..9d8045b 100644 --- a/src/travertino/declaration.py +++ b/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 new applicator assigned and style " + "already present. Node should be sufficiently initialized to " + "apply style before it has both a style and an applicator. This" + "will be an exception in a future version.", + RuntimeWarning, + stacklevel=2, + ) + ###################################################################### # Interface that style declarations must define ###################################################################### diff --git a/src/travertino/node.py b/src/travertino/node.py index abbeb63..2766176 100644 --- a/src/travertino/node.py +++ b/src/travertino/node.py @@ -1,13 +1,7 @@ -from warnings import filterwarnings, warn - -# Make sure deprecation warnings are shown by default -filterwarnings("default", category=DeprecationWarning) - - class Node: - def __init__(self, style, applicator=None, children=None): - self.applicator = applicator + def __init__(self, style=None, applicator=None, children=None): self.style = style + self.applicator = applicator self._parent = None self._root = None @@ -25,8 +19,9 @@ def style(self): Assigning a style triggers an application of that style if an applicator has already been assigned. """ - - return getattr(self, "_style", None) + # Has to have a fallback, because it's accessed before its assignment in the + # setter for applicator + return self._style @style.setter def style(self, style): @@ -41,48 +36,24 @@ def style(self, style): if self.applicator: self.style._applicator = self.applicator - try: - self.style.reapply() - # Backwards compatibility for Toga - except AttributeError: - warn( - "Failed to apply style, when new style assigned and applicator " - "already present. Node should be sufficiently initialized to " - "apply style before it has both a style and an applicator.", - DeprecationWarning, - stacklevel=2, - ) - @property def applicator(self): """This node's applicator, which handles applying the style. - Assigning an applicator triggers an application of that style if a style has - already been assigned. + Assigning an applicator triggers an application of the node's style if a style + has already been assigned. """ return getattr(self, "_applicator", None) @applicator.setter def applicator(self, applicator): self._applicator = applicator + if applicator: + applicator.node = self if self.style is not None: self.style._applicator = applicator - if applicator: - - try: - self.style.reapply() - # Backwards compatibility for Toga - except AttributeError: - warn( - "Failed to apply style, when new applicator assigned and style " - "already present. Node should be sufficiently initialized to " - "apply style before it has both a style and an applicator.", - DeprecationWarning, - stacklevel=2, - ) - @property def root(self): """The root of the tree containing this node. diff --git a/tests/test_node.py b/tests/test_node.py index 70f598c..17d9228 100644 --- a/tests/test_node.py +++ b/tests/test_node.py @@ -1,3 +1,5 @@ +from unittest.mock import Mock + import pytest from tests.utils import mock_attr, prep_style_class @@ -27,7 +29,7 @@ def layout(self, root, viewport): @prep_style_class class BrokenStyle(BaseStyle): def reapply(self): - raise AttributeError + raise Exception class IntrinsicSize(BaseIntrinsicSize): pass @@ -243,62 +245,144 @@ def test_clear(): assert node.children == [] -def test_style_and_applicator_assignment(): - style_1 = Style(int_prop=5) - node = Node(style=style_1) +def test_create_with_only_style(): + 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_assign_only_style(): + style = Style(int_prop=5) + node = Node() + node.style = style # Style copies on assignment. assert isinstance(node.style, Style) - assert node.style == style_1 - assert node.style is not style_1 + 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() - applicator = object() + +def test_create_with_only_applicator(): + applicator = Mock() + + # No reapply should be attempted. If it were, it would raise an AttributeError as + # None has no reapply() method. + node = Node(applicator=applicator) + + # Applicator assignment does *not* copy. + assert node.applicator is applicator + + assert applicator.node is node + + +def test_assign_only_applicator(): + applicator = Mock() + node = Node() + + # No reapply should be attempted. If it were, it would raise an AttributeError as + # None has no reapply() method. node.applicator = applicator # Applicator assignment does *not* copy. assert node.applicator is applicator - # Should have also assigned to the style. + + +def test_create_with_style_and_applicator(): + style = Style(int_prop=5) + applicator = Mock() + node = Node(style=style, applicator=applicator) + + # With both style and applicator present, style should be applied. + node.style.reapply.assert_called_once() + # Should have also assigned the applicator the style. assert node.style._applicator is applicator - # Reapply should be called, now that we have both style and applicator. + + +def test_style_then_applicator(): + style = Style(int_prop=5) + applicator = Mock() + node = Node(style=style) + node.applicator = applicator + + # With both style and applicator present, style should be applied. node.style.reapply.assert_called_once() + # Should have also assigned the applicator the style. + assert node.style._applicator is applicator + - style_2 = Style(int_prop=6) - node.style = style_2 +def test_applicator_then_style(): + style = Style(int_prop=5) + applicator = Mock() + node = Node(applicator=applicator) + node.style = style - assert node.style == style_2 - # Since applicator is already assigned, assigning style triggers reapply. + # With both style and applicator present, style should be applied. node.style.reapply.assert_called_once() + # Should have also assigned the applicator the style. + assert node.style._applicator is applicator + - node.style.reapply.reset_mock() +@pytest.mark.parametrize( + "node", + [ + Node(), + Node(style=Style()), + Node(applicator=Mock()), + Node(style=Style(), applicator=Mock()), + ], +) +def test_assign_applicator_none(node): + if node.style is not None: + node.style.reapply.reset_mock() - # Assigning None to applicator does not trigger reapply. node.applicator = None assert node.applicator is None - assert node.style._applicator is None - node.style.reapply.assert_not_called() + if node.style is not 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() + + +@pytest.mark.parametrize( + "node", + [ + Node(), + Node(style=Style()), + Node(applicator=Mock()), + Node(style=Style(), applicator=Mock()), + ], +) +def test_assign_style_none(node): node.style = None assert node.style is None - # There's no mock to test, but the fact that this doesn't raise an exception means - # it didn't try to call reapply. - node.applicator = object() + # No reapply should be attempted. If it were, it would raise an AttributeError as + # None has no reapply() method. -def test_style_and_applicator_assignment_deprecated(): +def test_apply_before_node_is_ready(): style = BrokenStyle() - applicator = object() + applicator = Mock() - with pytest.warns(DeprecationWarning): + with pytest.warns(RuntimeWarning): Node(style=style, applicator=applicator) - with pytest.warns(DeprecationWarning): + with pytest.warns(RuntimeWarning): node = Node(style=style) node.applicator = applicator - with pytest.warns(DeprecationWarning): - node = Node(style=None, applicator=applicator) + with pytest.warns(RuntimeWarning): + node = Node(applicator=applicator) node.style = style From 7850926a56bc6bb3b4a4ab76388fd8071ee8ccf2 Mon Sep 17 00:00:00 2001 From: Charles Whittington Date: Wed, 30 Oct 2024 08:33:12 -0400 Subject: [PATCH 09/20] moved comment to the right place --- src/travertino/node.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/travertino/node.py b/src/travertino/node.py index 2766176..b852843 100644 --- a/src/travertino/node.py +++ b/src/travertino/node.py @@ -19,8 +19,6 @@ def style(self): Assigning a style triggers an application of that style if an applicator has already been assigned. """ - # Has to have a fallback, because it's accessed before its assignment in the - # setter for applicator return self._style @style.setter @@ -43,6 +41,8 @@ def applicator(self): Assigning an applicator triggers an application of the node's style if a style has already been assigned. """ + # Has to have a fallback, because it's accessed before its assignment in the + # setter for style return getattr(self, "_applicator", None) @applicator.setter From 426a29d8cc98bb5d03fd5525447452fabfe880b6 Mon Sep 17 00:00:00 2001 From: Charles Whittington Date: Wed, 30 Oct 2024 08:48:10 -0400 Subject: [PATCH 10/20] Fixed deprecated copy test --- tests/test_declaration.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/tests/test_declaration.py b/tests/test_declaration.py index 36269c8..753362a 100644 --- a/tests/test_declaration.py +++ b/tests/test_declaration.py @@ -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,11 +126,13 @@ def test_create_and_copy(StyleClass): assert dup.implicit == VALUE3 -@pytest.mark.parametrize("StyleClass", [Style, DeprecatedStyle]) -def test_deprecated_copy(StyleClass): - style = StyleClass(explicit_const=VALUE2) +def test_deprecated_copy(): + style = MockedReapplyStyle() + with pytest.warns(DeprecationWarning): - style.copy(applicator=object()) + style_copy = style.copy(applicator=object()) + + style_copy.reapply.assert_called_once() @pytest.mark.parametrize("StyleClass", [Style, DeprecatedStyle]) From 2bd31a03be6c8a5341b53305103028e5205108d5 Mon Sep 17 00:00:00 2001 From: Charles Whittington Date: Wed, 30 Oct 2024 18:27:58 -0400 Subject: [PATCH 11/20] Note: changed changenotes --- changes/224.bugfix.rst | 1 - changes/224.feature.rst | 1 + changes/224.removal.1.rst | 1 + changes/224.removal.2.rst | 1 + 4 files changed, 3 insertions(+), 1 deletion(-) delete mode 100644 changes/224.bugfix.rst create mode 100644 changes/224.feature.rst create mode 100644 changes/224.removal.1.rst create mode 100644 changes/224.removal.2.rst diff --git a/changes/224.bugfix.rst b/changes/224.bugfix.rst deleted file mode 100644 index 6b676b9..0000000 --- a/changes/224.bugfix.rst +++ /dev/null @@ -1 +0,0 @@ -Fixed a bug that could cause nodes/widgets to attempt to apply their styles before their implementation is available. diff --git a/changes/224.feature.rst b/changes/224.feature.rst new file mode 100644 index 0000000..ec5c45f --- /dev/null +++ b/changes/224.feature.rst @@ -0,0 +1 @@ +It is now possible to create a node without supplying an initial style. diff --git a/changes/224.removal.1.rst b/changes/224.removal.1.rst new file mode 100644 index 0000000..48dc554 --- /dev/null +++ b/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. (In normal usage, though, assigning an applicator to a style is handled automatically when styles and/or applicators are assigned to nodes.) diff --git a/changes/224.removal.2.rst b/changes/224.removal.2.rst new file mode 100644 index 0000000..15f1e4b --- /dev/null +++ b/changes/224.removal.2.rst @@ -0,0 +1 @@ +The mechanisms for assigning styles and applicators to nodes, and applying styles, has been reworked. A node will now attempt to apply its style as soon as it has both a style and an applicator assigned to it; this means you should wait to assign one or both until a 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. From 8cb132d76c6cb7cab8aff8e66ee95ab5a17329e0 Mon Sep 17 00:00:00 2001 From: Charles Whittington Date: Wed, 30 Oct 2024 18:33:56 -0400 Subject: [PATCH 12/20] Proofreading --- changes/224.removal.2.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changes/224.removal.2.rst b/changes/224.removal.2.rst index 15f1e4b..e29c292 100644 --- a/changes/224.removal.2.rst +++ b/changes/224.removal.2.rst @@ -1 +1 @@ -The mechanisms for assigning styles and applicators to nodes, and applying styles, has been reworked. A node will now attempt to apply its style as soon as it has both a style and an applicator assigned to it; this means you should wait to assign one or both until a 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. +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 has both a style and an applicator assigned to it; this means you should wait to assign one or both until a 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. From c12e4f572a1b47c5a711e623c7233bf9a76cc908 Mon Sep 17 00:00:00 2001 From: Charles Whittington Date: Thu, 31 Oct 2024 18:06:40 -0400 Subject: [PATCH 13/20] Reverted style default None arg, and removed logic checking for None style --- changes/224.feature.rst | 1 - src/travertino/node.py | 19 +++--- tests/test_node.py | 144 ++++++++++++++++++---------------------- 3 files changed, 71 insertions(+), 93 deletions(-) delete mode 100644 changes/224.feature.rst diff --git a/changes/224.feature.rst b/changes/224.feature.rst deleted file mode 100644 index ec5c45f..0000000 --- a/changes/224.feature.rst +++ /dev/null @@ -1 +0,0 @@ -It is now possible to create a node without supplying an initial style. diff --git a/src/travertino/node.py b/src/travertino/node.py index b852843..06306c4 100644 --- a/src/travertino/node.py +++ b/src/travertino/node.py @@ -1,5 +1,9 @@ class Node: - def __init__(self, style=None, applicator=None, children=None): + 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 @@ -23,10 +27,6 @@ def style(self): @style.setter def style(self, style): - if style is None: - self._style = None - return - self._style = style.copy() self.intrinsic = self.style.IntrinsicSize() self.layout = self.style.Box(self) @@ -41,19 +41,16 @@ def applicator(self): Assigning an applicator triggers an application of the node's style if a style has already been assigned. """ - # Has to have a fallback, because it's accessed before its assignment in the - # setter for style - return getattr(self, "_applicator", None) + return self._applicator @applicator.setter def applicator(self, applicator): self._applicator = applicator + self.style._applicator = applicator + if applicator: applicator.node = self - if self.style is not None: - self.style._applicator = applicator - @property def root(self): """The root of the tree containing this node. diff --git a/tests/test_node.py b/tests/test_node.py index 17d9228..1df2ea3 100644 --- a/tests/test_node.py +++ b/tests/test_node.py @@ -245,7 +245,7 @@ def test_clear(): assert node.children == [] -def test_create_with_only_style(): +def test_create_with_no_applicator(): style = Style(int_prop=5) node = Node(style=style) @@ -258,131 +258,113 @@ def test_create_with_only_style(): node.style.reapply.assert_not_called() -def test_assign_only_style(): +def test_create_with_applicator(): style = Style(int_prop=5) - node = Node() - node.style = style + 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 - # Since no applicator has been assigned, style wasn't applied. - node.style.reapply.assert_not_called() - - -def test_create_with_only_applicator(): - applicator = Mock() - - # No reapply should be attempted. If it were, it would raise an AttributeError as - # None has no reapply() method. - node = 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 - -def test_assign_only_applicator(): - applicator = Mock() - node = Node() - - # No reapply should be attempted. If it were, it would raise an AttributeError as - # None has no reapply() method. - node.applicator = applicator - - # Applicator assignment does *not* copy. - assert node.applicator is applicator - - -def test_create_with_style_and_applicator(): - style = Style(int_prop=5) - applicator = Mock() - node = Node(style=style, applicator=applicator) - - # With both style and applicator present, style should be applied. + # Assigning a non-None applicator should always apply style. node.style.reapply.assert_called_once() - # Should have also assigned the applicator the style. - assert node.style._applicator is applicator -def test_style_then_applicator(): - style = Style(int_prop=5) +@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 = Node(style=style) node.applicator = applicator - # With both style and applicator present, style should be applied. - node.style.reapply.assert_called_once() - # Should have also assigned the applicator the 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 - -def test_applicator_then_style(): - style = Style(int_prop=5) - applicator = Mock() - node = Node(applicator=applicator) - node.style = style - - # With both style and applicator present, style should be applied. + # Assigning a non-None applicator should always apply style. node.style.reapply.assert_called_once() - # Should have also assigned the applicator the style. - assert node.style._applicator is applicator @pytest.mark.parametrize( "node", [ - Node(), Node(style=Style()), - Node(applicator=Mock()), Node(style=Style(), applicator=Mock()), ], ) def test_assign_applicator_none(node): - if node.style is not None: - node.style.reapply.reset_mock() + node.style.reapply.reset_mock() node.applicator = None assert node.applicator is None - if node.style is not 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() + # 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() -@pytest.mark.parametrize( - "node", - [ - Node(), - Node(style=Style()), - Node(applicator=Mock()), - Node(style=Style(), applicator=Mock()), - ], -) -def test_assign_style_none(node): - node.style = None - assert node.style is None +def test_assign_style_with_applicator(): + style_1 = Style(int_prop=5) + node = Node(style=style_1, applicator=Mock()) - # No reapply should be attempted. If it were, it would raise an AttributeError as - # None has no reapply() method. + 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(style=style, applicator=applicator) - with pytest.warns(RuntimeWarning): node = Node(style=style) node.applicator = applicator with pytest.warns(RuntimeWarning): - node = Node(applicator=applicator) - node.style = style + Node(style=style, applicator=applicator) From 132cf85f8a704b184ee8d13830b99f936091af63 Mon Sep 17 00:00:00 2001 From: Charles Whittington Date: Thu, 31 Oct 2024 18:11:16 -0400 Subject: [PATCH 14/20] Tweaked doc string and mentioned self.node reference --- changes/224.misc.rst | 1 + src/travertino/node.py | 3 +-- 2 files changed, 2 insertions(+), 2 deletions(-) create mode 100644 changes/224.misc.rst diff --git a/changes/224.misc.rst b/changes/224.misc.rst new file mode 100644 index 0000000..ef86edb --- /dev/null +++ b/changes/224.misc.rst @@ -0,0 +1 @@ +An applicator, once assigned to a node, now receives a reference to its node (as self.node). diff --git a/src/travertino/node.py b/src/travertino/node.py index 06306c4..bed40fb 100644 --- a/src/travertino/node.py +++ b/src/travertino/node.py @@ -38,8 +38,7 @@ def style(self, style): def applicator(self): """This node's applicator, which handles applying the style. - Assigning an applicator triggers an application of the node's style if a style - has already been assigned. + Assigning an applicator triggers an application of the node's style. """ return self._applicator From 415c531fb988f3f8cc68b16d51e590a5d8728760 Mon Sep 17 00:00:00 2001 From: Charles Whittington Date: Thu, 31 Oct 2024 18:20:28 -0400 Subject: [PATCH 15/20] One more check for unready node --- tests/test_node.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/test_node.py b/tests/test_node.py index 1df2ea3..485d8be 100644 --- a/tests/test_node.py +++ b/tests/test_node.py @@ -366,5 +366,8 @@ def test_apply_before_node_is_ready(): node = Node(style=style) node.applicator = applicator + with pytest.warns(RuntimeWarning): + node.style = BrokenStyle() + with pytest.warns(RuntimeWarning): Node(style=style, applicator=applicator) From 5799f1cc52f6fa452c489b0fab5ab53380582186 Mon Sep 17 00:00:00 2001 From: Charles Whittington Date: Thu, 31 Oct 2024 18:26:12 -0400 Subject: [PATCH 16/20] One more wording fix... --- src/travertino/declaration.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/travertino/declaration.py b/src/travertino/declaration.py index 9d8045b..1e0aa36 100644 --- a/src/travertino/declaration.py +++ b/src/travertino/declaration.py @@ -305,10 +305,10 @@ def _applicator(self, value): # implementation is available. except Exception: warn( - "Failed to apply style, when new applicator assigned and style " - "already present. Node should be sufficiently initialized to " - "apply style before it has both a style and an applicator. This" - "will be an exception in a future version.", + "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, ) From 6d5569859e1a8fc980b3fc7a38367b0b38b6cd59 Mon Sep 17 00:00:00 2001 From: Charles Whittington Date: Sat, 2 Nov 2024 20:54:24 -0400 Subject: [PATCH 17/20] Update change note to match code changes --- changes/224.removal.2.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changes/224.removal.2.rst b/changes/224.removal.2.rst index e29c292..399240f 100644 --- a/changes/224.removal.2.rst +++ b/changes/224.removal.2.rst @@ -1 +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 has both a style and an applicator assigned to it; this means you should wait to assign one or both until a 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. +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 wait to do so until a 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. (Assiging a new style will also trigger a reapply, as long as the applicator is already present.) From 3084327b0e64f306d86093952aa958480ad7160c Mon Sep 17 00:00:00 2001 From: Charles Whittington Date: Sun, 3 Nov 2024 21:45:31 -0500 Subject: [PATCH 18/20] Apply suggested changenote tweak Co-authored-by: Russell Keith-Magee --- changes/224.removal.2.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changes/224.removal.2.rst b/changes/224.removal.2.rst index 399240f..caa1a23 100644 --- a/changes/224.removal.2.rst +++ b/changes/224.removal.2.rst @@ -1 +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 wait to do so until a 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. (Assiging a new style will also trigger a reapply, as long as the applicator is already present.) +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. Assigning a new style will also trigger a reapply, as long as the applicator is already present. From 603adbc834bf7dc5ddb8c14785ab98e3dd0d99f1 Mon Sep 17 00:00:00 2001 From: Charles Whittington Date: Sun, 3 Nov 2024 21:59:46 -0500 Subject: [PATCH 19/20] Moved comment about assigning new style to a separate bugfix changenote --- changes/224.bugfix.rst | 1 + changes/224.removal.1.rst | 2 +- changes/224.removal.2.rst | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) create mode 100644 changes/224.bugfix.rst diff --git a/changes/224.bugfix.rst b/changes/224.bugfix.rst new file mode 100644 index 0000000..0ddcf45 --- /dev/null +++ b/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. diff --git a/changes/224.removal.1.rst b/changes/224.removal.1.rst index 48dc554..a25da88 100644 --- a/changes/224.removal.1.rst +++ b/changes/224.removal.1.rst @@ -1 +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. (In normal usage, though, assigning an applicator to a style is handled automatically when styles and/or applicators are assigned to nodes.) +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. diff --git a/changes/224.removal.2.rst b/changes/224.removal.2.rst index caa1a23..377527f 100644 --- a/changes/224.removal.2.rst +++ b/changes/224.removal.2.rst @@ -1 +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. Assigning a new style will also trigger a reapply, as long as the applicator is already present. +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. From fb33dc1164ac07f7f30f3cc8e87d4a410eb0d835 Mon Sep 17 00:00:00 2001 From: Charles Whittington Date: Sun, 3 Nov 2024 22:04:43 -0500 Subject: [PATCH 20/20] Raise instance/subclass --- tests/test_node.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_node.py b/tests/test_node.py index 485d8be..c281eb1 100644 --- a/tests/test_node.py +++ b/tests/test_node.py @@ -29,7 +29,7 @@ def layout(self, root, viewport): @prep_style_class class BrokenStyle(BaseStyle): def reapply(self): - raise Exception + raise AttributeError("Missing attribute, node not ready for style application") class IntrinsicSize(BaseIntrinsicSize): pass