Skip to content

Update System.Drawing to reflect GDI+ changes #32873

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

Merged
safern merged 3 commits into from
Mar 19, 2020
Merged

Update System.Drawing to reflect GDI+ changes #32873

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c90c59f
Update System.Drawing to reflect GDI+ changes
qmfrederik Feb 26, 2020
24bf7df
Skip test on NetFx
qmfrederik Feb 27, 2020
b9f76ee
Add XML documentation
qmfrederik Mar 13, 2020
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
4 changes: 4 additions & 0 deletions src/libraries/System.Drawing.Common/ref/System.Drawing.Common.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2105,6 +2105,9 @@ public sealed partial class Encoder
public static readonly System.Drawing.Imaging.Encoder ScanMethod;
public static readonly System.Drawing.Imaging.Encoder Transformation;
public static readonly System.Drawing.Imaging.Encoder Version;
public static readonly System.Drawing.Imaging.Encoder ColorSpace;
public static readonly System.Drawing.Imaging.Encoder ImageItems;
public static readonly System.Drawing.Imaging.Encoder SaveAsCmyk;
public Encoder(System.Guid guid) { }
public System.Guid Guid { get { throw null; } }
}
Expand Down Expand Up @@ -2152,6 +2155,7 @@ public enum EncoderParameterValueType
ValueTypeLongRange = 6,
ValueTypeUndefined = 7,
ValueTypeRationalRange = 8,
ValueTypePointer = 9,
}
public enum EncoderValue
{
Expand Down
15 changes: 15 additions & 0 deletions src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/Encoder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,21 @@ public sealed class Encoder
public static readonly Encoder ChrominanceTable = new Encoder(new Guid(unchecked((int)0xf2e455dc), unchecked((short)0x09b3), unchecked((short)0x4316), new byte[] { 0x82, 0x60, 0x67, 0x6a, 0xda, 0x32, 0x48, 0x1c }));
public static readonly Encoder SaveFlag = new Encoder(new Guid(unchecked((int)0x292266fc), unchecked((short)0xac40), unchecked((short)0x47bf), new byte[] { 0x8c, 0xfc, 0xa8, 0x5b, 0x89, 0xa6, 0x55, 0xde }));

/// <summary>
/// An <see cref="Encoder" /> object that is initialized with the globally unique identifier for the color space category.
/// </summary>
public static readonly Encoder ColorSpace = new Encoder(new Guid(unchecked((int)0xae7a62a0), unchecked((short)0xee2c), unchecked((short)0x49d8), new byte[] { 0x9d, 0x07, 0x1b, 0xa8, 0xa9, 0x27, 0x59, 0x6e }));

/// <summary>
/// An <see cref="Encoder" /> object that is initialized with the globally unique identifier for the image items category.
/// </summary>
public static readonly Encoder ImageItems = new Encoder(new Guid(unchecked((int)0x63875e13), unchecked((short)0x1f1d), unchecked((short)0x45ab), new byte[] { 0x91, 0x95, 0xa2, 0x9b, 0x60, 0x66, 0xa6, 0x50 }));

/// <summary>
/// An <see cref="Encoder" /> object that is initialized with the globally unique identifier for the save as CMYK category.
Copy link
Member

@safern safern Mar 16, 2020

Choose a reason for hiding this comment

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

I think we try to use the full name even if it is not needed, but not sure. @carlossanlop ?

Copy link
Contributor

@carlossanlop carlossanlop Mar 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Do you mean using <see cref="T:System.Drawing.Imaging.Encoder" />? No, it's not needed in triple slash comments. If the API was not recognized within this context, VS would give you an error. But in this case, we recognize the Encoder type.
When the project gets built and the triple slash comments get converted to their respective xml files, all API names will be automatically converted to their full name.

Copy link
Contributor Author

@qmfrederik qmfrederik Mar 16, 2020

Choose a reason for hiding this comment

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

Using the T: prefix results in build errors: error CA1200: Avoid using cref tags with a prefix

/// </summary>
public static readonly Encoder SaveAsCmyk = new Encoder(new Guid(unchecked((int)0xa219bbc9), unchecked((short)0x0a9d), unchecked((short)0x4005), new byte[] { 0xa3, 0xee, 0x3a, 0x42, 0x1b, 0x8b, 0xb0, 0x6c }));
Copy link
Member

@stephentoub stephentoub Mar 13, 2020

Choose a reason for hiding this comment

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

@safern, @carlossanlop, I assume we want to add XML comments to these in order to populate the docs? For consistency maybe we should do so for all the fields here.

Copy link
Member

@safern safern Mar 13, 2020

Choose a reason for hiding this comment

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

@stephentoub, good catch. @qmfrederik could you please also include XML docs for these new APIs?

Copy link
Contributor

@carlossanlop carlossanlop Mar 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@qmfrederik the only information that the existing APIs need on top of them as triple slash is their summary, which you can get from their MS Docs documentation.


private Guid _guid;

public Encoder(Guid guid)
Expand Down
3 changes: 3 additions & 0 deletions src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/EncoderParameter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,9 @@ public EncoderParameter(Encoder encoder, int numberValues, EncoderParameterValue
case EncoderParameterValueType.ValueTypeRationalRange:
size = 2 * 2 * 4;
break;
case EncoderParameterValueType.ValueTypePointer:
size = IntPtr.Size;
break;
default:
throw Gdip.StatusException(Gdip.WrongState);
}
Expand Down
6 changes: 5 additions & 1 deletion src/libraries/System.Drawing.Common/src/System/Drawing/Imaging/EncoderParameterValueType.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ public enum EncoderParameterValueType
/// Two Rationals. The first Rational specifies the lower end and the second specifies the higher end.
/// All values are inclusive at both ends
/// </summary>
ValueTypeRationalRange = 8
ValueTypeRationalRange = 8,
/// <summary>
/// The parameter is a pointer to a block of custom metadata.
/// </summary>
ValueTypePointer = 9,
}
}
17 changes: 17 additions & 0 deletions src/libraries/System.Drawing.Common/tests/ImageTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,23 @@ public static IEnumerable<object[]> GetEncoderParameterList_ReturnsExpected_Test
new Guid(unchecked((int)0xa219bbc9), unchecked((short)0x0a9d), unchecked((short)0x4005), new byte[] { 0xa3, 0xee, 0x3a, 0x42, 0x1b, 0x8b, 0xb0, 0x6c }) /* Encoder.SaveAsCmyk.Guid */
}
};

#if !NETFRAMEWORK
Copy link
Member

@safern safern Mar 10, 2020

Choose a reason for hiding this comment

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

Nit: for tests and test data, we usually use, if (!PlatformDetection.IsFullFramework)

Copy link
Contributor Author

@qmfrederik qmfrederik Mar 17, 2020

Choose a reason for hiding this comment

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

Hmm,

ImageTests.cs(161,36): error CS0117: (NETCORE_ENGINEERING_TELEMETRY=Build) 'PlatformDetection' does not contain a definition for 'IsFullFramework'

What am I missing here?

Copy link
Member

@filipnavara filipnavara Mar 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

IsFullFramework, obviously :)

It was removed in dotnet/corefx#37972.

Copy link
Contributor Author

@qmfrederik qmfrederik Mar 17, 2020

Choose a reason for hiding this comment

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

Ugh 😄 .

I'm a bit confused, because dotnet/corefx#37972 and #28619 seem to indicate tests would no longer run on full framework, yet they do for System.Drawing.Common.

@safern What's the way forward here?

Copy link
Member

@filipnavara filipnavara Mar 17, 2020

Choose a reason for hiding this comment

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

The test should run on netfx only if the library targets netstandard2.0. In that case they should test the functionality of the netstandard2.0 library running on NetFX, not testing NetFX code itself.

Copy link
Member

@safern safern Mar 17, 2020

Choose a reason for hiding this comment

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

It's somewhat weird to have new APIs that are not present on certain platform. What will happen if you call them? I'd except something like PlatformNotSupportedException but that only works for new types and only if partial facade is generated. In this case it's new members on existing type.

You will not be able to reference them anymore as for ref we only build live for netcoreapp3.0 and netcoreapp5.0. The netfx assets harvested. So now that you mention it we no longer build netfx assets for System.Drawing.Common anymore as we just get them from the latest package and re-ship them in the new package. Maybe it is fine removing the netfx tests for this library.

cc: @ericstj @ViktorHofer @danmosemsft @JeremyKuhne -- I'm leaning towards removing testing netfx for S.Drawing.Common. What do you guys think?

Copy link
Member

@JeremyKuhne JeremyKuhne Mar 17, 2020

Choose a reason for hiding this comment

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

What do you guys think?

My only concern is that surfacing unintentional deviation in behavior from Framework becomes more difficult without runs there. If we had better test coverage I'd worry less, but I think we're pretty far from adequately covering GDI+ behavior in our current tests.

Copy link
Member

@safern safern Mar 17, 2020

Choose a reason for hiding this comment

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

Yeah that's a fair point. I would suggest leaving the tests on this PR and using the NetFramework property in PlatformDetection; then we can discuss further if we want to stop running tests for netfx in this project.

Copy link
Contributor Author

@qmfrederik qmfrederik Mar 18, 2020

Choose a reason for hiding this comment

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

So, unfortunately we went the full circle on this one - just using PlatformDetection does not work, because there's a reference to Encoder.ImageItems which was added as part of this PR, but is not present in NetFx.

Copy link
Member

@safern safern Mar 18, 2020

Choose a reason for hiding this comment

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

You're right. Ok, well this at least was a good discussion. Then we will need to go back to the compile time condition, sorry about that.

// NetFX doesn't support pointer-type encoder parameters, and doesn't define Encoder.ImageItems. Skip this test
// on NetFX.
yield return new object[]
{
ImageFormat.Jpeg,
new Guid[]
{
Encoder.Transformation.Guid,
Encoder.Quality.Guid,
Encoder.LuminanceTable.Guid,
Encoder.ChrominanceTable.Guid,
Encoder.ImageItems.Guid
}
};
#endif
}

[ActiveIssue("https://github.com/dotnet/runtime/issues/22221", TestPlatforms.AnyUnix)]
Expand Down