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.

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

For All CommunityToolkit.Maui.Behaviors, Ensure BindingContext Is Automatically Set to the Attached View's BindingContext #1791

Merged
merged 22 commits into from
Apr 3, 2024

Conversation

brminnick
Copy link
Collaborator

@brminnick brminnick commented Apr 3, 2024

Description of Change

The goal of this Pull Request is to ensure that every Behavior in CommunityToolkit.Maui.Behavior automatically set's the its BindingContext to match the BindingContext of the attached View.

This PR creates/updates the following classes/interfaces:

  • ICommunityToolkitBehavior - an interface meant for internal consumption, used to align the functionality of BaseBehavior and BasePlatformBehavior
    protected TView? View { get; set; }
    
    internal bool TrySetBindingContextToAttachedViewBindingContext();
    internal bool TryRemoveBindingContext();
    internal void AssignViewAndBingingContext(TView bindable);
    internal void UnassignViewAndBingingContext(TView bindable);
    protected void OnViewPropertyChanged(object? sender, PropertyChangedEventArgs e)
    protected void OnViewPropertyChanged(TView sender, PropertyChangedEventArgs e);
  • BaseBehavior : ICommunityToolkitBehavior
    • BaseBehavior now implements ICommunityToolkitBehavior
  • BasePlatformBehavior<TView> : BasePlatformBehavior<TView, PlatformView> where TView : Element
    • Uses the default PlatformView for each operating system
  • BasePlatformBehavior<TView, TPlatformView> : PlatformBehavior<TView, TPlatformView>, ICommunityToolkitBehavior<TView> where TView : Element where TPlatformView : class

Linked Issues

PR Checklist

  • Has a linked Issue, and the Issue has been approved(bug) or Championed (feature/proposal)
  • Has tests (if omitted, state reason in description)
  • Has samples (if omitted, state reason in description)
  • Rebased on top of main at time of PR
  • Changes adhere to coding standard
  • Documentation created or updated: https://github.com/MicrosoftDocs/CommunityToolkit/pulls

Additional information

The unit tests for BasePlatformBehavior can't test OnAttachedTo() because OnAttachedTo() is only called in platform-specific code for Microsoft.Maui.Controls.PlatformBehavior.

I also updated the Unit Tests to use Collection Expressions for a small performance bump.

Copy link
Member

@pictos pictos left a comment

Choose a reason for hiding this comment

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

it looks a lot of work, great that you could fit it soo fast! I've some points there

@Axemasta
Copy link
Contributor

Axemasta commented Apr 3, 2024

Does the BaseBehavior need to account for being attached to multiple VisualElements? Currently with the TouchBehavior it will get wierd... all views attached will receive inputs but only the last view the behavior was added to will be animated.

Since specifically the TouchBehavior needs to be applied 1:1 (behavior : view) should the BaseBehavior have a way of preventing attaching to multiple views?

@brminnick
Copy link
Collaborator Author

Does the BaseBehavior need to account for being attached to multiple VisualElements? Currently with the TouchBehavior it will get weird... all views attached will receive inputs but only the last view the behavior was added to will be animated.

Since specifically the TouchBehavior needs to be applied 1:1 (behavior : view) should the BaseBehavior have a way of preventing attaching to multiple views?

Good question - yes! I followed the existing pattern I found in BaseBehavior and extended it to BasePlatformBehavior via ICommunityToolkitBehavior. It'll automatically set the Behavior.BindingContext to match the BindingContext of the first VisualElement it is attached to. If the same Behavior is then attached to another VisualElement, it will keep the BindingContext from the first VisualElement.

Co-authored-by: Pedro Jesus <pedrojesus.cefet@gmail.com>
@brminnick brminnick added the needs discussion Discuss it on the next Monthly standup label Apr 3, 2024
@brminnick brminnick requested a review from pictos April 3, 2024 18:52
@brminnick brminnick enabled auto-merge (squash) April 3, 2024 22:11
@brminnick
Copy link
Collaborator Author

Ok gang! I think this is ready to go.

I refactored a couple things just now:

  • In ICommunityToolkitBehavior, changed internal OnViewPropertyChanged(object? sender, PropertyChangedEventArgs e) -> protected OnViewPropertyChanged(object? sender, PropertyChangedEventArgs e)
  • Implemented CollectionExpressions for Unit Tests

@brminnick brminnick merged commit d7a84df into main Apr 3, 2024
8 checks passed
@brminnick brminnick deleted the Update-All-Behaviors-to-Inherit-from-`BaseBehavior` branch April 3, 2024 23:15
@brminnick brminnick mentioned this pull request Apr 4, 2024
6 tasks
@tranb3r
Copy link

tranb3r commented Apr 5, 2024

@brminnick @pictos
I think there is a regression in 8.0.1.
In a CollectionView, the BindingContext is not set correctly when using TouchBehavior. When I tap on an item, the command from another item is executed. Maybe because of weird recycling stuff?
I did not have this problem with 8.0.0 and setting the BindingContext manually.
Also, with 8.0.1, even if I set the BindingContext manually, it does not fix the issue.
Could you please double-check? I do not have the time to build a repro right now, but it's easy to reproduce.

@brminnick brminnick removed the needs discussion Discuss it on the next Monthly standup label May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants