search

mono / api-doc-tools

.NET Reference API Toolchain
MIT License
68 stars 48 forks source link

Some attached properties still missing #426

Open mairaw opened 5 years ago

mairaw commented 5 years ago

We still have some missing attached properties

mairaw commented 5 years ago

After running mdoc 5.7.4.9, this list is reduced.

Currently, the following attached properties are still missing:

joelmartinez commented 5 years ago

Internal item logged

mairaw commented 5 years ago

Another thing I've noticed is that some of the ones that were added are only identified as .NET Core 3.0.

Different issue?

Example: https://docs.microsoft.com/en-us/dotnet/api/system.windows.navigation.baseurihelper.baseuri?view=netcore-3.0

joelmartinez commented 5 years ago

@mairaw offhand I think yes? but it shows here that the method exists in other monikers as well, so we should be seeing it defined for the .net fw: https://docs.microsoft.com/en-us/dotnet/api/system.windows.navigation.baseurihelper.getbaseuri?view=netcore-3.0

mairaw commented 5 years ago

Adding some other missing APIs to the list:

joelmartinez commented 5 years ago

@mairaw I checked with the latest release (https://github.com/dotnet/dotnet-api-docs/pull/2773) the first one on the list IsHitTestVisibleInChrome ... and the issue here is that it doesn't have a getter method, there's only the field IsHitTestVisibleInChromeProperty ... and a set method.

The existing understanding for implementing this feature came from here

The attached property provider must also provide static GetPropertyName and SetPropertyName methods as accessors for the attached property; failing to do this will result in the property system being unable to use your attached property.

In a previous release, we modified it to allow for read-only attached properties (ie. only a "get" accessor) ... this would be a new feature, effectively to allow for write-only attached properties.

mairaw commented 4 years ago

The current list of missing attached properties:

I'd check with @SamBent the expectation/definition of attached properties and whether they can just have a setter.

I also opened two additional bugs: Bug 1: missing value tag Bug 2: missing versions o the Applies to section

SamBent commented 4 years ago

The first two have both setters and getters in the code. Maybe your discovery process is confused because the argument type is IInputElement (instead of DependencyObject)?

Similarly, the next two have setters and getters with argument type Object (instead of DependencyObject). Here's the code.

Similarly, the last one also has argument type Object. Code.

Most attached properties use DependencyObject's built-in property storage facility (the "effective value table"). But an attached property can work with other kinds of objects, provided the owner is willing to use some other way to store the property values.

mairaw commented 4 years ago

Found one more while validating the latest IntelliSense: https://msdn.microsoft.com/en-us/library/system.windows.media.animation.storyboard.targetproperty?toc=xxx

This was erroneously redirected to https://docs.microsoft.com/en-us/dotnet/api/system.windows.media.animation.storyboard.targetproperty

I also noticed that the link from https://docs.microsoft.com/en-us/dotnet/api/system.windows.media.animation.storyboard.targetpropertyproperty?view=netframework-4.8 summary goes to the field, even though we're linking to the property there

FYI @billwagner @gewarren - you'd need to work with @Kexu to fix the redirect. For others that are missing, we temporarily have the page on previous versions until this issue is fixed.

adegeo commented 4 years ago

@kexu was the redirect fixed? @gewarren how do we verify the page generation is fixed?

gewarren commented 4 years ago

@adegeo I'm not sure, but I verified that all the missing attached properties in Maira's most recent list are now present.

adegeo commented 4 years ago

EDIT The linked issue does show the problem that still exists. While the field is there, the attached property is in fact missing. If the attached property is generated and added to the docs, it ends up conflicting with one of the fields

Field Attached
TargetProperty Target
TargetNameProperty TargetName
TargetPropertyProperty TargetProperty <-- error, same name as other field

The TOC shows it missing:

image

gewarren commented 4 years ago

@mimisasouvanh We still have a problem with at least one missing attached property, where its name clashes with a field of the same name and presumably this is why it doesn't appear in the ECMAXML. See Andy's comment above.

joelmartinez commented 4 years ago

@gewarren @mimisasouvanh Yes this is something that's currently explicitly looked for in the code, and we don't generate an attached property if there's a field or property that already has that name.

I'm not sure that we are able to support it because the docid will clash. What would you recommend here?

/cc @TianqiZhang

gewarren commented 4 years ago

@joelmartinez The DocId would be differentiated by the F: or P: in front of it, respectively.

<MemberSignature Language="DocId" Value="F:System.Windows.Media.Animation.Storyboard.TargetProperty" /> <MemberSignature Language="DocId" Value="P:System.Windows.Media.Animation.Storyboard.TargetProperty" />

joelmartinez commented 4 years ago

@gewarren ahh ... of course you're right, however that still doesn't 100% resolve the issue, because the site's URL doesn't have a reference to that prefix. So we have this URL, which currently goes to the field

https://docs.microsoft.com/en-us/dotnet/api/System.Windows.Media.Animation.Storyboard.TargetProperty

The attached property would want that exact same endpoint

adegeo commented 4 years ago

The resulting link though, would not. It would just be the full type name.

gewarren commented 4 years ago

It seems like C# gives a compile time error when you try to add two members with the same name, so this clash can only occur with attached properties? I think it's a corner case, but could we add a suffix to the URL for the attached property, like "AP" or ".AP"?

https://docs.microsoft.com/dotnet/api/System.Windows.Media.Animation.Storyboard.TargetPropertyAP

joelmartinez commented 4 years ago

@gewarren I think this is something that in general we have tried to avoid ... but this is quite an odd situation. You should probably create a new devops bug for this scenario specifically, because I suspect it will require quite a different mitigation strategy than what we've done for the rest of the attached properties here.

gewarren commented 4 years ago

I logged a bug here: https://dev.azure.com/ceapex/Engineering/_workitems/edit/283041