Make bindings which produce UnsetValue revert to default value for property #10189
Conversation
This changes the behavior of bindings slightly, in that previously a binding which produced `UnsetValue` (or a binding error) caused the value of any lower priority value of value to take effect. After this change, the binding reverts to the property's default value. This behavior more closely matches WPF and fixes #10110.
Local value bindings with a type of `BindingValue<T>` were not being validated.
| /// Checks whether a value is assigned to the property, or that there is a binding to the | ||
| /// property that is producing a value other than <see cref="AvaloniaProperty.UnsetValue"/>. | ||
| /// Returns true if <paramref name="property"/> is a styled property which has a value | ||
| /// assigned to it or a binding targeting it; otherwise false. |
There was a problem hiding this comment.
Due to the change in semantics, the meaning of IsSet had to change as well.
| owner.ClearLocalValue(property); | ||
| { | ||
| var effectiveValue = value.Value; | ||
| if (property.ValidateValue?.Invoke(effectiveValue) == false) |
There was a problem hiding this comment.
Fixes a bug here where we actually weren't validating the value in this case. Also added a unit test.
|
You can test this PR using the following package version. |
|
I've not run this code but it looks like exactly what I had in mind, so 👍 from me.
Not strictly related to this PR, but if Avalonia doesn't update the value of non-effective bindings, then what happens should they become effective later? Do we see a stale value, or is the current value retrieved from the source? |
Yes, when a binding becomes effective, it is subscribed, so the current value will be retrieved from the source. |
|
I think this is ready for review now. The only thing I'm not sure about is the behavior of bindings before they produce a value. Currently, there is an exception to the "one active binding subscription per property" rule, in that until a binding produces a value, multiple subscriptions can be active. This shouldn't be encountered in normal circumstances because standard bindings produce a value immediately in all cases except animation bindings. I started making a change whereby bindings that don't produce a value when subscribed cause the default value to be produced, but some of the animation tests started failing. Problem is that there are so few animation tests that I'm unsure whether this will cause a real problem with animations, or if it's just how the tests were written. Given the above, I think I'm going to leave this behavior as-is until it causes a problem, or we rewrite the animation stuff and add proper unit tests. |
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
What does the pull request do?
As mentioned in #10110, #8600 introduced a problem whereby if a binding produces
UnsetValue(or a binding error) then the binding is unsubscribed.This started happening because one of the aims of #8600 was to only have a subscription to one single active binding at a time 1, but historically our behavior when a binding produces
UnsetValuewas for a binding or value with a lower priority to take effect; requiring multiple bindings to be subscribed.As discussed in #10039, this historical behavior is different from WPF and I think can be changed. In WPF when a property produces
UnsetValue, the property value is simply set to the default value of the property ignoring any lower priority values.This PR makes that change. I'm leaving it as a draft for the moment to discuss the implications of this if any.
Breaking changes
Changes the behavior of bindings. If anyone's binding was producing
UnsetValueexpecting the property value to revert to the value from a style etc then this will break.Fixed issues
Fixes #10110
cc: @tomenscape for review/comments because this is something you've encountered.
Footnotes
Previously all bindings were subscribed, even if their value was not visible which caused performance problems in some cases ↩