-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[C] reset the BindingContext on template change #998
Conversation
Unit tests aren't happy. |
@@ -27,5 +27,31 @@ public ExpectedView() | |||
{ | |||
} | |||
} | |||
|
|||
public class MyTemplate : StackLayout |
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.
Huh. This formatting is correct but the rest of the file uses spaces..
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.
¯_(ツ)_/¯
}; | ||
cv.BindingContext = "Foo"; | ||
|
||
Assume.That(label.Text, Is.EqualTo("Foo")); |
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.
Cool. So you use an assume to indicate this is not the test but an invariant of the the test itself.
if (oldValue == null) | ||
return; | ||
|
||
base.OnControlTemplateChanged(oldValue, newValue); |
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.
Why not have this before line 28? Is it always the case that the base class will similarly do nothing if the oldValue
is null?
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.
safer, in case the base is modified one day
base.OnControlTemplateChanged(oldValue, newValue); | ||
View content = Content; | ||
ControlTemplate controlTemplate = ControlTemplate; | ||
if (content != null && controlTemplate != null) |
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.
Why the trap for content == null
? Seems TemplateUtilities.cs
L122 ensures that's the case. Should we throw instead?
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.
that code is, on purpose, a line-by-line copy of OnBindingContextChanged
* [C] reset the BindingContext on template change * fix test
Description of Change
When we remove an old template before setting a new one, all Bindings are unapplied (as part of unparenting).
This fix force a setting back the binding context when the an existing ControlTemplate is swapped for another one.
Bugs Fixed
API Changes
/
Behavioral Changes
/
PR Checklist