Standard: System.Drawing.SystemColors available in netcoreapp2.0 but not in netstandard2.0

[danmoseley]

@qmfrederik commented on Sun Mar 12 2017

System.Drawing.SystemColors exists in System.Drawing.Primitives 4.1.0.0 for netcoreapp2.0 (as part of Microsoft.Netcore.App 2.0.0-beta-001737-00), but does not exist in System.Drawing or System.Drawing.Primitives for netstandard2.0

Although not a bug per se, since netcoreapp2.0 implements SystemColors, and NetFX and Mono also do, all .NET platforms implement SystemColors so it might be a candidate to end up in the netstandard2.0 specification.

Alternatively, if it is deemed it SystemColors should not be part of netstandard2.0, I'm not sure it makes much sense to have it in netcoreapp2.0, either.

Background: I have ported Mono's System.Drawing to .NET Core and repeating the excercise for netstandard2.0.

When compiling Mono's System.Drawing for netstandard2.0, I have to include SystemColors as it is not part of the netstandard2.0 specification. This version can then be consumed on NetFX (given System.Drawing is not referenced) but not on netcoreapp2.0 because you'll get compiler errors trying to distinguish between System.Drawing.SystemColors in netcoreapp2.0 and Mono's System.Drawing.

Net, the gist of this issues is to request to have the same API surface for System.Drawing on netcoreapp2.0 and netstandard2.0.

Makes sense?

---

@danmosemsft commented on Mon Mar 13 2017

We already have some small pieces of System.Drawing in standard including System.Drawing.Color. Looks like bringing SystemColors would not pull with it much more. Moving there for consideration.

[weshaggard]

The reason this was left out is because they aren't in Xamarin. If we can get them included there then we could consider putting them in the standard. cc @marek-safar @akoeplinger

[qmfrederik]

@weshaggard If they don't make it into Xamarin (they are quite Windows-centric), would it be an option to remove them from netcoreapp2.0? I'm fine either way, just wondering if netstandard2.0 and netcoreapp2.0 could be consistent here.

[akoeplinger]

SystemColors is already in Xamarin.Android and Xamarin.Mac, but not yet in Xamarin.iOS/WatchOS/TVOS. It shouldn't be hard to add it if that's what we want.

[weshaggard]

I'm fine either way, just wondering if netstandard2.0 and netcoreapp2.0 could be consistent here.

netcoreapp2.0 is not supposed to be exactly equal to netstandard2.0 it will always be a superset by design as the standard will evolve slower then .NET Core. So I would still say we should keep SystemColors in .NET Core even if we don't yet add it to the standard.

SystemColors is already in Xamarin.Android and Xamarin.Mac, but not yet in Xamarin.iOS/WatchOS/TVOS. It shouldn't be hard to add it if that's what we want.

I don't know the current schedule you guys are on but if we were to add these would we have time to get them included in the others?

[akoeplinger]

I don't know the current schedule you guys are on but if we were to add these would we have time to get them included in the others?

Yes, that should work.

[weshaggard]

@terrajobst any issues with me including SystemColors as part of the netstandard 2.0?

[terrajobst]

I'd say SystemColor should be in if Color is in. Do we really need Color though? From a scenario standpoint I don't think Color makes a lot of sense for .NET Standard. Should/can it be avaialable on top of .NET Standard? Sure. I'd say the API is in the same bucket as Registry or SqlClient, except the implementation happens to be more lightweight. But from a "does this API make sense everywhere in this shape", I'd say the answer is no.

Others?

[akoeplinger]

I'd say SystemColor should be in if Color is in

👍

Do we really need Color though?

IIRC it was added primarily because it is used in a lot of cases and prevented porting libraries from desktop.

But from a "does this API make sense" everywhere in this shape, I'd say the answer is no.

Maybe for SystemColors since that one is about GUI menu colors etc, but Color itself does make sense everywhere IMHO. It is just a container for (Alpha,Red,Green,Blue) after all.

I don't think it's worth the complexity putting just SystemColors in a separate package.

[weshaggard]

I think we should keep Color in netstandard as I feel it is general enough and it is a primitive drawing type. Along with it SystemColor doesn't add any additional complexity and is just a wrapper around KnownColors so I would also include it.

[jarenduan]

Hi, guys, I don't know if I have the right to vote, but I support including Color, and excluding SystemColor.

Just like @weshaggard mentioned, the Color is general enough. But for SystemColor, it kind of use a series of traditional windows desktop GUI primitive conceptions: menu, control, hottrack, inactiveBorder, and a Desktop member even. If we include SystemColor in the standard, it means we are asking every platform to use these GUI primitives, and that seems excessive. I mean it is weird that a mobile phone system has something called InactiveBorder, right?

[terrajobst]

So after talking to a few folks here it seems my approach is coming too much from a purity angle. Taking my own advice that .NET is about being approachable and pragmatic, I propose the following:

• We keep Color
• We don't expose SystemColors. Yes, Color exposes similar concepts via KnownColor, but SystemColors type is clearly very UI specific while Color generally isn't.

Any concerns?

[qmfrederik]

That makes sense to me, but following this logic, wouldn't it be better to remove SystemColors from netcoreapp2.0 as well? It is very UI specific and I don't think it makes much sense having it in netcoreapp2.0 if it is not in netstandard2.0

[terrajobst]

Good point, if we don't add SystemColors to .NET Standard 2.0, we shouldn't add it to .NET Core 2.0 either.

[weshaggard]

I'm OK with this approach. @danmosemsft should should we file an issue to remove from in corefx or use this issue for that?

[qmfrederik]

I guess dotnet/corefx#17024 could be re-opened to track removing SystemColors from .NET Core 2.0?

[weshaggard]

Thanks for the pointer. I reopened that issue and I'm closing this one.

[stephentoub]

Is it strange that we're removing SystemColor, but still have Color.IsSystemColor? Seems like given we have the latter, we'd want to keep the former.

[qmfrederik]

When trying to actually implement this in dotnet/corefx#17669, we found out that:

1. The system colors persist in the KnownColor enumeration, such as KnownColor.ActiveBorder
2. The notion of system colors persists in Color.IsSystemColor

so I guess we're back to this issue to double-check that removing SystemColor is really the way to go. By the way, what is IsSystemColor supposed to return on a platform that implements Color.IsSystemColor but not SystemColor?

My main concern is to have the System.Drawing API surface consistent between .NET Core and .NET Standard. It will make porting the remainder of System.Drawing (from Mono or NetFX) to .NET Core easier.

I cannot reopen this issue, perhaps someone else can?

[terrajobst]

Is it strange that we're removing SystemColor, but still have Color.IsSystemColor? Seems like given we have the latter, we'd want to keep the former.

Agreed. I'd say we should remove this member too. It's not very useful in .NET Standard and I'd argue that this is never going to be a scenario for it either.

[weshaggard]

@terrajobst what about the KnownColor enum? All the system colors have an entry in that enum and we have to provide a value for them in .NET Core.

[terrajobst]

Ah, good point. I'd say same thing applies there: should be removed.

[weshaggard]

Do we really want to subset the enum?

See https://github.com/dotnet/corefx/blob/master/src/System.Drawing.Primitives/src/System/Drawing/KnownColor.cs#L19, there are various assumptions about the values so at a minimum we will need to ensure the values remain the same or we will likely break some folks.

[weshaggard]

The more I think about it the less I like removing these enum values as we know removing values makes things difficult for platforms that do support those values. So we should treat them like any other enums where not all platforms actually need to support all enums but they should at least have all of them so we don't end up with duplicated values across platforms if folks mutate them.

[weshaggard]

Also if we do end up keeping the KnownColor's enum then I think the implementation we currently have https://github.com/dotnet/corefx/blob/master/src/System.Drawing.Primitives/src/System/Drawing/Color.cs#L372 is reasonable and doesn't really require any OS knowledge just the enum ranges.

[terrajobst]

I don't think removing the enum is a high priority issue. These are essentially consts, so no bad dependencies. Given usage of FromColor is low, I think we can leave them in. We could for cleanliness reasons hide them but I don't see a strong enough reason to do so.

[weshaggard]

OK I'm going to keep things as is in .NET Standard then.

[stephentoub]

I don't understand where things have landed then. We're going to keep in core Color.IsSystemColor but get rid of SystemColor? Why not keep SystemColor then as well?

[jarenduan]

Should remove SystemColor and Color.IsSystemColor, should also remove KnownColor and Color.FromKnownColor, I mean from the standard.

Whether keeping them or not in the .Net Core is another question. If I understand it correctly, .Net Core 2.0's APIs doesn't have to exactly equal .Net Standard 2.0's.

If a developer really needs these APIs, he/she'd best to multi-target. And even if these APIs are proven as platform-independent in the future, then we could include them back in the new version of standard.

[weshaggard]

The SystemColor type itself is merely a wrapper around the enum so from that prospective I don't care if it is included in the standard or not. What I care about is that Color is in the standard and that the KnownColor enum is not filtered as that will just cause many folks unnecessary grief later. We could consider removing the KnownColor enum completely but that would cause Color to remove APIs which we really are trying to avoid in standard.

Color.IsSystemColor is purely a function of the enum values so it also doesn't need to be removed as long as the enum value remain. The only question is whether or not we want the SystemColor type itself in the standard and in core and I'd say it doesn't really matter. The usage data is relatively small for everything but our compat lab which is where WinForms applications end up using it.

For that reason I would just keep SystemColors out of the standard.

[ViktorHofer]

What I care about is that Color is in the standard

YES, absolutely!!

and that the KnownColor enum is not filtered as that will just cause many folks unnecessary grief later

I don't like the idea of keeping the original KnownColor enum in standard without filtering it first. IMO the web color items make sense as they are platform agnostic, but that's not the case for the System and Whibdey colors:

// "System" colors
        ActiveBorder = 1,
        ActiveCaption,
        ActiveCaptionText,
        AppWorkspace,
        Control,
        ControlDark,
        ControlDarkDark,
        ControlLight,
        ControlLightLight,
        ControlText,
        Desktop,
        GrayText,
        Highlight,
        HighlightText,
        HotTrack,
        InactiveBorder,
        InactiveCaption,
        InactiveCaptionText,
        Info,
        InfoText,
        Menu,
        MenuText,
        ScrollBar,
        Window,
        WindowFrame,
        WindowText,

        // NEW ADDITIONS IN WHIDBEY - DO NOT MOVE THESE UP OR IT WILL BE A BREAKING CHANGE

        ButtonFace,
        ButtonHighlight,
        ButtonShadow,
        GradientActiveCaption,
        GradientInactiveCaption,
        MenuBar,
        MenuHighlight,

[weshaggard]

We should not exclude only part of the enum members because that can break folks. Sure we can forcibly set the remaining enum values so that they remain the exact underlying value which would help with some of these breaks, but there are still potential inconsistencies and binary breaking changes that we cannot fix.

Another issue that often occurs is that when one platform adds values to this enum we need to be certain that they don't stomp over or reuse existing values and the best way to do that is to keep all the values that are in that enum already on any platform. Our general API stance on enums is to not sub-set them.

I'd be open to potentially excluding the entire enum but then we would also have to subset the APIs on the Color type which is also against our principles for standard, which is why I still believe we should keep all the enums in the standard.

[terrajobst]

If the primary concern is the inconsistency in the enum, I propose the following:

 namespace System.Drawing {
  public struct Color {
         public static readonly Color Empty;
         public byte A { get; }
         public static Color AliceBlue { get; }
         public static Color AntiqueWhite { get; }
         public static Color Aqua { get; }
         public static Color Aquamarine { get; }
         public static Color Azure { get; }
         public byte B { get; }
         public static Color Beige { get; }
         public static Color Bisque { get; }
         public static Color Black { get; }
         public static Color BlanchedAlmond { get; }
         public static Color Blue { get; }
         public static Color BlueViolet { get; }
         public static Color Brown { get; }
         public static Color BurlyWood { get; }
         public static Color CadetBlue { get; }
         public static Color Chartreuse { get; }
         public static Color Chocolate { get; }
         public static Color Coral { get; }
         public static Color CornflowerBlue { get; }
         public static Color Cornsilk { get; }
         public static Color Crimson { get; }
         public static Color Cyan { get; }
         public static Color DarkBlue { get; }
         public static Color DarkCyan { get; }
         public static Color DarkGoldenrod { get; }
         public static Color DarkGray { get; }
         public static Color DarkGreen { get; }
         public static Color DarkKhaki { get; }
         public static Color DarkMagenta { get; }
         public static Color DarkOliveGreen { get; }
         public static Color DarkOrange { get; }
         public static Color DarkOrchid { get; }
         public static Color DarkRed { get; }
         public static Color DarkSalmon { get; }
         public static Color DarkSeaGreen { get; }
         public static Color DarkSlateBlue { get; }
         public static Color DarkSlateGray { get; }
         public static Color DarkTurquoise { get; }
         public static Color DarkViolet { get; }
         public static Color DeepPink { get; }
         public static Color DeepSkyBlue { get; }
         public static Color DimGray { get; }
         public static Color DodgerBlue { get; }
         public static Color Firebrick { get; }
         public static Color FloralWhite { get; }
         public static Color ForestGreen { get; }
         public static Color Fuchsia { get; }
         public byte G { get; }
         public static Color Gainsboro { get; }
         public static Color GhostWhite { get; }
         public static Color Gold { get; }
         public static Color Goldenrod { get; }
         public static Color Gray { get; }
         public static Color Green { get; }
         public static Color GreenYellow { get; }
         public static Color Honeydew { get; }
         public static Color HotPink { get; }
         public static Color IndianRed { get; }
         public static Color Indigo { get; }
         public bool IsEmpty { get; }
-        public bool IsKnownColor { get; }
         public bool IsNamedColor { get; }
-        public bool IsSystemColor { get; }
         public static Color Ivory { get; }
         public static Color Khaki { get; }
         public static Color Lavender { get; }
         public static Color LavenderBlush { get; }
         public static Color LawnGreen { get; }
         public static Color LemonChiffon { get; }
         public static Color LightBlue { get; }
         public static Color LightCoral { get; }
         public static Color LightCyan { get; }
         public static Color LightGoldenrodYellow { get; }
         public static Color LightGray { get; }
         public static Color LightGreen { get; }
         public static Color LightPink { get; }
         public static Color LightSalmon { get; }
         public static Color LightSeaGreen { get; }
         public static Color LightSkyBlue { get; }
         public static Color LightSlateGray { get; }
         public static Color LightSteelBlue { get; }
         public static Color LightYellow { get; }
         public static Color Lime { get; }
         public static Color LimeGreen { get; }
         public static Color Linen { get; }
         public static Color Magenta { get; }
         public static Color Maroon { get; }
         public static Color MediumAquamarine { get; }
         public static Color MediumBlue { get; }
         public static Color MediumOrchid { get; }
         public static Color MediumPurple { get; }
         public static Color MediumSeaGreen { get; }
         public static Color MediumSlateBlue { get; }
         public static Color MediumSpringGreen { get; }
         public static Color MediumTurquoise { get; }
         public static Color MediumVioletRed { get; }
         public static Color MidnightBlue { get; }
         public static Color MintCream { get; }
         public static Color MistyRose { get; }
         public static Color Moccasin { get; }
         public string Name { get; }
         public static Color NavajoWhite { get; }
         public static Color Navy { get; }
         public static Color OldLace { get; }
         public static Color Olive { get; }
         public static Color OliveDrab { get; }
         public static Color Orange { get; }
         public static Color OrangeRed { get; }
         public static Color Orchid { get; }
         public static Color PaleGoldenrod { get; }
         public static Color PaleGreen { get; }
         public static Color PaleTurquoise { get; }
         public static Color PaleVioletRed { get; }
         public static Color PapayaWhip { get; }
         public static Color PeachPuff { get; }
         public static Color Peru { get; }
         public static Color Pink { get; }
         public static Color Plum { get; }
         public static Color PowderBlue { get; }
         public static Color Purple { get; }
         public byte R { get; }
         public static Color Red { get; }
         public static Color RosyBrown { get; }
         public static Color RoyalBlue { get; }
         public static Color SaddleBrown { get; }
         public static Color Salmon { get; }
         public static Color SandyBrown { get; }
         public static Color SeaGreen { get; }
         public static Color SeaShell { get; }
         public static Color Sienna { get; }
         public static Color Silver { get; }
         public static Color SkyBlue { get; }
         public static Color SlateBlue { get; }
         public static Color SlateGray { get; }
         public static Color Snow { get; }
         public static Color SpringGreen { get; }
         public static Color SteelBlue { get; }
         public static Color Tan { get; }
         public static Color Teal { get; }
         public static Color Thistle { get; }
         public static Color Tomato { get; }
         public static Color Transparent { get; }
         public static Color Turquoise { get; }
         public static Color Violet { get; }
         public static Color Wheat { get; }
         public static Color White { get; }
         public static Color WhiteSmoke { get; }
         public static Color Yellow { get; }
         public static Color YellowGreen { get; }
         public override bool Equals(object obj);
         public static Color FromArgb(int argb);
         public static Color FromArgb(int alpha, Color baseColor);
         public static Color FromArgb(int red, int green, int blue);
         public static Color FromArgb(int alpha, int red, int green, int blue);
-        public static Color FromKnownColor(KnownColor color);
         public static Color FromName(string name);
         public float GetBrightness();
         public override int GetHashCode();
         public float GetHue();
         public float GetSaturation();
         public static bool operator ==(Color left, Color right);
         public static bool operator !=(Color left, Color right);
         public int ToArgb();
-        public KnownColor ToKnownColor();
         public override string ToString();
     }
 }

Any concerns with that approach?

[weshaggard]

That is sub-setting Color which is generally against our principles for standard which is why I don't like it (especially just for an enum) but I wouldn't fight it if folks felt that was reasonable.

[terrajobst]

I think subsetting enums is quite problematic because it makes it virtually impossible to cleanly evolve it without accidentally adding conflicting values eventually.

We have removed members from types if they were causing issues (like CAS APIs on AppDomains). I think the members I listed above fall into the same category. I've talked to @kieranmo and he proposed removing SystemColors but keeping Color. SystemColors is inherently a problematic type to implement and quite ill-defined outside of Win32.

What do others think? Reasonable?

[ViktorHofer]

I guess this issue isn't resolved yet.

[ViktorHofer]

@weshaggard Am I missing something? I don't think that @terrajobst proposal was subsetting Color

[terrajobst]

It was. I propose to remove the following members from Color:

 namespace System.Drawing {
  public struct Color {
-        public bool IsKnownColor { get; }
-        public bool IsSystemColor { get; }
-        public static Color FromKnownColor(KnownColor color);
-        public KnownColor ToKnownColor();
     }
 }

[akoeplinger]

I agree, let's get rid of SystemColors and those members 👍

[ViktorHofer]

@terrajobst oh I see, thanks! +1 for getting rid of SystemColors and the corresponding members in Color.