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

[XC] Propagate x:DataType from parent scope to "standalone bindings" #25152

Merged

Conversation

simonrozsival
Copy link
Member

@simonrozsival simonrozsival commented Oct 9, 2024

Description of Change

The x:DataType should always match the current BindingContext of the current scope. When you pass a binding to the Picker component, the binding context for that binding will be each individual item in the list of items to choose from and not the parent binding context. This is similar to how we have a warning when you don't set x:DataType on DataTemplate and instead the data type from the parent scope would be inherited by the template. That is likely an oversight and should be fixed.

In the cases mentioned in #25141, DataTrigger and MultiBinding, the same reasoning doesn't apply, I think. It makes sense for the binding to inherit the parent binding context type.

Looking at this design in #22023 again after some time, I don't find it intuitive and rather confusing. Especially since the warning doesn't mention that there is a special rule for "standalone" bindings. Unless we are able to distinguish the "Picker" scenario from the "Multibinding" scenario, I think we should remove the restriction. If we do that, we should also revisit #24513 to match the behavior in runtime XAML parsing.

Issues Fixed

Fixes #25141
Fixes #5342
This restriction was introduced in #22023
This change is also related to #24513

@bcaceiro
Copy link

Hi @simonrozsival so, to be clear, with this PR, code like this:

  <Border.Triggers>
      <DataTrigger Binding="{Binding Quantity}"
                   TargetType="Border"
                   Value="0">
          <Setter Property="IsVisible" Value="False" />
      </DataTrigger>
  </Border.Triggers>

won't need to specify the DataType ( i'm getting warnings for these in net9) or what should I do?

@simonrozsival
Copy link
Member Author

@bcaceiro you would still need to specify the x:DataType somewhere in the parent scope. The compiler needs to know the exact type of the source type so it can turn the "Quantity" path into a Func<MyViewModel, string> getter = (MyViewModel source) => source.Quantity. We don't want to guess what the right type might be, so you need to do this manually.

at the moment, you need to do this:

  <Border.Triggers>
      <DataTrigger Binding="{Binding Quantity, x:DataType=MyViewModel}"
                   TargetType="Border"
                   Value="0">
          <Setter Property="IsVisible" Value="False" />
      </DataTrigger>
  </Border.Triggers>

if we remove the restriction, it could look like this:

<ContentPage ... x:DataType="MyViewModel">
  <!-- ... -->
  <Border.Triggers>
      <DataTrigger Binding="{Binding Quantity}" <-- this x:DataType would be inherited from the ContentPage
                   TargetType="Border"
                   Value="0">
          <Setter Property="IsVisible" Value="False" />
      </DataTrigger>
  </Border.Triggers>
  <!-- ... -->
</ContentPage>

If you are not worried about non-compiled warnings, for example if your app doesn't have performance issues and you don't have the time to update all the bindings, you can suppress this warning by setting the NoWarn property in your project file:

<NoWarn>$(NoWarn);XC0022</NoWarn>

Does this answer your question? If you have some specific problem you want to discuss, I recommend opening a new issue so we can look into it separately.

@bcaceiro
Copy link

@simonrozsival thank you for your explanation. Indeed I already have the x:DataType in the parent scope, in this case the Page, that's why I was wondering if I need to replace, like you suggested right now, or in a future release it won't be necessary. I indeed am most interested in compiled bindings and performance.
I will probably open a new issue for other binding error, namely StaticResource , but will ping you once I have a small repo.

@StephaneDelcroix
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@dotnet dotnet deleted a comment from azure-pipelines bot Oct 10, 2024
@simonrozsival
Copy link
Member Author

The ItemDisplayBindingWithoutDataTypeFails is now failing because of the bug that will be fixed by #25151

@simonrozsival simonrozsival force-pushed the xdatatype-is-not-propagated-to-multibinding-and-datatrigger branch from 3c3503b to 73fa87a Compare October 14, 2024 12:48
@simonrozsival simonrozsival marked this pull request as ready for review October 14, 2024 12:48
@simonrozsival simonrozsival requested a review from a team as a code owner October 14, 2024 12:48
@PureWeen PureWeen merged commit 008c6af into net9.0 Oct 15, 2024
121 checks passed
@PureWeen PureWeen deleted the xdatatype-is-not-propagated-to-multibinding-and-datatrigger branch October 15, 2024 19:10
@PureWeen
Copy link
Member

/backport to release/9.0.1xx

Copy link
Contributor

Started backporting to release/9.0.1xx: https://github.com/dotnet/maui/actions/runs/11352802876

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants