search

AvaloniaUI / Avalonia

Develop Desktop, Embedded, Mobile and WebAssembly apps with C# and XAML. The most popular .NET UI client technology
https://avaloniaui.net
MIT License
26.16k stars 2.27k forks source link

Convert Most `DirectProperty` Occurrences to `StyledProperty` #9944

Closed robloo closed 1 year ago

robloo commented 1 year ago

Is your feature request related to a problem? Please describe.

This one is going to cause some controversy. However, I believe it's time to significantly narrow the use-case of DirectProperty(ies) which are unable to be set by styles in most cases.

Some additional points for discussion:

With the significant optimization done to the property value store over the past few years, I don't expect a significant performance impact moving most properties over. I could be wrong though.

Describe the solution you'd like

Almost all current DirectProperty occurrences should be converted to StyledProperty so they can be used fully within styles. My original comment was:

direct properties should be reserved only for "data"... which includes content, read-only collections

@tomenscape had some good points and narrowed the scope again:

I would go even further than robloo's suggestion: in light of these changes, DirectProperty should be restricted to read-only properties. This is the only safe way to proceed. Even "data" values can reasonably set in a style, if you have multiple distinct values that you want to switch between depending on UI state.

This means ALL direct properties such as TextBlock.Text, Expander.IsExpanded and SplitPane.IsPaneOpen (which are UI state information) would be converted to styled properties.

Describe alternatives you've considered

There are two alternatives:

  1. Extend the styling system to allow settings DirectProperty as if it was a StyledProperty. No idea how to do this.
  2. No changes...

Additional context

Some recent discussions include:

@tomenscape Feel free to duplicate your past comments here. It will put everything in one spot for the future.

Current status is tracked below. Note that animations and certain drawing primitives are currently out-of-scope. There is a strong chance of performance regressions in these areas and the use-cases of changing such properties in styles is not yet clear. Therefore, the focus is primarily controls:

amwx commented 1 year ago

DirectProperty is still useful in some cases (read-only properties and lists) and must not go away

direct properties should be reserved only for "data"... which includes content, read-only collections

Enumerable type properties should also be able to be styled. There's been some discussion on this in the past for Grid but that lost traction a while ago (#4397 & #4464). Doing so would also allow moving towards tomenscape's idea, which I don't necessarily disagree with, but I think there's still some use cases for having writable DirectProperties.

This means ALL properties such as Expander.IsExpanded and SplitPane.IsPaneOpen (which are UI state information) would be converted to direct.

(I think you meant "converted to styled")

And to add another property: TextBlock.Text needs to be styleable.

Also FWIW w/ SplitView, the only reason it's Direct is because I was following Expander's IsExpanded as a model and this was still my early days using Avalonia.

robloo commented 1 year ago

Enumerable type properties should also be able to be styled.

Enumerables and lists/collections are almost a separate topic. It goes all the way back to WPF and SelectedItems which doesn't support binding at all (without some tricky work-arounds). Avalonia as I understand it solves this problem by passing around IEnumerable and allowing get/set of the property itself. I'm not sure that is a good solution but at least it supports binding...

(I think you meant "converted to styled")

Thanks, fixed.

And to add another property: TextBlock.Text needs to be styleable.

Yea, I'll add it and link to some of those older discussions.

Also FWIW w/ SplitView, the only reason it's Direct is because I was following Expander's IsExpanded as a model and this was still my early days using Avalonia.

Ah, I didn't realize you did that control. If that's the case I can do a PR eventually to switch that one over. It doesn't seem too confrontational.

Separately: Changing Expander.IsExpanded is problematic now because I added cancellation which requires a direct property to intercept before the property is set. We would need an OnPropertyChanging callback with cancellation to get around this now.

TomEdwardsEnscape commented 1 year ago

Thanks for making this issue @robloo. I was looking at the new ValueStore last night and I agree that there doesn't seem to be much of a performance difference between setting a StyledProperty with local priority and setting a DirectProperty. It seems that the only remaining benefit of DirectProperty is providing support for read-only properties.

You have described the issue well, so I don't need to replicate most of what I said. I'll just add these details about the use case at my company:

We are using ControlTheme everywhere, and those themes often contains activatable styles. Our use case is actually low-level controls (e.g. Button) which appear within the templates of high-level controls, and the selectors are something like ^[Foo=bar] /template/ #SomeButton.

I understand the problems that come with setting direct properties from this context, but since we never wanted to remove these values, only to switch between multiple different options, that has never been a problem for us.

I would implement this change by first removing DirectProperty.Setter and AvaloniaObject.SetDirectValueUnchecked, and then fixing all the compile errors that arise. The fix for each error would be switching to StyledProperty or removing the setter and making the property read-only. AvaloniaObject.SetAndRaise (which is a protected method) would remain for use within private/protected C# property setters.

Separately: Changing Expander.IsExpanded is problematic now because I added cancellation which requires a direct property to intercept before the property is set. We would need an OnPropertyChanging callback with cancellation to get around this now.

Once the property is changed to StyledProperty, a coercion method could be executed before the value is written.

grokys commented 1 year ago

I actually mostly agree with this. The recent changes to the value store should at least alleviate some of the previous performance problems, and if performance is still a problem with reading in some cases then the value can be cached in a field.

I think the only remaining problems with styled properties are:

Direct properties should be obviously kept for read-only properties, and read/write direct properties should be kept around for use by client code.

Separately: Changing Expander.IsExpanded is problematic now because I added cancellation which requires a direct property to intercept before the property is set. We would need an OnPropertyChanging callback with cancellation to get around this now.

Yep, as @tomenscape mentioned I think coercion should be able to solve this.

TomEdwardsEnscape commented 1 year ago

I'm going to make a start on these changes now, since they will prevent us from upgrading to the next preview otherwise.

I can't assign the issue to myself though.

robloo commented 1 year ago

@tomenscape

I'll handle at least Expander by this weekend at the latest. If you need me to do others let me know (was already thinking about TextBlock). I was going to look at doing this on a control-by-control basis to start because I think several controls are doing things in the property setter so it's going to require some thought to change them and review. Will know more once the code is more broadly looked at though.

Once the property is changed to StyledProperty, a coercion method could be executed before the value is written.

Yes, good idea. I spent too much time with UWP/WinUI and often forget to consider coercion like I should.

@grokys

As mentioned, styling collections is a problem we inherited from WPF

I'm going to break this out as a new issue later. This doesn't have to be fully solved to close this issue I think. Conceptually, I've been thinking about it for some time too. The property system needs to itself be aware of and synchronize changes within a type. Even in WPF its really only designed for single values instead of multi-values per property. That line of thought needs a lot of work though and probably a new interface and special, new collection types. Even higher level conceptually, it's going from one dimension to two in the property system.

Styled properties don't currently support data validation. This could be added.

Didn't know that myself. Will definitely have to add that in the future.

TomEdwardsEnscape commented 1 year ago

I've pushed a branch. It's untested as it doesn't compile yet, but I've made a lot of progress. Remaining types to refactor in the Controls assembly are ItemsControl (and inheritors), Expander, and SplitView.

TomEdwardsEnscape commented 1 year ago

@robloo I've now completed the first pass of this change. The control gallery compiles and runs, but renders incorrectly. There are also many failing tests which I'm working through next, and I hope that once everything is green the gallery will work again.

I refactored SplitView, and then when I looked at Expander realised that they are both solving the same problem. So I went ahead brought the two into line with each other. Both now raise a RoutedEvent with an explicit Cancel property. Handling the event is no longer required.

Some classes with weird behaviour that I had to make substantial internal changes to:

I'll write more about these when the time to open a PR comes.

robloo commented 1 year ago

Had an issue with my phone. Replacing last message...

It's great you are getting so much done here. However, I'm a bit concerned after looking briefly at the changes that this is going deeper than expected in some areas. Generally, I would be uncomfortable making such a large PR changing all this functionality. It's easy to make even a little mistake here and it's even easier to get lost in a giant PR review. I know the core maintainers do this though... and if they can review it I guess it's no issue. However, for me I would be splitting this out into separate PRs especially for controls with deeper changes.

I have some specific comments concerning Expander since I was still planning on working on that... I just don't always have the time during the day or even week to jump into things that come up unexpected. I did commit to this weekend though but you didn't give me a chance.

It's entirely possible this level of debate is needed for each control. So again, I really advice against doing this all at once.

TomEdwardsEnscape commented 1 year ago

I'm definitely in favour of slicing this up into multiple PRs. Once everything has settled down I'll start extracting clean commits for different areas of the solution. With sensible management we can then open and complete them one by one without breaking master in between. Most changes are quite boring and can come back at the same time, so there shouldn't be too many separate PRs.

I don't mind what the ultimate design is for cancelling Expander events, I just did something that works and was relatively quick to implement. Feel free to change it later. :)

robloo commented 1 year ago

@tomenscape Ok, perfect, sounds like we are on the same page. Thanks for doing all this, really is great work!

TomEdwardsEnscape commented 1 year ago

I've hit a bump in this project: there is no equivalent to WPF's DependencyObject.SetCurrentValue method. This is necessary when a control is internally setting its own StyledProperty values, because if the property has had its value set by another component then you need to know which BindingPriority to set your value with. Too low a priority and your new value doesn't become effective, but too high a priority and it overrides any value coming from the binding/style/etc. that wants to set a value too.

A secondary issue is that there are two read-only binding priorities: Unset and Inherited. You can't set a value with either of these priorities.

To complicate matters further, bindings don't modify the target property until they change its value. So if you place a binding on a property, and the binding produces the property's default value, the current BindingPriority remains Unset even though there is a binding is attached and active.

None of this is an issue for DirectProperty because that has no concept of priorities.

WPF

Here's what WPF does: SetCurrentValue allows you to provide a new value for any ValueSource (i.e. BindingPriority). This includes default/unset! Here is some WPF code that demonstrates this:

var oldSource = DependencyPropertyHelper.GetValueSource(this, DataContextProperty);
SetCurrentValue(DataContextProperty, new object());
var newSource = DependencyPropertyHelper.GetValueSource(this, DataContextProperty);

Debugger output:

oldSource
{System.Windows.ValueSource}
    BaseValueSource: Default
    IsAnimated: false
    IsCoerced: false
    IsCurrent: false
    IsExpression: false
newSource
{System.Windows.ValueSource}
    BaseValueSource: Default
    IsAnimated: false
    IsCoerced: true
    IsCurrent: true
    IsExpression: false

As you can see, our value source is still default, but the actual value has changed. It's a weird situation, but since the value source is normally private to the binding system it's at least contained within the framework.

Solutions

I'm not sure what the best way to solve this in Avalonia is. We could replicate what WPF does, but it seems pretty ugly to me. And because Avalonia, unlike WPF, preserves existing bindings when setting a value, I don't think we need to have another code path in the value store.

Another potential solution is to:

  1. Add a SetCurrentValue method which reads the current BindingPriority and writes a new value with it.
  2. Create a new priority called (e.g.) "Current", which sits between Style and Inherited in the enum. If SetCurrentValue finds that the current priority is Inherited or Unset, then it sets the value with this "Current" priority.

Any thoughts?

robloo commented 1 year ago

@tomenscape Can you estimate how widespread this problem is? How many controls are affected? My initial thought is just to skip those controls for 11.0. This change doesn't have to be all or nothing -- it can be just what is currently possible. I highly doubt the changes you are requested will make it in for the next release (even if the spec was decided). Could be wrong though and it's not for me to say -- that's just my impression.

TomEdwardsEnscape commented 1 year ago

Firstly, a correction: Avalonia does detach bindings when SetValue is called, but only for StyledProperty.

This means that the lack of SetCurrentValue affects every control that sets its own StyledProperty values. On master there are few if any which do this, because DirectProperty is used for these cases instead. Perhaps for this very reason.

The only problematic case I've noticed on master is where a two-way TemplateBinding sets a value on the source object. The source property could easily be a StyledProperty with a binding of its own.

However, the number of problematic cases will leap upwards when DirectProperty becomes read-only. I noticed ScrollViewer breaking on my branch, which is due to it having several sub-components which synchronise values between one another, yet also setting the scroll offset internally (e.g. when dragging the scroll bar or scrolling the mouse wheel). There will be more cases like that, but I haven't looked. Each one will be a serious bug.

Because this is a blocker for this issue, and because this issue in turn blocks my company's transition to Avalonia, I have actually already spent some time implementing SetCurrentValue. I want to write some more tests, then I'll submit a PR for review. It wasn't as complicated as I feared.

BTW, I will be occupied by other tasks between Tuesday and Friday and not able to work on this full time.

robloo commented 1 year ago

However, the number of problematic cases will leap upwards when DirectProperty becomes read-only

I wouldn't make DirectProerty read-only anytime soon. It is used in apps and we don't want to break any existing functionality there. I thought you were disabling that functionally only to identify areas that need to change in Avalonia. We certainly don't want to restrict direct property like this until this change in direction is well-tested and well-established. This is something @grokys also stated:

and read/write direct properties should be kept around for use by client code.

So I think we need to narrow the scope a bit -- which won't affect Avalonia or your companies transition I think.

However, the number of problematic cases will leap upwards when DirectProperty becomes read-only. I noticed ScrollViewer breaking on my branch

Yea, that's certainly more problematic and the kind of issues I was expected. My first thought was perhaps we need to ignore these places for now... however:

I have actually already spent some time implementing SetCurrentValue

That's great!

TomEdwardsEnscape commented 1 year ago

Those are fair points regarding outright removing DirectProperty setters and their supporting methods. Their continued presence won't hurt anyone.

But I would like to at least deprecate them with ObsoleteAttribute. We are here because DirectProperty setter functionality has already been restricted, and IMO writeable properties in new code should always use StyledProperty instead.

Gillibald commented 1 year ago

Syncing state between TextBox and TextPresenter is already a nightmare. Converting shared properties to StyledProperty makes this even harder. I don't see any reason to apply this conversion on TextBox/TextPresenter. Changing TextBlock isn't that of an issue.

robloo commented 1 year ago

I don't see any reason to apply this conversion on TextBox/TextPresenter. Changing TextBlock isn't that of an issue.

TextBlock.Text is the main one that needs to be switched to a styled property anyway: #7982

robloo commented 1 year ago

@tomenscape Just an update on this. I'll have time to get back to this in about two weeks. I'm hoping that is enough time before an RC but could be wrong. Before then, I'll complete the other two controls already started.

Hopefully I can then take up several more controls and we can work to merge in your branch piece by piece. I dont think there are any blocking PRs now.

grokys commented 1 year ago

I think I still need to add data validation support to styled properties. Will try to start that this week.

TomEdwardsEnscape commented 1 year ago

Spotted data validation yesterday while preparing the first refactor branch. That branch contains all of the easy refactorings, and I'll open a PR for it as soon as the data validation PR above is complete.

robloo commented 1 year ago

I've added a tracking list to the original issue description. Apologies for the length but this will help us move this issue to closure.

I will try to keep this up-to-date. I still have to go through the controls a bit more and find the ones that don't need to be updated (no direct properties).

TomEdwardsEnscape commented 1 year ago

The first PR is back! The next control I am going to extract from my mega branch is TextBlock. I see that TextBox has been marked as out of scope, but I actually converted it back when this issue was first opened. As predicted it was a bit of a pain, but the port was successful and the control now works very nicely and is fully styleable/animatable.

Maybe it's too big a change to accept at this stage? I think it can wait, since TextBox's nature as a user input control means it is far less likely to have styled values. But there is a catch: TextBox.TextProperty is the result of a call to TextBlock.TextProperty.AddOwner. This means that if we don't convert both to styled properties at the same time, we need to break that link and have two different Text properties floating around, each incompatible with the other. That could become confusing.

robloo commented 1 year ago

I see that TextBox has been marked as out of scope, but I actually converted it back when this issue was first opened. As predicted it was a bit of a pain, but the port was successful and the control now works very nicely and is fully styleable/animatable.

I think due to the complexity of the control there was not only worry about difficult changing these properties -- but issues with synchronization and secondary bugs. There still may be issues with SetCurrentValue() as well.

This means that if we don't convert both to styled properties at the same time, we need to break that link and have two different Text properties floating around, each incompatible with the other. That could become confusing.

That's a good point though. If you are confident with the changes it does sound best to do both. Hope there aren't a lot of issues. @Gillibald probably needs to give the final go-ahead.

For TextBlock.Text, don't forget about the original issue: https://github.com/AvaloniaUI/Avalonia/issues/7982

TomEdwardsEnscape commented 1 year ago

Next up: RangeBase and derived types.

robloo commented 1 year ago

@tomenscape I was planning to give RangeBase an attempt this weekend. As usual you beat me to it :) It seems like a good idea to target base types next. So I'll find another and mention it before I start. Hoping to do 1 or 2 this weekend.

robloo commented 1 year ago

I'm going to do Track and then AutoCompleteBox next. SelectingItemsControl probably is another good base type to target after that but it's going to take a bit more thought.

grokys commented 1 year ago

We're getting very close to 11.0-rc where breaking changes will be halted - what is the status of these changes? Looks like there are still a few controls to convert - are they difficult? Are they necessary?

TomEdwardsEnscape commented 1 year ago

SelectingItemsControl and descendants are important. There has been a lot of activity in ItemsControl recently though. Have things settled now?

grokys commented 1 year ago

SelectingItemsControl and descendants are important.

Agreed, do you already have changes to these controls partially completed, or should I handle this?

There has been a lot of activity in ItemsControl recently though. Have things settled now?

Yep, I don't think that there are any more incoming changes which will affect the properties of ItemsControl.

TomEdwardsEnscape commented 1 year ago

I started work on them a long time ago, but those changes are going to be incompatible with master now. Starting again is the best bet, so go ahead!

robloo commented 1 year ago

I'll do Track and ProgressBar by this weekend (not sure how much is required for these). Otherwise will find something else that needs it.

Does everyone agree that ItemsRepeater and DataGrid should be skipped for v11?

grokys commented 1 year ago

Does everyone agree that ItemsRepeater and DataGrid should be skipped for v11?

Fine with me!

grokys commented 1 year ago

I've run into a hitch when converting SelectingItemsControl's SelectedIndex and SelectedItem to be styled properties.

At the moment, they're simply projections of the SelectionModel's SelectedIndex and SelectedItem properties, and these properties only raise change notifications after both properties are updated and in sync.

If we switch to styled properties then when change notifications for SelectedIndex and SelectedItem are raised, they will not be in sync - one will necessarily change before the other.

Now, WPF also has this problem, e.g. in WPF:

  <ListBox Name="lb" ItemsSource="{Binding}"/>
            var items = new List<string> { "Item 0", "Item 1", "Item 2" };
            DataContext = items;

            var itemChanged = DependencyPropertyDescriptor.FromProperty(Selector.SelectedItemProperty, typeof(ListBox));

            itemChanged.AddValueChanged(lb, (s, e) =>
            {
                System.Diagnostics.Debug.WriteLine($"Selected item changed: {lb.SelectedItem} {lb.SelectedIndex}");
            });

            var indexChanged = DependencyPropertyDescriptor.FromProperty(Selector.SelectedIndexProperty, typeof(ListBox));

            indexChanged.AddValueChanged(lb, (s, e) =>
            {
                System.Diagnostics.Debug.WriteLine($"Selected index changed: {lb.SelectedItem} {lb.SelectedIndex}");
            });

You see that when the properties change, first SelectedIndex changes and SelectedItem is out of sync, then the notification fires for SelectedItem:

Selected index changed:  0
Selected item changed: Item 0 0
Selected index changed: Item 0 1
Selected item changed: Item 1 1
Selected index changed: Item 1 2
Selected item changed: Item 2 2

Are we OK with changing our SelectingItemsControls to have this behavior? In WPF it's less of a problem because it's harder to listen for individual property changed events, but in our own codebase I've already found a place where this breaks things (was easy enough to fix but may be confusing and hard to debug).

robloo commented 1 year ago

That's tricky and another special case the property system can't handle. I'm almost wondering if we shouldn't pick one to be styled and let the other be direct. Then it seems changes and events could be synronized. Not sure how I would like that design myself though.

Usually, I would just say when in doubt follow WPF when a better idea isn't yet found.

jp2masa commented 1 year ago

Maybe there could be a new property type (some kind of ProxyProperty), which would just act as a proxy to a state property?

Example: there would be a SelectionState property (styled), and then proxy properties like SelectedIndex and SelectedItem, which would reflect the state, with getters state => state.Index, state => state.Item, and setters (state, index) => state.WithIndex(index) and (state, item) => state.WithItem(item).

robloo commented 1 year ago

@jp2masa Proxy properties have come up a few times before as you probably know. Slightly different usage (usually to alias an existing property) but the functionality is similar.

My guess is it is too late in the 11.0 development process to introduce such a new property type.

jp2masa commented 1 year ago

Proxy properties have come up a few times before as you probably know.

You're right, it sounds familiar now that you point it out, I probably forgot it.

My guess is it is too late in the 11.0 development process to introduce such a new property type.

Yes, I guess it's too late as well. I was just suggesting it because I hate inconsistency and it was the best solution I could think about, even though there are probably better solutions.

robloo commented 1 year ago

Yes, I guess it's too late as well. I was just suggesting it because I hate inconsistency and it was the best solution I could think about, even though there are probably better solutions.

Definitely agree, I think I'll file another issue for that sometime as well. There are two property-system shortcoming that have come up in the past month or two (weren't solved in WPF either).

TomEdwardsEnscape commented 1 year ago

Another option is a "property update batch" object which defers notifications for property changes made during its lifetime until disposed. This can be initiated from a control's OnPropertyChanged, since it is executed first.

robloo commented 1 year ago

Track and ProgressBar were reviewed and didn't require any changes. ProgressBar was the most interesting case and it had several direct properties in TemplateSettings. However, these are considered out-of-scope as they are basically calculated and used in animations.

Next up is TreeView. There are only two properties to consider SelectedItem and SelectedItems. Did we resolve what to do about SelectedItems? Styled properties with lists are not really supported (I still have to make that other issues). So it seems to me only SelectedItem should change. I want to confirm that everyone is on the same page for this though:

https://github.com/AvaloniaUI/Avalonia/blob/0495f101748696779c05e6cc36ebae5f9a556aa1/src/Avalonia.Controls/TreeView.cs#L42-L46

robloo commented 1 year ago

Another option is a "property update batch" object which defers notifications for property changes made during its lifetime until disposed. This can be initiated from a control's OnPropertyChanged, since it is executed first.

To me that sounds like the only solution that might be doable in the 11.0 timeframe.

grokys commented 1 year ago

Did we resolve what to do about SelectedItems

We didn't, no. I have a WIP which converts SelectedItem and SelectedIndex in SelectingItemsControl to styled properties, but everything breaks :( Selection is a real PITA because there are so many competing sources of selection state, and all of them need to be in sync. We've had quite a few breakages in selection recently due to this. At this point I'm tempted to just say that selection properties need to stay as direct properties, at least until 12.0.

Another option is a "property update batch" object

We actually have this in 0.10.x but I removed it because it was

  1. Complicated and bug-prone. You need to handle situations such as

    • 8442 "running an empty batch update of >1 value which causes new, empty batch update to run in a PropertyChanged handler triggered from the first batch update"

    • 5686 "a property was cleared during a batch update and then re-set in the PropertyChanged notification raised on ending the batch update"

  2. Not great for performance, as allocations need to be made to store the state, and each property get/set needs to check for the presence of a batch update

I'd not be totally opposed to adding batch updates back in, but I'd like to think carefully about whether it's really needed. Maybe some kind of simpler mechanism could be introduced (thinking out loud: an API for setting multiple properties at once?)

grokys commented 1 year ago

ScrollBar, ScrollContentPresenter and MenuBase marked as done as the only remaining direct properties are read-only properties.

robloo commented 1 year ago

Did we resolve what to do about SelectedItems

We didn't, no. I have a WIP which converts SelectedItem and SelectedIndex in SelectingItemsControl to styled properties, but everything breaks :( Selection is a real PITA because there are so many competing sources of selection state, and all of them need to be in sync. We've had quite a few breakages in selection recently due to this. At this point I'm tempted to just say that selection properties need to stay as direct properties, at least until 12.0.

I'm tempted to say you're right. In use-cases I can think of I also don't see a reason one would actually want to set the SelectedItem or SelectedItems in styles. This just isn't really done. @tomenscape Can you think of some reasons you need this on your end? Otherwise, I think @grokys is right and we should drop it for now.

I also finally got around to creating a separate issue for lists in the property system: https://github.com/AvaloniaUI/Avalonia/issues/11285.

(thinking out loud: an API for setting multiple properties at once?)

That seems promising!

TomEdwardsEnscape commented 1 year ago

Our use case is described in #11220. The SelectedItem binding is usually made in the body of a UserControl, but it could conceivably be set via a style too.

It's not a high priority though, since an attached styled property can be used as a proxy.

grokys commented 1 year ago

an attached styled property can be used as a proxy.

Yeah I'd recommend this approach if selection really needs to be styleable. Just make sure you stick to a single selection property (per ItemsControl) and you shouldn't run into problems.

robloo commented 1 year ago

@grokys @tomenscape

ListBox, MenuFlyout, SelectingItemsConteol and TreeView are all that are left unchecked on the list.

Looks like MenuFlyout is done so will mark that complete.

TreeView, ListBox and SelectingItemsControl still have direct properties for SelectedItem and SelectedItems. It's not clear to me what should change with these. Due to the discussion above it sounds like:

  1. A test of converting these properties broke a lot of stuff. So its difficult to change them
  2. We are probably OK leaving them as direct for 11.x series. Work-arounds are available as mentioned above.
  3. This is really the only case that needs to be thought through further and may require changes to the property system to support things like SelectedItems binding (#11285).

With all of this in mind (and without doing a last DirectProperty search to final check) I think this issue can be closed.

Any objections or concerns?

robloo commented 1 year ago

ListBox, SelectingItemsConteol and TreeView were noted as partially complete in the list above with the comment:

Has SelectedItem, SelectedIndex or SelectedItems properties which were not fully converted due to the complexity discussed in this issue: https://github.com/AvaloniaUI/Avalonia/issues/9944#issuecomment-1538012823. This may be separately addressed in a future version of Avalonia.

As far as I know this completes review of the relevant control properties.

I'm closing this issue as complete since most properties were converted and no more will be done for v11. Great job everyone!