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

Added ItemsControl.ItemsSource. #10590

Merged
merged 12 commits into from
Mar 16, 2023
Merged

Added ItemsControl.ItemsSource. #10590

merged 12 commits into from
Mar 16, 2023

Conversation

grokys
Copy link
Member

@grokys grokys commented Mar 7, 2023

What does the pull request do?

Adds ItemsControl.ItemsSource with a similar behavior to WPF/UWP:

  • ItemsControl.Items is now of type IList so that the collection can be modified using Add/Remove etc
  • When ItemsControl.ItemsSource is set, Items goes into read-only mode and functions as a view over the items source
  • Because ItemsSource is not initialized with an empty collection, it can be a styled property

There is one main difference from WPF/UWP, added for backwards compatibility:

  • The Items property can still be bound and set (though this is deprecated). In this case, the items collection can still be accessed via the (pre-existing) ItemsView property. For this reason Items is of type IList and not ItemCollection

To allow both Items and ItemsSource to be bound, I had to add support for multiple InheritDataTypeFromItems attributes to be applied to a property. In this case that won't be a problem because only one can be set, though I'm not sure if it could could cause confusion in future?

Doesn't correct usages of Items to ItemsSource except where necessary (i.e. IEnumerable was being assigned to Items). Done in a separate PR as it's a big noisy change

Doesn't change the name of the Items property or add an ItemsSource to other ItemsControl-like controls. That can also be done later.

Depends on #10589

Fixed issues

Fixes #7553
Fixes #10509

`ItemsControl` now works more like WPF, in that there are separate `Items` and `ItemsSource` properties. For backwards compatibility `Items` can still be set, though the setter is deprecated. `Items` needed to be changed from `IEnumerable` to `IList` though.
To allow binding both `ItemsControl.Items` and `ItemsSource` we need to support multiple `InheritDataTypeFromItems` attributes.
It's a view so makes sense to expose the read-only view interface.
@grokys grokys changed the title WIP: Added ItemsControl.ItemsSource. Added ItemsControl.ItemsSource. Mar 8, 2023
@grokys grokys marked this pull request as ready for review March 8, 2023 15:20
@grokys
Copy link
Member Author

grokys commented Mar 8, 2023

Ready for review, I think.

@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0031738-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

@hez2010
Copy link
Contributor

hez2010 commented Mar 12, 2023

To allow both Items and ItemsSource to be bound, I had to add support for multiple InheritDataTypeFromItems attributes to be applied to a property. In this case that won't be a problem because only one can be set, though I'm not sure if it could could cause confusion in future?

How about breaking it and dropping the support for binding to Items, then change the type of Items to ItemCollection?
We already have plenty of breaking changes in v11.0 and need both control and app authors to migrate, why not another breaking change when we still can? It won't be a problem if we document it properly.
Otherwise, if we ship it as is, we will lose the chance to make breaking changes around here.

@Mrxx99
Copy link
Contributor

Mrxx99 commented Mar 12, 2023

How about breaking it and dropping the support for binding to Items, then change the type of Items to ItemCollection?

Together with a specialized error message telling to bind to ItemsSource when trying to bind to Items instead of some cryptic or general error, that sounds like a good idea. Not showing a specific error would make it confusing as everyone first learned to use Items in Avalonia.

@amwx
Copy link
Contributor

amwx commented Mar 12, 2023

I agree with above. I think changing the setter to throw an error that says change to ItemsSource is the best. Then in the future, the setter and DirectProperty can be removed without really being a breaking change. Since most binding usages are in Xaml, how noticeable is it that binding (or setting Items) is obsolete. I don't know about Rider, but in VS I know we don't tag obsolete warnings in the xaml document. VS also doesn't show all errors/warnings/messages by default unless you do a full rebuild, so unless you pay attention to the command line while building or open all files, this isn't 100% transparent.

@grokys
Copy link
Member Author

grokys commented Mar 13, 2023

The feedback we've been getting from customers is that port a large project from 0.10.x to 11.x results in an almost overwhelming number of compile errors. Given that Items is a very common property, the idea was at least they could get up-and-running using Items as previously, and then migrate to using ItemsSource afterwards.

I'm 100% open to making Items read-only, but given that the downside should be very minor, I'm not sure that we should.

@timunie
Copy link
Contributor

timunie commented Mar 13, 2023

I'm 100% open to making Items read-only, but given that the downside should be very minor, I'm not sure that we should.

How painful would it be to revert this change if the complaints are getting too much?

@hez2010
Copy link
Contributor

hez2010 commented Mar 13, 2023

The feedback we've been getting from customers is that port a large project from 0.10.x to 11.x results in an almost overwhelming number of compile errors.

Unlike other breaking changes, this one can be fixed easily with a simple find-and-replace.

@grokys
Copy link
Member Author

grokys commented Mar 14, 2023

I've spoken to one of the customers who gave us that feedback and they feel that this wouldn't be a particularly bad breaking change. Given that, and the feedback here so I think we'll be ok to remove the Items setter 🎉

We should do it in a subsequent PR though I feel, so it can potentially be reverted if we get more feedback.

Copy link
Member

@danwalmsley danwalmsley left a comment

Choose a reason for hiding this comment

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

LGTM: Agreed YOLO to get this tested

@danwalmsley danwalmsley added this pull request to the merge queue Mar 16, 2023
Merged via the queue into master with commit 9413d93 Mar 16, 2023
@danwalmsley danwalmsley deleted the feature/itemssource branch March 16, 2023 15:26
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.

ListBox is leaking memory in preview5 (and not in preview4) Items vs ItemsSource
7 participants