dotnet / maui

.NET MAUI is the .NET Multi-platform App UI, a framework for building native device applications spanning mobile, tablet, and desktop.
https://dot.net/maui
MIT License
22.16k stars 1.74k forks source link

Wrong Binding Source #25189

Open eeegs opened 2 weeks ago

eeegs commented 2 weeks ago

Description

I have a Seat entity that can be occupied by a Player entity. The Seat has an IsOccupied flag. I am binding a set of Seats to a layout and expect the bind at the data template level to be to each Seat. In the data template I am binding each Seat occupier (a Player) to a Player control.

If the Seat is unoccupied then I don't want to show the Player control.

                <DataTemplate x:DataType="viewModels:Seat">
                        <controls:Player IsVisible="{Binding IsOccupied}"
                                         BindingContext="{Binding Player}" />
                </DataTemplate>

The binding resolving engine tries to resolve IsOccupied against the Player entity, not the Seat one. Given I am binding Player entity to the Player control the bindingcontect at this point is the Seat (as that's what provides the Player entity), so I'd expect IsOccupied to resolve to the current bindingcontext the Seat, not the Player. I'd would only expect Player entity to be used for properties bounded inside the Player control.

Now you are going to tell me that IsVisible is a Player control property so will be bound to the play context; however, I argue that that is logically incorrect as I'm binding "outside" the control.

It is also inconsistent, with how the Player control's BindingContext is being sourced in the data template.

Steps to Reproduce

public partial class Seat(int number) : ObservableObject { [ObservableProperty] [NotifyPropertyChangedFor(nameof(IsOccupied))] Player? player; public bool IsOccupied => Player is not null; }

Have a MAUI View of type Player. List several seats in a layout and used databinding to draw them.

Link to public reproduction project repository

https://github.com/eeegs/demo

Version with bug

9.0.0-rc.1.24453.9

Is this a regression from previous behavior?

Not sure, did not test other versions

Last version that worked well

Unknown/Other

Affected platforms

I was not able test on other platforms

Affected platform versions

No response

Did you find any workaround?

No response

Relevant log output

Severity Data Context Binding Path Target Target Type Description File Line Project Error Player IsOccupied Player.IsVisible Boolean 'IsOccupied' property not found on 'xxxx.Domain.Player', target property: 'xxxx.Controls.Player.IsVisible' ...\Views\StartPage.xaml Projectname

suugbut commented 2 weeks ago

@eeegs Please provide a github repo with minimal code to illustrate the issue.

eeegs commented 2 weeks ago

Done:

https://github.com/eeegs/demo

suugbut commented 2 weeks ago

Do you know binding in custom controls need bindable properties?

eeegs commented 2 weeks ago

I'm not sure what you mean by this:

Do you know binding in custom controls need bindable properties?

IsVisible is bindable, it is inherited from VisualElement and Seat.Player and Seat.IsOccupied, provide IPropertyNotifyChange notifications.

suugbut commented 2 weeks ago

Minimal code to illustrate the solution.

public class Player
{
    public string Name { get; set; } = string.Empty;
}

public partial class Seat : ObservableObject
{
    [ObservableProperty]
    [NotifyPropertyChangedFor(nameof(IsOccupied))]
    Player? _player;

    public bool IsOccupied => Player is not null;
}

public partial class ViewModel : ObservableObject
{
    // No ObservableProperty attribute is needed here because  we are only interested in detecting changes in elements.
   // Seats is never reassigned to another instance of ObservableProperty<Seat>. 
    public ObservableCollection<Seat> Seats { get; } =
    [
        new(),
        new(),
        new(),
        new(),
        new()
    ];

    public ViewModel()
    {
        Seats[0].Player = new Player() { Name = "Alice" };
        Seats[1].Player = new Player() { Name = "Bob" };
    }
}

You need to have bindable property for Name.

public partial class PlayerControl : ContentView
{
    public PlayerControl()
    {
        InitializeComponent();
    }

    public static readonly BindableProperty NameProperty
        = BindableProperty.Create(
            nameof(Name),
            typeof(string),
            typeof(PlayerControl)
        );

    public string Name
    {
        get => (string)GetValue(NameProperty);
        set => SetValue(NameProperty, value);
    }
}
<ContentView ...
             x:Class="BugOrNot.PlayerControl"
             x:Name="this">
    <Label BindingContext="{Reference this}"
           Text="{Binding Name}" />
</ContentView>
<ContentPage ...
             xmlns:loc="clr-namespace:BugOrNot"
             x:Class="BugOrNot.MainPage"
             x:DataType="loc:ViewModel"
             BindingContext="{loc:ViewModel}"
             >
    <CollectionView ItemsSource="{Binding Seats}">
        <CollectionView.ItemTemplate>
            <DataTemplate x:DataType="loc:Seat">
                <loc:PlayerControl IsVisible="{Binding IsOccupied}"
                                   Name="{Binding Player.Name}" /> <!-- Here -->
            </DataTemplate>
        </CollectionView.ItemTemplate>
    </CollectionView>
</ContentPage>

I don't use DI here for the sake of simplicity.

public partial class App : Application
{
    public App()
    {
        InitializeComponent();

        MainPage = new MainPage();
    }
}

Other notes:

Because you don't inject AppShell in App , you don't need to register AppShell in DI. https://github.com/eeegs/demo/blob/8a90dc706fd3776c945a87e07a40ec61b1487f32/App/App.xaml.cs#L9 https://github.com/eeegs/demo/blob/8a90dc706fd3776c945a87e07a40ec61b1487f32/App/MauiProgram.cs#L20

eeegs commented 2 weeks ago

Thanks for the feedback. In this case I don't want to expose all of the internal of the player control externally to it. Me showing the Name was just an example. I'd like the PlayerControl to know how to render a Player, and not expose all the details to the world.

I still think my original concern stands. The inconsistency of

In my view anything bound at this level should bind to the Seat.

Thanks for the heads up in the DI.

I can, and have, worked around this. There are several ways. Infact just hiding the Player Control is probably not the best option. If there is no Player occupying the Seat then there should be no PlayerControl created at all. I noticed this binding issue and am simply bringing to the team's attention.

Thanks,

suugbut commented 2 weeks ago

Or you can populate Seats with GetOccupiedSeats() that returns a list of occupied Seat.

eeegs commented 2 weeks ago

I think you are missing my point a little. I understand exactly what is and why it is happening. I am suggesting that this is a mistake. At the DataTemplate level all binding to any XMAL in the data template should use the current item from the ItemSource, in this case a Seat. Only inside the PlayerControl (ie in the xmal defining it) should the object bound to it be the source.

As I need to keep the PlayerControl binding to a Player as it is used elsewhere, I will write a wrapping control that only renders its child if the DataContext is not null.

My point, is while it might be "by design behaviour", I think it is logically wrong, as it introduces an inconsistency about what is being bound in the DataTemplate.

suugbut commented 2 weeks ago

No. It is logically correct.

For example, both Name and IsVisible belong to PlayerControl. So they must be bound with the same context.

public partial class PlayerControl : ContentView
{
    public PlayerControl()
    {
        InitializeComponent();
    }

    public static readonly BindableProperty NameProperty
        = BindableProperty.Create(
            nameof(Name),
            typeof(string),
            typeof(PlayerControl)
        );

    public string Name
    {
        get => (string)GetValue(NameProperty);
        set => SetValue(NameProperty, value);
    }
}

So the correct way to use it in DataTemplate are:

When binding context inherits from DataTemplate, the BindingContext is a Seat.

<loc:PlayerControl IsVisible="{Binding IsOccupied}" Name="{Binding Player.Name}" />

but if you set BindingContext to Player

<loc:PlayerControl BindingContext="{Binding Player}" Name="{Binding Name}"  x:DataType="loc:Player"/>

IsOccupied cannot be used for IsVisible because IsVisible (and Name) is bound to a Player that has no IsOccupied property.

suugbut commented 2 weeks ago

If each bindable property of the same control need to be bound to a different context, you can use Source={RelativeSource} markup extension.

<loc:PlayerControl BindingContext="{Binding Player}"
                    x:DataType="loc:Player"
                    Name="{Binding Name}" 
                    IsVisible="{Binding IsOccupied,Source={RelativeSource AncestorType={x:Type loc:Seat}}}"
                    />
Zhanglirong-Winnie commented 1 week ago

This issue has been verified using Visual Studio 17.12.0 Preview 2.1(8.0.91 & 8.0.90 & 9.0.0-rc.1.24453.9). Can repro this issue.