feat: New DependencyPropertyHelper class #16315
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
carldebilly
force-pushed
the
dev/cdb/dependency-property-helper
branch
from
572b18d
to
bad5a7c
Compare
|
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-16315/index.html |
carldebilly
force-pushed
the
dev/cdb/dependency-property-helper
branch
2 times, most recently
from
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 |
carldebilly
force-pushed
the
dev/cdb/dependency-property-helper
branch
from
761cfe1
to
e993d1e
Compare
agneszitte
reviewed
Apr 16, 2024
| /// 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.
Suggested change
| /// 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.
Suggested change
| /// 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.
Suggested change
| /// 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.
Suggested change
| /// 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.
Suggested change
| /// <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.
Suggested change
| /// 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
|
dr1rrb
approved these changes
Apr 16, 2024
| /// 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.
I would remove the Property from all those methods, e.g. GetOwnerType here.
Youssef1313
reviewed
Apr 17, 2024
Comment on lines
+10
to
+12
| /// <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.
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.
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
Youssef1313
reviewed
Apr 17, 2024
Comment on lines
+31
to
+32
| public static object? GetDefaultValue(DependencyProperty dependencyProperty, Type forType) | ||
| => dependencyProperty.GetMetadata(forType)?.DefaultValue; |
There was a problem hiding this comment.
Both GetMetadata and DefaultValue are already available publicly I think?
Youssef1313
reviewed
Apr 17, 2024
Comment on lines
+46
to
+48
| public static object? GetDefaultValue<T>(DependencyProperty dependencyProperty) | ||
| where T : DependencyObject | ||
| => dependencyProperty.GetMetadata(typeof(T))?.DefaultValue; |
There was a problem hiding this comment.
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
Youssef1313
reviewed
Apr 17, 2024
Comment on lines
+65
to
+66
| public static object? GetDefaultValue(DependencyProperty dependencyProperty) | ||
| => dependencyProperty.GetMetadata(dependencyProperty.OwnerType)?.DefaultValue; |
There was a problem hiding this comment.
The only part that's not publicly available here is OwnerType, which is easily accessible via reflection or unsafe accessor already.
Youssef1313
reviewed
Apr 17, 2024
Comment on lines
+78
to
+79
| public static DependencyProperty? GetDependencyPropertyByName(Type ownerType, string propertyName) | ||
| => DependencyProperty.GetProperty(ownerType, propertyName); |
There was a problem hiding this comment.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 Runresults.Other information
Internal Issue (If applicable):