-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
feat: x:Shared #16644
feat: x:Shared #16644
Conversation
You can test this PR using the following package version. |
I will take a better look next week. @MrJul might be also interested in reviewing it. |
You can test this PR using the following package version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation can be simplified by removing duplicate code, reusing existing types, and having only the dictionary know whether an item is shared or not.
.../Avalonia.Markup.Xaml.Loader/CompilerExtensions/AstNodes/XamlNotSharedDeferredContentNode.cs
Outdated
Show resolved
Hide resolved
src/Markup/Avalonia.Markup.Xaml.Loader/CompilerExtensions/Transformers/XSharedTransformer.cs
Outdated
Show resolved
Hide resolved
src/Markup/Avalonia.Markup.Xaml.Loader/CompilerExtensions/Visitors/NotSharedVisitor.cs
Outdated
Show resolved
Hide resolved
src/Markup/Avalonia.Markup.Xaml/XamlIl/Runtime/XamlIlRuntimeHelpers.cs
Outdated
Show resolved
Hide resolved
.../Avalonia.Markup.Xaml.Loader/CompilerExtensions/Transformers/AvaloniaXamlIlWellKnownTypes.cs
Outdated
Show resolved
Hide resolved
tests/Avalonia.Markup.Xaml.UnitTests/Xaml/XSharedDirectiveTests.cs
Outdated
Show resolved
Hide resolved
tests/Avalonia.Markup.Xaml.UnitTests/Xaml/XSharedDirectiveTests.cs
Outdated
Show resolved
Hide resolved
Just a though for discussion: While the feature is useful, needed, and matches WPF, Avalonia usually uses explicit factory types such as Shouldn't we go the same way here, by specializing |
My two cents as an user, while I agree Template is a good replacement, it would be very convenient to have x:Shared when porting WPF code. |
You can test this PR using the following package version. |
a11881d
to
242a467
Compare
You can test this PR using the following package version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the changes, it's looking way better.
I've left a couple of comments for easily fixed issues. After that it should be good to go :)
.../Avalonia.Markup.Xaml.Loader/CompilerExtensions/Transformers/AvaloniaXamlIlWellKnownTypes.cs
Outdated
Show resolved
Hide resolved
.../Avalonia.Markup.Xaml.Loader/CompilerExtensions/Transformers/AvaloniaXamlIlWellKnownTypes.cs
Outdated
Show resolved
Hide resolved
LGTM, thanks! |
You can test this PR using the following package version. |
What does the pull request do?
What is the current behavior?
What is the updated/expected behavior with this PR?
How was the solution implemented (if it's not obvious)?
Checklist
Breaking changes
Obsoletions / Deprecations
Fixed issues
Fixes #15518