Colorspace support for CGColor and improvements to the class.#2244
Conversation
| @@ -0,0 +1,850 @@ | |||
| //****************************************************************************** | |||
| id _pattern; | ||
| __CGColorQuad _components; | ||
| // Note: This should be part of the CGColor struct | ||
| woc::StrongCF<CGColorSpaceRef> _colorSpace; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
| return nullptr; | ||
| if (ret) { | ||
| auto grayColorSpace = woc::MakeStrongCF<CGColorSpaceRef>(CGColorSpaceCreateDeviceGray()); | ||
| [static_cast<UIColor*>(ret) setColorSpace:grayColorSpace]; |
There was a problem hiding this comment.
This is only the appropriate fix for black/white/clear! #Resolved
There was a problem hiding this comment.
the Caveat indicates it's limited. Should probably move it up so the 'future' is not clouded. #Resolved
|
|
||
| RETURN_NULL_IF(!colorSpace); | ||
| RETURN_NULL_IF(!components); | ||
| CGColorRef ret = static_cast<CGColorRef>( |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
| */ | ||
| size_t CGColorGetNumberOfComponents(CGColorRef color) { | ||
| return 4; | ||
| return CGColorSpaceGetNumberOfComponents(CGColorGetColorSpace(color)) + 1; |
There was a problem hiding this comment.
this +1 is somewhat worrying #ByDesign
There was a problem hiding this comment.
specifically: not all colours have alphas #ByDesign
There was a problem hiding this comment.
this is based off of the limited support. Again a while fix to the colorspace is needed.
In reply to: 106507058 [](ancestors = 106507058)
|
|
||
| - (const __CGColorQuad*)_getColors; | ||
| - (BrushType)_type; | ||
| @property CGColorSpaceRef colorSpace; |
There was a problem hiding this comment.
whoa boy, give this a lifetime and an atomicity specifier! #Resolved
| EXPECT_EQ((a)[3], (b)[3]) | ||
|
|
||
| DISABLED_TEST(CGColor, CGColorGetComponents) { | ||
| TEST(CGColor, CGColorGetComponents) { |
There was a problem hiding this comment.
it doesn't look like this test fixture anchors the UIKit symbols. #Resolved
… 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.
fae1f0a to
0048efb
Compare
The CGColor is still a UIColor class [re-implementation is planned], but we have improved the functionality.
fixes #2041