Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

TypeConverters for System.Drawing #33092

Merged
merged 14 commits into from
Nov 9, 2018
Merged

TypeConverters for System.Drawing #33092

merged 14 commits into from
Nov 9, 2018

Conversation

satano
Copy link
Member

@satano satano commented Oct 27, 2018 •
edited
Loading

Closes #21129

Added type converters according to this comment. All the classes and their tests were just taken from previous PR.

@satano
Copy link
Member Author

satano commented Oct 28, 2018

@safern I looked at the logs of those two failing checks, but I do not understand what to do.

@safern
Copy link
Member

safern commented Oct 29, 2018

test Linux x64 Release Build please

@safern
Copy link
Member

safern commented Oct 29, 2018

FYI: @ericstj

@satano
Copy link
Member Author

satano commented Oct 31, 2018

test Linux arm64 Release Build

@safern
Copy link
Member

safern commented Oct 31, 2018

@satano ApiCompat was not running and was just fixed in: #33152

Would you mind rebasing on top of master and building the project, it should fail, then fixing the errors by adding these attributes to the ref?

@ericstj I expect this to fail for netcoreapp2.0 as well since the ref is built against netstandard and we're not including these attributes to netcoreapp2.0 implementation since System.Windows.Extensions is not supported in netcoreapp2.0. So should we add a netcoreapp3.0 ref and if def those attributes in the ref as well?

@safern
Copy link
Member

safern commented Nov 2, 2018

I suspect this is the error you're seeing:

15:13:28 D:\j\workspace\windows-TGrou---0d2c9ac4.packages\microsoft.dotnet.apicompat\1.0.0-beta.18530.4\build\Microsoft.DotNet.ApiCompat.targets(68,5): error : CannotRemoveAttribute : Attribute 'System.ComponentModel.TypeConverterAttribute' exists on 'System.Drawing.Imaging.ImageFormat' in the contract but not the implementation. [D:\j\workspace\windows-TGrou---0d2c9ac4\src\System.Drawing.Common\src\System.Drawing.Common.csproj]

That error is for netcoreapp2.0 -- this is what I was talking about in:

@ericstj I expect this to fail for netcoreapp2.0 as well since the ref is built against netstandard and we're not including these attributes to netcoreapp2.0 implementation since System.Windows.Extensions is not supported in netcoreapp2.0. So should we add a netcoreapp3.0 ref and if def those attributes in the ref as well?

So in order to fix it, add a netcoreapp configuration to the ref configurations.props and then the attributes that you added to the ref file, wrap them up inside an #if netcoreapp as well.

@safern
Copy link
Member

safern commented Nov 7, 2018

@satano since we're going to soon be forking our release branch for 3.0 and this is a priority for that I pushed the remaining changes. Thanks for pushing so hard towards getting this into netcoreapp3.0 😄

ericstj
ericstj reviewed Nov 7, 2018
View reviewed changes
src/System.Drawing.Common/ref/System.Drawing.Common.cs
@@ -279,6 +279,9 @@ public enum CopyPixelOperation
SourcePaint = 15597702,
Whiteness = 16711778,
}
#if netcoreapp
Copy link
Member

Choose a reason for hiding this comment

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

NIT: if you wanted to avoid the define you could put these in a separate file. EG: System.Drawing.Common.netcoreapp.cs

    [System.ComponentModel.TypeConverter("System.Drawing.FontConverter, System.Windows.Extensions, Version=4.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51")]
    public sealed partial class Font { }
    ...

I've done that in some other places.

src/System.Windows.Extensions/src/System.Windows.Extensions.csproj Outdated
</ItemGroup>
<ItemGroup Condition="'$(TargetGroup)' == 'netcoreapp'">
<Reference Include="System.Buffers" />
<Reference Include="System.Memory" />
Copy link
Member

Choose a reason for hiding this comment

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

Nit: ordering

src/System.Windows.Extensions/src/System.Windows.Extensions.csproj
<Compile Include="$(CommonPath)\CoreLib\System\Text\ValueStringBuilder.cs">
<Link>CoreLib\System\Text\ValueStringBuilder.cs</Link>
</Compile>
<Compile Include="System\Drawing\FontConverter.cs" />
Copy link
Member

@ericstj ericstj Nov 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I noticed that a lot of these look different from desktop. Looking through the PR history I see that these were ported from mono rather than desktop. That's a good start but some callers may care about the desktop behavior on Windows /cc @dreddy-work @safern

ericstj
ericstj reviewed Nov 7, 2018
View reviewed changes
src/System.Drawing.Common/ref/System.Drawing.Common.csproj
@@ -2,7 +2,7 @@
<PropertyGroup>
<ProjectGuid>{D7AEA698-275D-441F-B7A7-8491D1F0EFF0}</ProjectGuid>
<IsPartialFacadeAssembly Condition="'$(TargetsNetFx)' == 'true'">true</IsPartialFacadeAssembly>
<Configurations>net461-Debug;net461-Release;netfx-Debug;netfx-Release;netstandard-Debug;netstandard-Release</Configurations>
<Configurations>net461-Debug;net461-Release;netcoreapp-Debug;netcoreapp-Release;netfx-Debug;netfx-Release;netstandard-Debug;netstandard-Release</Configurations>
Copy link
Member

Choose a reason for hiding this comment

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

This is what broke the build. NETCoreApp builds from source, so you need to use project references for this.

@satano
Copy link
Member Author

satano commented Nov 7, 2018

@safern Im sorry I was quiet for some time. I did not have time for last week or so to look at this. Is there anything more to do? The build are still failing.

@satano satano closed this Nov 7, 2018
@satano satano reopened this Nov 7, 2018
@satano
Copy link
Member Author

satano commented Nov 7, 2018

Sorry. I accidently clicked "Close and comment".

@safern
Copy link
Member

safern commented Nov 8, 2018

The builds were still failing because we needed to change all the references to be project reference and only for netcoreapp. For netstandard we don't need references. I also rebased on top of master.

@danmoseley
Copy link
Member

@dotnet-bot test this please (rudely terminated)

@safern
Copy link
Member

safern commented Nov 8, 2018

test NETFX x86 Release Build please

safern
safern approved these changes Nov 9, 2018
View reviewed changes
Copy link
Member

@safern safern left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for all the hard effort @satano!!

@safern safern merged commit 78cf4e4 into dotnet:master Nov 9, 2018
@satano
Copy link
Member Author

satano commented Nov 9, 2018

@safern You (all) are welcome. :)

@danmoseley
Copy link
Member

Yes, your work and patience much appreciated @satano . If you choose to consider another up-for-grabs issue, I am confident it will not take 5 months to merge this time 😊

@satano satano deleted the 21129-SystemDrawingTypeConverters-3 branch November 15, 2018 18:38
@karelz karelz added this to the 3.0 milestone Nov 15, 2018
@safern safern mentioned this pull request Nov 21, 2018
Merged
@guylando guylando mentioned this pull request Jan 9, 2019
Closed
@yousiftouma
Copy link

What would be required to use this in a .net standard 2.0 project? Is this possible?

@safern
Copy link
Member

safern commented Sep 9, 2019

Is this possible?

Unfortunately no. These converters are only available when you target netcoreapp3.0 and you need to have a reference to both: System.Drawing.Common and System.Windows.Extensions.

@ericstj ericstj mentioned this pull request Jan 31, 2020
Closed
@danmoseley danmoseley mentioned this pull request Jan 31, 2020
Closed
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
@satano @safern
TypeConverters for System.Drawing (dotnet/corefx#33092)
2dcd694 
* Moved InvalidAsynchronousStateExceptionTests to the InvalidAsynchronousStateException project.

* Added Font, Icon, Image and ImageFormat converters.

* Source code in "ref" is in one file

* Converters are only for Windows.

* TypeConverter attribute is only for netcoreapp.

* Icon can be converted to Bitmap, some tests skipped on NETFX.

* Bitmap as not supported conversion removed from tests.

* Typo

* TypeConverterAttribute in ref project

* TypeConverterAttribute is with full name in ref project

* IconConverter added to unix Icon

* Add netcoreapp3.0 ref assembly containing TypeConverters in windows.extensions

* Addressed comments: project reference, invalid cast.

* Make all references for netcoreapp project references, delete references for netstandard


Commit migrated from dotnet/corefx@78cf4e4
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ericstj ericstj ericstj left review comments

@jkotas jkotas jkotas left review comments

@safern safern safern approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.0
Development

Successfully merging this pull request may close these issues.

TypeConverters for System.Drawing
7 participants
@satano @safern @danmoseley @yousiftouma @jkotas @ericstj @karelz