Colorspace support for CGColor and improvements to the class. #2244

msft-Jeyaram · 2017-03-16T00:03:16Z

The CGColor is still a UIColor class [re-implementation is planned], but we have improved the functionality.

fixes #2041

aballway · 2017-03-16T15:53:45Z

Frameworks/UIKit/UIColor.mm~RF5a7c3ee.TMP

@@ -0,0 +1,850 @@
+//******************************************************************************


.TMP 😆 #Resolved

aballway · 2017-03-16T15:59:14Z

Frameworks/UIKit/UIColor.mm

@@ -226,6 +226,8 @@ @implementation UIColor {
    UIImage* _image;
    id _pattern;
    __CGColorQuad _components;
+    // Note: This should be part of the CGColor struct
+    woc::StrongCF<CGColorSpaceRef> _colorSpace;


I really don't think this is the way to fix this. This change set should be included in the separation of CGColor from UIColor, rather than piling on additional short term fixes in UIColor which will add more things to remove. #ByDesign

Talked to raj regarding this, ideally, the whole thing should be fixed!
but given the load this iteration and the re-write is coming up, this should be sufficient.
as much as I don't like doing this, until the re-write happens this would fix up some stuff for us. #ByDesign

DHowett-MSFT · 2017-03-16T19:09:12Z

Frameworks/CoreGraphics/CGColor.mm

-    return nullptr;
+    if (ret) {
+        auto grayColorSpace = woc::MakeStrongCF<CGColorSpaceRef>(CGColorSpaceCreateDeviceGray());
+        [static_cast<UIColor*>(ret) setColorSpace:grayColorSpace];


This is only the appropriate fix for black/white/clear! #Resolved

the Caveat indicates it's limited. Should probably move it up so the 'future' is not clouded. #Resolved

DHowett-MSFT · 2017-03-16T19:09:48Z

Frameworks/CoreGraphics/CGColor.mm

-
+    RETURN_NULL_IF(!colorSpace);
+    RETURN_NULL_IF(!components);
+    CGColorRef ret = static_cast<CGColorRef>(


it might be safe to assert that the #components in the color space is 3-4. Just so that we don't index beyond the end of components. #ByDesign

hmm i don't want to do this, as it's only supporting RGB, not even grayscale.
This is not the main purpose of the CR. The main purpose is to move it to Add CGColorGetColorSpace only.

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

DHowett-MSFT · 2017-03-16T19:11:02Z

Frameworks/CoreGraphics/CGColor.mm

 */
 size_t CGColorGetNumberOfComponents(CGColorRef color) {
-    return 4;
+    return CGColorSpaceGetNumberOfComponents(CGColorGetColorSpace(color)) + 1;


this +1 is somewhat worrying #ByDesign

specifically: not all colours have alphas #ByDesign

this is based off of the limited support. Again a while fix to the colorspace is needed.

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

DHowett-MSFT · 2017-03-16T19:11:34Z

Frameworks/include/UIColorInternal.h

 - (const __CGColorQuad*)_getColors;
 - (BrushType)_type;
+@property CGColorSpaceRef colorSpace;


whoa boy, give this a lifetime and an atomicity specifier! #Resolved

DHowett-MSFT · 2017-03-16T19:12:06Z

tests/unittests/CoreGraphics/CGColorTests.mm

 #define EXPECT_EQ_COMPONENTS(a, b) \
    EXPECT_EQ((a)[0], (b)[0]);     \
    EXPECT_EQ((a)[1], (b)[1]);     \
    EXPECT_EQ((a)[2], (b)[2]);     \
    EXPECT_EQ((a)[3], (b)[3])

-DISABLED_TEST(CGColor, CGColorGetComponents) {
+TEST(CGColor, CGColorGetComponents) {


it doesn't look like this test fixture anchors the UIKit symbols. #Resolved

msft-Jeyaram · 2017-03-17T23:32:35Z

@DHowett @aballway ping!

… the class. Note: Given CGColor is now implemented via UIColor, the var to hold the colorspace is added int UIColor (in the Extended Interface). This should move to the CGColor struct when CGColor is implemented via CPPBase. Implements: CGColorGetColorSpace The following Apis were improved: CGColorGetConstantColor - now supports colorspace for (black,white, and clear) CGColorCreate, CGColorCreateCopyWithAlpha & CGColorCreateWithPattern - create with an apporiate colorspace CGColorRelease CGColorEqualToColor CGColorGetAlpha

…f the colorspace that is associated with the CGColor. Our Color is limited in support, we only support RGB & Grayscale colors.

…onents. Enabling disabled tests + enhancing existing tests.

…to CGColor is an instance of UIColor (which is lazy loaded). Thus for CGColorTests to run we need the UIKit DLL to be loaded. Note: The CGColorTests are appended with [UIColor class] to support the UIKit dll to be part of the test run.

msft-Jeyaram requested review from DHowett-MSFT, rajsesh and MSFTFox March 16, 2017 00:03

msftclas added the cla-not-required label Mar 16, 2017

aballway reviewed Mar 16, 2017

View reviewed changes

DHowett-MSFT suggested changes Mar 16, 2017

View reviewed changes

DHowett-MSFT approved these changes Mar 17, 2017

View reviewed changes

msft-Jeyaram added 4 commits March 19, 2017 22:11

CGColorGetNumberOfComponents should return the number of components o…

93e15c1

…f the colorspace that is associated with the CGColor. Our Color is limited in support, we only support RGB & Grayscale colors.

Adding CGColorTests for CGColorGetColorSpace & CGColorGetNumberOfComp…

fafc5da

…onents. Enabling disabled tests + enhancing existing tests.

msft-Jeyaram force-pushed the colorspace_improv branch from fae1f0a to 0048efb Compare March 20, 2017 05:32

msft-Jeyaram merged commit f5d89da into microsoft:develop Mar 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Colorspace support for CGColor and improvements to the class. #2244

Colorspace support for CGColor and improvements to the class. #2244

msft-Jeyaram commented Mar 16, 2017

aballway Mar 16, 2017 •

edited by msft-Jeyaram

Loading

aballway Mar 16, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram Mar 16, 2017 •

edited

Loading

DHowett-MSFT Mar 16, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram Mar 16, 2017 •

edited

Loading

DHowett-MSFT Mar 16, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram Mar 17, 2017 •

edited

Loading

DHowett-MSFT Mar 16, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Mar 16, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram Mar 17, 2017

DHowett-MSFT Mar 16, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Mar 16, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram commented Mar 17, 2017

		@@ -0,0 +1,850 @@
		//******************************************************************************

Colorspace support for CGColor and improvements to the class. #2244

Colorspace support for CGColor and improvements to the class. #2244

Conversation

msft-Jeyaram commented Mar 16, 2017

aballway Mar 16, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

aballway Mar 16, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

msft-Jeyaram Mar 16, 2017 • edited Loading

Choose a reason for hiding this comment

DHowett-MSFT Mar 16, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

msft-Jeyaram Mar 16, 2017 • edited Loading

Choose a reason for hiding this comment

DHowett-MSFT Mar 16, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

msft-Jeyaram Mar 17, 2017 • edited Loading

Choose a reason for hiding this comment

DHowett-MSFT Mar 16, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

DHowett-MSFT Mar 16, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

msft-Jeyaram Mar 17, 2017

Choose a reason for hiding this comment

DHowett-MSFT Mar 16, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

DHowett-MSFT Mar 16, 2017 • edited by msft-Jeyaram Loading

Choose a reason for hiding this comment

msft-Jeyaram commented Mar 17, 2017

aballway Mar 16, 2017 •

edited by msft-Jeyaram

Loading

aballway Mar 16, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram Mar 16, 2017 •

edited

Loading

DHowett-MSFT Mar 16, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram Mar 16, 2017 •

edited

Loading

DHowett-MSFT Mar 16, 2017 •

edited by msft-Jeyaram

Loading

msft-Jeyaram Mar 17, 2017 •

edited

Loading

DHowett-MSFT Mar 16, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Mar 16, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Mar 16, 2017 •

edited by msft-Jeyaram

Loading

DHowett-MSFT Mar 16, 2017 •

edited by msft-Jeyaram

Loading