Removed usage of InternalsVisibleTo in all components #446
Conversation
- Copied minor helpers such as ArgumentNullExceptionExtensions - Made public any internal code required to build, mostly around Animations references in Media and Extensions references in Animations.
|
Tagging @Sergio0694 in on some of these questions, as a lot of the internal APIs are from his Animation library, so he may know more of why they were internal only. |
There was a problem hiding this comment.
@Sergio0694 can comment on the Animations changes.
Would like @Ryken100 to comment on the Shadow ones though, as those are mostly internal implementation details, but think there's the challenge that we have the base composition shadow and then the Win2D based one.
There was a problem hiding this comment.
minor: There's a note on L27 about better support in .NET for this method, so we could do a conditional here so it's better for WinUI?
There was a problem hiding this comment.
Yes, I think we're able to address this now, though we should do it as an improvement from a new Issue/PR.
| @@ -57,7 +57,7 @@ public sealed class AnimationSet : DependencyObjectCollection | |||
| /// <summary> | |||
| /// Gets or sets the weak reference to the parent that owns the current animation collection. | |||
| /// </summary> | |||
| internal WeakReference<UIElement>? ParentReference { get; set; } | |||
| public WeakReference<UIElement>? ParentReference { get; set; } | |||
There was a problem hiding this comment.
These seems like a case where protected is probably the better intent over public as this is an internal holder of a reference and not something anyone should just get at/use.
Wish we could easily see references from PRs on GitHub and what's using this... 😋
There was a problem hiding this comment.
Wish we could easily see references from PRs on GitHub and what's using this... 😋
You can do this from the general file browser on github, just select the right branch and file. Works from the "Files Changed" tabs in PRs, too.
| @@ -31,7 +31,7 @@ public sealed class AttachedDropShadow : AttachedShadowBase | |||
| #endif | |||
|
|
|||
| /// <inheritdoc/> | |||
| protected internal override bool SupportsOnSizeChangedEvent => true; | |||
| public override bool SupportsOnSizeChangedEvent => true; | |||
There was a problem hiding this comment.
Curious for all these protected internal cases why protected isn't enough either... @Ryken100 thoughts?
There was a problem hiding this comment.
For this SupportsOnSizeChangedEvent, it's referenced in
Which doesn't derive AttachedDropShadow. It seems like protected internal was allowing protected or internal access, losing internal and being protected means only derived classes can access it. Correct me if I'm wrong, @Ryken100!
We're testing these changes for three reasons:
The last reason is dismissed, CommunityToolkit/Tooling-Windows-Submodule#195 (comment) helped us verify that it was unrelated, and the fix offered by Mano and yourself @Sergio0694 double confirms that. The first two are still a possible concern. If we can verify that the use of |
This PR:
InternalsVisibleToin components in this repo, closes Problems with InternalsVisibleTo #441.ArgumentNullExceptionExtensions, keeping them internal.For 3), it's worth noting that several API surfaces were marked as public which were previously internal. Only the areas that needed to be exposed for other toolkit packages to build on top were updated.
These changes were required for these components to be extended by other components in the toolkit. I'm in favor of publishing them as public for others to build on to the same extent we have, unless we have information on why these were originally made internal and good reason to keep it.