Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Mar 20, 2017
+129 −34
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
969aab2
Implementation of Color Space support for CGColor and improvements to…
msft-Jeyaram Mar 20, 2017
93e15c1
CGColorGetNumberOfComponents should return the number of components o…
msft-Jeyaram Mar 20, 2017
fafc5da
Adding CGColorTests for CGColorGetColorSpace & CGColorGetNumberOfComp…
msft-Jeyaram Mar 20, 2017
0048efb
Adding UIKit as a dependence for CoreGraphics UnitTests, this is due …
msft-Jeyaram Mar 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 46 additions & 31 deletions Frameworks/CoreGraphics/CGColor.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
//******************************************************************************
//
// Copyright (c) 2016 Microsoft Corporation. All rights reserved.
// Copyright (c) Microsoft. All rights reserved.
//
// This code is licensed under the MIT License (MIT).
//
Expand Down Expand Up @@ -31,25 +31,34 @@
@Notes Limited constants supported
*/
CGColorRef CGColorGetConstantColor(CFStringRef name) {
CGColorRef ret = nullptr;
woc::StrongCF<CGColorSpaceRef> colorspace;
if (CFEqual(kCGColorBlack, name)) {
return [[__LazyUIColor blackColor] CGColor];
ret = static_cast<CGColorRef>([[__LazyUIColor blackColor] CGColor]);
colorspace = woc::MakeStrongCF<CGColorSpaceRef>(CGColorSpaceCreateDeviceGray());
} else if (CFEqual(kCGColorWhite, name)) {
return [[__LazyUIColor whiteColor] CGColor];
ret = static_cast<CGColorRef>([[__LazyUIColor whiteColor] CGColor]);
colorspace = woc::MakeStrongCF<CGColorSpaceRef>(CGColorSpaceCreateDeviceGray());
} else if (CFEqual(kCGColorClear, name)) {
return [[__LazyUIColor clearColor] CGColor];
ret = static_cast<CGColorRef>([[__LazyUIColor clearColor] CGColor]);
colorspace = woc::MakeStrongCF<CGColorSpaceRef>(CGColorSpaceCreateDeviceGray());
} else {
UNIMPLEMENTED_WITH_MSG("CGColorGetConstantColor does not support color %s", CFStringGetCStringPtr(name, kCFStringEncodingUTF8));
}

UNIMPLEMENTED_WITH_MSG("CGColorGetConstantColor does not support color %s", CFStringGetCStringPtr(name, kCFStringEncodingUTF8));
return nullptr;
if (ret) {
[static_cast<UIColor*>(ret) setColorSpace:colorspace];
}

return ret;
}

/**
@Status Interoperable
*/
void CGColorRelease(CGColorRef color) {
if (color != nullptr) {
CFRelease(color);
}
RETURN_IF(!color);
CFRelease(color);
}

/**
Expand All @@ -62,18 +71,20 @@ CGColorRef CGColorRetain(CGColorRef color) {
}

/**
@Status Interoperable
@Status Caveat
@Notes only supports RGB
*/
CGColorRef CGColorCreate(CGColorSpaceRef colorSpace, const CGFloat* components) {
CGColorRef ret =
(CGColorRef)[[__LazyUIColor colorWithRed:components[0] green:components[1] blue:components[2] alpha:components[3]] retain];

RETURN_NULL_IF(!colorSpace);
RETURN_NULL_IF(!components);
CGColorRef ret = static_cast<CGColorRef>(
Copy link

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@msft-Jeyaram msft-Jeyaram Mar 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

[[__LazyUIColor colorWithRed:components[0] green:components[1] blue:components[2] alpha:components[3]] retain]);
[static_cast<UIColor*>(ret) setColorSpace:colorSpace];
return ret;
}

/**
@Status Caveat
@Notes TODO: need to revisit if retail is enough for CreateCopy
@Status Interoperable
*/
CGColorRef CGColorCreateCopy(CGColorRef color) {
return CGColorRetain(color);
Expand All @@ -83,21 +94,28 @@ CGColorRef CGColorCreateCopy(CGColorRef color) {
@Status Interoperable
*/
CGColorRef CGColorCreateCopyWithAlpha(CGColorRef color, float alpha) {
RETURN_NULL_IF(!color);
static __CGColorQuad defaultColor{ 0.0f, 0.0f, 0.0f, 0.0f };
const __CGColorQuad* curColor = [(UIColor*)color _getColors];
if (!curColor) {
curColor = &defaultColor;
}

return static_cast<CGColorRef>([[__LazyUIColor colorWithRed:curColor->r green:curColor->g blue:curColor->b alpha:alpha] retain]);
CGColorRef ret =
static_cast<CGColorRef>([[__LazyUIColor colorWithRed:curColor->r green:curColor->g blue:curColor->b alpha:alpha] retain]);
[static_cast<UIColor*>(ret) setColorSpace:CGColorGetColorSpace(color)];
return ret;
}

/**
@Status Interoperable
@Status Caveat
@Notes components are not supported
*/
CGColorRef CGColorCreateWithPattern(CGColorSpaceRef colorSpace, CGPatternRef pattern, const CGFloat* components) {
CGColorRef ret = (CGColorRef)[[__LazyUIColor _colorWithCGPattern:(CGPatternRef)pattern] retain];

RETURN_NULL_IF(!colorSpace);
RETURN_NULL_IF(!pattern);
CGColorRef ret = static_cast<CGColorRef>([[__LazyUIColor _colorWithCGPattern:(CGPatternRef)pattern] retain]);
[static_cast<UIColor*>(ret) setColorSpace:colorSpace];
return ret;
}

Expand All @@ -115,28 +133,25 @@ bool CGColorEqualToColor(CGColorRef color1, CGColorRef color2) {
const __CGColorQuad* components2 = [(UIColor*)color2 _getColors];

return ((components1->r == components2->r) && (components1->g == components2->g) && (components1->b == components2->b) &&
(components1->a == components2->a));
(components1->a == components2->a)) &&
(CGColorSpaceGetModel(CGColorGetColorSpace(color1)) == CGColorSpaceGetModel(CGColorGetColorSpace(color2)));
}

/**
@Status Interoperable
*/
CGFloat CGColorGetAlpha(CGColorRef color) {
RETURN_RESULT_IF(!color, 0.0f);
const __CGColorQuad* curColor = [(UIColor*)color _getColors];

if (curColor) {
return curColor->a;
}
return 0.0f;
return curColor->a;
}

/**
@Status Stub
@Notes
@Status Interoperable
*/
CGColorSpaceRef CGColorGetColorSpace(CGColorRef color) {
UNIMPLEMENTED();
return StubReturn();
RETURN_NULL_IF(!color);
return [static_cast<UIColor*>(color) colorSpace];
}

/**
Expand All @@ -149,10 +164,10 @@ CGColorSpaceRef CGColorGetColorSpace(CGColorRef color) {

/**
@Status Caveat
@Notes Not all colors have 4 components, but all of the ones we currently support do!
@Notes Limited support for colorspace
*/
size_t CGColorGetNumberOfComponents(CGColorRef color) {
return 4;
return CGColorSpaceGetNumberOfComponents(CGColorGetColorSpace(color)) + 1;
Copy link

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this +1 is somewhat worrying #ByDesign

Copy link

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

specifically: not all colours have alphas #ByDesign

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

}

/**
Expand Down
10 changes: 10 additions & 0 deletions Frameworks/UIKit/UIColor.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@msft-Jeyaram msft-Jeyaram Mar 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

/**
Expand Down Expand Up @@ -615,6 +617,14 @@ - (const __CGColorQuad*)_getColors {
return &_components;
}

- (CGColorSpaceRef)colorSpace {
return _colorSpace;
}

- (void)setColorSpace:(CGColorSpaceRef)colorSpace {
_colorSpace = colorSpace;
}

/**
@Status Interoperable
*/
Expand Down
2 changes: 1 addition & 1 deletion Frameworks/include/UIColorInternal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ inline void _ClearColorQuad(__CGColorQuad& color) {
}
+ (UIColor*)_colorWithCGPattern:(CGPatternRef)pattern;
+ (UIColor*)_windowsTableViewCellSelectionBackgroundColor;

- (const __CGColorQuad*)_getColors;
- (BrushType)_type;
@property (nonatomic, assign) CGColorSpaceRef colorSpace;
@end
3 changes: 3 additions & 0 deletions build/Tests/UnitTests/CoreGraphics/CoreGraphics.UnitTests.vcxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@
<ProjectReference Include="..\..\..\CoreImage\dll\CoreImage.vcxproj">
<Project>{3EFCDFF3-6013-448F-8611-534D0F819D6B}</Project>
</ProjectReference>
<ProjectReference Include="..\..\..\UIKit\dll\UIKit.vcxproj">
<Project>{43D2CFE5-0711-4E61-8F5C-F8D3B65D3A49}</Project>
</ProjectReference>
</ItemGroup>
<PropertyGroup Label="Globals">
<ProjectGuid>{DA6C01EA-A22E-4807-BACE-63C5C6ABAE77}</ProjectGuid>
Expand Down
71 changes: 69 additions & 2 deletions tests/unittests/CoreGraphics/CGColorTests.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,19 +17,33 @@
#import <TestFramework.h>
#import <CoreGraphics/CoreGraphics.h>

// TODO #2243: Remove the UIKit dependency
#if WINOBJC
#include <UIKit/UIColor.h>
#endif

#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) {
Copy link

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#if WINOBJC
[UIColor class];
#endif
CGFloat colors[] = { 1, 0, 0, 1 }; // bright red

CGColorSpaceRef clrRgb = CGColorSpaceCreateDeviceRGB();
CGColorRef clr = CGColorCreate(clrRgb, colors);
CGColorRef copy = CGColorCreateCopyWithAlpha(clr, 0.5); // transparent red

EXPECT_EQ(CGColorSpaceGetModel(clrRgb), CGColorSpaceGetModel(CGColorGetColorSpace(copy)));
EXPECT_EQ(CGColorSpaceGetModel(clrRgb), CGColorSpaceGetModel(CGColorGetColorSpace(clr)));

EXPECT_EQ(4, CGColorGetNumberOfComponents(clr));
EXPECT_EQ(4, CGColorGetNumberOfComponents(copy));

const CGFloat* components = CGColorGetComponents(copy);
CGFloat expected[] = { 1, 0, 0, 0.5 };
EXPECT_EQ_COMPONENTS(components, expected);
Expand All @@ -42,7 +56,45 @@
CGColorSpaceRelease(clrRgb);
}

DISABLED_TEST(CGColor, CGColorEquals) {
TEST(CGColor, CGColorEquals) {
#if WINOBJC
[UIColor class];
#endif

CGFloat colors[] = { 1, 0, 0, 1 }; // bright red

CGColorSpaceRef clrRgb = CGColorSpaceCreateDeviceRGB();
CGColorRef clr1 = CGColorCreate(clrRgb, colors);
CGColorRef clr2 = CGColorCreateCopy(clr1);
CGColorRef clr3 = CGColorCreateCopyWithAlpha(clr1, 0.9);
CGColorRef clr4 = CGColorCreate(clrRgb, colors);

EXPECT_EQ(CGColorSpaceGetModel(clrRgb), CGColorSpaceGetModel(CGColorGetColorSpace(clr1)));
EXPECT_EQ(CGColorSpaceGetModel(clrRgb), CGColorSpaceGetModel(CGColorGetColorSpace(clr2)));
EXPECT_EQ(CGColorSpaceGetModel(clrRgb), CGColorSpaceGetModel(CGColorGetColorSpace(clr3)));
EXPECT_EQ(CGColorSpaceGetModel(clrRgb), CGColorSpaceGetModel(CGColorGetColorSpace(clr4)));

EXPECT_EQ(4, CGColorGetNumberOfComponents(clr1));
EXPECT_EQ(4, CGColorGetNumberOfComponents(clr2));
EXPECT_EQ(4, CGColorGetNumberOfComponents(clr3));
EXPECT_EQ(4, CGColorGetNumberOfComponents(clr4));

EXPECT_TRUE(CGColorEqualToColor(clr1, clr1));
EXPECT_TRUE(CGColorEqualToColor(clr1, clr2));
EXPECT_TRUE(CGColorEqualToColor(clr1, clr4));
EXPECT_FALSE(CGColorEqualToColor(clr1, clr3));

CGColorRelease(clr1);
CGColorRelease(clr2);
CGColorRelease(clr3);
CGColorRelease(clr4);
CGColorSpaceRelease(clrRgb);
}

TEST(CGColor, GetColorSpace) {
#if WINOBJC
[UIColor class];
#endif
CGFloat colors[] = { 1, 0, 0, 1 }; // bright red

CGColorSpaceRef clrRgb = CGColorSpaceCreateDeviceRGB();
Expand All @@ -62,3 +114,18 @@
CGColorRelease(clr4);
CGColorSpaceRelease(clrRgb);
}

TEST(CGColor, GetConstantColor) {
#if WINOBJC
[UIColor class];
#endif
auto grayColorSpace = woc::MakeStrongCF<CGColorSpaceRef>(CGColorSpaceCreateDeviceGray());

CFStringRef colors[] = { kCGColorWhite, kCGColorBlack, kCGColorClear };

for (int i = 0; i < std::extent<decltype(colors)>::value; ++i) {
CGColorRef col = CGColorGetConstantColor(colors[i]);
EXPECT_EQ(CGColorSpaceGetModel(grayColorSpace), CGColorSpaceGetModel(CGColorGetColorSpace(col)));
EXPECT_EQ(2, CGColorGetNumberOfComponents(col));
}
}