Convert Most DirectProperty Occurrences to StyledProperty
#9944
Comments
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.
(I think you meant "converted to styled") And to add another property: 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. |
Enumerables and lists/collections are almost a separate topic. It goes all the way back to WPF and
Thanks, fixed.
Yea, I'll add it and link to some of those older discussions.
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 |
|
Thanks for making this issue @robloo. I was looking at the new 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:
I would implement this change by first removing
Once the property is changed to |
|
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.
Yep, as @tomenscape mentioned I think coercion should be able to solve this. |
|
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. |
|
@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.
Yes, good idea. I spent too much time with UWP/WinUI and often forget to consider coercion like I should.
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.
Didn't know that myself. Will definitely have to add that in the future. |
|
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 |
|
@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 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. |
|
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. |
|
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. :) |
|
@tomenscape Ok, perfect, sounds like we are on the same page. Thanks for doing all this, really is great work! |
|
I've hit a bump in this project: there is no equivalent to WPF's A secondary issue is that there are two read-only binding 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 None of this is an issue for WPFHere's what WPF does: var oldSource = DependencyPropertyHelper.GetValueSource(this, DataContextProperty);
SetCurrentValue(DataContextProperty, new object());
var newSource = DependencyPropertyHelper.GetValueSource(this, DataContextProperty);Debugger output: 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. SolutionsI'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:
Any thoughts? |
|
@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. |
|
Firstly, a correction: Avalonia does detach bindings when This means that the lack of The only problematic case I've noticed on master is where a two-way However, the number of problematic cases will leap upwards when 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 BTW, I will be occupied by other tasks between Tuesday and Friday and not able to work on this full time. |
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:
So I think we need to narrow the scope a bit -- which won't affect Avalonia or your companies transition I think.
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:
That's great! |
|
Those are fair points regarding outright removing But I would like to at least deprecate them with |
|
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. |
This is a direct analogue to WPF's DependencyObject.SetCurrentValue method. It sets the effective value of the property without detaching any bindings or style setters applied to it. The existing SetValue method is sufficient when the property's value has local priority. If the priority is anything else, or unknown, SetCurrentValue should be used instead. In particular, it should be used when a control is setting its own StyledProperty values. This commit only calls SetCurrentValue in one place: TemplateBinding. More uses will be added in future commits.
This is a direct analogue to WPF's DependencyObject.SetCurrentValue method. It sets the effective value of the property without detaching any bindings or style setters applied to it. The existing SetValue method is sufficient when the property's value has local priority. If the priority is anything else, or unknown, SetCurrentValue should be used instead. In particular, it should be used when a control is setting its own StyledProperty values. This commit only calls SetCurrentValue in one place: TemplateBinding. More uses will be added in future commits.
|
This is a direct analogue to WPF's DependencyObject.SetCurrentValue method. It sets the effective value of the property without detaching any bindings or style setters applied to it. The existing SetValue method is sufficient when the property's value has local priority. If the priority is anything else, or unknown, SetCurrentValue should be used instead. In particular, it should be used when a control is setting its own StyledProperty values. This commit only calls SetCurrentValue in one place: TemplateBinding. More uses will be added in future commits.
|
I'm going to do |
|
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? |
|
|
Agreed, do you already have changes to these controls partially completed, or should I handle this?
Yep, I don't think that there are any more incoming changes which will affect the properties of |
|
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! |
|
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? |
Fine with me! |
|
I've run into a hitch when converting At the moment, they're simply projections of the If we switch to styled properties then when change notifications for 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 Are we OK with changing our |
|
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. |
|
Maybe there could be a new property type (some kind of Example: there would be a |
|
@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. |
You're right, it sounds familiar now that you point it out, I probably forgot it.
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). |
|
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 |
|
Next up is Avalonia/src/Avalonia.Controls/TreeView.cs Lines 42 to 46 in 0495f10 |
To me that sounds like the only solution that might be doable in the 11.0 timeframe. |
We didn't, no. I have a WIP which converts
We actually have this in 0.10.x but I removed it because it was
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?) |
|
|
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: #11285.
That seems promising! |
|
Our use case is described in #11220. The It's not a high priority though, since 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 |
|
@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:
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? |
|
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! |
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
DirectPropertyoccurrences should be converted toStyledPropertyso they can be used fully within styles. My original comment was:@tomenscape had some good points and narrowed the scope again:
This means ALL direct properties such as
TextBlock.Text,Expander.IsExpandedandSplitPane.IsPaneOpen(which are UI state information) would be converted to styled properties.Describe alternatives you've considered
There are two alternatives:
DirectPropertyas if it was aStyledProperty. No idea how to do this.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:
BorderIsPressedis a read-only property so remains unchangedButtonSpinnerDirectPropertydefinitions toStyledProperty#10370DirectPropertydefinitions toStyledProperty#10370CanvasCarouselCheckBoxDirectPropertydefinitions toStyledProperty#10370ContentControlDirectPropertydefinitions toStyledProperty#10370DataGridDirectPropertyOccurrences toStyledProperty#9944 (comment). It is already a separate package.DirectPropertydefinitions toStyledProperty#10370DatePickerPresenterDecoratorDockPanelDropDownButtonDirectPropertydefinitions toStyledProperty#10370GridGridSplitterImageItemsControlDirectPropertydefinitions toStyledProperty#10370ItemsRepeaterDirectPropertyOccurrences toStyledProperty#9944 (comment). It is already a separate package.DirectPropertydefinitions toStyledProperty#10370SelectedItem,SelectedIndexorSelectedItemsproperties which were not fully converted due to the complexity discussed in this issue: Convert MostDirectPropertyOccurrences toStyledProperty#9944 (comment). This may be separately addressed in a future version of Avalonia.MenuMenuItemContextMenuDirectPropertydefinitions toStyledProperty#10370DirectPropertydefinitions toStyledProperty#10370DirectPropertydefinitions toStyledProperty#10370PanelDirectPropertydefinitions toStyledProperty#10370Percentageis correct as a direct property. This is a read-only calculated value.RangeBaseDirectPropertydefinitions toStyledProperty#10370RangeBaseto useStyledProperty#10803RectangleRelativePanelRepeatButtonIsExpandedthat is likely already correctSelectedItem,SelectedIndexorSelectedItemsproperties which were not fully converted due to the complexity discussed in this issue: Convert MostDirectPropertyOccurrences toStyledProperty#9944 (comment). This may be separately addressed in a future version of Avalonia.SeparatorSliderSpinnerSplitButtonStackPanelTabControlTabItemTabStripDirectPropertydefinitions toStyledProperty#10370DirectPropertyOccurrences toStyledProperty#9944 (comment). However, it shares the TextBlock property using AddOwner so was decided to include and avoid breaking the link.DirectPropertydefinitions toStyledProperty#10370TimePickerPresenterDirectPropertydefinitions toStyledProperty#10370ToggleSplitButtonToggleSwitchToolTipTrackSelectedItem,SelectedIndexorSelectedItemsproperties which were not fully converted due to the complexity discussed in this issue: Convert MostDirectPropertyOccurrences toStyledProperty#9944 (comment). This may be separately addressed in a future version of Avalonia.DirectPropertydefinitions toStyledProperty#10370ViewboxWrapPanelDirectPropertydefinitions toStyledProperty#10370PathIconThe text was updated successfully, but these errors were encountered: