Fix TouchBehavior for CollectionView #1815
Conversation
| @@ -21,7 +21,12 @@ internal bool TrySetBindingContextToAttachedViewBindingContext() | |||
| throw new InvalidOperationException($"{nameof(ICommunityToolkitBehavior<TView>)} can only be used for a {nameof(Behavior)}"); | |||
| } | |||
|
|
|||
| if (behavior.IsSet(BindableObject.BindingContextProperty) || View is null) | |||
There was a problem hiding this comment.
What will happen in scenarios where a developer explicitly sets their own BindingContext for the behavior? I think this could would overwrite it with the BindingContext from the View?
There was a problem hiding this comment.
Agreed. I added behavior.IsSet when working on the TouchBehavior PR for two reasons here:
- To ensure we don't override a manually set BindingContext
- If a Behavior is reused and attached to a second View, it will
maintain the BindingContext of the first View it is attached to
There was a problem hiding this comment.
Perhaps the safer change is to remove the binding altogether and subscribe to the BindingContext changed event on the View and assign to the Behavior?
There was a problem hiding this comment.
Not being able to re-assing the BindingContext will make all Behaviors un-usable on recycling/virtualization scenarios.
What do you think the correct approach would be?
There was a problem hiding this comment.
I have changed the OnAttachedTo method in BaseBehavior to look as follows:
protected override void OnAttachedTo(TView bindable)
{
base.OnAttachedTo(bindable);
bindable.PropertyChanging += (sender, args) =>
{
var currentViewBindingContext = ((TView)sender).BindingContext;
if (args.PropertyName == nameof(BindingContext) &&
ReferenceEquals(this.BindingContext, currentViewBindingContext))
{
this.BindingContext = null;
}
};
bindable.PropertyChanged += (sender, args) =>
{
if (args.PropertyName == nameof(BindingContext) &&
this.BindingContext is null)
{
this.BindingContext = ((TView?)sender)?.BindingContext;
}
};
}It might not be the prettiest but I believe it should work. What does everyone think? Any ideas on how to make it nicer?
There was a problem hiding this comment.
I have reverted the change and added in two examples from @tranb3r example repo. Perhaps @tranb3r you can test this sample and let us know how it compares to your expectation. The code works as I expect on iOS/macOS in our sample app
These are indeed the same cases as in my repro. Not working on android, unless using 8.0.0.
There was a problem hiding this comment.
Sorry to ask again, but I'm still wondering if this BindingContext auto-set feature (introduced in 8.0.1) is really required?
It seems to me that 8.0.0 was fine. If the goal was only to simplify xaml code, I'm not sure it's worth it, considering the difficulty you have to figure out how to fix this regression.
I'm probably missing something, but could you please explain which bug did you try to fix in 8.0.1? Or at least post a code sample for the original issue?
There was a problem hiding this comment.
I recall there was a lot of developers reporting an issue that their bindings didn't work. @brminnick Is this correct?
There was a problem hiding this comment.
I suppose their bindings didn't work because they were not setting the BindingContext manually.
…e sample application
…ommunityToolkit/Maui into pj/fix-binding-context-for-collections
…gContext and we are inside a CollectionView then we can assume that we are recycling views and need to assign the BindingContext again.
| break; | ||
| } | ||
|
|
||
| if (parent is CollectionView) |
There was a problem hiding this comment.
I believe for testing this is enough for now... But collectionView isn't the only control that may have virtualization
There was a problem hiding this comment.
I've been looking for a suitable interface or base class but I don't think I have found one yet. I am thinking ItemsView and possibly IItemsView. What do you think?
There was a problem hiding this comment.
still not enough, because devs can apply virtualization to any kind of control or layout... I'm still didn't get the reason to be so conservative on allowing changing the BindingContext. In other words, we shouldn't prevent the BindingContext to be mutable. You may have cases where the dev reuses the control across the page and wants the control's BindingContext to change when that happens
There was a problem hiding this comment.
I see that we want to automatically inherit the BindingContext from the View it is attached to only if the BindingContext is not already set. It appears there are issues around checking whether the BindingContext is already set especially when the behavior is used inside something like the CollectionView and the behavior is attached to recycled views.
There was a problem hiding this comment.
Yeah, but the thing is that after the first set, that value will always be true, and that's causing the issue. Because of that I removed the IsSet check, as mentioned on the PR description.
I think this happens because when setting the new value, they don't use the RemoveBinding method, just BindinContext = someValue, causing the IsSet to never be false again
There was a problem hiding this comment.
I didn't confirm the instances but I did discover that OnAttachedTo was being called before OnDetachedFrom when the recycling was happening. Which may explain why IsSet is always true after the first time.
Description of Change
Remove the
IsSetcheck onICommunityToolkitBehavior, 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 theBindingContext.Linked Issues
PR Checklist
approved(bug) orChampioned(feature/proposal)mainat time of PRAdditional information