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.

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 support for System.Drawing on Windows. #20593

Merged
merged 1 commit into from Jun 5, 2017

Conversation

@mellinoe
Contributor

mellinoe commented Jun 1, 2017

This change adds a new library, System.Drawing.Common, which supports a large subset of the System.Drawing API. It is a direct port of the code from the .NET Framework codebase. The code has been cleaned, formatted, and sanitized to match the code style of corefx, but is otherwise very
similar to the .NET Framework version. There are a few key differences:

  • TypeConverter's have been omitted for now. If we want to add these to .NET Core, we should add them to the TypeConverter library, rather than System.Drawing.Common itself. Note that there's already a few TypeConverter's for the types that are in System.Drawing.Primitives.
  • Some parts of the library do not respond to "System Events", because they are not yet implemented in .NET Core. This means that fonts and colors will not automatically update when the system defaults change, as they do in .NET Framework. The code is still there, but behind the feature flag "FEATURE_SYSTEM_EVENTS".
  • Various attributes have been removed from the codebase, because they are no longer applicable. Examples:
    • Designer-related attributes (DefaultProperty, DefaultEvent, Editor, etc.)
    • CAS-related attributes. CAS-related method calls (Security demands and asserts) have also been removed.

There is still further cleanup that can be done in order to match the style and structure of corefx. For example, the interop code should be reorganized to match other BCL libraries.

NOTE: This implementation will only work on Windows, and not in UWP. This is a strictly compatibility-driven feature. Any changes made to it should not significantly diverge from the .NET Framework implementation. Libraries should only take a dependency on System.Drawing.Common if they
are relying on legacy components from .NET Framework; newly-written code and features should rely on modern alternatives.

Add support for System.Drawing on Windows.
This change adds a new library, System.Drawing.Common, which supports a
large subset of the System.Drawing API. It is a direct port of the code
from the .NET Framework codebase. The code has been cleaned, formatted,
and sanitized to match the code style of corefx, but is otherwise very
similar to the .NET Framework version. There are a few key differences:

* TypeConverter's have been omitted for now. If we want to add these to
  .NET Core, we should add them to the TypeConverter library, rather than
  System.Drawing.Common itself.
* Some parts of the library do not respond to "System Events", because
  they are not yet implemented in .NET Core. This means that fonts and
  colors will not automatically update when the system defaults change,
  as they do in .NET Framework. The code is still there, but behind the
  feature flag "FEATURE_SYSTEM_EVENTS".
* Various attributes have been removed from the codebase, because they are
  no longer applicable. Examples:
  * Designer-related attributes (DefaultProperty, DefaultEvent, Editor,
    etc.)
  * CAS-related attributes. CAS-related method calls (Security demands and
    asserts) have also been removed.

There is still further cleanup that can be done in order to match the
style and structure of corefx. For example, the Interop code should be
reorganized to match other BCL libraries.

NOTE: This implementation will only work on Windows, and not in UWP. This
is a strictly compatibility-driven feature. Any changes made to it should
not significantly diverge from the .NET Framework implementation.
Libraries should only take a dependency on System.Drawing.Common if they
are relying on legacy components from .NET Framework; newly-written code
and features should rely on modern alternatives.
@stephentoub

This comment has been minimized.

Show comment
Hide comment
@stephentoub

stephentoub Jun 1, 2017

Member

Thanks, @mellinoe. What kind of / level of code review are you looking for on this?

Member

stephentoub commented Jun 1, 2017

Thanks, @mellinoe. What kind of / level of code review are you looking for on this?

@mellinoe

This comment has been minimized.

Show comment
Hide comment
@mellinoe

mellinoe Jun 1, 2017

Contributor

@stephentoub I think there's a few things I'd like to accomplish before being "ready to merge":

  • Make sure the build configurations are specified correctly for System.Drawing.Common.
  • Make sure that .NET Framework code built against System.Drawing will work / type-forward correctly with this new library. This needs a shim project to be configured (I think).
  • Finalize a list of code style and refactoring work items that should be done as a follow-up. I just spent a couple full days getting to this state, but this code is very old and could still use some more work. I've compiled a list as I worked, but I'm sure that I've not caught everything that needs fixed.
Contributor

mellinoe commented Jun 1, 2017

@stephentoub I think there's a few things I'd like to accomplish before being "ready to merge":

  • Make sure the build configurations are specified correctly for System.Drawing.Common.
  • Make sure that .NET Framework code built against System.Drawing will work / type-forward correctly with this new library. This needs a shim project to be configured (I think).
  • Finalize a list of code style and refactoring work items that should be done as a follow-up. I just spent a couple full days getting to this state, but this code is very old and could still use some more work. I've compiled a list as I worked, but I'm sure that I've not caught everything that needs fixed.

@karelz karelz added this to the 2.1.0 milestone Jun 2, 2017

@danmosemsft

This comment has been minimized.

Show comment
Hide comment
@danmosemsft

danmosemsft Jun 2, 2017

Member

@mellinoe what is your test strategy? Does Mono have much we can reuse? Have you taken a look at what Desktop has?

Member

danmosemsft commented Jun 2, 2017

@mellinoe what is your test strategy? Does Mono have much we can reuse? Have you taken a look at what Desktop has?

@mellinoe

This comment has been minimized.

Show comment
Hide comment
@mellinoe

mellinoe Jun 2, 2017

Contributor

@danmosemsft Yes, mono has tests that we will re-use. .NET Framework does not have great test assets -- they are mainly for higher-level components like WinForms which indirectly test System.Drawing.

Contributor

mellinoe commented Jun 2, 2017

@danmosemsft Yes, mono has tests that we will re-use. .NET Framework does not have great test assets -- they are mainly for higher-level components like WinForms which indirectly test System.Drawing.

@mellinoe mellinoe referenced this pull request Jun 2, 2017

Closed

Support Full System.Drawing Functionality on .NET Core #20325

4 of 5 tasks complete
@hughbe

This comment has been minimized.

Show comment
Hide comment
@hughbe

hughbe Jun 5, 2017

Collaborator

I could look in to adding/porting some Mono tests if you're not doing so already

Collaborator

hughbe commented Jun 5, 2017

I could look in to adding/porting some Mono tests if you're not doing so already

/// </devdoc>
Manual = SafeNativeMethods.DMBIN_MANUAL,
/// <include file='doc\PaperSourceKind.uex' path='docs/doc[@for="PaperSourceKind.Envelope"]/*' />

This comment has been minimized.

@hughbe

hughbe Jun 5, 2017

Collaborator

Do we usually have doc-comments like this?

@hughbe

hughbe Jun 5, 2017

Collaborator

Do we usually have doc-comments like this?

This comment has been minimized.

@mellinoe

mellinoe Jun 5, 2017

Contributor

We don't. Cleaning these up is on my list of follow-up items, which I'll make sure to file tomorrow. Unfortunately, this style of comment is pretty common in our .NET Framework codebase; I'm not even sure what the names and paths refer to.

@mellinoe

mellinoe Jun 5, 2017

Contributor

We don't. Cleaning these up is on my list of follow-up items, which I'll make sure to file tomorrow. Unfortunately, this style of comment is pretty common in our .NET Framework codebase; I'm not even sure what the names and paths refer to.

@mellinoe

This comment has been minimized.

Show comment
Hide comment
@mellinoe

mellinoe Jun 5, 2017

Contributor

I could look in to adding/porting some Mono tests if you're not doing so already

Sounds great; I could definitely use some help with that. I ported a select few test cases and tried running them against the implementation here, but there's still a ton of work to be done there. Porting from Nunit to Xunit was fairly annoying and time-consuming, and after that there were still minor differences that caused lots of tests to fail. I will file another issue tomorrow about this so we can try to plan it out and split up the work.

Contributor

mellinoe commented Jun 5, 2017

I could look in to adding/porting some Mono tests if you're not doing so already

Sounds great; I could definitely use some help with that. I ported a select few test cases and tried running them against the implementation here, but there's still a ton of work to be done there. Porting from Nunit to Xunit was fairly annoying and time-consuming, and after that there were still minor differences that caused lots of tests to fail. I will file another issue tomorrow about this so we can try to plan it out and split up the work.

@mellinoe

This comment has been minimized.

Show comment
Hide comment
@mellinoe

mellinoe Jun 5, 2017

Contributor

Merging this in now. I've filed several follow-up issues for further System.Drawing work:

Contributor

mellinoe commented Jun 5, 2017

Merging this in now. I've filed several follow-up issues for further System.Drawing work:

@mellinoe mellinoe merged commit e15746e into dotnet:master Jun 5, 2017

40 checks passed

CROSS Check Build finished.
Details
Innerloop OSX10.12 Debug x64 Build and Test Build finished.
Details
Innerloop OSX10.12 Release x64 Build and Test Build finished.
Details
Innerloop Tizen armel Debug Cross Build Build finished.
Details
Innerloop Tizen armel Release Cross Build Build finished.
Details
Innerloop Ubuntu14.04 arm Release Cross Build Build finished.
Details
Innerloop Ubuntu16.04 arm Debug Cross Build Build finished.
Details
Innerloop netfx Windows_NT Debug x86 Build and Test Build finished.
Details
Innerloop netfx Windows_NT Release x64 Build and Test Build finished.
Details
Linux x64 Tests - Debug - Centos.73.Amd64.Open Passed 188827 (273 skipped)
Details
Linux x64 Tests - Debug - Debian.87.Amd64.Open Passed 188902 (268 skipped)
Details
Linux x64 Tests - Debug - RedHat.73.Amd64.Open Passed 188827 (273 skipped)
Details
Linux x64 Tests - Debug - Ubuntu.1404.Amd64.Open Passed 188890 (272 skipped)
Details
Linux x64 Tests - Debug - Ubuntu.1604.Amd64.Open Passed 188901 (272 skipped)
Details
Linux x64 Tests - Debug - Ubuntu.1610.Amd64.Open Passed 188901 (272 skipped)
Details
Linux x64 Tests - Debug - fedora.25.amd64.Open Passed 188826 (273 skipped)
Details
Linux x64 Tests - Debug - suse.422.amd64.Open Passed 188862 (268 skipped)
Details
Linux x64 Tests - Release - Centos.73.Amd64.Open Passed 188826 (271 skipped)
Details
Linux x64 Tests - Release - Debian.87.Amd64.Open Passed 188901 (266 skipped)
Details
Linux x64 Tests - Release - RedHat.73.Amd64.Open Passed 188826 (271 skipped)
Details
Linux x64 Tests - Release - Ubuntu.1404.Amd64.Open Passed 188889 (270 skipped)
Details
Linux x64 Tests - Release - Ubuntu.1604.Amd64.Open Passed 188900 (270 skipped)
Details
Linux x64 Tests - Release - Ubuntu.1610.Amd64.Open Passed 188900 (270 skipped)
Details
Linux x64 Tests - Release - fedora.25.amd64.Open Passed 188825 (271 skipped)
Details
Linux x64 Tests - Release - suse.422.amd64.Open Passed 188861 (266 skipped)
Details
Portable Linux x64 Debug Build Build finished.
Details
Portable Linux x64 Release Build Build finished.
Details
Portable Windows x64 Debug Build Build finished.
Details
Portable Windows x64 Release Build Build finished.
Details
Vertical uap Build Build finished.
Details
Vertical uapaot Build Build finished.
Details
Win x64 tests - Debug - Windows.10.Amd64.Open Passed 196891 (1072 skipped)
Details
Win x64 tests - Debug - Windows.10.Nano.Amd64.Open Passed 196286 (1298 skipped)
Details
Win x64 tests - Debug - Windows.7.Amd64.Open Passed 196708 (1150 skipped)
Details
Win x64 tests - Debug - Windows.81.Amd64.Open Passed 196826 (1089 skipped)
Details
Win x64 tests - Release - Windows.10.Amd64.Open Passed 196890 (1070 skipped)
Details
Win x64 tests - Release - Windows.10.Nano.Amd64.Open Passed 196285 (1296 skipped)
Details
Win x64 tests - Release - Windows.7.Amd64.Open Passed 196707 (1148 skipped)
Details
Win x64 tests - Release - Windows.81.Amd64.Open Passed 196825 (1087 skipped)
Details
Windows_NT Debug AllConfigurations Build Build finished.
Details
@@ -7,7 +7,7 @@
namespace System.Drawing
{
[SuppressMessage("Microsoft.Design", "CA1008:EnumsShouldHaveZeroValue")]
internal enum KnownColor
public enum KnownColor

This comment has been minimized.

@weshaggard

weshaggard Jun 6, 2017

Member

Adding this as a public type in System.Drawing.Primitives is going to cause issues with netstandard. In particular currently the NETStandard.Library contains a shim for System.Drawing.Primitives so at a minimum we need to bump the minor version of System.Drawing.Primitives to ensure folks targeting .NET Core will get the later version.

@weshaggard

weshaggard Jun 6, 2017

Member

Adding this as a public type in System.Drawing.Primitives is going to cause issues with netstandard. In particular currently the NETStandard.Library contains a shim for System.Drawing.Primitives so at a minimum we need to bump the minor version of System.Drawing.Primitives to ensure folks targeting .NET Core will get the later version.

This comment has been minimized.

@weshaggard

weshaggard Jun 6, 2017

Member

(assuming we haven't already bumped the minor version, I didn't verify)

@weshaggard

weshaggard Jun 6, 2017

Member

(assuming we haven't already bumped the minor version, I didn't verify)

This comment has been minimized.

@mellinoe

mellinoe Jun 6, 2017

Contributor

I haven't bumped the minor version; will do.

@mellinoe

mellinoe Jun 6, 2017

Contributor

I haven't bumped the minor version; will do.

<Reference Include="System.Runtime.InteropServices" />
<Reference Include="System.ComponentModel" />
<Reference Include="System.ComponentModel.Primitives" />
<Reference Include="System.Security.Permissions" />

This comment has been minimized.

@weshaggard

weshaggard Jun 6, 2017

Member

We should try to avoid depending on System.Security.Permissions.

@weshaggard

weshaggard Jun 6, 2017

Member

We should try to avoid depending on System.Security.Permissions.

This comment has been minimized.

@danmosemsft

danmosemsft Jun 6, 2017

Member

@mellinoe looks like it's needed for two things -- a bunch of use of CAS types, which we should just rip out; and definition of PrintingPermission/PPAttribute, which is already in S.S.Permissions and we should just rip out. perhaps you could open an issue to do this, and remove this reference?

@danmosemsft

danmosemsft Jun 6, 2017

Member

@mellinoe looks like it's needed for two things -- a bunch of use of CAS types, which we should just rip out; and definition of PrintingPermission/PPAttribute, which is already in S.S.Permissions and we should just rip out. perhaps you could open an issue to do this, and remove this reference?

This comment has been minimized.

@mellinoe

mellinoe Jun 6, 2017

Contributor

Cleaning up the unnecessary permissions stuff is on my list. I will track removing this reference entirely, as well. #20706

@mellinoe

mellinoe Jun 6, 2017

Contributor

Cleaning up the unnecessary permissions stuff is on my list. I will track removing this reference entirely, as well. #20706

<Import Project="..\dir.props" />
<PropertyGroup>
<AssemblyVersion>4.1.0.0</AssemblyVersion>
<AssemblyKey>MSFT</AssemblyKey>

This comment has been minimized.

@weshaggard

weshaggard Jun 6, 2017

Member

We should use the Open key for new libraries like this.

@weshaggard

weshaggard Jun 6, 2017

Member

We should use the Open key for new libraries like this.

This comment has been minimized.

@mellinoe

mellinoe Jun 6, 2017

Contributor

Will change.

@mellinoe

mellinoe Jun 6, 2017

Contributor

Will change.

@weshaggard

This comment has been minimized.

Show comment
Hide comment
@weshaggard

weshaggard Jun 6, 2017

Member

@mellinoe do you also need follow-up items to enable packaging for this library when it is ready?

Member

weshaggard commented Jun 6, 2017

@mellinoe do you also need follow-up items to enable packaging for this library when it is ready?

@mellinoe

This comment has been minimized.

Show comment
Hide comment
@mellinoe

mellinoe Jun 6, 2017

Contributor

@mellinoe do you also need follow-up items to enable packaging for this library when it is ready?
Delete branch

@weshaggard I'll file an issue to track that explicitly.

Contributor

mellinoe commented Jun 6, 2017

@mellinoe do you also need follow-up items to enable packaging for this library when it is ready?
Delete branch

@weshaggard I'll file an issue to track that explicitly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment