Skip to content

Commit

Permalink
Fixes spurious DataGrid data validation error (#15716)
Browse files Browse the repository at this point in the history
* Added failing tests for #15081.

* Provide target property in BindingExpression ctor.

Usually it is not necessary to provide the target property when creating a `BindingExpression` because the property will be assigned when the binding expression is attached to the target in `BindingExpressionBase.Attach`.

This is however one case where `Attach` is not called: when the obsolete `binding.Initiate` method is called and then an observable is read from the `InstancedBinding` without the binding actually being attached to the target object. In this case, prior to the binding refactor in #13970 the value produced by the observable was still converted to the target type. After #13970, because the target property (and hence the target type) is not yet set, the conversion is to the target type is no longer done.

`DataGrid` uses this obsolete method when editing cells, causing #15081. Ideally we'd fix that in `DataGrid` but I'm not happy making this change so close to 11.1, so instead fix this use-case to behave as before.

Fixes #15081
  • Loading branch information
grokys authored May 14, 2024
1 parent 3975dbe commit 8fe6e08
Show file tree
Hide file tree
Showing 6 changed files with 65 additions and 4 deletions.
4 changes: 3 additions & 1 deletion src/Avalonia.Base/Data/Core/BindingExpression.cs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ internal partial class BindingExpression : UntypedBindingExpressionBase, IDescri
/// <param name="mode">The binding mode.</param>
/// <param name="priority">The binding priority.</param>
/// <param name="stringFormat">The format string to use.</param>
/// <param name="targetProperty">The target property being bound to.</param>
/// <param name="targetNullValue">The null target value.</param>
/// <param name="targetTypeConverter">
/// A final type converter to be run on the produced value.
Expand All @@ -65,9 +66,10 @@ public BindingExpression(
BindingPriority priority = BindingPriority.LocalValue,
string? stringFormat = null,
object? targetNullValue = null,
AvaloniaProperty? targetProperty = null,
TargetTypeConverter? targetTypeConverter = null,
UpdateSourceTrigger updateSourceTrigger = UpdateSourceTrigger.PropertyChanged)
: base(priority, enableDataValidation)
: base(priority, targetProperty, enableDataValidation)
{
if (mode == BindingMode.Default)
throw new ArgumentException("Binding mode cannot be Default.", nameof(mode));
Expand Down
8 changes: 7 additions & 1 deletion src/Avalonia.Base/Data/Core/UntypedBindingExpressionBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,16 @@ public abstract class UntypedBindingExpressionBase : BindingExpressionBase,
/// <param name="defaultPriority">
/// The default binding priority for the expression.
/// </param>
/// <param name="targetProperty">The target property being bound to.</param>
/// <param name="isDataValidationEnabled">Whether data validation is enabled.</param>
public UntypedBindingExpressionBase(
BindingPriority defaultPriority,
AvaloniaProperty? targetProperty = null,
bool isDataValidationEnabled = false)
{
Priority = defaultPriority;
TargetProperty = targetProperty;
TargetType = targetProperty?.PropertyType ?? typeof(object);
_isDataValidationEnabled = isDataValidationEnabled;
}

Expand Down Expand Up @@ -86,7 +90,7 @@ public UntypedBindingExpressionBase(
/// Gets the target type of the binding expression; that is, the type that values produced by
/// the expression should be converted to.
/// </summary>
public Type TargetType { get; private set; } = typeof(object);
public Type TargetType { get; private set; }

AvaloniaProperty IValueEntry.Property => TargetProperty ?? throw new Exception();

Expand Down Expand Up @@ -262,6 +266,8 @@ private void AttachCore(
{
if (_sink is not null)
throw new InvalidOperationException("BindingExpression was already attached.");
if (TargetProperty is not null && TargetProperty != targetProperty)
throw new InvalidOperationException("BindingExpression was already attached to a different property.");

_sink = sink;
_frame = frame;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ private BindingExpression InstanceCore(
priority: Priority,
stringFormat: StringFormat,
targetNullValue: TargetNullValue,
targetProperty: targetProperty,
targetTypeConverter: TargetTypeConverter.GetDefaultConverter(),
updateSourceTrigger: trigger);
}
Expand Down
1 change: 1 addition & 0 deletions src/Markup/Avalonia.Markup/Data/Binding.cs
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ private UntypedBindingExpressionBase InstanceCore(
mode: mode,
priority: Priority,
stringFormat: StringFormat,
targetProperty: targetProperty,
targetNullValue: TargetNullValue,
targetTypeConverter: TargetTypeConverter.GetReflectionConverter(),
updateSourceTrigger: trigger);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
using System.Reactive.Linq;
using Avalonia.Data;
using Avalonia.Markup.Xaml.MarkupExtensions;
using Xunit;

#nullable enable
#pragma warning disable CS0618 // Type or member is obsolete

namespace Avalonia.Base.UnitTests.Data.Core;

public abstract partial class BindingExpressionTests
{
public partial class Reflection
{
[Fact]
public void Obsolete_Initiate_Method_Produces_Observable_With_Correct_Target_Type()
{
// Issue #15081
var viewModel = new ViewModel { DoubleValue = 42.5 };
var target = new TargetClass { DataContext = viewModel };
var binding = new Binding(nameof(viewModel.DoubleValue));
var instanced = binding.Initiate(target, TargetClass.StringProperty);

Assert.NotNull(instanced);

var value = instanced.Observable.First();

Assert.Equal("42.5", value);
}
}

public partial class Compiled
{
[Fact]
public void Obsolete_Initiate_Method_Produces_Observable_With_Correct_Target_Type()
{
// Issue #15081
var viewModel = new ViewModel { DoubleValue = 42.5 };
var target = new TargetClass { DataContext = viewModel };
var path = CompiledBindingPathFromExpressionBuilder.Build<ViewModel, double>(x => x.DoubleValue, true);
var binding = new CompiledBindingExtension(path);
var instanced = binding.Initiate(target, TargetClass.StringProperty);

Assert.NotNull(instanced);

var value = instanced.Observable.First();

Assert.Equal("42.5", value);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ namespace Avalonia.Base.UnitTests.Data.Core;
[InvariantCulture]
public abstract partial class BindingExpressionTests
{
public class Reflection : BindingExpressionTests
public partial class Reflection : BindingExpressionTests
{
private protected override (TargetClass, BindingExpression) CreateTargetCore<TIn, TOut>(
Expression<Func<TIn, TOut>> expression,
Expand Down Expand Up @@ -73,7 +73,7 @@ private protected override (TargetClass, BindingExpression) CreateTargetCore<TIn
}
}

public class Compiled : BindingExpressionTests
public partial class Compiled : BindingExpressionTests
{
private protected override (TargetClass, BindingExpression) CreateTargetCore<TIn, TOut>(
Expression<Func<TIn, TOut>> expression,
Expand Down

0 comments on commit 8fe6e08

Please sign in to comment.