Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix TouchBehavior for CollectionView #1815

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Fix TouchBehavior for CollectionView #1815

Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
733028e
remove the IsSet check
pictos Apr 14, 2024
5416550
add verification to not set when the BindingContext is the same
pictos Apr 15, 2024
6a0ba42
something
bijington Apr 17, 2024
dc97e91
Merge branch 'main' into pj/fix-binding-context-for-collections
bijington May 1, 2024
f59357e
Revert IsSet check and add example to expose the original issue to th…
bijington Jun 4, 2024
2f42fd0
Merge branch 'pj/fix-binding-context-for-collections' of github.com:C…
bijington Jun 4, 2024
2be91a1
Some code tidy up
bijington Jun 4, 2024
6fa5dc0
Fix the merge mess
bijington Jun 4, 2024
0e9df40
Merge branch 'main' into pj/fix-binding-context-for-collections
pictos Jun 4, 2024
ffb6dcc
If we have a BindingContext, the type is the same as the Views Bindin…
bijington Jun 5, 2024
0812a7d
Merge branch 'main' into pj/fix-binding-context-for-collections
brminnick Jul 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/CommunityToolkit.Maui/Behaviors/ICommunityToolkitBehavior.shared.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I added behavior.IsSet when working on the TouchBehavior PR for two reasons here:

  1. To ensure we don't override a manually set BindingContext
  2. If a Behavior is reused and attached to a second View, it will
    maintain the BindingContext of the first View it is attached to

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tranb3r thank you for confirming.

@tranb3r and @pictos the changes that I have just pushed appear to solve the issue on Android based on my testing.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recall there was a lot of developers reporting an issue that their bindings didn't work. @brminnick Is this correct?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose their bindings didn't work because they were not setting the BindingContext manually.

if (View is null)
{
return false;
}

if (View.BindingContext == behavior.BindingContext)
{
return false;
}
Expand All @@ -33,7 +38,6 @@ internal bool TrySetBindingContextToAttachedViewBindingContext()
});

return true;

}

internal bool TryRemoveBindingContext()
Expand Down