Skip to content
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.

Sign up for GitHub

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

Add type-forwarders for Xamarin.Android compatibility to System.Drawing.Common.dll #82839

Merged
merged 3 commits into from
Mar 8, 2023
Merged

Add type-forwarders for Xamarin.Android compatibility to System.Drawing.Common.dll #82839

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
72a8123
Add type-forwarders for Xamarin.Android compatibility to System.Drawi…
akoeplinger Mar 1, 2023
7f1bded
Add ApiCompat suppressions
akoeplinger Mar 2, 2023
c0046aa
PR feedback
akoeplinger Mar 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/libraries/System.Drawing.Common/src/System.Drawing.Common.Forwards.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

// This is required for back-compatibility with legacy Xamarin which had these types in System.Drawing.Common.dll
Copy link
Member

Choose a reason for hiding this comment

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

You are adding these to the implementation assembly only. That's intentional, right?

Copy link
Member

@filipnavara filipnavara Mar 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think that is intentional. We only need the type-forwarders for binary compatibility at runtime. I am not sure how it works with the ApiCompat checks though.

SystemColors and ColorTranslator have their respective TypeForwardedTo in the ref sources. These two types were moved between assemblies between .NET Core 2 and 3 IIRC. Do I understand correctly that the implementation assembly previously got the TypeForwardedTo for these two types thanks to IsPartialFacadeAssembly = true?

Copy link
Member Author

@akoeplinger akoeplinger Mar 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes it is intentional. I'll suppress the ApiCompat checks since we don't expect having these types in the ref.

Do I understand correctly that the implementation assembly previously got the TypeForwardedTo for these two types thanks to IsPartialFacadeAssembly = true?

Yes. We could remove the IsPartialFacadeAssembly=true given we have SystemColors and ColorTranslator explicitly here, or remove these two from the explicit list and let the partial facade generator cover them. @ViktorHofer any preference?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, we'd still need the IsPartialFacadeAssembly for the other TFMs so we'd need a conditional. I'll just leave it as is.

Copy link
Member

@ViktorHofer ViktorHofer Mar 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm fine with that approach and it follows what we do in our shims builds (manual type forwards only exposed in src but not ref). Just to get a second pair of eyes, let me cc @ericstj

[assembly: System.Runtime.CompilerServices.TypeForwardedTo(typeof(System.Drawing.Color))]
[assembly: System.Runtime.CompilerServices.TypeForwardedTo(typeof(System.Drawing.ColorTranslator))]
[assembly: System.Runtime.CompilerServices.TypeForwardedTo(typeof(System.Drawing.KnownColor))]
[assembly: System.Runtime.CompilerServices.TypeForwardedTo(typeof(System.Drawing.Point))]
[assembly: System.Runtime.CompilerServices.TypeForwardedTo(typeof(System.Drawing.PointF))]
[assembly: System.Runtime.CompilerServices.TypeForwardedTo(typeof(System.Drawing.Rectangle))]
[assembly: System.Runtime.CompilerServices.TypeForwardedTo(typeof(System.Drawing.RectangleF))]
[assembly: System.Runtime.CompilerServices.TypeForwardedTo(typeof(System.Drawing.Size))]
[assembly: System.Runtime.CompilerServices.TypeForwardedTo(typeof(System.Drawing.SizeF))]
[assembly: System.Runtime.CompilerServices.TypeForwardedTo(typeof(System.Drawing.SystemColors))]
1 change: 1 addition & 0 deletions src/libraries/System.Drawing.Common/src/System.Drawing.Common.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ Since .NET 7, non-Windows platforms are not supported, even with the runtime con

<ItemGroup Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'">
<Compile Include="SRDescriptionAttribute.cs" />
<Compile Include="System.Drawing.Common.Forwards.cs" />
<Compile Include="System\Drawing\Bitmap.cs" />
<Compile Include="System\Drawing\BitmapSuffixInSameAssemblyAttribute.cs" />
<Compile Include="System\Drawing\BitmapSuffixInSatelliteAssemblyAttribute.cs" />
Expand Down