-
-
Notifications
You must be signed in to change notification settings - Fork 18
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
Changes from all commits
17c6213
6b6f27c
ddab78e
28ddb50
3cc9760
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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. |
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 | ||
|
||
|
@@ -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() | ||
|
@@ -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) | ||
|
||
|
@@ -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) | ||
|
@@ -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() | ||
|
@@ -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 | ||
|
@@ -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()) | ||
|
||
|
@@ -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) | ||
|
||
|
@@ -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() | ||
|
||
|
@@ -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(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
"""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()) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.