[Regression] Fix TypedBinding nested property re-subscription (#34428)#34449
[Regression] Fix TypedBinding nested property re-subscription (#34428)#34449simonrozsival merged 1 commit intomainfrom
Conversation
…ate becomes non-null When an intermediate object in a TypedBinding path starts as null and later becomes non-null (or is replaced with a different object), the binding must re-establish INPC subscriptions to the new object's nested properties. The previous optimization used a '_isSubscribed' boolean flag to skip IPropertyChangeHandler.Subscribe() after the first Apply. This prevented re-subscribing when intermediate objects changed. For example: vm.Child = null // subscribe to vm, but NOT vm.Child.Name (null) vm.Child = new Child() // Subscribe skipped due to _isSubscribed=true vm.Child.Name = "x" // binding never fires — regression! The fix removes '_isSubscribed' and always calls Subscribe on each Apply. Both IPropertyChangeHandler implementations (PropertyChangeHandler and LegacyPropertyChangeHandler) are already idempotent — they use reference equality checks to avoid re-subscribing to unchanged objects — so calling Subscribe on every Apply is safe and has minimal overhead. Adds two regression tests: - TypedBinding_NestedProperty_ResubscribesAfterNullIntermediateBecomesNonNull - TypedBinding_NestedProperty_ResubscribesAfterIntermediateReplaced Reported in: #32382 (comment) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
StephaneDelcroix
added
i/regression
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 34449Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 34449" |
|
/backport to release/10.0.1xx-sr5 |
|
Started backporting to |
There was a problem hiding this comment.
Pull request overview
Fixes a regression in TypedBinding where compiled bindings with nested property paths could stop updating after an intermediate object was replaced, by ensuring subscription updates are re-evaluated during binding application.
Changes:
- Removed the
_isSubscribedguard soTypedBindingcan re-subscribe when intermediateINotifyPropertyChangedobjects change. - Added two regression tests covering null-to-non-null and replaced-intermediate scenarios for nested property paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Controls/src/Core/TypedBinding.cs | Ensures Subscribe() is invoked during applies so nested-path subscriptions are refreshed when intermediates change. |
| src/Controls/tests/Core.UnitTests/TypedBindingUnitTests.cs | Adds regression coverage for nested property re-subscription when intermediates become non-null or are replaced. |
| // Subscribe on every Apply so that intermediate objects that changed are re-subscribed. | ||
| // Subscribe() is idempotent: it diffs old vs new subscription targets and only | ||
| // updates what changed, so calling this repeatedly is safe. | ||
| if (isTSource && (mode == BindingMode.OneWay || mode == BindingMode.TwoWay) && _handlers != null) |
There was a problem hiding this comment.
Subscribe() is now called on every ApplyCore, including when fromTarget == true for BindingMode.TwoWay (i.e., target-driven updates). That means each UI-originated change will re-run the handler chain and subscription diffing, which can be a measurable regression compared to the _isSubscribed optimization, even though subscriptions likely don’t need to change on target updates. Consider gating the Subscribe(...) call to only run when fromTarget is false (or when needsGetter is true), so intermediate re-subscription still happens for source/context changes without adding per-keystroke overhead for TwoWay bindings.
| if (isTSource && (mode == BindingMode.OneWay || mode == BindingMode.TwoWay) && _handlers != null) | |
| if (isTSource && (mode == BindingMode.OneWay || mode == BindingMode.TwoWay) && _handlers != null && (!fromTarget || needsGetter)) |
|
/azp run maui-pr-uitests, maui-pr-devicetests |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
/backport to release/10.0.1xx-sr5 |
|
Started backporting to |
…re-subscription (#34428) (#34450) Backport of #34449 to release/10.0.1xx-sr5 /cc @StephaneDelcroix Co-authored-by: Stephane Delcroix <stephane@delcroix.org> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Description
Cherry-pick of
5d6e5a20fromnet11.0, adapted formain.Fixes #34428
Problem
Release 10.0.50 introduced a performance optimization (
_isSubscribedflag) inTypedBindingthat prevented re-subscribing to intermediate INPC objects when they changed. This caused compiled bindings with nested property paths (e.g.{Binding ViewModel.Text}) to stop updating when the intermediate object was replaced.Fix
Remove the
_isSubscribedguard and always callSubscribe()on everyApply. TheSubscribe()implementation is already idempotent — it diffs old vs new subscription targets — so calling it repeatedly is safe with minimal overhead.Tests
Two regression tests added to
TypedBindingUnitTests.cs:TypedBinding_NestedProperty_ResubscribesAfterNullIntermediateBecomesNonNullTypedBinding_NestedProperty_ResubscribesAfterIntermediateReplaced