Implement CGColor independent of UIColor #2538

msft-Jeyaram · 2017-04-17T18:37:09Z

Implement CGColor independent of UIColor
Implement UIColor as a class cluster
Removal of BrushTypes
Overall code improvements
Support for grayscale
Remove UIColor usage from CALayer
Update UIKit to properly confirm to UIColor
Implement newly added CGColor Apis #2614,
The current changes, which touch CoreGraphics, help remove the dependencies CoreGraphics.UnitTests & CoreGraphics.DrawingTests had on UIKit. CoreGraphics.UnitTests & CoreGraphics.DrawingTests pass.

DHowett-MSFT · 2017-04-17T18:59:25Z

Frameworks/CoreGraphics/CGColor.mm

+        return _id;
+    }
+
+    __CGColor(CGColorSpaceRef colorspace, CGFloat r, CGFloat g, CGFloat b, CGFloat alpha) : _colorSpace(colorspace), _pattern(nullptr) {


you can struct-initialize components to remove the constructor body here #Resolved

DHowett-MSFT · 2017-04-17T19:00:24Z

Frameworks/CoreGraphics/CGColor.mm

+    }
+
+    // CreateCopyWithAlpha
+    __CGColor(CGColorRef color, float alpha) : _colorSpace(color->_colorSpace), _pattern(color->_pattern) {


You might like to make this more like a copy constructor and take const __CGColor& #Resolved

DHowett-MSFT · 2017-04-17T19:01:05Z

Frameworks/CoreGraphics/CGColor.mm

+        _components.a = alpha;
+    }
+
+    __CGColor(CGColorSpaceRef colorSpace, CGPatternRef pattern) : _colorSpace(colorSpace), _pattern(pattern) {


whack some TODOs in here about support components #Resolved

DHowett-MSFT · 2017-04-17T19:01:20Z

Frameworks/CoreGraphics/CGColor.mm

+    }
+
+    bool operator==(const __CGColor& other) const {
+        RETURN_FALSE_IF(isPatternColor() != other.isPatternColor());


my word, these macros generate an explosion of branches #Resolved

DHowett-MSFT · 2017-04-17T19:01:44Z

Frameworks/CoreGraphics/CGColor.mm

+        return true;
+    }
+
+    inline bool isPatternColor() const {


nit: this is the only one that's lowercase? #Resolved

is it private? #Resolved

DHowett-MSFT · 2017-04-17T19:03:34Z

Frameworks/CoreGraphics/CGColor.mm

-    [static_cast<UIColor*>(ret) setColorSpace:colorSpace];
-    return ret;
-}
+    RETURN_NULL_IF(CGColorSpaceGetNumberOfComponents(colorSpace) == 0);


will a colorspace with an RGB or Monochrome model ever have 0? #ByDesign

if colorspacemodel is unimplemented.
#ByDesign

This is extraneous check. The switch statement below will suffice. #Resolved

kCGColorSpaceModelUnknown would do.

In reply to: 113351799 [](ancestors = 113351799)

DHowett-MSFT · 2017-04-17T19:04:43Z

Frameworks/CoreGraphics/CGColor.mm

-    return ((components1->r == components2->r) && (components1->g == components2->g) && (components1->b == components2->b) &&
-            (components1->a == components2->a)) &&
-           (CGColorSpaceGetModel(CGColorGetColorSpace(color1)) == CGColorSpaceGetModel(CGColorGetColorSpace(color2)));
+    RETURN_RESULT_IF(color1 == color2, true);


CF takes care of ==, but not NULL #ByDesign

early return would void the call into CFEqual

In reply to: 111798718 [](ancestors = 111798718)

DHowett-MSFT · 2017-04-17T19:05:00Z

Frameworks/CoreGraphics/CGColor.mm

 }

 /**
 @Status Caveat
 @Notes Limited support for colorspace
 */
 size_t CGColorGetNumberOfComponents(CGColorRef color) {
-    return CGColorSpaceGetNumberOfComponents(CGColorGetColorSpace(color)) + 1;
+    return CGColorSpaceGetNumberOfComponents(color->ColorSpace()) + 1;


nit: seems like not the most necessary change #ByDesign

hmm +2 calls vs +1.
actually -1 since it's inlined #ByDesign

This doesn't test for a null color #Resolved

DHowett-MSFT · 2017-04-17T19:05:26Z

Frameworks/CoreGraphics/CGColor.mm

+    return __CGColor::GetTypeID();
+}
+
+#pragma region InternalHelpers


nit: regions can be named in normal english #Resolved

DHowett-MSFT · 2017-04-17T19:07:04Z

Frameworks/CoreGraphics/CGColor.mm

+
+#pragma region InternalHelpers
+
+CGColorRef _CGColorCreateWithComponents(CGColorSpaceRef colorspace, CGFloat r, CGFloat g, CGFloat b, CGFloat alpha) {


this is an incompatible concept. you can't provide a generic colorspace and then require r/g/b/a.

Please reimplement callers in terms of CGColorCreate or CGColorCreateGenericRGB #Resolved

CGColorCreateGenericRGB now how did I forget about that :(
this will allow us to add the missing apis #Resolved

aballway

for this half of the change 😸

aballway · 2017-04-17T19:01:43Z

Frameworks/CoreGraphics/CGColor.mm

+    }
+
+    bool operator==(const __CGColor& other) const {
+        RETURN_FALSE_IF(isPatternColor() != other.isPatternColor());


nit: this should be unneeded since you already compare _pattern, #Resolved

aballway · 2017-04-17T19:02:10Z

Frameworks/CoreGraphics/CGColor.mm

+
+    bool operator==(const __CGColor& other) const {
+        RETURN_FALSE_IF(isPatternColor() != other.isPatternColor());
+        RETURN_FALSE_IF(CGColorSpaceGetModel(_colorSpace) != CGColorSpaceGetModel(other._colorSpace));


nit: would it be worth adding a CFEqual for CGColorSpace? #WontFix

aballway · 2017-04-17T19:02:39Z

Frameworks/CoreGraphics/CGColor.mm

+        RETURN_FALSE_IF(isPatternColor() != other.isPatternColor());
+        RETURN_FALSE_IF(CGColorSpaceGetModel(_colorSpace) != CGColorSpaceGetModel(other._colorSpace));
+        RETURN_FALSE_IF(_pattern != other._pattern);
+        RETURN_FALSE_IF(_components.r != other._components.r);


nit: add an operator== for __CGColorQuad? #Resolved

aballway · 2017-04-17T19:06:24Z

Frameworks/CoreGraphics/CGColor.mm

+    return __CGColor::CreateInstance(kCFAllocatorDefault, colorSpace, pattern);
+}
+
+CGColorRef __CreateBlackColor() {


nit: could just inline these in CGColorGetConstantColor and share a static colorspace #Resolved

aballway · 2017-04-17T19:07:40Z

Frameworks/CoreGraphics/CGColor.mm

+            return __CGColor::CreateInstance(kCFAllocatorDefault, colorSpace, components[0], components[0], components[0], components[1]);
+        default:
+            UNIMPLEMENTED_WITH_MSG("Colorspace Unsupported. Model: %d", model);
+            return nullptr;


nit: extraneous return #Resolved

aballway · 2017-04-17T19:08:39Z

Frameworks/CoreGraphics/CGColor.mm

-    return ((components1->r == components2->r) && (components1->g == components2->g) && (components1->b == components2->b) &&
-            (components1->a == components2->a)) &&
-           (CGColorSpaceGetModel(CGColorGetColorSpace(color1)) == CGColorSpaceGetModel(CGColorGetColorSpace(color2)));
+    RETURN_RESULT_IF(color1 == color2, true);


nit: this case is handled by CFEqual #ByDesign

aballway · 2017-04-17T19:10:08Z

Frameworks/include/CGColorInternal.h

+    CGFloat g;
+    CGFloat b;
+    CGFloat a;
+} __CGColorQuad;


where is this being used outside of CGColor? #ByDesign

in UIKit #ByDesign

aballway · 2017-04-20T15:54:17Z

Frameworks/CoreGraphics/CGColor.mm

+    }
+
+    bool operator==(const __CGColor& other) const {
+        if ((CGColorSpaceGetModel(_colorSpace) != CGColorSpaceGetModel(other._colorSpace)) || (_pattern != other._pattern) ||


nit: return _ == _ && #Resolved

this is still unresolved #Resolved

aballway · 2017-04-20T15:58:50Z

Frameworks/include/CGColorInternal.h

+    CGFloat a;
+
+    bool operator==(const __CGColorQuad& other) const {
+        if ((r != other.r) || (g != other.g) || (b != other.b) || (a != other.a)) {


nit: return _ == other._ && ... #Resolved

aballway · 2017-04-20T15:59:05Z

build/CoreGraphics/dll/CoreGraphics.def

@@ -74,6 +75,8 @@ LIBRARY CoreGraphics
        CGColorGetPattern
        CGColorGetTypeID
        CGColorGetConstantColor
+        CGColorCreateGenericRGB


👍 #ByDesign

aballway · 2017-04-20T16:00:50Z

include/CoreGraphics/CGColor.h

@@ -40,3 +40,5 @@ COREGRAPHICS_EXPORT size_t CGColorGetNumberOfComponents(CGColorRef color);
 COREGRAPHICS_EXPORT const CGFloat* CGColorGetComponents(CGColorRef color);
 COREGRAPHICS_EXPORT CGPatternRef CGColorGetPattern(CGColorRef color);
 COREGRAPHICS_EXPORT CFTypeID CGColorGetTypeID();
+COREGRAPHICS_EXPORT CGColorRef CGColorCreateGenericRGB(CGFloat red, CGFloat green, CGFloat blue, CGFloat alpha);


It looks like there are a couple other CGColor functions which aren't in here like CGColorCreateCopyByMatchingToColorSpace #Closed

That would be done by an extra issue.

In reply to: 112492607 [](ancestors = 112492607)

Should add the issue number here then, or at least add declaration so it gets tracked as STUB api #Resolved

aballway · 2017-04-20T16:02:25Z

Frameworks/CoreGraphics/CGColor.mm

+    return __CGColor::CreateInstance(kCFAllocatorDefault, colorSpace, pattern);
+}
+
+static inline CGColorRef __CreateBlackColor() {


nit: could just inline these in CGColorGetConstantColor and share a static colorspace #Resolved

msft-Jeyaram · 2017-04-21T22:12:00Z

@DHowett-MSFT @aballway ping #ByDesign

msft-Jeyaram · 2017-04-25T18:56:54Z

@DHowett-MSFT @aballway @MSFTFox Ping #ByDesign

msft-Jeyaram · 2017-04-25T18:58:11Z

Frameworks/UIKit/UICGColor.mm

+#endif
+#endif
+
+hsv rgb2hsv(rgb in) {


the below two are just moved. These are from the earlier implementation.

aballway · 2017-04-25T19:15:02Z

Frameworks/CoreGraphics/CGColor.mm

+    }
+
+    bool operator==(const __CGColor& other) const {
+        if ((CGColorSpaceGetModel(_colorSpace) != CGColorSpaceGetModel(other._colorSpace)) || (_pattern != other._pattern) ||


this is still unresolved #Resolved

aballway · 2017-04-25T19:16:24Z

Frameworks/CoreGraphics/CGColor.mm

 }

 /**
 @Status Caveat
 @Notes Limited support for colorspace
 */
 size_t CGColorGetNumberOfComponents(CGColorRef color) {
-    return CGColorSpaceGetNumberOfComponents(CGColorGetColorSpace(color)) + 1;
+    return CGColorSpaceGetNumberOfComponents(color->ColorSpace()) + 1;


This doesn't test for a null color #Resolved

aballway · 2017-04-25T19:16:55Z

Frameworks/QuartzCore/CALayer.mm

-#include "UIKit/UIColor.h"
-#include "UIColorInternal.h"
-#include "UIKit/NSValue+UIKitAdditions.h"
+#include <UIKit/UIApplication.h>


nit: consistent on import/include #Resolved

aballway · 2017-04-25T19:18:59Z

Frameworks/QuartzCore/CALayer.mm

@@ -1570,9 +1568,9 @@ - (CATransform3D)sublayerTransform {
 */
 - (void)setBackgroundColor:(CGColorRef)color {
    if (color != nil) {
-        priv->backgroundColor = *[static_cast<UIColor*>(color) _getColors];
+        priv->backgroundColor = *(reinterpret_cast<const __CGColorQuad*>(CGColorGetComponents(color)));


! this feels super unsafe! just construct a __CGColorQuad if you need this #ByDesign

This is with the knowledge of transparent information.
the backing of CGColorGetComponents are a quad struct. Another way is to have a private getter or just take it out of the Quad struct and put it into another Quad struct.

In reply to: 113286126 [](ancestors = 113286126)

This is already assigning into a new struct, so it wouldn't be costly to take the extra safety of constructing a __CGColorQuad with the pointer and moving it #Resolved

also _backgroundColor is a CGColorRef, so I'm not sure what this is doing. We should probably just purge QuartzCore of UIColor and __CGColorQuad since they're not the same #ByDesign

that is not the scope of this PR.The main purpose to detangle CGColor<->UIColor dependencies
We should do that at some point, Raj could weight in on the priority of this.

In reply to: 113504346 [](ancestors = 113504346)

why not eh

In reply to: 113503612 [](ancestors = 113503612)

aballway · 2017-04-25T19:19:19Z

Frameworks/QuartzCore/CALayer.mm

@@ -1583,7 +1581,7 @@ - (void)setBackgroundColor:(CGColorRef)color {
    // is updated almost immediately.By setting the priv->background first,
    // we can ensure both states are kept in sync which is useful when performing
    // automated testing.
-    [CATransaction _setPropertyForLayer:self name:@"backgroundColor" value:(NSObject*)color];
+    [CATransaction _setPropertyForLayer:self name:@"backgroundColor" value:(NSObject*)[UIColor colorWithCGColor:color]];


QuartzCore shouldn't use UIColor #Resolved

consider them dead

In reply to: 113286201 [](ancestors = 113286201)

aballway · 2017-04-25T19:23:27Z

Frameworks/UIKit/UIColor.mm

-    [coder encodeFloat:_components.g forKey:@"UIGreen"];
-    [coder encodeFloat:_components.b forKey:@"UIBlue"];
-    [coder encodeFloat:_components.a forKey:@"UIAlpha"];
+ (UIColor*)colorWithWhite:(float)gray alpha:(float)alpha {


These should be CGFloats #Resolved

aballway · 2017-04-25T19:24:02Z

Frameworks/UIKit/UIColor.mm

-    static UIColor* color = [_UICachedColor colorWithRed:0.25f green:0.25f blue:0.25f alpha:1.0f];
-    return color;
+- (instancetype)initWithHue:(float)h saturation:(float)s brightness:(float)v alpha:(float)a {
+    return NSInvalidAbstractInvocationReturn();


does this need to be NSIAIR? #ByDesign

yes, it's our own implementation.

In reply to: 113287177 [](ancestors = 113287177)

aballway · 2017-04-25T19:26:25Z

Frameworks/UIKit/UIColor.mm

-
-    return YES;
+ (UIColor*)orangeColor {
+    static UIColor* color = [UIColor colorWithRed:1.0f green:0.5f blue:0.0f alpha:1.0f];


nit: could these be stuck in a StrongId so they get unloaded w/ the module... or something like that #Resolved

aballway · 2017-04-25T19:26:56Z

Frameworks/UIKit/UINavigationBar.mm

@@ -146,7 +146,7 @@ - (instancetype)initWithFrame:(CGRect)pos {

    [super initWithFrame:pos];

-    [[self layer] setBackgroundColor:(CGColorRef)[UIColor whiteColor]];
+    [[self layer] setBackgroundColor:[[UIColor whiteColor] CGColor]];


nit: use CGConstantColor directly #Resolved

aballway · 2017-04-25T19:27:21Z

Frameworks/include/BridgeHelpers.h

@@ -132,7 +132,7 @@ static inline bool shouldUseConcreteClass(Class self, Class base, Class derived)

 // Helper macro for implementing allocWithZone
 #define ALLOC_PROTOTYPE_SUBCLASS_WITH_ZONE(NSBridgedType, NSBridgedPrototypeType)                            \
-    (NSObject*) allocWithZone : (NSZone*)zone {                                                              \
+    (NSObject*)allocWithZone : (NSZone*)zone {                                                               \


nit: revert this file #WontFix

would like to +1 this: formatting-only changes like this clutter up git history and make it harder to figure out when a regression might've occurred #ByDesign

separated out commits are wonderful though

In reply to: 113529420 [](ancestors = 113529420)

msft-Jeyaram · 2017-04-25T23:12:47Z

@DHowett-MSFT @aballway @MSFTFox Ping

DHowett-MSFT · 2017-04-25T23:14:19Z

When you merge this, I would like to see it squashed into two separate commits -- one for CGColor, and one for UIColor.

DHowett-MSFT · 2017-04-25T23:18:08Z

Frameworks/UIKit/UICGColor.mm

+
+#pragma region Inits
+
+@implementation UIColorPrototype {


you will want to name this _UIColorConcrete; it's not a prototype #Resolved

in fact, you have combined the prototype and the non-prototype #Resolved

that's actually really quite bad; the prototype is a singleton (!!!!) #Resolved

DHowett-MSFT · 2017-04-25T23:53:35Z

oh! this could be very important later.

Please annotate -[UIColor CGColor] in the header with NS_RETURNS_INNER_POINTER!

This is a hint to ARC that the return value from that method may become invalid if the owning object gets released. #Resolved

aballway · 2017-04-26T16:39:36Z

Frameworks/CoreGraphics/CGColor.mm

    if (CFEqual(kCGColorBlack, name)) {
-        ret = static_cast<CGColorRef>([[__LazyUIColor blackColor] CGColor]);
+        static woc::StrongCF<CGColorRef> s_black{ woc::TakeOwnership, CGColorCreateGenericGray(0.0f, 1.0f) };


what is the color space of these on the reference platform? #ByDesign

grayspace

In reply to: 113502948 [](ancestors = 113502948)

aballway · 2017-04-26T16:40:10Z

Frameworks/CoreGraphics/CGColor.mm

+    }
+
+    bool operator==(const __CGColor& other) const {
+        return (CGColorSpaceGetModel(_colorSpace) == CGColorSpaceGetModel(other._colorSpace)) && (_pattern == other._pattern) &&


how will the reference platform handle the same color in different color spaces? #ByDesign

nit: would still like a CFEqual implementation for CGColorSpace for this #WontFix

that's not the scope of this PR. What we have is sufficient.

In reply to: 113503142 [](ancestors = 113503142)

what do you mean by handle?
so far we only support RGB and Grayscale.
so you are limited to RGBColspace:{r,g,b,alpha} and GrayScaleColSpace:{gray,alpha}

In reply to: 113503071 [](ancestors = 113503071)

like if there is RBG(.5, .5, .5, 1) and Gray(.5, 1) will the reference platform consider them the same color? #ByDesign

Yep, but the colorspace is still different if you are asking if it optimizes, nope.

In reply to: 113512713 [](ancestors = 113512713)

aballway · 2017-04-26T16:42:29Z

Frameworks/QuartzCore/CALayer.mm

@@ -1570,9 +1568,9 @@ - (CATransform3D)sublayerTransform {
 */
 - (void)setBackgroundColor:(CGColorRef)color {
    if (color != nil) {
-        priv->backgroundColor = *[static_cast<UIColor*>(color) _getColors];
+        priv->backgroundColor = *(reinterpret_cast<const __CGColorQuad*>(CGColorGetComponents(color)));


This is already assigning into a new struct, so it wouldn't be costly to take the extra safety of constructing a __CGColorQuad with the pointer and moving it #Resolved

aballway · 2017-04-26T16:45:26Z

Frameworks/QuartzCore/CALayer.mm

@@ -1570,9 +1568,9 @@ - (CATransform3D)sublayerTransform {
 */
 - (void)setBackgroundColor:(CGColorRef)color {
    if (color != nil) {
-        priv->backgroundColor = *[static_cast<UIColor*>(color) _getColors];
+        priv->backgroundColor = *(reinterpret_cast<const __CGColorQuad*>(CGColorGetComponents(color)));


also _backgroundColor is a CGColorRef, so I'm not sure what this is doing. We should probably just purge QuartzCore of UIColor and __CGColorQuad since they're not the same #ByDesign

rajsesh · 2017-04-26T01:52:05Z

Frameworks/CoreGraphics/CGColor.mm

+        : _colorSpace(colorspace), _pattern(nullptr), _components{ r, g, b, alpha } {
+    }
+
+    __CGColor(CGColorSpaceRef colorSpace, CGPatternRef pattern) : _colorSpace(colorSpace), _pattern(pattern) {


_components uninitialized. #Resolved

rajsesh · 2017-04-26T01:55:29Z

Frameworks/CoreGraphics/CGColor.mm

+        return _components.a;
+    }
+
+    inline CGColorSpaceRef ColorSpace() const {


nit: when you are returning Ref, it is not const, so the method can't be const.

rajsesh · 2017-04-26T02:02:39Z

Frameworks/CoreGraphics/CGColor.mm

-    [static_cast<UIColor*>(ret) setColorSpace:colorSpace];
-    return ret;
-}
+    RETURN_NULL_IF(CGColorSpaceGetNumberOfComponents(colorSpace) == 0);


This is extraneous check. The switch statement below will suffice. #Resolved

rajsesh · 2017-04-26T18:18:08Z

Frameworks/UIKit/UICGColor.mm

+static const wchar_t* TAG = L"UIColor";
+
+#if defined(WIN32) || defined(WINPHONE)
+int isnan(double x) {


eh? do we need all this stuff?

ms-jihua

need to separate the prototype and concrete, as has been noted.

ms-jihua · 2017-04-26T17:56:38Z

Frameworks/CoreGraphics/CGColor.mm

    } else if (CFEqual(kCGColorClear, name)) {
-        ret = static_cast<CGColorRef>([[__LazyUIColor clearColor] CGColor]);
+        static woc::StrongCF<CGColorRef> s_clear{ woc::TakeOwnership, CGColorCreateGenericGray(0.0f, 0.0f) };
+        return s_clear;
    } else {


nit: can drop the 'else' here #Resolved

ms-jihua · 2017-04-26T18:32:08Z

Frameworks/include/BridgeHelpers.h

@@ -132,7 +132,7 @@ static inline bool shouldUseConcreteClass(Class self, Class base, Class derived)

 // Helper macro for implementing allocWithZone
 #define ALLOC_PROTOTYPE_SUBCLASS_WITH_ZONE(NSBridgedType, NSBridgedPrototypeType)                            \
-    (NSObject*) allocWithZone : (NSZone*)zone {                                                              \
+    (NSObject*)allocWithZone : (NSZone*)zone {                                                               \


would like to +1 this: formatting-only changes like this clutter up git history and make it harder to figure out when a regression might've occurred #ByDesign

rajsesh · 2017-04-26T18:36:24Z

Frameworks/UIKit/UIColor.mm

-
- (CGColorSpaceRef)colorSpace {
-    return _colorSpace;
+ (BOOL)supportsSecureCoding {


brownie points (and actual brownies) if you implement the coding stubs.

wait are there actual brownies at the office today?? am i missing out? #ByDesign

rajsesh · 2017-04-26T18:37:10Z

Frameworks/UIKit/UIColor.mm

-+ (UIColor*)groupTableViewBackgroundColor {
-    return [self colorWithRed:197.f / 255.f green:204.f / 255.f blue:210.f / 255.f alpha:1.0f];
+ (UIColor*)brownColor {
+    static UIColor* color = [UIColor colorWithRed:0.65f green:0.35f blue:0.0f alpha:1.0f];


these statics should be strong ids. #Resolved

msft-Jeyaram · 2017-04-26T20:17:01Z

Ping!

msft-Jeyaram · 2017-04-26T20:26:03Z

Frameworks/UIKit/UITableViewCell.mm

@@ -1090,7 +1090,7 @@ - (void)dealloc {

 - (void)_addBottomBorder:(UITableView*)parentTable {
    if (_borderView == nil) {
-        const __CGColorQuad* color = [[parentTable backgroundColor] _getColors];
+        const __CGColorQuad* color = reinterpret_cast<const __CGColorQuad*>(CGColorGetComponents([[parentTable backgroundColor] CGColor]));


reinterpret_cast<const __CGColorQuad*> [](start = 37, length = 38)

changing this #Resolved

ms-jihua · 2017-04-26T20:28:15Z

Frameworks/UIKit/UICGColor.mm

+    return [UIColor colorWithCGColor:color];
+}
+
+ (UIColor*)colorWithColor:(UIColor*)copyclr {


this doesn't seem to be used anywhere, and isn't a part of the docs. let's get rid of it (if we do need it for some reason, it seems like [[copyclr copy] autorelease] would be the more sensical return value?) #Resolved

DHowett-MSFT · 2017-04-26T21:04:48Z

Frameworks/QuartzCore/CALayer.mm

@@ -1567,7 +1567,8 @@ - (CATransform3D)sublayerTransform {
 */
 - (void)setBackgroundColor:(CGColorRef)color {
    if (color != nil) {
-        priv->backgroundColor = *(reinterpret_cast<const __CGColorQuad*>(CGColorGetComponents(color)));


nit: consider adding a todo here, because this assumes a 4-component color space!

DHowett-MSFT · 2017-04-26T21:05:37Z

Frameworks/UIKit/UIColor.mm

@@ -138,10 +147,6 @@ - (UIColor*)colorWithAlphaComponent:(CGFloat)alpha {
    return NSInvalidAbstractInvocationReturn();
 }

-+ (UIColor*)colorWithColor:(UIColor*)copyclr {


is this not an API that exists?

This removed one of the UIKit dependence CoreGraphics has.

…nvalid colorspace.

This will help represent the component values of colors with respect to their colorspace

- Implement CGColor backed by RGBA values - This would support Grayscale by only using a {gray,gray,gray,alpha} value vs {r,g,b,a} - Support RGBA and Grayscale colorspace - Implement via runtime base - Implement CGColorGetPattern - Removed Lazy loading of UIColor - Removed dependency on UIColor - Properly implemented CGColorEqualToColor Fixes microsoft#2243

- Implemented CGColorSpaceCreateDeviceRGB and CGColorSpaceCreateDeviceGray - Added Unittests for the functions Fixes microsoft#2614

aballway

Would still like the unrelated file reverted

- This supports proper class clustering for UIColor.

- Implement UIColor as a class cluster - Support singleton const color - Support for getRed&getWhite Apis to respect colorspace - Removed direct access of internal structs - Removed unnecessary functionality - Improve Code quality Fixes microsoft#2524, microsoft#2243

The test was attempting to create a Grayscale color as an RGBA color and compare it with a Grayscale color. This validation makes sure that the colors are compared per component. Note: This is due to the fact that Color's are represented as RGB in Windows OS.

msft-Jeyaram requested review from DHowett-MSFT, rajsesh and MSFTFox April 17, 2017 18:37

msftclas added the cla-not-required label Apr 17, 2017

DHowett-MSFT suggested changes Apr 17, 2017

View reviewed changes

aballway reviewed Apr 17, 2017

View reviewed changes

aballway suggested changes Apr 20, 2017

View reviewed changes

msft-Jeyaram commented Apr 25, 2017

View reviewed changes

msft-Jeyaram force-pushed the CGColorRemoveDep branch from 9bce6f8 to 53025b7 Compare April 25, 2017 19:04

aballway suggested changes Apr 25, 2017

View reviewed changes

DHowett-MSFT suggested changes Apr 25, 2017

View reviewed changes

aballway reviewed Apr 26, 2017

View reviewed changes

rajsesh reviewed Apr 26, 2017

View reviewed changes

ms-jihua suggested changes Apr 26, 2017

View reviewed changes

rajsesh reviewed Apr 26, 2017

View reviewed changes

msft-Jeyaram commented Apr 26, 2017

View reviewed changes

ms-jihua reviewed Apr 26, 2017

View reviewed changes

ms-jihua approved these changes Apr 26, 2017

View reviewed changes

DHowett-MSFT approved these changes Apr 26, 2017

View reviewed changes

rajsesh approved these changes Apr 26, 2017

View reviewed changes

msft-Jeyaram added 3 commits April 27, 2017 07:50

Fix formatting issues with Bridge helpers

9a0acab

Remove internal UIKit import.

6b6e0ae

This removed one of the UIKit dependence CoreGraphics has.

Update CGColorSpaceGetModel to return kCGColorSpaceModelUnknown for i…

34e8bdd

…nvalid colorspace.

msft-Jeyaram added 10 commits April 27, 2017 07:50

Remove Lazy loading of UIColor for CoreGraphicsTests

0c80b69

Implement internal representation of Color.

b53962f

This will help represent the component values of colors with respect to their colorspace

Implementation of CGColor::CGColorGetTypeID

1f829e0

Implement CGColor generic RGBA and Grayscale creation APIs.

fe5c65e

- Implemented CGColorSpaceCreateDeviceRGB and CGColorSpaceCreateDeviceGray - Added Unittests for the functions Fixes microsoft#2614

Remove UIKit from CoreGraphics Tests

6b44967

Add stub methods for newly supported CGColor Apis

95cafb4

Annote UIColor:CGColor to be NS_RETURNS_INNER_POINTER

7e27245

Switch Xaml LayerProxy to use CGColor

683d1db

Implement CALayer color transaction via CGColor

28cb9b9

msft-Jeyaram force-pushed the CGColorRemoveDep branch from e76c460 to abec632 Compare April 27, 2017 14:51

aballway approved these changes Apr 27, 2017

View reviewed changes

msft-Jeyaram added 4 commits April 27, 2017 14:34

Implement UICGColor-class cluster via CGColor

25f81fb

- This supports proper class clustering for UIColor.

Implement a concrete singleton for constant UIColors

e5e0182

Fix UINavigationBar and UITableViewCell to use CGColor

d6f0988

msft-Jeyaram force-pushed the CGColorRemoveDep branch from abec632 to 5fada9a Compare April 27, 2017 21:38

msft-Jeyaram added 2 commits April 27, 2017 14:42

Add UIColor comparison tests to validate const colors vs normal colors

ff26cab

msft-Jeyaram force-pushed the CGColorRemoveDep branch from 5fada9a to ff26cab Compare April 27, 2017 21:43

msft-Jeyaram merged commit 5c7b79a into microsoft:develop Apr 27, 2017

msft-Jeyaram mentioned this pull request Apr 27, 2017

Break CoreGraphics and CoreText's dependencies on Foundation + UIKit #2026

Closed


		#pragma region InternalHelpers

		CGColorRef _CGColorCreateWithComponents(CGColorSpaceRef colorspace, CGFloat r, CGFloat g, CGFloat b, CGFloat alpha) {

Implement CGColor independent of UIColor #2538

Implement CGColor independent of UIColor #2538

Conversation

msft-Jeyaram commented Apr 17, 2017 • edited Loading

DHowett-MSFT Apr 17, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

DHowett-MSFT Apr 17, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

DHowett-MSFT Apr 17, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

DHowett-MSFT Apr 17, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

DHowett-MSFT Apr 17, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

DHowett-MSFT Apr 17, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

DHowett-MSFT Apr 17, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

msft-Jeyaram Apr 20, 2017 • edited Loading

Choose a reason for hiding this comment

rajsesh Apr 26, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett-MSFT Apr 17, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett-MSFT Apr 17, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

msft-Jeyaram Apr 17, 2017 • edited Loading

Choose a reason for hiding this comment

aballway Apr 25, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

DHowett-MSFT Apr 17, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

DHowett-MSFT Apr 17, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

msft-Jeyaram Apr 17, 2017 • edited Loading

Choose a reason for hiding this comment

aballway left a comment

Choose a reason for hiding this comment

aballway Apr 17, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

aballway Apr 17, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

aballway Apr 17, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

aballway Apr 17, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

aballway Apr 17, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

aballway Apr 17, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

aballway Apr 17, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

msft-Jeyaram Apr 20, 2017 • edited Loading

Choose a reason for hiding this comment

aballway Apr 20, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

aballway Apr 25, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

aballway Apr 20, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

aballway Apr 20, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

aballway Apr 20, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aballway Apr 21, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

aballway Apr 20, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

msft-Jeyaram commented Apr 21, 2017 • edited Loading

msft-Jeyaram commented Apr 25, 2017 • edited Loading

msft-Jeyaram Apr 25, 2017 • edited Loading

Choose a reason for hiding this comment

aballway Apr 25, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

aballway Apr 25, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

aballway Apr 25, 2017 • edited by msft-Jeyaram Loading

msft-Jeyaram commented Apr 17, 2017 •

edited

Loading

DHowett-MSFT Apr 17, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 17, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 17, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 17, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 17, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 17, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 17, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram Apr 20, 2017 •

edited

Loading

rajsesh Apr 26, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 17, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 17, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram Apr 17, 2017 •

edited

Loading

aballway Apr 25, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 17, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 17, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram Apr 17, 2017 •

edited

Loading

aballway Apr 17, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 17, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 17, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 17, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 17, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 17, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 17, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram Apr 20, 2017 •

edited

Loading

aballway Apr 20, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 25, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 20, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 20, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 20, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 21, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 20, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram commented Apr 21, 2017 •

edited

Loading

msft-Jeyaram commented Apr 25, 2017 •

edited

Loading

msft-Jeyaram Apr 25, 2017 •

edited

Loading

aballway Apr 25, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 25, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 25, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 25, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 26, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 26, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 25, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 25, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 25, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 25, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 25, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 25, 2017 •

edited by msft-Jeyaram

Loading

ms-jihua Apr 26, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 25, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 25, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Apr 25, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT commented Apr 25, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 26, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 26, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 26, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 26, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 26, 2017 •

edited by msft-Jeyaram

Loading

aballway Apr 26, 2017 •

edited by msft-Jeyaram

Loading