TypeConverters for System.Drawing

[satano]

Closes #21129

@safern I closed the first PR #28426, because I messed it with rebase onto master. So review this please.

[satano]

Well. Builds are failing with error

Assembly 'System.ComponentModel.TypeConverter' is missing dependency 'System.Drawing.Common'.

I had the same error in the first PR and I do not know what it means and how to get rid of it. I added reference to System.Drawing.Common to the project in src and also in ref folder.

So I need help here what to do.

Thanks

[safern]

The build is failing because we can't add the System.Drawing.Common dependency to System.ComponentModel.TypeConverter because System.Drawing.Common ships as an OOB package and it is not part of the inbox .NET Core Framework closure. Since System.ComponentModel.TypeConverter ships as part of the framework it can't depend on something that is not part of the framework closure.

So I think here what we would need to do is add this types into System.Drawing.Common and add the converter's to their corresponding types through a TypeConverterAttribute.

@ericstj correct me if I'm wrong.

[ericstj]

So I think here what we would need to do is add this types into System.Drawing.Common and add the converter's to their corresponding types through a TypeConverterAttribute.

Well, we specifically don't want the dependency to be from Drawing.Common > TypeConverter. At least that was @weshaggard goal here: https://github.com/dotnet/corefx/issues/21129#issuecomment-309170203
As well as here: https://github.com/dotnet/corefx/issues/29957#issuecomment-392957109

To understand the error, look at the context:

13:05:29   Verifying closure of runtime.linux-arm64.Microsoft.Private.CoreFx.NETCoreApp runtime assemblies
13:05:29 /mnt/j/workspace/dotnet_corefx/master/linux-TGroup_netcoreapp+CGroup_Release+AGroup_arm64+TestOuter_false_prtest/pkg/frameworkPackage.targets(124,5): error : Assembly 'System.ComponentModel.TypeConverter' is missing dependency 'System.Drawing.Common' 

So the problem is that TypeConverters is part of the shared framework but Drawing.Common is not, as @safern mentioned.

For now, lets add Drawing.Common to the shared framework. You can do so by setting <IsNetCoreApp>true</IsNetCoreApp> here: https://github.com/dotnet/corefx/blob/master/src/System.Drawing.Common/dir.props

I'd also recommend you open an issue and assign it to @weshaggard for reviewing this decision, since he is out at the moment. We need to decide if its worth it to preserve layering goals at the cost of shared framework growth.

[safern]

For now, lets add Drawing.Common to the shared framework. You can do so by setting true here:

Then we would need to add it to Uap as well right? <IsUAP>true</IsUAP> Since TypeConverters is also part of UAP: https://github.com/dotnet/corefx/blob/master/src/System.ComponentModel.TypeConverter/dir.props#L8

However it will be odd to have a Turd assembly being part of the UAP framework: https://github.com/dotnet/corefx/blob/master/src/System.Drawing.Common/src/System.Drawing.Common.csproj#L15

[ericstj]

Typically we don't add binaries to the CoreFx repo. Instead we add them to https://github.com/dotnet/corefx-testdata and consume the package in CoreFx.

[satano]

OK. I will move them.

[ericstj]

Then we would need to add it to Uap as well right

Not necessarily. You can ifdef these converters out of the UAP build if they aren't supported there.

[satano]

@safern So what to do next? Add <IsNetCoreApp>true</IsNetCoreApp> and ifdef converters for UAP? If yes, what is the name of constant for ifdef?

[weshaggard]

I don't have a problem with adding System.Drawing.Common to the NETCoreApp as I expect it will end up there for other reasons as well.

If we are only adding these converters so that mono can share share sources we could also just #ifdef that source code and not worry about adding support for these converters to .NET Core and UAP. So far I've not seen any requests for them beyond the source sharing aspects so I'd be included to not have them.

[ericstj]

/cc @Tanya-Solyanik
These might be needed for WinForm's binding/designer support in 3.0 (I think, not sure all the use cases for converters).

[Tanya-Solyanik]

@ericstj - yes, converters are primarily used by the design time and binding. They are also used in winforms propertygrid runtime control, I don't see Font or Image converters needed when the OOB package is not included though.

[karelz]

What is status of this PR? There hasn't been any activity for last 4 weeks?
@satano do you need some advice/guidance from us or what else is left?

[safern]

I think since this is needed for Winforms what we should do is add System.Drawing.Common to the framework (I would only add it to netcoreapp since in UAP it is a not supported assembly). And then in System.ComponentModel.TypeConverters I would only add the converters for netcoreapp -- by conditionally including the files and then if def the code piece where we add them to the table.

[safern]

@weshaggard I guess wpf/winforms now is a strong reason to include this converters to netcoreapp and not if def them just for Mono sources.

[weshaggard]

I don't have an issue with having these TypeConverters added to the TypeConverter library.

[satano]

@karelz Well. I did not have too much time to look again at this in last weeks. But i will certainly do in upcoming days. I I will need some assistance, I will write.

[satano]

@safern, @weshaggard

It seems there is another problem with this. I added <IsNetCoreApp>true</IsNetCoreApp> to the System.Drawwing.Common. Than I got error about packageIndex.json saying that I have to add InboxOn for that assembly. So I added there (I just made up the version numbers):

• "netcoreapp2.0": "4.0.0.0",
• "netcoreapp2.1": "4.0.1.0",

And now I am getting the error which is the same as before but one level deeper - now with Microsoft.Win32.SystemEvents.

error : Assembly 'System.Drawing.Common' is missing dependency 'Microsoft.Win32.SystemEvents' [...\corefx\pkg\Microsoft.Private.CoreFx.NETCoreApp\Microsoft.Private.CoreFx.NETCoreApp.pkgproj]

So what now? Probably the same steps for the Microsoft.Win32.SystemEvents will resolve this. But I do not think that this should be added to the netcoreapp. And I checked the dependencies and System.Drawing.Common has also a dependency on Microsoft.Win32.Primitives, so that will be the same case.

[satano]

Hmm. Now I found that there is a constant FEATURE_SYSTEM_EVENTS. So probably those Win32 dependencies should be conditional.

[weshaggard]

@satano that just means the SystemEvents dependency is missing from NetCoreApp and if so we either need to pull that in as well or break the dependency.

@ericstj looks like you added the dependency from System.Drawing.Common to Microsoft.Win32.SystemEvents in c56fdf1. Do you feel that dependency is important enough to warrant pulling in Microsoft.Win32.SystemEvents into the shared framework?

[ericstj]

That tracking is important so that WinForms apps can respond to theme changes IIRC (/cc @Tanya-Solyanik). I think this is still relevant in modern windows, though probably less so since Aero is the default theme.

I imagine you could make it lightup, by hooking SystemEvents via reflection. That way we'd only react to the color changes if SystemEvents was present and it would be for a WinForms app.

Alternatively we could look at lifting the dependency to the app model by exposing a new API. We could make WinForms itself responsible for hooking SystemEvents. We'd need to keep the tracker present since it registers on construction, then expose something publicly like public static SystemColorTracker.ReactToColorsChanged() and it would just go and do this. I could imagine that getting this right (and called in the right order) could be challenging.

Finally, if you just cut this entirely, I would imagine it would mean that an application would need to restart to react to theme changes.

[satano]

Well. I added <IsNetCoreApp>true</IsNetCoreApp> to the Microsoft.Win32.SystemEvents and now I got circular reference. 😟

error : Cycle detected for ...\corefx\bin\AnyOS.AnyCPU.Debug\System.ComponentModel.TypeConverter\netcoreapp\System.ComponentModel.TypeConverter.dll. [...\corefx\pkg\Microsoft.Private.CoreFx.NETCoreApp\Microsoft.Private.CoreFx.NETCoreApp.pkgproj]

System.ComponentModel.TypeConverter > System.Drawing.Common > Microsoft.Win32.SystemEvents > System.ComponentModel.TypeConverter [...\corefx\pkg\Microsoft.Private.CoreFx.NETCoreApp\Microsoft.Private.CoreFx.NETCoreApp.pkgproj]

[ericstj]

Interesting. Looks like that's only for InvalidAsynchronousStateException. We could push that exception down I suppose.

[safern]

LGTM.

[safern]

test Linux-musl x64 Debug Build please

[karelz]

@safern is anything blocking merge of this PR?

[safern]

@safern is anything blocking merge of this PR?

This will bring an "external" dependency into the shared framework for Unix (libgdiplus) since System.Drawing.Common is being moved inbox into the framework in this PR. So we're evaluating if we want to have that dependency and think how to address that if so. I will update the PR once that is decided.

We didn't think of that dependency when we decided it to move it inbox but it came up in one internal discussion.

We're still going to use this code as is, but we're still deciding where would be the right place for it to go.

[karelz]

Makes sense, marking as blocked until the decision is made. Any rough ETA?

[karelz]

@safern any rough ETA on the decision?

[karelz]

@safern ping?

[safern]

Sorry I missed this somehow.

I will prioritize figuring out this tomorrow and update the PR accordingly.

[karelz]

@safern any update?

[safern]

@safern any update?

No, we're still discussing what to do. I'm doing some investigations on how adding a dependency to TypeConverters affects the Drawing closure.

[safern]

I'm still looking for the best solution on this. We don't have very clear what we want to do YET, since we are not sure if we want System.Drawing to be inbox yet just for the justification of not depending on TypeConverters.

Since this TypeConverters are for Winforms, we're exploring other possibilities i.e add them to a separate assembly that is part of the Winforms framework instead of adding the converters directly in System.ComponentModel.TypeConverters

[karelz]

@safern still blocked?

[safern]

@safern still blocked?

No actually we came with a plan this week. I just hadn't updated the PR cause plan to work in it next week.

But basically we're not taking the PR the way it is, it needs some changes because we don't want to pull in System.Drawing inbox just yet.

@satano I'm sorry about this and that this have become more complicated that it seemed to be. I will be taking the code for the tests and typeconverters that you added and move them to another library that will be part of Winforms and then System.Drawing.Common will have a runtime dependency on it. If you feel like doing it I can definitely help you, if not, I totally understand and I'll update your PR or create a new one.

[satano]

@safern I can try to do it. But I do not understand what to do. 😃

[safern]

@safern I can try to do it. But I do not understand what to do. 😃

Thanks for your will to help and still tackle this PR 😄, it helps a lot!

Basically you would need to move the all the TypeConverters into System.Windows.Extensions project.

Then to all the System.Drawing types that you're adding type converters you need to add the TypeConverterAttribute to every type that we're adding a TypeConverter for, however this TypeConverterAttribute we need to add it with a string parameter pointing to the fully qualified name, so it will look something like:

[TypeConverter("System.Drawing.FontConverter, System.Drawing, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a")]

Does it makes sense?

[satano]

Yes it does (for now). I will look at it in couple of days (during the weekend at the earliest).

There is a notice here that there are some conflicts that needs to be resolved from master branch. And probably master should be merged here because this branch is too old - what do you think?

[safern]

Since we're completely changing the plan here I don't think it makes sense of going through the pain of solving merge conflicts. I would probably just open up a new PR in a clean branch since it will have a big change of where the types/tests are going to be and we don't need to to the IsNetCoreApp/UAP stuff, nor moving types down to other assemblies.

So I think it will be less painfull for you to do it in a separate branch created from latest master.

[satano]

OK 👍

[satano]

@safern I have problems with building corefx. I did not do it for a couple of weeks. I have pulled last version (even tried to clone the repo again) and the build is still failing. The last time the build worked I had a bit older Visual Studio. I do not know the exact version, but probably it was 15.7.x. The end of the output from build.cmd is:

C:\projekty\corefx\src\Native\build-native.cmd x64 Debug Windows_NT

---

** Visual Studio 2017 Developer Command Prompt v15.8.7
** Copyright (c) 2017 Microsoft Corporation

---

---

** Visual Studio 2017 Developer Command Prompt v15.8.7
** Copyright (c) 2017 Microsoft Corporation

---

[vcvarsall.bat] Environment initialized for: 'x86_x64'
Commencing build of native components

CMake Error at CMakeLists.txt:21 (project):
-- Configuring incomplete, errors occurred!
See also "C:/projekty/corefx/bin/obj/Windows_NT.x64.Debug/native/CMakeFiles/CMakeOutput.log".
Failed to generate native component build project!
Failed to run MSBuild command:

C:/Program Files (x86)/Microsoft Visual Studio/2017/Community/MSBuild/15.0/Bin/MSBuild.exe

to get the value of VCTargetsPath:

Unhandled Exception: System.IO.FileNotFoundException: Could not load file or assembly 'Microsoft.Build.Framework, Version=15.1.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' or one of its dependencies. The system cannot find the file specified.
   at Microsoft.Build.CommandLine.MSBuildApp.Execute(String commandLine)
   at Microsoft.Build.CommandLine.MSBuildApp.Main()

Exit code: -532462766

In the mentioned CMakeOutput.log, there is just one line: The system is: Windows - 10.0.17134 - AMD64.

[safern]

Hey @satano could you please validate that you have all the requirements to build correctly and that you're on the latest cmake machine? I've also hit issues like that when updating VS because I didn't restart my computer after the update, so after restarting my computer it worked fine.

https://github.com/dotnet/corefx/blob/master/Documentation/building/windows-instructions.md#visual-studio-2017---workloads-based-install

[satano]

Hm. I was missing something, but just installing it did not help. I had to repair Visual Studio Installation. Now everything works. Thanks a lot.

[safern]

@satano any update on this? I guess you will be opening a new PR and we can close this one right?

[satano]

Yes. I have it halfway through on my computer. I will open a new PR in couple of days.

[safern]

Yes. I have it halfway through on my computer. I will open a new PR in couple of days.

Thanks you very much!