Switch Expander.IsExpanded to a StyledProperty #9979

robloo · 2023-01-15T03:39:44Z

What does the pull request do?

Switches the Expander.IsExpanded property to a StyledProperty and adds a new CancelRoutedEventArgs based on CancelEventArgs.

This is an update to PR #9555 based on discussion in #9944

What is the current behavior?

Expander.IsExpanded is a DirectProperty and cannot be used with Styling.

What is the updated/expected behavior with this PR?

Expander.IsExpanded is now a full StyledProperty and usable in the styling system.
New CancelRoutedEventArgs used for both the Expanding and Collapsing events based on CancelEventArgs.

How was the solution implemented (if it's not obvious)?

See code.

I had to go a different direction than what @tomenscape has done in #9944's working branch. It was necessary to add back some of the original code to:

Call OnExpanding()/OnCollapsing() internally
Notify the property has changed even when it is cancelled. This ensures the control templates toggle button doesn't desync and the animation states are correct.

Checklist

~~[ ] Added unit tests (if possible)?~~
Added XML documentation to any related classes?
~~[ ] Consider submitting a PR to https://github.com/AvaloniaUI/Documentation with user documentation~~

Breaking changes

Yes, StyledProperty instead of DirectProperty but functionality is largely only expanded for app developers.

Obsoletions / Deprecations

None

Fixed issues

Relates to #9944

avaloniaui-team · 2023-01-15T04:09:04Z

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

TomEdwardsEnscape · 2023-01-15T09:53:12Z

src/Avalonia.Controls/Expander.cs

+                RaisePropertyChanged(
+                    IsExpandedProperty,
+                    oldValue: value,
+                    newValue: !value,
+                    BindingPriority.LocalValue,
+                    isEffectiveValue: true);


It's pretty weird that this can be done for a StyledProperty, which is meant to automatically manage value storage and change events.

This method call is claiming that a) the new value has local priority and b) that it's an effective value change. But the actual property might not agree with this. What happens when IsExpanded has a two-way binding within a template, for example?

Thinking about it some more, this is a local solution to a general problem. The problem is that when two-way bindings transfer a new value in either direction, they assume that it was accepted by the recipient.

If the recipient object changes the value to something other than the one that was set, a property changed event is raised and the binding can update the sender object. But if the recipient object instead ignores the new value (as is the case here), then no event is raised and no action is taken.

Hopefully we can go with the local fix here, but this is something that should also be considered.

I think everyone agrees that this is non-ideal and kind-of 'hacky'. It was first discussed here: #9555 (comment).

This is why I was hoping coercion would handle this automatically for us but unfortunately it doesn't. Therefore, I had to add this back (you can remove it and test the control catalog to see the issues). I think WPF has issues here as well but for sure it would be better to improve coercion and actually handle the real property value that comes back after coercion itself.

For now, it must be done to ensure external code state is correct. (It is essentially just used to notify of a property changed event and force updates) Keep in mind the expander itself isn't the main 'source' of the IsExpanded property -- it is actually the checked state of a toggle button in the control template... so we have to be careful about this.

The problem is that when two-way bindings transfer a new value in either direction, they assume that it was accepted by the recipient.

Yes, that's the real issue. One I think exists in WPF as well.

the new value has local priority

That's something to think about some more I suppose. I copy pasted the code and when it was a direct-property LocalValue made sense. Now perhaps it doesn't. Since it isn't actually setting the value here though -- maybe it's harmless the value used here.

This is why I was hoping coercion would handle this automatically for us but unfortunately it doesn't.

Yeah I think we should probably make coercion handle this if it's not too difficult. What doesn't work with coercion?

What doesn't work with coercion?

Here was a nice summary:

If the recipient object changes the value to something other than the one that was set, a property changed event is raised and the binding can update the sender object. But if the recipient object instead ignores the new value (as is the case here), then no event is raised and no action is taken.

Basically when a cancel happens in the coerce method the property value is reverted back to its original value. This means the value hasn't actually changed and no change notification happens -- seems to make sense. However, if two controls are two-way bound this actually causes a desync. Because the sender will still change its state.

My simple understanding (without looking at any control flow) is:

Sender changes value (say true -> false)

Receiver coerces value

Coercion raises cancel events and external code cancels the change (value switch false -> true)

The value 'true' is set to the control... but this is the same as it was before. Therefore no change notification and updates.

The sender still has a false value instead of true because it never got a change notification because the value didn't actually change.

So an additional check is needed to see if coercion changes the value to a value other than what went into the coerce method. If so, and there is a two-way binding, it needs to notify the sender of a change even when the value set to the control is actually the same.

True, and the ViewModel case is the issue I've seen in WPF before if I recall correctly. So this is an old problem.

Still having a bit of trouble understanding. What are the sender and receiver in the case of Expander.IsExpanded?

@grokys In the example I gave:

sender : This is the IsChecked property of the ToggleButton within the control template.

receiver : The actual IsExpanded property of the Expander control itself

This is a general problem though with two-way binding if the "receiver" doesn't accept the sender's value for whatever reason.

Ok, thanks for the clarification, that makes sense.

The problem with the RaisePropertyChanged hack here is that this method is internal so the hack will only work within Avalonia itself. We probably need to work out how to do this properly.

Supporting cancellation in events is a fairly common practice from my experience and the API is mostly standardized as discussed above. It really should be supported in a UI framework IMO. Flyout is another example. If it currently requires an internal-use-only work-around for now that seems justifiable to me (especially a harmless one). The issue with the property system has existed for almost 20 years.

It's common in some cases to need to raise a property changed notification "hack" in apps as well. Especially due to the view model example above. It might also be possible to promote this idea from a "hack" into a "feature" by allowing a clean way to force a refresh.

src/Avalonia.Controls/Expander.cs

robloo · 2023-01-16T01:32:28Z

Tests are failing because this:

        public void AddHandler<TEventArgs>(
            RoutedEvent<TEventArgs> routedEvent,
            EventHandler<TEventArgs>? handler,
            RoutingStrategies routes = RoutingStrategies.Direct | RoutingStrategies.Bubble,
            bool handledEventsToo = false) where TEventArgs : RoutedEventArgs

Is not matching for the CancelRoutedEventArgs handler... which derives directly from RoutedEventArgs instead this method is called:

        public void AddHandler(
            RoutedEvent routedEvent,
            Delegate handler,
            RoutingStrategies routes = RoutingStrategies.Direct | RoutingStrategies.Bubble,
            bool handledEventsToo = false)

This almost seems like a compiler bug to me. If this really doesn't work there is no point in even having that method generic.

grokys · 2023-01-16T09:44:12Z

Is not matching for the CancelRoutedEventArgs handler... which derives directly from RoutedEventArgs instead this method is called:

I think we can probably remove the overload that accepts a Delegate?

robloo · 2023-01-16T13:01:29Z

Is not matching for the CancelRoutedEventArgs handler... which derives directly from RoutedEventArgs instead this method is called:

I think we can probably remove the overload that accepts a Delegate?

Unfortunately, it's a deeper than that. The overload that accept a delegate is required to implement IInputElement. I suspect it exists there for a reason as well. I'm not sure the method matching precedence in the compiler but this really doesn't make sense.

However, it should be possible to fix this by adding a third method that works only with CancelRoutedEventArgs. It's kind-of a waste but seems to be a necessary work-around.

This hopefully fixes unit tests due to wrong overload being selected by the compiler.

TomEdwardsEnscape · 2023-01-16T14:26:23Z

This almost seems like a compiler bug to me.

Perhaps it is. I ran all tests locally on the previous commit in your branch, i.e. without AddHandler, and they succeeded. I'm using the latest build (17.4.4) of VS 2022. Which test was failing for you?

robloo · 2023-01-16T16:35:50Z

Which test was failing for you?

All tests are failing in the PR (which you might not be able to see) because it won't compile -- it isn't an actual test failure:

[ERR] Compile: D:\a\1\s\src\Avalonia.Controls\Expander.cs(157,48): error CS8604: Possible null reference argument for parameter 'handler' in 'void Interactive.AddHandler(RoutedEvent routedEvent, Delegate handler, RoutingStrategies routes = RoutingStrategies.Direct | RoutingStrategies.Bubble, bool handledEventsToo = false)'. [D:\a\1\s\src\Avalonia.Controls\Avalonia.Controls.csproj::TargetFramework=net6.0]
[ERR] Compile: D:\a\1\s\src\Avalonia.Controls\Expander.cs(158,54): error CS8604: Possible null reference argument for parameter 'handler' in 'void Interactive.RemoveHandler(RoutedEvent routedEvent, Delegate handler)'. [D:\a\1\s\src\Avalonia.Controls\Avalonia.Controls.csproj::TargetFramework=net6.0]
[ERR] Compile: D:\a\1\s\src\Avalonia.Controls\Expander.cs(166,46): error CS8604: Possible null reference argument for parameter 'handler' in 'void Interactive.AddHandler(RoutedEvent routedEvent, Delegate handler, RoutingStrategies routes = RoutingStrategies.Direct | RoutingStrategies.Bubble, bool handledEventsToo = false)'. [D:\a\1\s\src\Avalonia.Controls\Avalonia.Controls.csproj::TargetFramework=net6.0]
[ERR] Compile: D:\a\1\s\src\Avalonia.Controls\Expander.cs(167,52): error CS8604: Possible null reference argument for parameter 'handler' in 'void Interactive.RemoveHandler(RoutedEvent routedEvent, Delegate handler)'. [D:\a\1\s\src\Avalonia.Controls\Avalonia.Controls.csproj::TargetFramework=net6.0]

Setting a breakpoint on the AddHandler methods and running the code confirmed that even on my local computer the wrong method is getting called. It's possible that the build server is set to treat these warnings as errors too which is why it isn't showing up locally (I didn't have a compile problem either).

Unfortunately, even the new AddHandler method with explicit CancelRoutedEventArgs didn't fix it -- which is very, very strange. Not sure where to go from here and will think about it some more.

TomEdwardsEnscape · 2023-01-17T20:45:40Z

I explicitly added generic parameters to the AddHandler calls and intellisense revealed the problem: you changed ExpandedEvent to CancelRoutedEventArgs, when you meant to change CollapsingEvent. These two events are on the lines mentioned by the compiler error.

Not sure why error CS8604 is appearing on the CI but not in local builds. Another thing to fix!

robloo · 2023-01-17T22:19:52Z

@tomenscape Well, I feel dumb and should have double checked it wasnt something simple. Thanks a lot for taking a look!

This reverts commit cfb90c8.

avaloniaui-team · 2023-01-18T15:26:04Z

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

avaloniaui-team · 2023-01-19T01:57:46Z

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

maxkatz6 · 2023-01-26T06:18:33Z

Do I understand it right, the only problem with this PR is RaisePropertyChanged hack?
If so, can we merge it and move RaisePropertyChanged discussion to the new issue?

robloo · 2023-01-26T13:08:01Z

Do I understand it right, the only problem with this PR is RaisePropertyChanged hack?

Yes

If so, can we merge it and move RaisePropertyChanged discussion to the new issue?

I've created a new issue: #10092.

It will take a while before I could dig into the property system and attempt a fix. Otherwise, I'm probably not the best person to do it (I do have to learn the internals here better someday though). This really does seem harmless for now to me.

avaloniaui-team · 2023-01-26T19:26:25Z

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

Switch Expander.IsExpanded to a StyledProperty

bc9f753

TomEdwardsEnscape reviewed Jan 15, 2023

View reviewed changes

robloo commented Jan 15, 2023

View reviewed changes

src/Avalonia.Controls/Expander.cs Outdated Show resolved Hide resolved

Add CancelRoutedEventArgs and use it in Expander

a68cce8

maxkatz6 reviewed Jan 16, 2023

View reviewed changes

src/Avalonia.Controls/Expander.cs Show resolved Hide resolved

grokys mentioned this pull request Jan 16, 2023

Add Expander Events #9555

Merged

2 tasks

Add Interactive.AddHandler() for CancelRoutedEventArgs

cfb90c8

This hopefully fixes unit tests due to wrong overload being selected by the compiler.

robloo added 2 commits January 18, 2023 09:55

Revert "Add Interactive.AddHandler() for CancelRoutedEventArgs"

ae78a2b

This reverts commit cfb90c8.

Fix expander event args type

c8c3b15

Merge branch 'master' into expander-isexpanded-styled-prop

f635f17

robloo mentioned this pull request Jan 26, 2023

Missing Property Change Notification to Source when Receiver Rejects/Reverts Change #10092

Open

maxkatz6 approved these changes Jan 26, 2023

View reviewed changes

maxkatz6 enabled auto-merge January 26, 2023 18:54

Merge branch 'master' into expander-isexpanded-styled-prop

ddd67de

maxkatz6 merged commit 2072b19 into AvaloniaUI:master Jan 26, 2023

robloo deleted the expander-isexpanded-styled-prop branch January 26, 2023 19:47

jp2masa mentioned this pull request Feb 5, 2023

Update to Avalonia 11 oxyplot/oxyplot-avalonia#49

Open

robloo mentioned this pull request Feb 26, 2023

Convert Most DirectProperty Occurrences to StyledProperty #9944

Closed

69 tasks

robloo mentioned this pull request Apr 24, 2024

New ToolTipClosing, ToolTipOpening attached events and ToolTip.Opened, ToolTip.Closed #15493

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch Expander.IsExpanded to a StyledProperty #9979

Switch Expander.IsExpanded to a StyledProperty #9979

robloo commented Jan 15, 2023 •

edited

Loading

avaloniaui-team commented Jan 15, 2023

TomEdwardsEnscape Jan 15, 2023

TomEdwardsEnscape Jan 15, 2023

robloo Jan 15, 2023 •

edited

Loading

grokys Jan 16, 2023

robloo Jan 16, 2023 •

edited

Loading

robloo Jan 16, 2023

grokys Jan 17, 2023

robloo Jan 17, 2023

grokys Jan 17, 2023

robloo Jan 17, 2023

robloo commented Jan 16, 2023

grokys commented Jan 16, 2023

robloo commented Jan 16, 2023

TomEdwardsEnscape commented Jan 16, 2023

robloo commented Jan 16, 2023

TomEdwardsEnscape commented Jan 17, 2023

robloo commented Jan 17, 2023

avaloniaui-team commented Jan 18, 2023

avaloniaui-team commented Jan 19, 2023

maxkatz6 commented Jan 26, 2023

robloo commented Jan 26, 2023

avaloniaui-team commented Jan 26, 2023

Switch Expander.IsExpanded to a StyledProperty #9979

Switch Expander.IsExpanded to a StyledProperty #9979

Conversation

robloo commented Jan 15, 2023 • edited Loading

What does the pull request do?

What is the current behavior?

What is the updated/expected behavior with this PR?

How was the solution implemented (if it's not obvious)?

Checklist

Breaking changes

Obsoletions / Deprecations

Fixed issues

avaloniaui-team commented Jan 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robloo Jan 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robloo Jan 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robloo commented Jan 16, 2023

grokys commented Jan 16, 2023

robloo commented Jan 16, 2023

TomEdwardsEnscape commented Jan 16, 2023

robloo commented Jan 16, 2023

TomEdwardsEnscape commented Jan 17, 2023

robloo commented Jan 17, 2023

avaloniaui-team commented Jan 18, 2023

avaloniaui-team commented Jan 19, 2023

maxkatz6 commented Jan 26, 2023

robloo commented Jan 26, 2023

avaloniaui-team commented Jan 26, 2023

robloo commented Jan 15, 2023 •

edited

Loading

robloo Jan 15, 2023 •

edited

Loading

robloo Jan 16, 2023 •

edited

Loading