Implement CGColor independent of UIColor#2538
Conversation
| return _id; | ||
| } | ||
|
|
||
| __CGColor(CGColorSpaceRef colorspace, CGFloat r, CGFloat g, CGFloat b, CGFloat alpha) : _colorSpace(colorspace), _pattern(nullptr) { |
There was a problem hiding this comment.
you can struct-initialize components to remove the constructor body here #Resolved
| } | ||
|
|
||
| // CreateCopyWithAlpha | ||
| __CGColor(CGColorRef color, float alpha) : _colorSpace(color->_colorSpace), _pattern(color->_pattern) { |
There was a problem hiding this comment.
You might like to make this more like a copy constructor and take const __CGColor& #Resolved
| _components.a = alpha; | ||
| } | ||
|
|
||
| __CGColor(CGColorSpaceRef colorSpace, CGPatternRef pattern) : _colorSpace(colorSpace), _pattern(pattern) { |
There was a problem hiding this comment.
whack some TODOs in here about support components #Resolved
| } | ||
|
|
||
| bool operator==(const __CGColor& other) const { | ||
| RETURN_FALSE_IF(isPatternColor() != other.isPatternColor()); |
There was a problem hiding this comment.
my word, these macros generate an explosion of branches #Resolved
| return true; | ||
| } | ||
|
|
||
| inline bool isPatternColor() const { |
There was a problem hiding this comment.
nit: this is the only one that's lowercase? #Resolved
| [static_cast<UIColor*>(ret) setColorSpace:colorSpace]; | ||
| return ret; | ||
| } | ||
| RETURN_NULL_IF(CGColorSpaceGetNumberOfComponents(colorSpace) == 0); |
There was a problem hiding this comment.
will a colorspace with an RGB or Monochrome model ever have 0? #ByDesign
There was a problem hiding this comment.
if colorspacemodel is unimplemented.
#ByDesign
There was a problem hiding this comment.
This is extraneous check. The switch statement below will suffice. #Resolved
There was a problem hiding this comment.
| 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); |
There was a problem hiding this comment.
CF takes care of ==, but not NULL #ByDesign
There was a problem hiding this comment.
| */ | ||
| size_t CGColorGetNumberOfComponents(CGColorRef color) { | ||
| return CGColorSpaceGetNumberOfComponents(CGColorGetColorSpace(color)) + 1; | ||
| return CGColorSpaceGetNumberOfComponents(color->ColorSpace()) + 1; |
There was a problem hiding this comment.
nit: seems like not the most necessary change #ByDesign
There was a problem hiding this comment.
hmm +2 calls vs +1.
actually -1 since it's inlined #ByDesign
There was a problem hiding this comment.
This doesn't test for a null color #Resolved
| return __CGColor::GetTypeID(); | ||
| } | ||
|
|
||
| #pragma region InternalHelpers |
There was a problem hiding this comment.
nit: regions can be named in normal english #Resolved
|
|
||
| #pragma region InternalHelpers | ||
|
|
||
| CGColorRef _CGColorCreateWithComponents(CGColorSpaceRef colorspace, CGFloat r, CGFloat g, CGFloat b, CGFloat alpha) { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
CGColorCreateGenericRGB now how did I forget about that :(
this will allow us to add the missing apis #Resolved
aballway
left a comment
There was a problem hiding this comment.
for this half of the change 😸
| } | ||
|
|
||
| bool operator==(const __CGColor& other) const { | ||
| RETURN_FALSE_IF(isPatternColor() != other.isPatternColor()); |
There was a problem hiding this comment.
nit: this should be unneeded since you already compare _pattern, #Resolved
|
|
||
| bool operator==(const __CGColor& other) const { | ||
| RETURN_FALSE_IF(isPatternColor() != other.isPatternColor()); | ||
| RETURN_FALSE_IF(CGColorSpaceGetModel(_colorSpace) != CGColorSpaceGetModel(other._colorSpace)); |
There was a problem hiding this comment.
nit: would it be worth adding a CFEqual for CGColorSpace? #WontFix
| 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); |
There was a problem hiding this comment.
nit: add an operator== for __CGColorQuad? #Resolved
| return __CGColor::CreateInstance(kCFAllocatorDefault, colorSpace, pattern); | ||
| } | ||
|
|
||
| CGColorRef __CreateBlackColor() { |
There was a problem hiding this comment.
nit: could just inline these in CGColorGetConstantColor and share a static colorspace #Resolved
| return __CGColor::CreateInstance(kCFAllocatorDefault, colorSpace, components[0], components[0], components[0], components[1]); | ||
| default: | ||
| UNIMPLEMENTED_WITH_MSG("Colorspace Unsupported. Model: %d", model); | ||
| return nullptr; |
There was a problem hiding this comment.
nit: extraneous return #Resolved
| 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); |
There was a problem hiding this comment.
nit: this case is handled by CFEqual #ByDesign
| CGFloat g; | ||
| CGFloat b; | ||
| CGFloat a; | ||
| } __CGColorQuad; |
There was a problem hiding this comment.
where is this being used outside of CGColor? #ByDesign
| } | ||
|
|
||
| bool operator==(const __CGColor& other) const { | ||
| if ((CGColorSpaceGetModel(_colorSpace) != CGColorSpaceGetModel(other._colorSpace)) || (_pattern != other._pattern) || |
There was a problem hiding this comment.
nit: return _ == _ && #Resolved
There was a problem hiding this comment.
this is still unresolved #Resolved
| CGFloat a; | ||
|
|
||
| bool operator==(const __CGColorQuad& other) const { | ||
| if ((r != other.r) || (g != other.g) || (b != other.b) || (a != other.a)) { |
There was a problem hiding this comment.
nit: return _ == other._ && ... #Resolved
| CGColorGetPattern | ||
| CGColorGetTypeID | ||
| CGColorGetConstantColor | ||
| CGColorCreateGenericRGB |
| 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); |
There was a problem hiding this comment.
It looks like there are a couple other CGColor functions which aren't in here like CGColorCreateCopyByMatchingToColorSpace #Closed
There was a problem hiding this comment.
There was a problem hiding this comment.
Should add the issue number here then, or at least add declaration so it gets tracked as STUB api #Resolved
| return __CGColor::CreateInstance(kCFAllocatorDefault, colorSpace, pattern); | ||
| } | ||
|
|
||
| static inline CGColorRef __CreateBlackColor() { |
There was a problem hiding this comment.
nit: could just inline these in CGColorGetConstantColor and share a static colorspace #Resolved
|
@DHowett-MSFT @aballway ping #ByDesign |
|
@DHowett-MSFT @aballway @MSFTFox Ping #ByDesign |
| #endif | ||
| #endif | ||
|
|
||
| hsv rgb2hsv(rgb in) { |
There was a problem hiding this comment.
the below two are just moved. These are from the earlier implementation.
9bce6f8 to
53025b7
Compare
| } | ||
|
|
||
| bool operator==(const __CGColor& other) const { | ||
| if ((CGColorSpaceGetModel(_colorSpace) != CGColorSpaceGetModel(other._colorSpace)) || (_pattern != other._pattern) || |
There was a problem hiding this comment.
this is still unresolved #Resolved
| */ | ||
| size_t CGColorGetNumberOfComponents(CGColorRef color) { | ||
| return CGColorSpaceGetNumberOfComponents(CGColorGetColorSpace(color)) + 1; | ||
| return CGColorSpaceGetNumberOfComponents(color->ColorSpace()) + 1; |
There was a problem hiding this comment.
This doesn't test for a null color #Resolved
| #include "UIKit/UIColor.h" | ||
| #include "UIColorInternal.h" | ||
| #include "UIKit/NSValue+UIKitAdditions.h" | ||
| #include <UIKit/UIApplication.h> |
There was a problem hiding this comment.
nit: consistent on import/include #Resolved
| - (void)setBackgroundColor:(CGColorRef)color { | ||
| if (color != nil) { | ||
| priv->backgroundColor = *[static_cast<UIColor*>(color) _getColors]; | ||
| priv->backgroundColor = *(reinterpret_cast<const __CGColorQuad*>(CGColorGetComponents(color))); |
There was a problem hiding this comment.
! this feels super unsafe! just construct a __CGColorQuad if you need this #ByDesign
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
| // 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]]; |
There was a problem hiding this comment.
QuartzCore shouldn't use UIColor #Resolved
There was a problem hiding this comment.
| [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 { |
There was a problem hiding this comment.
These should be CGFloats #Resolved
| 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(); |
There was a problem hiding this comment.
does this need to be NSIAIR? #ByDesign
There was a problem hiding this comment.
|
|
||
| return YES; | ||
| + (UIColor*)orangeColor { | ||
| static UIColor* color = [UIColor colorWithRed:1.0f green:0.5f blue:0.0f alpha:1.0f]; |
There was a problem hiding this comment.
nit: could these be stuck in a StrongId so they get unloaded w/ the module... or something like that #Resolved
| [super initWithFrame:pos]; | ||
|
|
||
| [[self layer] setBackgroundColor:(CGColorRef)[UIColor whiteColor]]; | ||
| [[self layer] setBackgroundColor:[[UIColor whiteColor] CGColor]]; |
There was a problem hiding this comment.
nit: use CGConstantColor directly #Resolved
| // Helper macro for implementing allocWithZone | ||
| #define ALLOC_PROTOTYPE_SUBCLASS_WITH_ZONE(NSBridgedType, NSBridgedPrototypeType) \ | ||
| (NSObject*) allocWithZone : (NSZone*)zone { \ | ||
| (NSObject*)allocWithZone : (NSZone*)zone { \ |
There was a problem hiding this comment.
nit: revert this file #WontFix
There was a problem hiding this comment.
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
There was a problem hiding this comment.
|
When you merge this, I would like to see it squashed into two separate commits -- one for CGColor, and one for UIColor. |
|
|
||
| #pragma region Inits | ||
|
|
||
| @implementation UIColorPrototype { |
There was a problem hiding this comment.
you will want to name this _UIColorConcrete; it's not a prototype #Resolved
There was a problem hiding this comment.
in fact, you have combined the prototype and the non-prototype #Resolved
There was a problem hiding this comment.
that's actually really quite bad; the prototype is a singleton (!!!!) #Resolved
|
oh! this could be very important later. Please annotate This is a hint to ARC that the return value from that method may become invalid if the owning object gets released. #Resolved |
| woc::StrongCF<CGColorSpaceRef> colorspace{ woc::TakeOwnership, CGColorSpaceCreateDeviceGray() }; | ||
| if (CFEqual(kCGColorBlack, name)) { | ||
| ret = static_cast<CGColorRef>([[__LazyUIColor blackColor] CGColor]); | ||
| static woc::StrongCF<CGColorRef> s_black{ woc::TakeOwnership, CGColorCreateGenericGray(0.0f, 1.0f) }; |
There was a problem hiding this comment.
what is the color space of these on the reference platform? #ByDesign
There was a problem hiding this comment.
| } | ||
|
|
||
| bool operator==(const __CGColor& other) const { | ||
| return (CGColorSpaceGetModel(_colorSpace) == CGColorSpaceGetModel(other._colorSpace)) && (_pattern == other._pattern) && |
There was a problem hiding this comment.
how will the reference platform handle the same color in different color spaces? #ByDesign
There was a problem hiding this comment.
nit: would still like a CFEqual implementation for CGColorSpace for this #WontFix
There was a problem hiding this comment.
that's not the scope of this PR. What we have is sufficient.
In reply to: 113503142 [](ancestors = 113503142)
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
like if there is RBG(.5, .5, .5, 1) and Gray(.5, 1) will the reference platform consider them the same color? #ByDesign
There was a problem hiding this comment.
Yep, but the colorspace is still different if you are asking if it optimizes, nope.
In reply to: 113512713 [](ancestors = 113512713)
| - (void)setBackgroundColor:(CGColorRef)color { | ||
| if (color != nil) { | ||
| priv->backgroundColor = *[static_cast<UIColor*>(color) _getColors]; | ||
| priv->backgroundColor = *(reinterpret_cast<const __CGColorQuad*>(CGColorGetComponents(color))); |
There was a problem hiding this comment.
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
| - (void)setBackgroundColor:(CGColorRef)color { | ||
| if (color != nil) { | ||
| priv->backgroundColor = *[static_cast<UIColor*>(color) _getColors]; | ||
| priv->backgroundColor = *(reinterpret_cast<const __CGColorQuad*>(CGColorGetComponents(color))); |
There was a problem hiding this comment.
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
| : _colorSpace(colorspace), _pattern(nullptr), _components{ r, g, b, alpha } { | ||
| } | ||
|
|
||
| __CGColor(CGColorSpaceRef colorSpace, CGPatternRef pattern) : _colorSpace(colorSpace), _pattern(pattern) { |
There was a problem hiding this comment.
_components uninitialized. #Resolved
| return _components.a; | ||
| } | ||
|
|
||
| inline CGColorSpaceRef ColorSpace() const { |
There was a problem hiding this comment.
nit: when you are returning Ref, it is not const, so the method can't be const.
| [static_cast<UIColor*>(ret) setColorSpace:colorSpace]; | ||
| return ret; | ||
| } | ||
| RETURN_NULL_IF(CGColorSpaceGetNumberOfComponents(colorSpace) == 0); |
There was a problem hiding this comment.
This is extraneous check. The switch statement below will suffice. #Resolved
| static const wchar_t* TAG = L"UIColor"; | ||
|
|
||
| #if defined(WIN32) || defined(WINPHONE) | ||
| int isnan(double x) { |
There was a problem hiding this comment.
eh? do we need all this stuff?
ms-jihua
left a comment
There was a problem hiding this comment.
need to separate the prototype and concrete, as has been noted.
| ret = static_cast<CGColorRef>([[__LazyUIColor clearColor] CGColor]); | ||
| static woc::StrongCF<CGColorRef> s_clear{ woc::TakeOwnership, CGColorCreateGenericGray(0.0f, 0.0f) }; | ||
| return s_clear; | ||
| } else { |
There was a problem hiding this comment.
nit: can drop the 'else' here #Resolved
| // Helper macro for implementing allocWithZone | ||
| #define ALLOC_PROTOTYPE_SUBCLASS_WITH_ZONE(NSBridgedType, NSBridgedPrototypeType) \ | ||
| (NSObject*) allocWithZone : (NSZone*)zone { \ | ||
| (NSObject*)allocWithZone : (NSZone*)zone { \ |
There was a problem hiding this comment.
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
|
|
||
| - (CGColorSpaceRef)colorSpace { | ||
| return _colorSpace; | ||
| + (BOOL)supportsSecureCoding { |
There was a problem hiding this comment.
brownie points (and actual brownies) if you implement the coding stubs.
There was a problem hiding this comment.
wait are there actual brownies at the office today?? am i missing out? #ByDesign
| + (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]; |
There was a problem hiding this comment.
these statics should be strong ids. #Resolved
|
Ping! |
| - (void)_addBottomBorder:(UITableView*)parentTable { | ||
| if (_borderView == nil) { | ||
| const __CGColorQuad* color = [[parentTable backgroundColor] _getColors]; | ||
| const __CGColorQuad* color = reinterpret_cast<const __CGColorQuad*>(CGColorGetComponents([[parentTable backgroundColor] CGColor])); |
There was a problem hiding this comment.
reinterpret_cast<const __CGColorQuad*> [](start = 37, length = 38)
changing this #Resolved
| return [UIColor colorWithCGColor:color]; | ||
| } | ||
|
|
||
| + (UIColor*)colorWithColor:(UIColor*)copyclr { |
There was a problem hiding this comment.
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
| */ | ||
| - (void)setBackgroundColor:(CGColorRef)color { | ||
| if (color != nil) { | ||
| priv->backgroundColor = *(reinterpret_cast<const __CGColorQuad*>(CGColorGetComponents(color))); |
There was a problem hiding this comment.
nit: consider adding a todo here, because this assumes a 4-component color space!
| return NSInvalidAbstractInvocationReturn(); | ||
| } | ||
|
|
||
| + (UIColor*)colorWithColor:(UIColor*)copyclr { |
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
e76c460 to
abec632
Compare
aballway
left a comment
There was a problem hiding this comment.
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
abec632 to
5fada9a
Compare
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.
5fada9a to
ff26cab
Compare
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.
fixes #2243, #2524