Assign node to applicator before reapply can happen #232
Conversation
| if applicator: | ||
| applicator.node = self | ||
|
|
||
| self._applicator = applicator | ||
| self.style._applicator = applicator |
There was a problem hiding this comment.
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.
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.
I hadn't thought of that, that's a good point.
| @@ -371,3 +383,12 @@ 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.
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.
Oops. Working on beeware/toga#2937 led me to notice I'd missed an important part of the necessary order here. The applicator should know what its node is before the applicator is assigned to the style, so it will have the reference when reapplying happens.
PR Checklist: