Remove SystemColors from .NET Core #17669
Conversation
|
I guess from CompnentModel we should just remove this line |
|
System.Drawing.ColorTable will also need some neutering. |
|
Plenty of things are in .NET Core but not in .NET Standard. Is that the primary motivation here? |
|
@mellinoe see discussion in #17024 |
| dict.Add("ScrollBar", Color.FromArgb(unchecked((int)0xffd4d0c8))); | ||
| dict.Add("Window", Color.FromArgb(unchecked((int)0xffffffff))); | ||
| dict.Add("WindowFrame", Color.FromArgb(unchecked((int)0xff000000))); | ||
| dict.Add("WindowText", Color.FromArgb(unchecked((int)0xff000000))); |
There was a problem hiding this comment.
Since we need this list anyway in order to populate the dictionary, did you consider just making SystemColors internal instead of public?
There was a problem hiding this comment.
@stephentoub Yeah, I was just thinking about that. I've updated the PR, this will probably be a more elegant fix. Thanks!
| else if (c.IsSystemColor) | ||
| { | ||
| member = typeof(SystemColors).GetProperty(c.Name); | ||
| } |
There was a problem hiding this comment.
That we're able to delete this without changing any tests suggests we don't have any tests covering this functionality?
There was a problem hiding this comment.
It seems so, indeed. I'm also guessing there are no unit tests for the IsSystemColor property, either.
|
@stephentoub I updated the PR after you approved it, can you have another look just to be sure? I had to move SystemColors.cs to the Common folder (not sure why Git didn't see it as a move) as it ends up being used by both System.ComponentModel and System.Drawing.Primimitives. Other than that, I think this PR is pretty much ready. |
|
Why does System.ComponentModel need it? Is that dead code now that could be removed, or is it functionality that would highlight a need for keeping SystemColors, or something else? I also think it's strange that we're getting rid of SystemColors when we have the public Color.IsSystemColor property. |
|
The reason ComponentModel needs it is because it takes a dependency on KnownColors and the "system colors" are still available on KnownColors. Net:
which is a bit strange indeed. Let's discuss that on dotnet/standard#240 |
|
I don't like moving the SystemColors enum into Common. We should completely remove the SystemColors enum, the members in KnownColor that are SystemColors, the IsSystemColor property on Color, and any usages of SystemColors in TypeConverter. |
|
Hold on this until questions in dotnet/standard#240 are resolved. I'm pretty confused at this point. @weshaggard |
|
I think we should remove the SystemColors type and its usages in the ColorTable from the implementation completely. |
|
@qmfrederik could you please update this PR to match the new surface area for .NET Standard in PR dotnet/standard#290. Before I finalize that surface area into the standard I want to make sure the implementation still make sense or if there are any other issues that arise in the implementation. Things like does the color converter still work or Color.FromName is still useful etc. |
|
@weshaggard I'll do the update but it may take a couple of days before I can get to it. Apologies for the delay. |
|
@qmfrederik OK I might go ahead and submit a PR to update corefx tomorrow as I would like to get the standard edits finalized this week. |
|
Thanks for the contribution @qmfrederik but I'm closing this PR in favor of #18010. |
As
SystemColorsis not in .NET Standard, it doesn't make much sense to have it in .NET Core, either.Fixes #17024