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

Correctly propagate BindingContext to Border StrokeShape #13793

Merged
merged 18 commits into from
Mar 22, 2023
Merged
Show file tree
Hide file tree
Changes from 8 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 @@ -7,7 +7,7 @@
<ContentPage.Resources>
<ResourceDictionary>

<Style TargetType="Border">
<Style x:Key="BorderStyle" TargetType="Border">
<Setter Property="StrokeShape" Value="RoundRectangle 10,10,10,10"/>
<Setter Property="Stroke" Value="Red"/>
<Setter Property="StrokeThickness" Value="1"/>
Expand All @@ -16,13 +16,34 @@
<Setter Property="BackgroundColor" Value="Transparent"/>
</Style>

<Style x:Key="StrokeShapeBindingParentBorderStyle" TargetType="Border">
<Setter Property="StrokeShape">
<RoundRectangle
CornerRadius="{Binding Source={RelativeSource AncestorType={x:Type Border}}, Path=Width}" />
</Setter>
<Setter Property="Stroke" Value="Red"/>
<Setter Property="StrokeThickness" Value="1"/>
<Setter Property="HeightRequest" Value="55"/>
<Setter Property="Padding" Value="10"/>
<Setter Property="BackgroundColor" Value="Transparent"/>
</Style>

</ResourceDictionary>
</ContentPage.Resources>
<ContentPage.Content>
<StackLayout
Margin="12"
Spacing="12">
<Border />
<Label
Text="Default"
Style="{StaticResource Headline}"/>
<Border
Style="{StaticResource BorderStyle}"/>
<Label
Text="StrokeShape binded to Parent"
Style="{StaticResource Headline}"/>
<Border
Style="{StaticResource StrokeShapeBindingParentBorderStyle}"/>
</StackLayout>
</ContentPage.Content>
</ContentPage>
44 changes: 42 additions & 2 deletions src/Controls/src/Core/Border.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System.Collections.Generic;
using System;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Collections.Specialized;
using System.Runtime.CompilerServices;
Expand All @@ -14,6 +15,10 @@ public class Border : View, IContentView, IBorderView, IPaddingElement
float[]? _strokeDashPattern;
ReadOnlyCollection<Element>? _logicalChildren;

WeakNotifyPropertyChangedProxy? _strokeShapeProxy = null;

~Border() => _strokeShapeProxy?.Unsubscribe();

internal ObservableCollection<Element> InternalChildren { get; } = new();

internal override IReadOnlyList<Element> LogicalChildrenInternal =>
Expand All @@ -37,7 +42,42 @@ public Thickness Padding
}

public static readonly BindableProperty StrokeShapeProperty =
BindableProperty.Create(nameof(StrokeShape), typeof(IShape), typeof(Border), new Rectangle());
BindableProperty.Create(nameof(StrokeShape), typeof(IShape), typeof(Border), new Rectangle(),
propertyChanging: (bindable, oldvalue, newvalue) =>
{
if (newvalue is not null)
(bindable as Border)?.StopNotifyingStrokeShapeChanges();
Copy link
Member

Choose a reason for hiding this comment

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

Should this always fire? If I set a value and then null, this will not unsub.

Copy link
Member

Choose a reason for hiding this comment

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

If this is wrong, can we add a test for this?

},
propertyChanged: (bindable, oldvalue, newvalue) =>
{
if (newvalue is not null)
(bindable as Border)?.NotifyStrokeShapeChanges();
});

void NotifyStrokeShapeChanges()
{
var strokeShape = StrokeShape;

if (strokeShape is VisualElement visualElement)
{
SetInheritedBindingContext(visualElement, BindingContext);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We were not propagating the BindingContext to the Shape used in the StrokeShape property or detecting changes to invalidate the Shape. These are the main changes.

Copy link
Member

Choose a reason for hiding this comment

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

Do we need Parent AND SetInheritedBindingContext? Does not setting Parent automatically set the binding context? I am not sure, so just checking... Does the order matter?

visualElement.Parent = this;
var proxy = _strokeShapeProxy ??= new();
proxy.Subscribe(visualElement, (sender, e) => OnPropertyChanged(nameof(StrokeShape)));
}
}

void StopNotifyingStrokeShapeChanges()
{
var strokeShape = StrokeShape;

if (strokeShape is VisualElement visualElement)
{
SetInheritedBindingContext(visualElement, null);
visualElement.Parent = null;
_strokeShapeProxy?.Unsubscribe();
}
}

public static readonly BindableProperty StrokeProperty =
BindableProperty.Create(nameof(Stroke), typeof(Brush), typeof(Border), null);
Expand Down
50 changes: 50 additions & 0 deletions src/Controls/src/Core/Internals/WeakEventProxy.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System;
using System.Collections.Specialized;
using System.ComponentModel;

// NOTE: warning disabled for netstandard projects
#pragma warning disable 0436
Expand Down Expand Up @@ -102,4 +103,53 @@ public override void Unsubscribe()
base.Unsubscribe();
}
}

/// <summary>
/// A "proxy" class for subscribing INotifyPropertyChanged via WeakReference.
/// General usage is to store this in a member variable and call Subscribe()/Unsubscribe() appropriately.
/// Your class should have a finalizer that calls Unsubscribe() to prevent WeakNotifyCollectionChangedProxy objects from leaking.
/// </summary>
class WeakNotifyPropertyChangedProxy : WeakEventProxy<INotifyPropertyChanged, PropertyChangedEventHandler>
{
public WeakNotifyPropertyChangedProxy() { }

public WeakNotifyPropertyChangedProxy(INotifyPropertyChanged source, PropertyChangedEventHandler handler)
{
Subscribe(source, handler);
}

void OnPropertyChanged(object? sender, PropertyChangedEventArgs e)
{
if (TryGetHandler(out var handler))
{
handler(sender, e);
}
else
{
Unsubscribe();
}
}

public override void Subscribe(INotifyPropertyChanged source, PropertyChangedEventHandler handler)
{
if (TryGetSource(out var s))
{
s.PropertyChanged -= OnPropertyChanged;
}

source.PropertyChanged += OnPropertyChanged;

base.Subscribe(source, handler);
}

public override void Unsubscribe()
{
if (TryGetSource(out var s))
{
s.PropertyChanged -= OnPropertyChanged;
}

base.Unsubscribe();
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Can we revert this whitespace change?

Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#nullable enable
*REMOVED*override Microsoft.Maui.Controls.RefreshView.MeasureOverride(double widthConstraint, double heightConstraint) -> Microsoft.Maui.Graphics.Size
Microsoft.Maui.Controls.Border.~Border() -> void
Microsoft.Maui.Controls.Handlers.Compatibility.FrameRenderer.FrameRenderer(Android.Content.Context! context, Microsoft.Maui.IPropertyMapper! mapper) -> void
Microsoft.Maui.Controls.Handlers.Compatibility.FrameRenderer.FrameRenderer(Android.Content.Context! context, Microsoft.Maui.IPropertyMapper! mapper, Microsoft.Maui.CommandMapper! commandMapper) -> void
Microsoft.Maui.Controls.LayoutOptions.Equals(Microsoft.Maui.Controls.LayoutOptions other) -> bool
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#nullable enable
*REMOVED*override Microsoft.Maui.Controls.RefreshView.MeasureOverride(double widthConstraint, double heightConstraint) -> Microsoft.Maui.Graphics.Size
Microsoft.Maui.Controls.Border.~Border() -> void
Microsoft.Maui.Controls.LayoutOptions.Equals(Microsoft.Maui.Controls.LayoutOptions other) -> bool
Microsoft.Maui.Controls.Region.Equals(Microsoft.Maui.Controls.Region other) -> bool
Microsoft.Maui.Controls.Shapes.Matrix.Equals(Microsoft.Maui.Controls.Shapes.Matrix other) -> bool
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#nullable enable
*REMOVED*override Microsoft.Maui.Controls.RefreshView.MeasureOverride(double widthConstraint, double heightConstraint) -> Microsoft.Maui.Graphics.Size
Microsoft.Maui.Controls.Border.~Border() -> void
Microsoft.Maui.Controls.LayoutOptions.Equals(Microsoft.Maui.Controls.LayoutOptions other) -> bool
Microsoft.Maui.Controls.Region.Equals(Microsoft.Maui.Controls.Region other) -> bool
Microsoft.Maui.Controls.Shapes.Matrix.Equals(Microsoft.Maui.Controls.Shapes.Matrix other) -> bool
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#nullable enable
Microsoft.Maui.Controls.Border.~Border() -> void
Microsoft.Maui.Controls.LayoutOptions.Equals(Microsoft.Maui.Controls.LayoutOptions other) -> bool
Microsoft.Maui.Controls.Region.Equals(Microsoft.Maui.Controls.Region other) -> bool
Microsoft.Maui.Controls.Shapes.Matrix.Equals(Microsoft.Maui.Controls.Shapes.Matrix other) -> bool
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#nullable enable
*REMOVED*override Microsoft.Maui.Controls.RefreshView.MeasureOverride(double widthConstraint, double heightConstraint) -> Microsoft.Maui.Graphics.Size
Microsoft.Maui.Controls.Border.~Border() -> void
Microsoft.Maui.Controls.LayoutOptions.Equals(Microsoft.Maui.Controls.LayoutOptions other) -> bool
Microsoft.Maui.Controls.Region.Equals(Microsoft.Maui.Controls.Region other) -> bool
Microsoft.Maui.Controls.Shapes.Matrix.Equals(Microsoft.Maui.Controls.Shapes.Matrix other) -> bool
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#nullable enable
*REMOVED*override Microsoft.Maui.Controls.RefreshView.MeasureOverride(double widthConstraint, double heightConstraint) -> Microsoft.Maui.Graphics.Size
Microsoft.Maui.Controls.Border.~Border() -> void
Microsoft.Maui.Controls.LayoutOptions.Equals(Microsoft.Maui.Controls.LayoutOptions other) -> bool
Microsoft.Maui.Controls.Region.Equals(Microsoft.Maui.Controls.Region other) -> bool
Microsoft.Maui.Controls.Shapes.Matrix.Equals(Microsoft.Maui.Controls.Shapes.Matrix other) -> bool
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#nullable enable
*REMOVED*override Microsoft.Maui.Controls.RefreshView.MeasureOverride(double widthConstraint, double heightConstraint) -> Microsoft.Maui.Graphics.Size
Microsoft.Maui.Controls.Border.~Border() -> void
Microsoft.Maui.Controls.LayoutOptions.Equals(Microsoft.Maui.Controls.LayoutOptions other) -> bool
Microsoft.Maui.Controls.Region.Equals(Microsoft.Maui.Controls.Region other) -> bool
Microsoft.Maui.Controls.Shapes.Matrix.Equals(Microsoft.Maui.Controls.Shapes.Matrix other) -> bool
Expand Down
54 changes: 53 additions & 1 deletion src/Core/tests/UnitTests/Views/BorderTests.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
using Microsoft.Maui.Controls;
using System;
using System.Threading.Tasks;
using Microsoft.Maui.Controls;
using Microsoft.Maui.Controls.Shapes;
using Xunit;

namespace Microsoft.Maui.UnitTests.Views
Expand Down Expand Up @@ -29,6 +32,55 @@ public void TestBorderPropagateBindingContext()
Assert.True(border.BindingContext == bindingContext);
}

[Fact]
public void TestStrokeShapeBindingContext()
{
var context = new object();

var parent = new Border
{
BindingContext = context
};

var strokeShape = new Rectangle();

parent.StrokeShape = strokeShape;

Assert.Same(context, ((Rectangle)parent.StrokeShape).BindingContext);
}
jsuarezruiz marked this conversation as resolved.
Show resolved Hide resolved

[Fact]
public void BorderStrokeShapeSubscribed()
{
var strokeShape = new RoundRectangle { CornerRadius = new CornerRadius(12) };
var border = new Border { StrokeShape = strokeShape };

bool fired = false;
border.PropertyChanged += (sender, e) =>
{
if (e.PropertyName == nameof(Border.StrokeShape))
fired = true;
};

strokeShape.CornerRadius = new CornerRadius(24);
Assert.True(fired, "PropertyChanged did not fire!");
}

[Theory(Skip = "Reviewing why is failing")]
[InlineData(typeof(Rectangle))]
[InlineData(typeof(Ellipse))]
public async Task BorderStrokeShapeDoesNotLeak(Type type)
{
var strokeShape = (Shape)Activator.CreateInstance(type);
var reference = new WeakReference(new Border { StrokeShape = strokeShape });

await Task.Yield();
GC.Collect();
GC.WaitForPendingFinalizers();

Assert.False(reference.IsAlive, "Border should not be alive!");
}
jsuarezruiz marked this conversation as resolved.
Show resolved Hide resolved

[Fact]
public void TestSetChild()
{
Expand Down