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

fix(binding): Avoid reading source property on UpdateSource for non-DP #13698

Merged
merged 3 commits into from
Sep 20, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -862,6 +862,85 @@ private async Task<RawBitmap> TakeScreenshot(FrameworkElement SUT)
}
#endif

[TestMethod]
public async Task When_SelectedItem_TwoWay_Binding_Clear()
{
var root = new Grid();

var comboBox = new ComboBox();

root.Children.Add(comboBox);

comboBox.SetBinding(ComboBox.ItemsSourceProperty, new Binding { Path = new("Items") });
comboBox.SetBinding(ComboBox.SelectedItemProperty, new Binding { Path = new("Item"), Mode = BindingMode.TwoWay });

WindowHelper.WindowContent = root;

var dc = new TwoWayBindingClearViewModel();
root.DataContext = dc;

await WindowHelper.WaitForIdle();

dc.Dispose();
root.DataContext = null;

Assert.AreEqual(1, dc.ItemGetCount);
Assert.AreEqual(1, dc.ItemSetCount);
}

public sealed class TwoWayBindingClearViewModel : IDisposable
{
public enum Themes
{
Invalid,
Day,
Night
}

public TwoWayBindingClearViewModel()
{
_item = Items[0];
}

public Themes[] Items { get; } = new Themes[] { Themes.Day, Themes.Night };
private Themes _item;
private bool _isDisposed;
public Themes Item
{
get
{
ItemGetCount++;

if (_isDisposed)
{
return Themes.Invalid;
}

return _item;
}
set
{
ItemSetCount++;

if (_isDisposed)
{
_item = Themes.Invalid;
return;
}

_item = value;
}
}

public int ItemGetCount { get; private set; }
public int ItemSetCount { get; private set; }

public void Dispose()
{
_isDisposed = true;
}
}

public class TwoWayBindingItem : System.ComponentModel.INotifyPropertyChanged
{
private int _selectedNumber;
Expand Down Expand Up @@ -932,6 +1011,8 @@ protected override void OnCollectionChanged(NotifyCollectionChangedEventArgs e)
}

}


#if __IOS__
#region "Helper classes for the iOS Modal Page (UIModalPresentationStyle.pageSheet)"
public partial class MultiFrame : Grid
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,9 @@ public async Task When_Animate()
await target.GetValue(ct, 3);
await Task.Yield();

target.History.Should().BeEquivalentTo(v1, v2, v3);
// v3 is repeated because the target property is not a DependencyProperty
// and no deduplication happens in the binding engine.
target.History.Should().BeEquivalentTo(v1, v2, v3, v3);
sut.State.Should().Be(Timeline.TimelineState.Filling);
}

Expand Down Expand Up @@ -147,7 +149,9 @@ public async Task When_PauseResume()
await target.GetValue(ct, 3);
await Task.Yield();

target.History.Should().BeEquivalentTo(v1, v2, v3);
// v3 is repeated because the target property is not a DependencyProperty
// and no deduplication happens in the binding engine.
target.History.Should().BeEquivalentTo(v1, v2, v3, v3);
sut.State.Should().Be(Timeline.TimelineState.Filling);
}

Expand Down Expand Up @@ -178,7 +182,9 @@ public async Task When_RepeatCount()
await target.GetValue(ct, 9);
await Task.Yield();

target.History.Should().BeEquivalentTo(v1, v2, v3, v1, v2, v3, v1, v2, v3);
// v3 is repeated because the target property is not a DependencyProperty
// and no deduplication happens in the binding engine.
target.History.Should().BeEquivalentTo(v1, v2, v3, v1, v2, v3, v1, v2, v3, v3);
sut.State.Should().Be(Timeline.TimelineState.Filling);
}

Expand Down
89 changes: 76 additions & 13 deletions src/Uno.UI/DataBinding/BindingPath.BindingItem.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
using System;
using Uno.Disposables;
using Uno.Foundation.Logging;
using Uno.UI.Xaml.Input;
using Windows.ApplicationModel.Appointments;
using Windows.UI.Xaml;

namespace Uno.UI.DataBinding
Expand All @@ -15,12 +17,10 @@ private sealed class BindingItem : IBindingItem, IDisposable
private delegate void PropertyChangedHandler(object? previousValue, object? newValue, bool shouldRaiseValueChanged);

private ManagedWeakReference? _dataContextWeakStorage;
private Flags _flags;

private readonly SerialDisposable _propertyChanged = new SerialDisposable();
private bool _disposed;
private readonly DependencyPropertyValuePrecedences? _precedence;
private readonly object? _fallbackValue;
private readonly bool _allowPrivateMembers;
private ValueGetterHandler? _valueGetter;
private ValueGetterHandler? _precedenceSpecificGetter;
private ValueGetterHandler? _substituteValueGetter;
Expand All @@ -32,26 +32,35 @@ private sealed class BindingItem : IBindingItem, IDisposable

private Type? _dataContextType;

public BindingItem(BindingItem next, string property, object fallbackValue) :
this(next, property, fallbackValue, null, false)
[Flags]
private enum Flags
{
None = 0,
Disposed = 1 << 0,
AllowPrivateMembers = 1 << 1,
IsDependencyProperty = 1 << 2,
IsDependencyPropertyValueSet = 1 << 3,
}

internal BindingItem(BindingItem? next, string property, object? fallbackValue, DependencyPropertyValuePrecedences? precedence, bool allowPrivateMembers)
public BindingItem(BindingItem next, string property) :
this(next, property, null, false)
{
}

internal BindingItem(BindingItem? next, string property, DependencyPropertyValuePrecedences? precedence, bool allowPrivateMembers)
{
Next = next;
PropertyName = property;
_precedence = precedence;
_fallbackValue = fallbackValue;
_allowPrivateMembers = allowPrivateMembers;
AllowPrivateMembers = allowPrivateMembers;
}

public object? DataContext
{
get => _dataContextWeakStorage?.Target;
set
{
if (!_disposed)
if (!IsDisposed)
{
// Historically, Uno was processing property changes using INPC. Since the inclusion of DependencyObject
// values changes are now filtered by DependencyProperty updates, making equality updates at this location
Expand Down Expand Up @@ -131,7 +140,7 @@ public Type? PropertyType
{
if (DataContext != null)
{
return BindingPropertyHelper.GetPropertyType(_dataContextType!, PropertyName, _allowPrivateMembers);
return BindingPropertyHelper.GetPropertyType(_dataContextType!, PropertyName, AllowPrivateMembers);
}
else
{
Expand Down Expand Up @@ -226,6 +235,7 @@ private void ClearCachedGetters()

if (_dataContextType != currentType && _dataContextType != null)
{
IsDependencyPropertyValueSet = false;
_valueGetter = null;
_precedenceSpecificGetter = null;
_substituteValueGetter = null;
Expand Down Expand Up @@ -312,15 +322,15 @@ private void BuildValueGetter()
{
if (_valueGetter == null && _dataContextType != null)
{
_valueGetter = BindingPropertyHelper.GetValueGetter(_dataContextType, PropertyName, _precedence, _allowPrivateMembers);
_valueGetter = BindingPropertyHelper.GetValueGetter(_dataContextType, PropertyName, _precedence, AllowPrivateMembers);
}
}

private void BuildPrecedenceSpecificValueGetter()
{
if (_precedenceSpecificGetter == null && _dataContextType != null)
{
_precedenceSpecificGetter = BindingPropertyHelper.GetValueGetter(_dataContextType, PropertyName, _precedence, _allowPrivateMembers);
_precedenceSpecificGetter = BindingPropertyHelper.GetValueGetter(_dataContextType, PropertyName, _precedence, AllowPrivateMembers);
}
}

Expand Down Expand Up @@ -478,10 +488,63 @@ private IDisposable SubscribeToPropertyChanged()

public void Dispose()
{
_disposed = true;
IsDisposed = true;
_propertyChanged.Dispose();
}

private bool IsDisposed
{
get => (_flags & Flags.Disposed) != 0;
set => SetFlag(value, Flags.Disposed);
}

private bool AllowPrivateMembers
{
get => (_flags & Flags.AllowPrivateMembers) != 0;
set => SetFlag(value, Flags.AllowPrivateMembers);
}

private bool IsDependencyPropertyValueSet
{
get => (_flags & Flags.IsDependencyPropertyValueSet) != 0;
set => SetFlag(value, Flags.IsDependencyPropertyValueSet);
}

internal bool IsDependencyProperty
{
get
{
if (!IsDependencyPropertyValueSet)
{
var isDP =
_dataContextType is not null
&& DependencyProperty.GetProperty(_dataContextType!, PropertyName) is not null;

SetFlag(isDP, Flags.IsDependencyProperty);

IsDependencyPropertyValueSet = true;

return isDP;
}
else
{
return (_flags & Flags.IsDependencyProperty) != 0;
}
}
}

private void SetFlag(bool value, Flags flag)
{
if (!value)
{
_flags &= ~flag;
}
else
{
_flags |= flag;
}
}

/// <summary>
/// Property changed value handler, used to avoid creating a delegate for processing
/// </summary>
Expand Down
12 changes: 9 additions & 3 deletions src/Uno.UI/DataBinding/BindingPath.cs
Original file line number Diff line number Diff line change
Expand Up @@ -195,8 +195,14 @@ public object? Value
{
if (!_disposed
&& _value != null
&& DependencyObjectStore.AreDifferent(value, _value.GetPrecedenceSpecificValue())
)
&& (
!_value.IsDependencyProperty

// Don't get the source value if we're not accessing a dependency property.
// WinUI does not read the property value before setting the value for a
// non-dependency property source.
jeromelaban marked this conversation as resolved.
Show resolved Hide resolved
|| DependencyObjectStore.AreDifferent(value, _value.GetPrecedenceSpecificValue())
))
{
_value.Value = value;
}
Expand Down Expand Up @@ -428,7 +434,7 @@ private static void TryPrependItem(
}

var itemPath = path.Substring(start, length);
var item = new BindingItem(head, itemPath, fallbackValue, precedence, allowPrivateMembers);
var item = new BindingItem(head, itemPath, precedence, allowPrivateMembers);

head = item;
tail ??= item;
Expand Down
Loading