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

Show file tree
Hide file tree
Changes from all 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
43 changes: 42 additions & 1 deletion samples/CommunityToolkit.Maui.Sample/Pages/Behaviors/TouchBehaviorPage.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@

<ScrollView>
<VerticalStackLayout Spacing="20">

<VerticalStackLayout Spacing="{StaticResource SectionSpacing}">
<Label
FontSize="Body"
Expand Down Expand Up @@ -229,6 +228,48 @@
</Grid>
</Border>
</VerticalStackLayout>

<CollectionView ItemsSource="{Binding Items}"
MaximumHeightRequest="250">
<CollectionView.ItemTemplate>
<DataTemplate x:DataType="vm:ItemViewModel">
<Border
Margin="10"
Padding="10"
x:Name="ItemViewRef">

<Border.Behaviors>
<mct:TouchBehavior BindingContext="{Binding BindingContext, Source={x:Reference ItemViewRef}}" Command="{Binding TapCommand}" />
</Border.Behaviors>

<Label Text="{Binding Title}" />

</Border>
</DataTemplate>
</CollectionView.ItemTemplate>

</CollectionView>

<CollectionView ItemsSource="{Binding Items}"
MaximumHeightRequest="250">
<CollectionView.ItemTemplate>
<DataTemplate x:DataType="vm:ItemViewModel">
<Border
Margin="10"
Padding="10"
x:Name="ItemViewRef">

<Border.Behaviors>
<mct:TouchBehavior Command="{Binding TapCommand}" />
</Border.Behaviors>

<Label Text="{Binding Title}" />

</Border>
</DataTemplate>
</CollectionView.ItemTemplate>

</CollectionView>
</VerticalStackLayout>
</ScrollView>
</pages:BasePage>
19 changes: 19 additions & 0 deletions samples/CommunityToolkit.Maui.Sample/ViewModels/Behaviors/ItemViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
using CommunityToolkit.Mvvm.Input;

namespace CommunityToolkit.Maui.Sample.ViewModels.Behaviors;

public partial class ItemViewModel : BaseViewModel
{
public string Title { get; }

public ItemViewModel(string title)
{
Title = title;
}

[RelayCommand]
public Task TapAsync()
{
return Application.Current?.MainPage?.DisplayAlert("Tap", Title, "OK") ?? Task.CompletedTask;
}
}
14 changes: 13 additions & 1 deletion samples/CommunityToolkit.Maui.Sample/ViewModels/Behaviors/TouchBehaviorViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using System.Collections.ObjectModel;
using CommunityToolkit.Mvvm.ComponentModel;
using CommunityToolkit.Mvvm.Input;

Expand All @@ -7,6 +8,8 @@ public partial class TouchBehaviorViewModel : BaseViewModel
{
[ObservableProperty]
int touchCount, longPressCount;

public ObservableCollection<ItemViewModel> Items { get; }

static Task DisplayAlert(string title, CancellationToken token)
=> Shell.Current.DisplayAlert(title, null, "Ok").WaitAsync(token);
Expand All @@ -19,6 +22,15 @@ static Task ParentClicked(CancellationToken token)
static Task ChildClicked(CancellationToken token)
=> DisplayAlert("Child Clicked", token);

public TouchBehaviorViewModel()
{
Items = [];
for (var i = 0; i < 50; ++i)
{
Items.Add(new ItemViewModel($"Item {i}"));
}
}

[RelayCommand]
async Task MonkeySelected(string? monkey, CancellationToken token)
{
Expand All @@ -42,4 +54,4 @@ void IncreaseLongPressCount()
{
LongPressCount++;
}
}
}
32 changes: 30 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,19 +21,47 @@ 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;
}

var parent = View.Parent;
var assignBindingContext = behavior.IsSet(BindableObject.BindingContextProperty) is false;

// If we have a BindingContext, the type is the same as the Views BindingContext and we are inside a CollectionView
// then we can assume that we are recycling views and need to assign the BindingContext again.
if (View.BindingContext?.GetType() == behavior.BindingContext?.GetType())
{
for (var i = 0; i < 10; i++)
{
if (parent is null)
{
break;
}

if (parent is CollectionView)
Copy link
Member Author

Choose a reason for hiding this comment

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

I believe for testing this is enough for now... But collectionView isn't the only control that may have virtualization

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point!

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

{
assignBindingContext = true;
break;
}

parent = parent.Parent;
}
}

if (assignBindingContext is false)
{
return false;
}

behavior.SetBinding(BindableObject.BindingContextProperty, new Binding
{
Source = View,
Path = BindableObject.BindingContextProperty.PropertyName
});

return true;

}

internal bool TryRemoveBindingContext()
Expand Down
Loading