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.

Sign up for GitHub

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

Jump to bottom

Switch Expander.IsExpanded to a StyledProperty #9979

Merged
merged 7 commits into from
Jan 26, 2023
Merged

Switch Expander.IsExpanded to a StyledProperty #9979

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bc9f753
Switch Expander.IsExpanded to a StyledProperty
robloo Jan 15, 2023
a68cce8
Add CancelRoutedEventArgs and use it in Expander
robloo Jan 16, 2023
cfb90c8
Add Interactive.AddHandler() for CancelRoutedEventArgs
robloo Jan 16, 2023
ae78a2b
Revert "Add Interactive.AddHandler() for CancelRoutedEventArgs"
robloo Jan 18, 2023
c8c3b15
Fix expander event args type
robloo Jan 18, 2023
f635f17
Merge branch 'master' into expander-isexpanded-styled-prop
robloo Jan 19, 2023
ddd67de
Merge branch 'master' into expander-isexpanded-styled-prop
maxkatz6 Jan 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions samples/ControlCatalog/Pages/ExpanderPage.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ public ExpanderPage()
var CollapsingDisabledExpander = this.Get<Expander>("CollapsingDisabledExpander");
var ExpandingDisabledExpander = this.Get<Expander>("ExpandingDisabledExpander");

CollapsingDisabledExpander.Collapsing += (s, e) => { e.Handled = true; };
ExpandingDisabledExpander.Expanding += (s, e) => { e.Handled = true; };
CollapsingDisabledExpander.Collapsing += (s, e) => { e.Cancel = true; };
ExpandingDisabledExpander.Expanding += (s, e) => { e.Cancel = true; };
}

private void InitializeComponent()
Expand Down
39 changes: 39 additions & 0 deletions src/Avalonia.Base/Interactivity/CancelRoutedEventArgs.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
namespace Avalonia.Interactivity
{
/// <summary>
/// Provides state information and data specific to a cancelable routed event.
/// </summary>
public class CancelRoutedEventArgs : RoutedEventArgs
{
/// <summary>
/// Initializes a new instance of the <see cref="CancelRoutedEventArgs"/> class.
/// </summary>
public CancelRoutedEventArgs()
{
}

/// <summary>
/// Initializes a new instance of the <see cref="CancelRoutedEventArgs"/> class.
/// </summary>
/// <param name="routedEvent">The routed event associated with these event args.</param>
public CancelRoutedEventArgs(RoutedEvent? routedEvent)
: base(routedEvent)
{
}

/// <summary>
/// Initializes a new instance of the <see cref="CancelRoutedEventArgs"/> class.
/// </summary>
/// <param name="routedEvent">The routed event associated with these event args.</param>
/// <param name="source">The source object that raised the routed event.</param>
public CancelRoutedEventArgs(RoutedEvent? routedEvent, object? source)
: base(routedEvent, source)
{
}

/// <summary>
/// Gets or sets a value indicating whether the routed event should be canceled.
/// </summary>
public bool Cancel { get; set; } = false;
}
}
130 changes: 72 additions & 58 deletions src/Avalonia.Controls/Expander.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,11 @@ public class Expander : HeaderedContentControl
/// <summary>
/// Defines the <see cref="IsExpanded"/> property.
/// </summary>
public static readonly DirectProperty<Expander, bool> IsExpandedProperty =
AvaloniaProperty.RegisterDirect<Expander, bool>(
public static readonly StyledProperty<bool> IsExpandedProperty =
AvaloniaProperty.Register<Expander, bool>(
nameof(IsExpanded),
o => o.IsExpanded,
(o, v) => o.IsExpanded = v,
defaultBindingMode: Data.BindingMode.TwoWay);
defaultBindingMode: BindingMode.TwoWay,
coerce: CoerceIsExpanded);
maxkatz6 marked this conversation as resolved.
Show resolved Hide resolved

/// <summary>
/// Defines the <see cref="Collapsed"/> event.
Expand All @@ -77,8 +76,8 @@ public class Expander : HeaderedContentControl
/// <summary>
/// Defines the <see cref="Collapsing"/> event.
/// </summary>
public static readonly RoutedEvent<RoutedEventArgs> CollapsingEvent =
RoutedEvent.Register<Expander, RoutedEventArgs>(
public static readonly RoutedEvent<CancelRoutedEventArgs> CollapsingEvent =
RoutedEvent.Register<Expander, CancelRoutedEventArgs>(
nameof(Collapsing),
RoutingStrategies.Bubble);

Expand All @@ -93,13 +92,12 @@ public class Expander : HeaderedContentControl
/// <summary>
/// Defines the <see cref="Expanding"/> event.
/// </summary>
public static readonly RoutedEvent<RoutedEventArgs> ExpandingEvent =
RoutedEvent.Register<Expander, RoutedEventArgs>(
public static readonly RoutedEvent<CancelRoutedEventArgs> ExpandingEvent =
RoutedEvent.Register<Expander, CancelRoutedEventArgs>(
nameof(Expanding),
RoutingStrategies.Bubble);

private bool _ignorePropertyChanged = false;
private bool _isExpanded;
private CancellationTokenSource? _lastTransitionCts;

/// <summary>
Expand Down Expand Up @@ -134,50 +132,8 @@ public ExpandDirection ExpandDirection
/// </summary>
public bool IsExpanded
{
get => _isExpanded;
set
{
// It is important here that IsExpanded is a direct property so events can be invoked
// BEFORE the property system gets notified of updated values. This is because events
// may be canceled by external code.
if (_isExpanded != value)
{
RoutedEventArgs eventArgs;

if (value)
{
eventArgs = new RoutedEventArgs(ExpandingEvent, this);
OnExpanding(eventArgs);
}
else
{
eventArgs = new RoutedEventArgs(CollapsingEvent, this);
OnCollapsing(eventArgs);
}

if (eventArgs.Handled)
{
// If the event was externally handled (canceled) we must still notify the value has changed.
// This property changed notification will update any external code observing this property that itself may have set the new value.
// We are essentially reverted any external state change along with ignoring the IsExpanded property set.
// Remember IsExpanded is usually controlled by a ToggleButton in the control theme.
_ignorePropertyChanged = true;

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

_ignorePropertyChanged = false;
}
else
{
SetAndRaise(IsExpandedProperty, ref _isExpanded, value);
}
}
}
get => GetValue(IsExpandedProperty);
set => SetValue(IsExpandedProperty, value);
}

/// <summary>
Expand All @@ -193,10 +149,10 @@ public event EventHandler<RoutedEventArgs>? Collapsed
/// Occurs as the content area is closing.
/// </summary>
/// <remarks>
/// The event args <see cref="RoutedEventArgs.Handled"/> property may be set to true to cancel the event
/// The event args <see cref="CancelRoutedEventArgs.Cancel"/> property may be set to true to cancel the event
/// and keep the control open (expanded).
/// </remarks>
public event EventHandler<RoutedEventArgs>? Collapsing
public event EventHandler<CancelRoutedEventArgs>? Collapsing
{
add => AddHandler(CollapsingEvent, value);
remove => RemoveHandler(CollapsingEvent, value);
Expand All @@ -215,10 +171,10 @@ public event EventHandler<RoutedEventArgs>? Expanded
/// Occurs as the content area is opening.
/// </summary>
/// <remarks>
/// The event args <see cref="RoutedEventArgs.Handled"/> property may be set to true to cancel the event
/// The event args <see cref="CancelRoutedEventArgs.Cancel"/> property may be set to true to cancel the event
/// and keep the control closed (collapsed).
/// </remarks>
public event EventHandler<RoutedEventArgs>? Expanding
public event EventHandler<CancelRoutedEventArgs>? Expanding
{
add => AddHandler(ExpandingEvent, value);
remove => RemoveHandler(ExpandingEvent, value);
Expand Down Expand Up @@ -332,5 +288,63 @@ private void UpdatePseudoClasses()

PseudoClasses.Set(":expanded", IsExpanded);
}

/// <summary>
/// Called when the <see cref="IsExpanded"/> property has to be coerced.
/// </summary>
/// <param name="value">The value to coerce.</param>
protected virtual bool OnCoerceIsExpanded(bool value)
{
CancelRoutedEventArgs eventArgs;

if (value)
{
eventArgs = new CancelRoutedEventArgs(ExpandingEvent, this);
OnExpanding(eventArgs);
}
else
{
eventArgs = new CancelRoutedEventArgs(CollapsingEvent, this);
OnCollapsing(eventArgs);
}

if (eventArgs.Cancel)
{
// If the event was externally canceled we must still notify the value has changed.
// This property changed notification will update any external code observing this property that itself may have set the new value.
// We are essentially reverted any external state change along with ignoring the IsExpanded property set.
// Remember IsExpanded is usually controlled by a ToggleButton in the control theme and is also used for animations.
_ignorePropertyChanged = true;

RaisePropertyChanged(
IsExpandedProperty,
oldValue: value,
newValue: !value,
BindingPriority.LocalValue,
isEffectiveValue: true);
Comment on lines +319 to +324
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@robloo robloo Jan 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@robloo robloo Jan 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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:

  1. Sender changes value (say true -> false)
  2. Receiver coerces value
  3. Coercion raises cancel events and external code cancels the change (value switch false -> true)
  4. The value 'true' is set to the control... but this is the same as it was before. Therefore no change notification and updates.
  5. 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


_ignorePropertyChanged = false;

return !value;
}

return value;
}

/// <summary>
/// Coerces/validates the <see cref="IsExpanded"/> property value.
/// </summary>
/// <param name="instance">The <see cref="Expander"/> instance.</param>
/// <param name="value">The value to coerce.</param>
/// <returns>The coerced/validated value.</returns>
private static bool CoerceIsExpanded(AvaloniaObject instance, bool value)
{
if (instance is Expander expander)
{
return expander.OnCoerceIsExpanded(value);
}

return value;
}
}
}