-
Notifications
You must be signed in to change notification settings - Fork 107
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
Make ICustomPropertyProvider AOT safe #1677
Conversation
{ | ||
for (var curSymbol = symbol; curSymbol != null; curSymbol = curSymbol.BaseType) | ||
{ | ||
foreach (var propertySymbol in curSymbol.GetMembers(). |
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.
Note: we should also make sure this works correctly with partial properties. We can handle this in a follow up.
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.
We are using symbol data to get this data, so I believe it should. But we can confirm after.
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.
I was more worried about the fact you might see two different property symbols for the same member (one being the definition, one being the implementation part), and that the fact they'd be "duplicate" might cause problems.
if (!RuntimeFeature.IsDynamicCodeCompiled) | ||
{ | ||
throw new NotSupportedException( | ||
$"ICustomProperty support used by XAML binding for '{target.GetType()}' requires the type to marked with 'WinRT.BindableCustomPropertyAttribute'. " + |
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.
any plans to document this in greater detail, like with an md on the repo? Xaml Compiler could then point errors/warnings to that page.
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.
I can get our aot.md updated to mention this in a follow up PR.
WF::IInspectable const& bindableObject, | ||
hstring property, | ||
Windows::UI::Xaml::Interop::TypeName const& indexerType, | ||
bool validateOnlyExists, |
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.
enums (or name:value syntax) would be easier to read at call sites
return true; | ||
} | ||
|
||
if (customProperty.Name() != property || |
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.
what's the purpose of passing these bools in? given they have to match customProperty, why not just use the latter? (they seem redundant)
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.
This is validating expected vs actual. I will have a follow up PR to address this along with your enum comment for easier readability.
BindableCustomPropertyAttribute
to allow to specify that public properties from the class are bindable. Either all public properties can be bindable by passing no arguments to the constructor. Or the names of the properties and the types of the indexer types can be passed to specify which ones to make bindable.Microsoft.UI.Xaml.Data.IBindableCustomPropertyImplementation
gets implemented on that class via the source generator which provides the implementation to supportICustomPropertyProvider
. This is done as an interface instead of an attribute because this allows to support generic types while the attribute model won't work with it. In addition, we only need to worry about supporting classes and not other types.