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

feat: Also allows using string instead of {x:Type} in ControlTemplate.TargetType #11575

Merged
merged 4 commits into from
Aug 31, 2023

Conversation

workgroupengineering
Copy link
Contributor

@workgroupengineering workgroupengineering commented May 30, 2023

What does the pull request do?

Allows using string instead of {x:Type} in ControlTemplate.TargetType

What is the current behavior?

Throw Unable to resolve suitable regular or attached property

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 #11334

@workgroupengineering workgroupengineering marked this pull request as draft May 30, 2023 18:05
@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0035851-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

@workgroupengineering workgroupengineering marked this pull request as ready for review May 31, 2023 08:18
@avaloniaui-team
Copy link
Contributor

You can test this PR using the following package version. 11.0.999-cibuild0035854-beta. (feed url: https://pkgs.dev.azure.com/AvaloniaUI/AvaloniaUI/_packaging/avalonia-all/nuget/v3/index.json) [PRBUILDID]

@workgroupengineering
Copy link
Contributor Author

@billhenn Can you try this PR if it solve your problem?

@billhenn
Copy link
Contributor

@workgroupengineering Yes, I can confirm this PR fixes my issue. Thank you.

@grokys
Copy link
Member

grokys commented Jun 2, 2023

I've not had time to review this PR yet, but I suspect that we're hitting ordering issues with the transformers as iirc there should be a transformer which transforms string into Type for relevant properties, but could be that it's getting run too late.

@workgroupengineering
Copy link
Contributor Author

I've not had time to review this PR yet, but I suspect that we're hitting ordering issues with the transformers as iirc there should be a transformer which transforms string into Type for relevant properties, but could be that it's getting run too late.

If you suggest the area to see, I'll try to check.

@grokys
Copy link
Member

grokys commented Jun 6, 2023

@workgroupengineering sorry I'm not really familiar enough with the XAML compiler internals to be able to point you to it, this was just from a vague memory of hitting something similar a while ago. Will probably need input from someone who knows this part of the code a bit better.

@workgroupengineering
Copy link
Contributor Author

@workgroupengineering sorry I'm not really familiar enough with the XAML compiler internals to be able to point you to it, this was just from a vague memory of hitting something similar a while ago. Will probably need input from someone who knows this part of the code a bit better.

I investigated, ControlTheme following the same path.

if (targetTypeNode.Values[0] is XamlTypeExtensionNode extension)
targetType = extension.Value.GetClrType();
else if (targetTypeNode.Values[0] is XamlAstTextNode text)
targetType = TypeReferenceResolver.ResolveType(context, text.Text, false, text, true).GetClrType();
else
throw new XamlParseException("Could not determine TargetType for ControlTheme.", targetTypeNode);

@avaloniaui-team
Copy link
Contributor

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

@billhenn
Copy link
Contributor

This PR has been floating out there for a while. Just curious as to the status since it would be nice to have the bug fix in place for standalone ControlTemplate usage.

Copy link
Member

@maxkatz6 maxkatz6 left a comment

Choose a reason for hiding this comment

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

LGTM
Even though it's probably not the best solution, it does match other transformers and adds a test, so it can be refactored in the future.

@maxkatz6 maxkatz6 added this pull request to the merge queue Aug 31, 2023
Merged via the queue into AvaloniaUI:master with commit 05d348b Aug 31, 2023
@workgroupengineering workgroupengineering deleted the features/Issue_11334 branch August 31, 2023 05:48
@grokys grokys added the backport-candidate-11.0.x Consider this PR for backporting to 11.0 branch label Sep 29, 2023
grokys pushed a commit that referenced this pull request Oct 2, 2023
feat: Also allows using string instead of {x:Type} in ControlTemplate.TargetType
@grokys grokys added backported-11.0.x and removed backport-candidate-11.0.x Consider this PR for backporting to 11.0 branch labels Oct 14, 2023
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.

Standalone ControlTemplate with TargetType fails to resolve properties
5 participants