-
Notifications
You must be signed in to change notification settings - Fork 690
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: New DependencyPropertyHelper class #16315
base: master
Are you sure you want to change the base?
Conversation
572b18d
to
bad5a7c
Compare
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-16315/index.html |
f9ed627
to
761cfe1
Compare
🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-16315/index.html |
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-16315/index.html |
761cfe1
to
e993d1e
Compare
/// external projects. | ||
/// </summary> | ||
/// <remarks> | ||
/// A small reflection is still required to access the class because those using it needs to know what they are doing. |
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.
/// A small reflection is still required to access the class because those using it needs to know what they are doing. | |
/// A small reflection is still required to access the class because those using it need to know what they are doing. |
/// Get the default value of a <see cref="DependencyProperty"/> for a given type. | ||
/// </summary> | ||
/// <remarks> | ||
/// This is the value defined in the metadata of the property, which _may_ be overriden |
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 the value defined in the metadata of the property, which _may_ be overriden | |
/// This is the value defined in the metadata of the property, which _may_ be overridden |
/// Get the default value of a <see cref="DependencyProperty"/> for a given type. | ||
/// </summary> | ||
/// <remarks> | ||
/// This is the value defined in the metadata of the property, which _may_ be overriden |
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 the value defined in the metadata of the property, which _may_ be overriden | |
/// This is the value defined in the metadata of the property, which _may_ be overridden |
/// Get the default value of a <see cref="DependencyProperty"/> for the owner type. | ||
/// </summary> | ||
/// <remarks> | ||
/// This is the value defined in the metadata of the property, which _may_ be overriden |
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 the value defined in the metadata of the property, which _may_ be overriden | |
/// This is the value defined in the metadata of the property, which _may_ be overridden |
/// <summary> | ||
/// Try to get all the <see cref="DependencyProperty"/> defined for a given type. | ||
/// </summary> | ||
/// <returns>False means it's not a dependency object</returns> |
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.
/// <returns>False means it's not a dependency object</returns> | |
/// <returns>False means it's not a dependency object.</returns> |
=> dependencyProperty.Name; | ||
|
||
/// <summary> | ||
/// Get the owner type of the 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.
/// Get the owner type of the property | |
/// Get the owner type of the property. |
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-16315/index.html |
🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-16315/index.html |
The build 121247 found UI Test snapshots differences: Details
|
/// This is the property that defines the property, not the type that uses it. | ||
/// It may also be overridden by a derived type. | ||
/// </remarks> | ||
public static Type GetPropertyOwnerType(DependencyProperty dependencyProperty) |
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 would remove the Property
from all those methods, e.g. GetOwnerType
here.
/// <summary> | ||
/// The goal of this class is to provide a set of helper methods to work with <see cref="DependencyProperty"/> from | ||
/// external projects. |
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 don't see why we need this. Reflection (or even better, UnsafeAccessorAttribute) could still be used to access such implementation details without the need for a new class
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.
Wouldn't that mean it would be easier to make unintended breaking changes in Uno that would break the users? This adds a layer of indirection that could help maybe
public static object? GetDefaultValue(DependencyProperty dependencyProperty, Type forType) | ||
=> dependencyProperty.GetMetadata(forType)?.DefaultValue; |
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.
Both GetMetadata
and DefaultValue
are already available publicly I think?
public static object? GetDefaultValue<T>(DependencyProperty dependencyProperty) | ||
where T : DependencyObject | ||
=> dependencyProperty.GetMetadata(typeof(T))?.DefaultValue; |
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.
Same here, both GetMetadata
and DefaultValue
are available publicly. It will be easier anyways to do the calls directly rather than using reflection or unsafe accessor
public static object? GetDefaultValue(DependencyProperty dependencyProperty) | ||
=> dependencyProperty.GetMetadata(dependencyProperty.OwnerType)?.DefaultValue; |
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 only part that's not publicly available here is OwnerType
, which is easily accessible via reflection or unsafe accessor already.
public static DependencyProperty? GetDependencyPropertyByName(Type ownerType, string propertyName) | ||
=> DependencyProperty.GetProperty(ownerType, propertyName); |
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.
Same here. It's already easy to access via reflection. Actually, it will be easier to access it when it's on DependencyProperty
when using unsafe accessor. Because currently unsafe accessor can't access something in a static class
GitHub Issue (If applicable): closes #
PR Type
What kind of change does this PR introduce?
What is the current behavior?
What is the new behavior?
PR Checklist
Please check if your PR fulfills the following requirements:
Screenshots Compare Test Run
results.Other information
Internal Issue (If applicable):