Binding System refactor #13970
Merged
Binding System refactor #13970
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- `ExpressionObserver` has been removed and its functionality merged with `BindingExpression` - `BindingExpression` handles all types of `BindingMode` itself; doesn't require `BindingOperations.Apply` to set up a separate observable for `TwoWay/`OneWayToSource` bindings - This allows us to fix some long-standing issues with `OneWayToSource` bindings - Expression nodes have been refactored - No longer split between `Avalonia.Base` and `Avalonia.Markup` - Categorize them according to whether they use reflection or not A few tests are failing around binding warnings: this is because the next step here is to fix binding warnings.
Null is a theoretically valid value for `Source`; setting it to null shouldn't mean "use the data context".
Previously, `CompiledBindingPathBuilder` didn't have a `TemplatedParent` method and instead the XAML compiler rewrite templated parent bindings to be a `$self.TemplateParent` property binding. resulting in extraneous logs. Add a constructor with an `apiVersion` to `CompiledBindingPathBuilder` which will be used by newer versions of the XAML compiler, and if a usage is detected using an `apiVersion` of 0, then upgrade `$self.TemplatedParent` to use a `TemplatedParentPathElement`.
...on elements which aren't yet rooted.
"Could not convert" instead of "Cannot convert".
A bit of a hack as we'd ideally not be using reflection when using compiled bindings.
Should only be used for tests.
And also use "identity equals" where value types and strings use `object.Equals` and reference types use `object.ReferenceEquals`.
Removed deleted files from csproj that were re-added due to indentation changes.
And `UntypedBindingExpressionBase`.
This matches WPF's API.
maxkatz6
added
enhancement
feature
and removed
customer-priority
2 tasks
This was referenced Feb 23, 2024
grokys
added a commit
that referenced
this pull request
Feb 29, 2024
Certain conversions rely on type converters, which were disabled in compiled bindings since #13970 due to warnings that type converters are not trimming friendly. Ideally we'd be generating the type conversion logic in the XAML compiler, but in reality the problem with type converters and trimming is limited to type converters with generics, which is an edge case. For the moment re-enable the usage of type converters in compiled bindings until we implement generating the conversion code in the XAML compiler.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Feb 29, 2024
* Update ncrunch config. * Add tests for converting strings to brushes. * Make complied bindings use TypeConverters. Certain conversions rely on type converters, which were disabled in compiled bindings since #13970 due to warnings that type converters are not trimming friendly. Ideally we'd be generating the type conversion logic in the XAML compiler, but in reality the problem with type converters and trimming is limited to type converters with generics, which is an edge case. For the moment re-enable the usage of type converters in compiled bindings until we implement generating the conversion code in the XAML compiler.
grokys
added a commit
that referenced
this pull request
May 14, 2024
Usually it is not necessary to provide the target property when creating a `BindingExpression` because the property will be assigned when the binding expression is attached to the target in `BindingExpressionBase.Attach`. This is however one case where `Attach` is not called: when the obsolete `binding.Initiate` method is called and then an observable is read from the `InstancedBinding` without the binding actually being attached to the target object. In this case, prior to the binding refactor in #13970 the value produced by the observable was still converted to the target type. After #13970, because the target property (and hence the target type) is not yet set, the conversion is to the target type is no longer done. `DataGrid` uses this obsolete method when editing cells, causing #15081. Ideally we'd fix that in `DataGrid` but I'm not happy making this change so close to 11.1, so instead fix this use-case to behave as before. Fixes #15081
github-merge-queue bot
pushed a commit
that referenced
this pull request
May 14, 2024
* Added failing tests for #15081. * Provide target property in BindingExpression ctor. Usually it is not necessary to provide the target property when creating a `BindingExpression` because the property will be assigned when the binding expression is attached to the target in `BindingExpressionBase.Attach`. This is however one case where `Attach` is not called: when the obsolete `binding.Initiate` method is called and then an observable is read from the `InstancedBinding` without the binding actually being attached to the target object. In this case, prior to the binding refactor in #13970 the value produced by the observable was still converted to the target type. After #13970, because the target property (and hence the target type) is not yet set, the conversion is to the target type is no longer done. `DataGrid` uses this obsolete method when editing cells, causing #15081. Ideally we'd fix that in `DataGrid` but I'm not happy making this change so close to 11.1, so instead fix this use-case to behave as before. Fixes #15081
grokys
added a commit
that referenced
this pull request
May 14, 2024
When #13970 was written, [a check](https://github.com/AvaloniaUI/Avalonia/pull/13970/files#diff-cfb25a491b9452e1815aa2c0d71465aaf81e99792a88a04a1a2ed572fd1930ffR60) was added to ensure that nested `BindingNotification`s didn't happen, and the refactor was written with the assumption that they wouldn't happen. The problem is that they _do_ happen: when a source object implements both `INotifyDataErrorInfo` and had data annotations, then the nested data validation plugins would each wrap the value coming from the previous plugin in a new `BindingNotification`, resulting in nested `BindingNotifications`. This adds support for nested binding notifications back in - even though IMO nesting binding notifications is a bug, if we're doing it and we previously supported it then we should continue to support it. Fixes #15201
github-merge-queue bot
pushed a commit
that referenced
this pull request
May 14, 2024
* Added failing test for #15201. * Handle nested BindingNotifications. When #13970 was written, [a check](https://github.com/AvaloniaUI/Avalonia/pull/13970/files#diff-cfb25a491b9452e1815aa2c0d71465aaf81e99792a88a04a1a2ed572fd1930ffR60) was added to ensure that nested `BindingNotification`s didn't happen, and the refactor was written with the assumption that they wouldn't happen. The problem is that they _do_ happen: when a source object implements both `INotifyDataErrorInfo` and had data annotations, then the nested data validation plugins would each wrap the value coming from the previous plugin in a new `BindingNotification`, resulting in nested `BindingNotifications`. This adds support for nested binding notifications back in - even though IMO nesting binding notifications is a bug, if we're doing it and we previously supported it then we should continue to support it. Fixes #15201
grokys
added a commit
that referenced
this pull request
Jun 3, 2024
* Added failing tests for #15081. * Provide target property in BindingExpression ctor. Usually it is not necessary to provide the target property when creating a `BindingExpression` because the property will be assigned when the binding expression is attached to the target in `BindingExpressionBase.Attach`. This is however one case where `Attach` is not called: when the obsolete `binding.Initiate` method is called and then an observable is read from the `InstancedBinding` without the binding actually being attached to the target object. In this case, prior to the binding refactor in #13970 the value produced by the observable was still converted to the target type. After #13970, because the target property (and hence the target type) is not yet set, the conversion is to the target type is no longer done. `DataGrid` uses this obsolete method when editing cells, causing #15081. Ideally we'd fix that in `DataGrid` but I'm not happy making this change so close to 11.1, so instead fix this use-case to behave as before. Fixes #15081
grokys
added a commit
that referenced
this pull request
Jun 3, 2024
* Added failing test for #15201. * Handle nested BindingNotifications. When #13970 was written, [a check](https://github.com/AvaloniaUI/Avalonia/pull/13970/files#diff-cfb25a491b9452e1815aa2c0d71465aaf81e99792a88a04a1a2ed572fd1930ffR60) was added to ensure that nested `BindingNotification`s didn't happen, and the refactor was written with the assumption that they wouldn't happen. The problem is that they _do_ happen: when a source object implements both `INotifyDataErrorInfo` and had data annotations, then the nested data validation plugins would each wrap the value coming from the previous plugin in a new `BindingNotification`, resulting in nested `BindingNotifications`. This adds support for nested binding notifications back in - even though IMO nesting binding notifications is a bug, if we're doing it and we previously supported it then we should continue to support it. Fixes #15201
This was referenced Jun 24, 2024
This was referenced Aug 21, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does the pull request do?
A major refactor of the binding system:
BindingExpressioncode: no more separateBindingObserverandBindingExpression- everything is now implemented inBindingExpressionitselfBindingExpressionBasethat acts as a base-class for the various types of binding expressionsBindmethod onAvaloniaObjectthat returns the createdBindingExpressionBaseBindingExpressions can now be put directly into theValueStoremeaning that no intermediate wrapper objects are required, reducing memory usageBindingExpressionnow has more information about what's being bound, we can log less extraneous warnings (Error logged to output window when binding to Run.Text #9422, Binding errors are being logged even beforeDataContextis set #5762)This allows us to add a new feature in this PR and more new features in future:
UpdateSourceTrigger(Porting WPF UpdateSourceTrigger #3754)BindingExpressionBase.UpdateSourceandUpdateTargetBenchmarks
masterrefactor/bindingexpressions-in-valuestoreAs can be seen, almost all of the benchmarks have significantly better memory usage and most have better or equal performance. There are a couple of exceptions though:
Setup_TemplateBinding_OneWayandProduce_TemplateBinding_Value_OneWay.What's happening here is that we're now storing the current value from the binding as a boxed value in
TemplateBindingwhereas before anIValueEntry<T>was being created for each template binding, meaning that the value didn't need to be boxed.Setup_TemplateBinding_OneWaybeing slightly slower is a result of this: a little more GC is taking place. This is offset by the reduced memory cost for setting up aTemplateBindingafter the refactor.Breaking changes
Binding.Sourcedefault value is nowUnsetvalue- setting it to null now means "bind to null source" instead of "bind to data context" (as in WPF)FallbackValueandTargetNullValueconversion errors are no longer passed to the binding target asBindingNotifications - just logs an errorTemplateBindingnow throws if Mode is set toOneTimeorOneWayToSource- previously it silently reverted toOneWayObsoletions / Deprecations
Fixed issues
Fixes #3754
Fixes #5762
Fixes #9422
Fixes #10469
Fixes #10793
Fixes #13533
Fixes #13734