CommunityToolkit
Fix TouchBehavior for CollectionView
Description of Change
Remove the IsSet check on ICommunityToolkitBehavior, this check will fail for any control that recycle views, since the behavior on that particular view will be already set, causing it to not update. Removing this check will make sure we can update the BindingContext.
Linked Issues
- Fixes #1804
- Fixes #1783
PR Checklist
- [x] Has a linked Issue, and the Issue has been
approved(bug) orChampioned(feature/proposal) - [ ] Has tests (if omitted, state reason in description)
- [x] Has samples (if omitted, state reason in description)
- [x] Rebased on top of
mainat time of PR - [x] Changes adhere to coding standard
- [ ] Documentation created or updated: https://github.com/MicrosoftDocs/CommunityToolkit/pulls
Additional information
Just simply push this solution. It is more performant than always set the binding context new:
internal bool TrySetBindingContextToAttachedViewBindingContext()
{
if (this is not Behavior behavior)
{
throw new InvalidOperationException($"{nameof(ICommunityToolkitBehavior<TView>)} can only be used for a {nameof(Behavior)}");
}
if (View is null)
{
return false;
}
if (!behavior.IsSet(BindableObject.BindingContextProperty))
{
behavior.SetBinding(BindableObject.BindingContextProperty, new Binding
{
Source = View,
Path = BindableObject.BindingContextProperty.PropertyName
});
return true;
}
if (!Equals(behavior.BindingContext, View.BindingContext))
{
behavior.BindingContext = View.BindingContext;
return true;
}
return false;
}
Hello, this is really important for my app. Would it be possible to merge this or is there a problem left?
@BusterIT I don't think that solutions works for all use cases. It won't work if you have a behavior attached to a view inside a CollectionView and it's bound to something outside of that view. It will work on the first pass but not if a view is recycled. Unless I am missing something
For the others asking, I don't recall the state of this PR. I can try to take a look at it soon but have any of you tested that this solves your problem?
@BusterIT I don't think that solutions works for all use cases. It won't work if you have a behavior attached to a view inside a CollectionView and it's bound to something outside of that view. It will work on the first pass but not if a view is recycled. Unless I am missing something
While the concerns about handling behaviors in recycled views within a CollectionView are valid, the modifications I introduced aim to ensure the Behavior's BindingContext is initially synchronized with the View's BindingContext more reliably than before. The original code did not address changes to the BindingContext post-initialization, which is a limitation that remains consistent even after my changes.
To address the concerns about recycling:
- The check
if (!Equals(behavior.BindingContext, View.BindingContext))ensures that if the View's BindingContext changes, which can happen in a recycled view, it updates the Behavior's BindingContext correspondingly. This means my solution should handle the initial cases more robustly. - The scenario where the binding might need to refresh dynamically due to external bindings outside of the view's direct context still needs consideration. To robustly handle the recycling scenarios, further enhancements might be required, such as hooking into more lifecycle events of the View or using additional observables to monitor changes in the BindingContext actively.
In summary, the modifications do not introduce new limitations but rather provide a foundation on which more robust handling of dynamic contexts and view recycling can be built.
@BusterIT thanks for the detailed response. If you look back through the review comments you will see that the use case that I am referring to was a blocking reason why this PR didn't get merged therefore it is something I think we need to consider. I believe that we can find some common ground between your proposed changes and the desired use cases we need to support 👍
