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

[X] do not apply Bindings if DataType doesnt match #22056

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+91 −8
Open

[X] do not apply Bindings if DataType doesnt match #22056

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6d4c525
[X] do not apply Bindings if DataType doesnt match
StephaneDelcroix Apr 25, 2024
df5f343
fixes
StephaneDelcroix May 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 9 additions & 2 deletions src/Controls/src/Core/Binding.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,8 @@ public string UpdateSourceEventName
}
}

internal Type DataType { get; set; }

internal override void Apply(bool fromTarget)
{
base.Apply(fromTarget);
Expand All @@ -120,7 +122,13 @@ internal override void Apply(object context, BindableObject bindObj, BindablePro
object src = _source;
var isApplied = IsApplied;

base.Apply(src ?? context, bindObj, targetProperty, fromBindingContextChanged, specificity);
var bindingContext = src ?? Context ?? context;
StephaneDelcroix marked this conversation as resolved.
Show resolved Hide resolved
if (DataType != null && bindingContext != null && !DataType.IsAssignableFrom(bindingContext.GetType()))
{
bindingContext = null;
Copy link
Member

Choose a reason for hiding this comment

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

Should we log a warning to make it easier to debug the type mismatch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm afraid that a warning will make HotReload fail

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's something that "works" but is not the correct or preferred way to bind, I rather hot reload not work and tell the user about the issue rather than let it through even though there is a type mismatch that "works" but is wrong. I'm also not sure if warnings in it of themselves would stop hot reload itself or if we could still show the warning and let the action through anyway.

@mgoertz-msft what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

You would need to ask @etvorun. I'm not sure what the exact condition is for errors/warnings to cause Hot Reload to skip requests. I also thought that HR only looks at these reported from IntelliSense and not build but I could be wrong.

Copy link
Contributor

@etvorun etvorun Jun 4, 2024 •
edited

Choose a reason for hiding this comment

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

My preference would be to follow WPF, which is to silently ignore bad binding and fire BindingErrorEventArgs. Devs can use VS support for binding diagnostics which supports MAUI. @spadapet is the expert in that area.

Copy link
Contributor

@spadapet spadapet Jun 4, 2024 •
edited

Choose a reason for hiding this comment

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

I agree with @etvorun. XAML Hot Reload will send the bad binding to the app since XAML parsing is valid (like @mgoertz-msft said). If this code triggers a BindingFailed event, then the UI in Visual Studio and the in-app toolbar will immediately show that a runtime binding failure occurred. Then the user can see a detailed error message and jump to the XAML.

image

NOTE: The BindingFailed event is sent by calling BindingDiagnostics.SendBindingFailure.

}

base.Apply(bindingContext, bindObj, targetProperty, fromBindingContextChanged, specificity);

if (src != null && isApplied && fromBindingContextChanged)
return;
Expand All @@ -131,7 +139,6 @@ internal override void Apply(object context, BindableObject bindObj, BindablePro
}
else
{
object bindingContext = src ?? Context ?? context;
if (_expression == null)
_expression = new BindingExpression(this, SelfPath);
_expression.Apply(bindingContext, bindObj, targetProperty, specificity);
Expand Down
5 changes: 5 additions & 0 deletions src/Controls/src/Core/IXamlDataTypeProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
namespace Microsoft.Maui.Controls.Xaml;
interface IXamlDataTypeProvider
{
string BindingDataType { get; }
}
13 changes: 11 additions & 2 deletions src/Controls/src/Xaml/MarkupExtensions/BindingExtension.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
using System;
using System.ComponentModel;
using System.Linq.Expressions;
using Microsoft.Maui.Controls.Internals;

namespace Microsoft.Maui.Controls.Xaml
{
[ContentProperty(nameof(Path))]
[AcceptEmptyServiceProvider]
public sealed class BindingExtension : IMarkupExtension<BindingBase>
StephaneDelcroix marked this conversation as resolved.
Show resolved Hide resolved
{
public string Path { get; set; } = Binding.SelfPath;
Expand All @@ -21,13 +21,22 @@ public sealed class BindingExtension : IMarkupExtension<BindingBase>

BindingBase IMarkupExtension<BindingBase>.ProvideValue(IServiceProvider serviceProvider)
{
if (TypedBinding == null)
if (TypedBinding is null) {
Type bindingXDataType = null;
if ((serviceProvider.GetService(typeof(IXamlTypeResolver)) is IXamlTypeResolver typeResolver)
&& (serviceProvider.GetService(typeof(IXamlDataTypeProvider)) is IXamlDataTypeProvider dataTypeProvider)
&& dataTypeProvider.BindingDataType != null)
{
typeResolver.TryResolve(dataTypeProvider.BindingDataType, out bindingXDataType);
}
return new Binding(Path, Mode, Converter, ConverterParameter, StringFormat, Source)
{
UpdateSourceEventName = UpdateSourceEventName,
FallbackValue = FallbackValue,
TargetNullValue = TargetNullValue,
DataType = bindingXDataType,
};
}

TypedBinding.Mode = Mode;
TypedBinding.Converter = Converter;
Expand Down
56 changes: 56 additions & 0 deletions src/Controls/src/Xaml/XamlServiceProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ internal XamlServiceProvider(INode node, HydrationContext context)
IXmlLineInfoProvider = new XmlLineInfoProvider(xmlLineInfo);

IValueConverterProvider = new ValueConverterProvider();

if (node is IElementNode elementNode)
Add(typeof(IXamlDataTypeProvider), new XamlDataTypeProvider(elementNode));
Comment on lines +31 to +32
Copy link
Member

Choose a reason for hiding this comment

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

Will this service be added even when XamlC creates the new XamlServiceProvider instance? And if it is, won't this be added for almost any extension, even if IXamlDataTypeProvider is not in its [RequiredService([...])]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

going to fix this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RequiredService isn't in main yet. also, this won't get invoked from compiled XAML

}

public XamlServiceProvider() => IValueConverterProvider = new ValueConverterProvider();
Expand Down Expand Up @@ -262,4 +265,57 @@ public string LookupNamespace(string prefix)
public string LookupPrefix(string namespaceName) => throw new NotImplementedException();
public void Add(string prefix, string ns) => namespaces.Add(prefix, ns);
}

class XamlDataTypeProvider : IXamlDataTypeProvider
{
public XamlDataTypeProvider(IElementNode node)
{
static IElementNode GetParent(IElementNode node)
{
return node switch
{
{ Parent: ListNode { Parent: IElementNode parentNode } } => parentNode,
{ Parent: IElementNode parentNode } => parentNode,
_ => null,
};
}

static bool IsBindingContextBinding(IElementNode node)
{
if ( ApplyPropertiesVisitor.TryGetPropertyName(node, node.Parent, out XmlName name)
&& name.NamespaceURI == ""
&& name.LocalName == nameof(BindableObject.BindingContext))
return true;
return false;
}

INode dataTypeNode = null;
IElementNode n = node as IElementNode;

// Special handling for BindingContext={Binding ...}
// The order of checks is:
// - x:DataType on the binding itself
// - SKIP looking for x:DataType on the parent
// - continue looking for x:DataType on the parent's parent...
IElementNode skipNode = null;
if (IsBindingContextBinding(node))
{
skipNode = GetParent(node);
}

while (n != null)
{
if (n != skipNode && n.Properties.TryGetValue(XmlName.xDataType, out dataTypeNode))
{
break;
}

n = GetParent(n);
}
if (dataTypeNode is ValueNode valueNode)
BindingDataType = valueNode.Value as string;

}
public string BindingDataType { get; }
}
}
14 changes: 10 additions & 4 deletions src/Controls/tests/Xaml.UnitTests/BindingsCompiler.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,9 @@ public class Tests
{
[SetUp] public void Setup() => DispatcherProvider.SetCurrent(new DispatcherProviderStub());
[TearDown] public void TearDown() => DispatcherProvider.SetCurrent(null);

[TestCase(false)]
[TestCase(true)]
public void Test(bool useCompiledXaml)

[Test]
public void TestCompiledBindings([Values(false, true)]bool useCompiledXaml)
{
if (useCompiledXaml)
MockCompiler.Compile(typeof(BindingsCompiler));
Expand Down Expand Up @@ -113,6 +112,13 @@ public void Test(bool useCompiledXaml)
layout.BindingContext = new object();
Assert.AreEqual(null, layout.label0.Text);
}

[Test]
public void BindingsNotAppliedWithWrongContext([Values(false, true)]bool useCompiledXaml)
{
var page = new BindingsCompiler(useCompiledXaml) { BindingContext = new {Text="Foo"} };
Assert.AreEqual(null, page.label0.Text);
}
}
}

Expand Down