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

Add Points collection observability support for Polygon and Polyline #15030

Merged
merged 9 commits into from
Apr 1, 2024

Conversation

SKProCH
Copy link
Contributor

@SKProCH SKProCH commented Mar 18, 2024

What does the pull request do?

Add Points binding observability support for Polygon and Polyline

What is the current behavior?

When ObservableCollection was bound to Points property, updating collection does not updates the geometry. See #3623

What is the updated/expected behavior with this PR?

When ObservableCollection was bound to Points property, updating collection updates the geometry (and control actially re-rendered with a new geometry)

Checklist

Breaking changes

None

Fixed issues

Fixes #3623

@maxkatz6
Copy link
Member

Can you add a simple unit test for this scenario?
It should be enough to check if shape.IsMeasureValid property returns false value.

@SKProCH
Copy link
Contributor Author

SKProCH commented Mar 18, 2024

@maxkatz6 done

@maxkatz6
Copy link
Member

@SKProCH ok... I have checked a little, and noticed that change notification should have already been working with this code:
https://github.com/AvaloniaUI/Avalonia/blob/master/src/Avalonia.Base/Media/PolylineGeometry.cs#L100-L107

But as you can see, it only supported IAvaloniaList. So, this PR should instead fix PolylineGeometry/PolygonGeometry not respecting INotifyCollectionChanged.

@SKProCH
Copy link
Contributor Author

SKProCH commented Mar 18, 2024

The problem with this geometry is under the hood implementation. We create a new Points collection inside geometry ctor:

public PolylineGeometry(IEnumerable<Point> points, bool isFilled)
{
_points = new Points(points);

Points its just a sealed inheritor of AvaloniaList:

public sealed class Points : AvaloniaList<Point>
{
public Points()
{
}
public Points(IEnumerable<Point> points) : base(points)
{
}

But as we can see inside AvaloniaList's ctor, it copying collection content from the collection parameter:

public AvaloniaList(IEnumerable<T> items)
{
_inner = new List<T>(items);
}

So, at the moment where we create an Points collection inside of Geometry ctor we just copying the elements of the original collection, and we don't have reference to original collection and the changes doesn't propagated to the Points.

It's actually has the same effect if we just call .ToList() to original collection.

So, currently there is no actual benefits of changing OnPointsChanged implementation as you suggest.

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0046333-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@maxkatz6
Copy link
Member

Try to replace

return new PolylineGeometry(Points, true);

with

return new PolylineGeometry
{
    Points = Points,
    IsFilled = true
}

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0046389-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0046393-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@SKProCH
Copy link
Contributor Author

SKProCH commented Mar 20, 2024

Actually some of tests failing, i'll fix that a little later

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0046408-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0046473-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.2.999-cibuild0046475-alpha. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@maxkatz6 maxkatz6 added the backport-candidate-11.1.x Consider this PR for backporting to 11.1 branch label Apr 1, 2024
@maxkatz6 maxkatz6 added this pull request to the merge queue Apr 1, 2024
Merged via the queue into AvaloniaUI:master with commit 45eb172 Apr 1, 2024
10 checks passed
maxkatz6 pushed a commit that referenced this pull request Apr 6, 2024
…15030)

* Add Points binding observability support for Polygon and Polyline

* Add simple tests for Polygon and Polyline Points updating

* Revert "Add Points binding observability support for Polygon and Polyline"

This reverts commit e16d987.

* Move Geometry.Changed handler to Shape class to make it available to all inheritors

* Fixes the event subscriptions

* Fix tests

* Add memory leak tests
@maxkatz6 maxkatz6 removed the backport-candidate-11.1.x Consider this PR for backporting to 11.1 branch label Apr 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Polygon and Polyline Points binding problem
3 participants