Fixed BindingEvaluator to properly support CompiledBinding in Avalonia 12 CR:xfortin - #623
Conversation
There was a problem hiding this comment.
Pull request overview
Updates BindingEvaluator for Avalonia 12 compiled bindings.
Changes:
- Matches the
CompiledBindingbase type. - Applies support to simple and intermediate binding paths.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🤖 Pull request was approved automatically: the AI review is complete and all its review threads are resolved. 🎉
Integration Details
{
"deliveryId": "2ab11460-9bda-11f1-9dbd-63e804826213",
"headSha": "912692fa7d99b436978da618f3d77f5cd670976e",
"reviewer": "copilot-pull-request-reviewer[bot]"
}There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/Devolutions.AvaloniaControls/Helpers/BindingEvaluator.cs:279
- Routing ordinary
CompiledBindingpaths through the fast getters changes failure behavior: these generated delegates do not catch exceptions thrown by property accessors, while the previous framework-delegated formatted and raw getters caught them and returned empty/null results. A throwing getter can now escape into grouping, sorting, or selection code. Preserve the delegated failure semantics by catching invocation failures in every fast getter/expression (returning unset/empty as appropriate), or keep ordinary compiled bindings on a guarded path.
CompiledBinding c when c.Converter is null
&& c.StringFormat is null
&& c.FallbackValue == AvaloniaProperty.UnsetValue
&& c.TargetNullValue == AvaloniaProperty.UnsetValue
&& c.Source == AvaloniaProperty.UnsetValue => c.Path?.ToString(),
src/Devolutions.AvaloniaControls/Helpers/BindingEvaluator.cs:231
Expression.Equalcan bind a receiver type's overloadedoperator ==, so a live object that compares equal to null is incorrectly treated as a missing path segment. Null propagation should use identity for reference receivers; retain equality only for nullable value types.
Expression.Equal(receiverValue, Expression.Constant(null, receiver.Type)),
There was a problem hiding this comment.
🤖 Pull request was approved automatically: the AI review is complete and all its review threads are resolved. 🎉
Integration Details
{
"deliveryId": "9dd7fdf0-9bda-11f1-9e97-dd075c0ae0ac",
"headSha": "e9093ddcecfb163e03c8bb36638d104900da55bf",
"reviewer": "copilot-pull-request-reviewer[bot]"
}There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/Devolutions.AvaloniaControls/Helpers/BindingEvaluator.cs:591
- This broader match also sends programmatically created
CompiledBindinginstances with converters throughBuildIntermediateGetter. A converter may validly returnAvaloniaProperty.UnsetValue; that method only checks for unset before conversion, so it later stringifies the sentinel instead of preserving the framework binding result. Keep converter-bearing compiled bindings on the delegated path unless the post-conversion sentinel is handled explicitly.
case CompiledBinding c:
src/Devolutions.AvaloniaControls/Helpers/BindingEvaluator.cs:241
- Use reference equality when checking reference-type path segments.
Expression.Equalcan bind a model's overloadedoperator ==, so a non-null receiver may be treated as missing (or the operator may throw) instead of evaluating the next segment. Nullable value types still requireExpression.Equal, but reference types should useExpression.ReferenceEqual.
Expression.Equal(receiverValue, Expression.Constant(null, receiver.Type)),
There was a problem hiding this comment.
🤖 Pull request was approved automatically: the AI review is complete and all its review threads are resolved. 🎉
Integration Details
{
"deliveryId": "0811bc80-9bfc-11f1-9368-0f7905954733",
"headSha": "cc7b91ecb3be8fd969ef0ef9779b98c0dde9f62a",
"reviewer": "copilot-pull-request-reviewer[bot]"
}BindingEvaluator to properly support CompiledBinding in Avalonia 12BindingEvaluator to properly support CompiledBinding in Avalonia 12 CR:xfortin
|
Copilot Re-review to trigger approval |
No additional code changes are needed from my side for this thread. The current branch already contains the BindingEvaluator fixes and tests; please re-run the Copilot PR review flow in GitHub to trigger approval. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/Devolutions.AvaloniaControls/Helpers/BindingEvaluator.cs:590
CompiledBindingPath.ToString()is not a reliable indication that the path contains only CLR property segments. Avalonia path elements such as method delegates and visual ancestors do not overrideToString(), so their default type names (for exampleAvalonia.Data.MethodAsDelegateElement) passIsSimpleDotPath. The formatted intermediate path then feeds that text toBuildPropertyGetter, whose construction exception escapesBuildFormattedGetter/BuildFormattedGetterExpressioninstead of falling back to the framework evaluator. Restrict this optimization to paths verified as property-only, or make failed intermediate getter construction returnfalseso these bindings use the proxy path.
case CompiledBinding c:
{
string? pathString = c.Path?.ToString();
if (string.IsNullOrEmpty(pathString) || !IsSimpleDotPath(pathString))
src/Devolutions.AvaloniaControls/Helpers/BindingEvaluator.cs:290
- This newly routes ordinary simple
CompiledBindinginstances through direct expression/property invocation. Unlike the previous framework-delegated path, neither fast formatted getter catches exceptions thrown by a source property getter, and the same applies to the raw fast paths. A throwing model getter will now escape into grouping/filtering callers instead of yielding the binding's unset/fallback result. Preserve framework binding error semantics by catching invocation failures in all fast paths or by excluding these bindings from the unguarded path.
CompiledBinding c when c.Converter is null
&& c.StringFormat is null
&& c.FallbackValue == AvaloniaProperty.UnsetValue
&& c.TargetNullValue == AvaloniaProperty.UnsetValue
&& c.Source == AvaloniaProperty.UnsetValue => c.Path?.ToString(),
src/Devolutions.AvaloniaControls/Helpers/BindingEvaluator.cs:243
Expression.Equalcan bind a receiver type's overloaded==operator. Consequently, a non-null intermediate object whose custom operator considers itself equal to null is incorrectly converted toUnsetValue. Use reference identity for reference types (andHasValuefor nullable value types) so path traversal tests actual nullness.
Expression.Condition(Expression.Equal(receiverValue, Expression.Constant(null, receiver.Type)),
Expression.Constant(AvaloniaProperty.UnsetValue, typeof(object)),
BuildNullSafePropertyAccess(Expression.PropertyOrField(receiverValue, propertyNames[index]), propertyNames, index + 1)));
src/Devolutions.AvaloniaControls/Helpers/BindingEvaluator.cs:537
- This handles
UnsetValueonly before conversion. AnIValueConverteris also allowed to returnAvaloniaProperty.UnsetValue; that result currently falls through toStringFormat/ToString()rather than selectingFallbackValue. Recheck the converter result before target-null and formatting logic.
if (value == AvaloniaProperty.UnsetValue)
{
return fallbackString ?? string.Empty;
}
There was a problem hiding this comment.
🤖 Pull request was approved automatically: the AI review is complete and all its review threads are resolved. 🎉
Integration Details
{
"deliveryId": "91336f40-9c06-11f1-8969-eac9843814a3",
"headSha": "493be1035c0561bb542ff0f7d1bcc63b08ae1e63",
"reviewer": "copilot-pull-request-reviewer[bot]"
}
No description provided.