Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assign node to applicator before reapply can happen #232

Merged
merged 5 commits into from
Nov 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changes/232.misc.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Applicator gets its reference to its node *before* the applicator is assigned to that node's style.
13 changes: 11 additions & 2 deletions src/travertino/node.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,21 @@ def applicator(self):

@applicator.setter
def applicator(self, applicator):
self._applicator = applicator
self.style._applicator = applicator
if self.applicator:
# If an existing applicator is present, clear its reference to this node.
self.applicator.node = None

if applicator:
# This needs to happen *before* assigning the applicator to the style,
# below, because as part of receiving the applicator, the style will
# reapply itself. How this happens will vary with applicator
# implementation, but will probably need access to the node.
applicator.node = self

self._applicator = applicator
# This triggers style.reapply():
self.style._applicator = applicator
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're thinking about the edge cases: if there's an existing applicator, should we be clearing the node reference on that applicator? I don't think it will make any difference in practice right now, but for symmetry, it couldn't hurt.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also - given this bit us, it would be worth adding a quick comment explaining why the order is significant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hadn't thought of that, that's a good point.


@property
def root(self):
"""The root of the tree containing this node.
Expand Down
58 changes: 58 additions & 0 deletions tests/test_node.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
from unittest.mock import Mock
from warnings import catch_warnings, filterwarnings

import pytest

Expand Down Expand Up @@ -43,6 +44,17 @@ def layout(self, root, viewport):
root.layout.content_height = viewport.height * 2


class AttributeTestStyle(BaseStyle):
class IntrinsicSize(BaseIntrinsicSize):
pass

class Box(BaseBox):
pass

def reapply(self):
assert self._applicator.node.style is self


def test_create_leaf():
"""A leaf can be created"""
style = Style()
Expand Down Expand Up @@ -246,6 +258,7 @@ def test_clear():


def test_create_with_no_applicator():
"""A node can be created without an applicator."""
style = Style(int_prop=5)
node = Node(style=style)

Expand All @@ -259,6 +272,7 @@ def test_create_with_no_applicator():


def test_create_with_applicator():
"""A node can be created with an applicator."""
style = Style(int_prop=5)
applicator = Mock()
node = Node(style=style, applicator=applicator)
Expand Down Expand Up @@ -286,6 +300,7 @@ def test_create_with_applicator():
],
)
def test_assign_applicator(node):
"""A node can be assigned an applicator after creation."""
node.style.reapply.reset_mock()

applicator = Mock()
Expand All @@ -309,6 +324,7 @@ def test_assign_applicator(node):
],
)
def test_assign_applicator_none(node):
"""A node can have its applicator set to None."""
node.style.reapply.reset_mock()

node.applicator = None
Expand All @@ -320,7 +336,34 @@ def test_assign_applicator_none(node):
node.style.reapply.assert_not_called()


def assign_new_applicator():
"""Assigning a new applicator clears reference to node on the old applicator."""
applicator_1 = Mock()
node = Node(style=Style(), applicator=applicator_1)

assert applicator_1.node is node

applicator_2 = Mock()
node.applicator = applicator_2

assert applicator_1.node is None
assert applicator_2.node is node


def assign_new_applicator_none():
"""Assigning None to applicator clears reference to node on the old applicator."""
applicator = Mock()
node = Node(style=Style(), applicator=applicator)

assert applicator.node is node

node.applicator = None

assert applicator.node is None


def test_assign_style_with_applicator():
"""Assigning a new style triggers a reapply if an applicator is already present."""
style_1 = Style(int_prop=5)
node = Node(style=style_1, applicator=Mock())

Expand All @@ -340,6 +383,7 @@ def test_assign_style_with_applicator():


def test_assign_style_with_no_applicator():
"""Assigning a new style doesn't trigger a reapply if an applicator isn't present."""
style_1 = Style(int_prop=5)
node = Node(style=style_1)

Expand All @@ -359,6 +403,7 @@ def test_assign_style_with_no_applicator():


def test_apply_before_node_is_ready():
"""Triggering a reapply raises a warning if the node is not ready to apply style."""
style = BrokenStyle()
applicator = Mock()

Expand All @@ -371,3 +416,16 @@ def test_apply_before_node_is_ready():

with pytest.warns(RuntimeWarning):
Node(style=style, applicator=applicator)


def test_applicator_has_node_reference():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It took me a moment to work out why this test was different to the last assertion in test_apply_before_node_is_ready, except that it wasn't actually asserting that the warning was raised. A comment on the fact that AttributeTestStyle asserts that it is re-applied would be helpful.

"""Applicator should have a reference to its node before style is first applied."""

# We can't just check it after creating the widget, because at that point the
# reapply will have already happened. AttributeTestStyle has a reapply() method
# that asserts the reference trail of style -> applicator -> node -> style is
# already intact at the point that reapply is called.

with catch_warnings():
filterwarnings("error", category=RuntimeWarning)
Node(style=AttributeTestStyle(), applicator=Mock())