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

Make ItemsControl.Items readonly. #10827

Merged
merged 3 commits into from
Mar 31, 2023
Merged

Conversation

grokys
Copy link
Member

@grokys grokys commented Mar 28, 2023

What does the pull request do?

#10590 added the ItemsControls.ItemsSource property to make our controls more like WPF and UWP, but left in a setter for the ItemsControl.Items property for compatibility reasons.

From the discussion in that PR and with customers we decided that it's acceptable to remove this setter and make ItemsControl.Items read-only. This will mean that clients will have to change any code that binds/sets Items to use ItemsSource when upgrading to Avalonia 11.

This PR also reverts the change to InheritDataTypeFromItems in #10590, meaning that it's only legal to apply a single one of these attributes to a property again.

Breaking changes

ItemsControl.Items is now readonly.

@maxkatz6 maxkatz6 added this pull request to the merge queue Mar 31, 2023
Merged via the queue into master with commit 7108484 Mar 31, 2023
@maxkatz6 maxkatz6 deleted the refactor/readonly-itemscontrol-items branch March 31, 2023 13:11
@aldelaro5
Copy link
Sponsor Contributor

Hey, I think there was a huge oversight with this pr because let's say I have items bound and I want to upgrade (to nightly with this pr or future versions).

There's actually no indications at compile time that this is no longer supported. I was trying to test an unrelated issue with a sandbox project on lattest masters with this pr and I didn't know that this was a problem because it gave me stuff like this (all of these UI elements were using items instead of itemsSource):
Screenshot from 2023-03-31 21-20-00

Here is the xaml:

<UserControl xmlns="https://github.com/avaloniaui"
             xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
             xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
             xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
             xmlns:s="clr-namespace:Sandbox"
             mc:Ignorable="d" d:DesignWidth="800" d:DesignHeight="450"
             x:CompileBindings="True"
             x:DataType="s:FirstViewModel"
             x:Class="Sandbox.FirstView">
  <StackPanel Orientation="Horizontal">
    <ListBox Items="{Binding Models}">
      <ListBox.ItemTemplate>
        <DataTemplate DataType="s:ObservableModel">
          <ComboBox Items="{Binding Strings}"
                    SelectedIndex="{Binding MyField}"/>
        </DataTemplate>
      </ListBox.ItemTemplate>
    </ListBox>
    <DataGrid AutoGenerateColumns="False"
              Items="{Binding Models}">
      <DataGrid.Columns>
        <DataGridTemplateColumn Header="Field">
          <DataTemplate DataType="s:ObservableModel">
            <ComboBox Items="{Binding Strings}"
                      SelectedIndex="{Binding MyField}"/>
          </DataTemplate>
        </DataGridTemplateColumn>
      </DataGrid.Columns>
    </DataGrid>
    
  </StackPanel>
</UserControl>

Obviously, changing these to use ItemsSource fixes the problem, but my issue more has to do with the fact an unsuspecting developer could take forever to find out what is wrong with their bindings. There's not even anything logged in the console.

Shouldn't we consider having a more clear message that this will not work?

@grokys
Copy link
Member Author

grokys commented Apr 1, 2023

Definitely need a clearer error message here. @kekekeks do you think we need to special-case this in the xaml compiler (and is this behaviour even correct?)

@grokys grokys self-assigned this Apr 1, 2023
@hez2010
Copy link
Contributor

hez2010 commented Apr 2, 2023

Maybe we can special-case this as a warning instead of error?

@grokys
Copy link
Member Author

grokys commented Apr 2, 2023

I'm not sure that adding a Binding object to the items collection will ever be something the user intended, so makes more sense as an error to me.

@hez2010
Copy link
Contributor

hez2010 commented Apr 2, 2023

How about some axaml playgrounds/galleries that want to demonstrate the result of binding a Binding object to the Items collection?

@timunie
Copy link
Contributor

timunie commented Apr 3, 2023

How about some axaml playgrounds/galleries that want to demonstrate the result of binding a Binding object to the Items collection?

OneWayToSource could be a valid Binding, I guess. But we should probably do this discussion in a new issue. Should I open one and move the comments there @grokys?

@grokys
Copy link
Member Author

grokys commented Apr 7, 2023

Issue created #10946

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

Successfully merging this pull request may close these issues.

5 participants