Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Nov 22, 2021

@adegeo, this PR is ready for your review.

Update snippets for attached-properties-overview and how-to-register-an-attached-property.

Internal review link for article.

@ghost ghost marked this pull request as ready for review November 22, 2021 17:42
@adegeo
Copy link
Contributor

adegeo commented Dec 2, 2021

Hi Tris! What is this PR for exactly? This will alter the code sample being referenced in the article, changing the base class from DependencyObject to UIElement. DependencyObject is specifically called out in the article as a prerequisite to attached properties.

@ghost
Copy link
Author

ghost commented Dec 2, 2021

Hi @adegeo, the article states: "follow the WPF model of having an attached property also be a dependency property, by deriving your class from DependencyObject." It doesn't say derive directly from DependencyObject.

Per UiElement docs, UiElement derives from DependencyObject:
image

If the Aquarium example derives directly from DependencyObject, the build will properly fail, as a result of the new snippet validation test code that ensures a valid type is passed to the accessors.

@adegeo
Copy link
Contributor

adegeo commented Dec 7, 2021

I guess my only issue with this is that we're speaking (usually) to people who don't really know much about WPF and how this system works. Maybe the example should be simplified if it requires using a derived type such as UIElement. The UIElement concept is potentially unknown to the reader at the time they read this article.

@ghost
Copy link
Author

ghost commented Dec 8, 2021

@adegeo, the .NET Framework 4.0 article as well as the 6.0 version use UIElement in the snippet. UIElement is explained in both versions of the article: "The target type can be more specific than object. For example, the DockPanel.GetDock accessor method types the target as UIElement because the attached property is intended to be set on UIElement instances." To make that even clearer, I've added "UiElement indirectly derives from DependencyObject".

Revisiting this article, I see an issue that stemmed from the .NET Framework 4.0 version. That version says "The signature for the GetPropertyName accessor must be: public static object GetPropertyName(object target)". Actually, the signature must be "public static object GetPropertyName(DependencyObject target)", because GetValue and SetValue are DependencyObject methods, not Object methods. I've updated the .NET 6.0 article accordingly.

I propose we leave the snippet as-is but add the extra article clarification and update accessor signatures (done in this PR). If you still want me to change UIElement to Dependency property, I'll make that change as well. This PR updates the .NET 6.0 article+snippet only.

Copy link
Contributor

@adegeo adegeo left a comment

Choose a reason for hiding this comment

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

I like your changes. Thanks!

@adegeo adegeo merged commit 71675be into dotnet:main Dec 9, 2021
@ghost ghost deleted the trisshores-attached-properties-snippet-update branch December 9, 2021 15:46
@ghost
Copy link
Author

ghost commented Dec 9, 2021

@adegeo, thank you.

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.

2 participants