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.

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

Implement the CGContext non-drawing path functions in terms of CGPath. #1240

Merged
merged 8 commits into from
Oct 31, 2016
Merged
150 changes: 112 additions & 38 deletions Frameworks/CoreGraphics/CGContext.mm
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ inline void ComputeStrokeStyle(ID2D1DeviceContext* deviceContext) {
deviceContext->GetFactory(&factory);

std::vector<float> adjustedDashes(dashes.size());
std::transform(dashes.cbegin(), dashes.cend(), adjustedDashes.begin(), [this](const float& f) -> float { return f / lineWidth; });
std::transform(dashes.cbegin(), dashes.cend(), adjustedDashes.begin(), [this](const CGFloat& f) -> float { return f / lineWidth; });
Copy link
Member

@bbowman bbowman Oct 27, 2016

Choose a reason for hiding this comment

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

float [](start = 107, length = 5)

is this actually needed? #ByDesign

Copy link
Contributor

@aballway aballway Oct 27, 2016

Choose a reason for hiding this comment

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

also should only need [&lineWidth] #ByDesign

Copy link
Author

Choose a reason for hiding this comment

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

It complained when I attempted to capture a member by reference. Don't think I didn't try ;P


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

Copy link
Author

Choose a reason for hiding this comment

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

In a future world where we differentiate CGFloat based on some platform-specific thing, yes.


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

Copy link
Contributor

@aballway aballway Oct 31, 2016

Choose a reason for hiding this comment

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

thought I added a different comment here... try [lineWidth = this.lineWidth] or something to that effect. and is -> float necessary? #WontFix

Copy link
Author

Choose a reason for hiding this comment

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

Chatted about this. I prefer explicit, and right now capturing this is a very minor worry.


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

FAIL_FAST_IF_FAILED(factory->CreateStrokeStyle(strokeProperties, adjustedDashes.data(), adjustedDashes.size(), &strokeStyle));
}

Expand Down Expand Up @@ -150,6 +150,21 @@ inline void ClearStrokeStyle() {
return GStateStack().top();
}

inline bool HasPath() {
return _impl.currentPath != nullptr;
}

inline CGMutablePathRef Path() {
if (!_impl.currentPath) {
Copy link
Member

Choose a reason for hiding this comment

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

currentPath [](start = 19, length = 11)

Might as well use your fancy HasPath here.

_impl.currentPath.reset(CGPathCreateMutable());
}
Copy link
Member

@bbowman bbowman Oct 27, 2016

Choose a reason for hiding this comment

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

I assume its not supposed to be thread safe? #ByDesign

Copy link
Author

Choose a reason for hiding this comment

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

Correct; CGContext is not explicitly thread-safe.


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

return _impl.currentPath.get();
}

inline void SetPath(CGMutablePathRef path) {
Copy link
Member

@MSFTFox MSFTFox Oct 31, 2016

Choose a reason for hiding this comment

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

SetPath [](start = 16, length = 7)

Should you close/release the previous path if there is one? #ByDesign

Copy link
Author

Choose a reason for hiding this comment

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

unique_cf takes care of this.


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

Copy link
Contributor

@msft-Jeyaram msft-Jeyaram Oct 31, 2016

Choose a reason for hiding this comment

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

inline void SetPath(CGMutablePathRef path) { [](start = 1, length = 47)

could we just switch over to the standard of having getters and setters to keep encapsulation throughout the code?
We have a mix of direct access and using getters and setters. It would be good to just follow one standard (encapsulation works the best here).

^I meant for the rest of the code that access the struct variables directly, we should have a setter/getter just like this for them.

Copy link
Author

Choose a reason for hiding this comment

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

I do not understand. You commented on a setter and asked if we could move to setters.


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

_impl.currentPath.reset(CGPathRetain(path));
}

inline void ClearPath() {
_impl.currentPath.reset();
}
Expand Down Expand Up @@ -450,23 +465,27 @@ void CGContextSetCTM(CGContextRef context, CGAffineTransform transform) {
*/
void CGContextBeginPath(CGContextRef context) {
NOISY_RETURN_IF_NULL(context);
UNIMPLEMENTED();

// All subsequent path functions will create the new path as necessary.
context->ClearPath();
}

/**
@Status Interoperable
*/
void CGContextClosePath(CGContextRef context) {
NOISY_RETURN_IF_NULL(context);
UNIMPLEMENTED();

CGPathCloseSubpath(context->Path());
}

/**
@Status Interoperable
*/
void CGContextAddRect(CGContextRef context, CGRect rect) {
NOISY_RETURN_IF_NULL(context);
UNIMPLEMENTED();

CGPathAddRect(context->Path(), nullptr, rect);
Copy link
Member

Choose a reason for hiding this comment

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

nullptr [](start = 35, length = 7)

Did we come to a conclusion on what to do with the transform here?

}

/**
Expand All @@ -489,93 +508,132 @@ void CGContextAddRects(CGContextRef context, const CGRect* rects, unsigned count
*/
void CGContextAddLineToPoint(CGContextRef context, CGFloat x, CGFloat y) {
NOISY_RETURN_IF_NULL(context);
UNIMPLEMENTED();

CGPathAddLineToPoint(context->Path(), nullptr, x, y);
}

/**
@Status Interoperable
*/
void CGContextAddCurveToPoint(CGContextRef context, CGFloat cp1x, CGFloat cp1y, CGFloat cp2x, CGFloat cp2y, CGFloat x, CGFloat y) {
NOISY_RETURN_IF_NULL(context);
UNIMPLEMENTED();

CGPathAddCurveToPoint(context->Path(), nullptr, cp1x, cp1y, cp2x, cp2y, x, y);
}

/**
@Status Interoperable
*/
void CGContextAddQuadCurveToPoint(CGContextRef context, CGFloat cpx, CGFloat cpy, CGFloat x, CGFloat y) {
NOISY_RETURN_IF_NULL(context);
UNIMPLEMENTED();

CGPathAddQuadCurveToPoint(context->Path(), nullptr, cpx, cpy, x, y);
}

/**
@Status Interoperable
*/
void CGContextMoveToPoint(CGContextRef context, CGFloat x, CGFloat y) {
NOISY_RETURN_IF_NULL(context);
UNIMPLEMENTED();

CGPathMoveToPoint(context->Path(), nullptr, x, y);
}

/**
@Status Interoperable
*/
void CGContextAddArc(CGContextRef context, CGFloat x, CGFloat y, CGFloat radius, CGFloat startAngle, CGFloat endAngle, int clockwise) {
NOISY_RETURN_IF_NULL(context);
UNIMPLEMENTED();

CGPathAddArc(context->Path(), nullptr, x, y, radius, startAngle, endAngle, clockwise);
}

/**
@Status Interoperable
*/
void CGContextAddArcToPoint(CGContextRef context, CGFloat x1, CGFloat y1, CGFloat x2, CGFloat y2, CGFloat radius) {
NOISY_RETURN_IF_NULL(context);
UNIMPLEMENTED();

CGPathAddArcToPoint(context->Path(), nullptr, x1, y1, x2, y2, radius);
}

/**
@Status Interoperable
*/
void CGContextAddEllipseInRect(CGContextRef context, CGRect rect) {
NOISY_RETURN_IF_NULL(context);
UNIMPLEMENTED();

CGPathAddEllipseInRect(context->Path(), nullptr, rect);
}

/**
@Status Interoperable
*/
void CGContextAddPath(CGContextRef context, CGPathRef path) {
NOISY_RETURN_IF_NULL(context);
// The Apple SDK docs imply that passing a NULL path is valid.
// So avoid calling into the backing if NULL.
if (path) {
UNIMPLEMENTED();
if (!path) {
return;
}
Copy link
Member

@bbowman bbowman Oct 27, 2016

Choose a reason for hiding this comment

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

whats the difference? Should be "noisy" here too? #ByDesign

Copy link
Author

Choose a reason for hiding this comment

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

I don't believe; all the context APIs are noisy for context, but they differ on their noisiness for parameters. We can definitely evaluate it.


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

Copy link
Contributor

@msft-Jeyaram msft-Jeyaram Oct 27, 2016

Choose a reason for hiding this comment

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

setters? to keep the encapsulation? #Resolved

Copy link
Author

Choose a reason for hiding this comment

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

mehhh fiiiineeee


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


if (!context->HasPath()) {
// If we don't curerntly have a path, take this one in as our own.
woc::unique_cf<CGMutablePathRef> copiedPath{ CGPathCreateMutableCopy(path) };
context->SetPath(copiedPath.get());
return;
Copy link
Contributor

@aballway aballway Oct 27, 2016

Choose a reason for hiding this comment

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

nit: else instead of early return #WontFix

Copy link
Author

Choose a reason for hiding this comment

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

no.


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

}

CGPathAddPath(context->Path(), nullptr, path);
}

/**
@Status Stub
*/
void CGContextReplacePathWithStrokedPath(CGContextRef context) {
NOISY_RETURN_IF_NULL(context);
UNIMPLEMENTED();

if (!context->HasPath()) {
return;
}

auto& state = context->CurrentGState();

// TODO GH#xxxx When CGPathCreateCopyByStrokingPath is no longer stubbed, remove the diagnostic suppression.
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wdeprecated-declarations"

woc::unique_cf<CGPathRef> newPath{ CGPathCreateCopyByStrokingPath(context->Path(),
&state.transform,
state.lineWidth,
(CGLineCap)state.strokeProperties.startCap,
(CGLineJoin)state.strokeProperties.lineJoin,
state.strokeProperties.miterLimit) };

#pragma clang diagnostic pop

woc::unique_cf<CGMutablePathRef> newMutablePath{ CGPathCreateMutableCopy(newPath.get()) };
context->SetPath(newMutablePath.get());
}

/**
@Status Interoperable
*/
bool CGContextIsPathEmpty(CGContextRef context) {
NOISY_RETURN_IF_NULL(context, StubReturn());
UNIMPLEMENTED();
return StubReturn();

return !context->HasPath() || CGPathIsEmpty(context->Path());
Copy link
Member

@MSFTFox MSFTFox Oct 31, 2016

Choose a reason for hiding this comment

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

!context->HasPath() [](start = 11, length = 19)

nit: this is a redundant check. CGPathIsEmpty will check if the path is null. #ByDesign

Copy link
Author

@DHowett-MSFT DHowett-MSFT Oct 31, 2016

Choose a reason for hiding this comment

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

This is not redundant, as ->Path() will create a useless path otherwise. #Closed

}

/**
@Status Interoperable
*/
CGRect CGContextGetPathBoundingBox(CGContextRef context) {
NOISY_RETURN_IF_NULL(context, StubReturn());
UNIMPLEMENTED();
return StubReturn();
NOISY_RETURN_IF_NULL(context, CGRectNull);

if (!context->HasPath()) {
return CGRectNull;
}
Copy link
Member

@bbowman bbowman Oct 27, 2016

Choose a reason for hiding this comment

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

both noisy? #ByDesign

Copy link
Member

Choose a reason for hiding this comment

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

neither noisy?


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

Copy link
Author

Choose a reason for hiding this comment

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

Context always noisy, empty path not noisy.


In reply to: 85251032 [](ancestors = 85251032,85251026)

Copy link
Member

@MSFTFox MSFTFox Oct 31, 2016

Choose a reason for hiding this comment

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

nit: redundant. CGPathGetBoundingBox will return CGRectNull if the path is empty. #ByDesign

Copy link
Author

@DHowett-MSFT DHowett-MSFT Oct 31, 2016

Choose a reason for hiding this comment

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

This is not redundant, as ->Path() will create a useless path otherwise. #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

To be the devil: RETURN_RESULT_IF(!context->HasPath(),CGRectNull);

You can use RETURN_IF(), etc for the non noisy conditional checks.

Copy link
Author

Choose a reason for hiding this comment

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

I cannot say I understand or agree with our convention of introducing A_BUNCH_OF_MACROS to do really common things; it flies in the face of our other coding convention, "don't use macros very often"


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


return CGPathGetBoundingBox(context->Path());
}

/**
Expand All @@ -588,11 +646,7 @@ void CGContextAddLines(CGContextRef context, const CGPoint* points, unsigned cou
return;
Copy link
Contributor

@aballway aballway Oct 27, 2016

Choose a reason for hiding this comment

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

should we do this validation here or let CGPath do whatever it wants? #WontFix

Copy link
Author

Choose a reason for hiding this comment

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

Either is fine.


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

}

// TODO(DH) GH#1066 Switch to CGPathAddLines(context->path, points, count);
CGContextMoveToPoint(context, points[0].x, points[0].y);
for (unsigned int i = 1; i < count; i++) {
CGContextAddLineToPoint(context, points[i].x, points[i].y);
}
CGPathAddLines(context->Path(), nullptr, points, count);
}
#pragma endregion

Expand All @@ -602,29 +656,37 @@ void CGContextAddLines(CGContextRef context, const CGPoint* points, unsigned cou
@Notes
*/
CGPathRef CGContextCopyPath(CGContextRef context) {
NOISY_RETURN_IF_NULL(context, StubReturn());
UNIMPLEMENTED();
return StubReturn();
NOISY_RETURN_IF_NULL(context, nullptr);

if (!context->HasPath()) {
return nullptr;
}
Copy link
Member

@MSFTFox MSFTFox Oct 31, 2016

Choose a reason for hiding this comment

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

nit: redundant. CGPath handles this. #ByDesign

Copy link
Author

@DHowett-MSFT DHowett-MSFT Oct 31, 2016

Choose a reason for hiding this comment

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

This is not redundant, as ->Path() will create a useless path otherwise. #Closed


return CGPathCreateCopy(context->Path());
}

/**
@Status Stub
@Notes
*/
CGPoint CGContextGetPathCurrentPoint(CGContextRef context) {
NOISY_RETURN_IF_NULL(context, StubReturn());
UNIMPLEMENTED();
return StubReturn();
NOISY_RETURN_IF_NULL(context, CGPointZero);

if (!context->HasPath()) {
return CGPointZero;
}
Copy link
Member

@MSFTFox MSFTFox Oct 31, 2016

Choose a reason for hiding this comment

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

nit: redundant. CGPath handles this. #Closed

Copy link
Author

@DHowett-MSFT DHowett-MSFT Oct 31, 2016

Choose a reason for hiding this comment

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

This is not redundant, as ->Path() will create a useless path otherwise. #ByDesign

Copy link
Member

Choose a reason for hiding this comment

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

Yup, forgot about that.


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


return CGPathGetCurrentPoint(context->Path());
}

/**
@Status Stub
@Notes
*/
bool CGContextPathContainsPoint(CGContextRef context, CGPoint point, CGPathDrawingMode mode) {
NOISY_RETURN_IF_NULL(context, StubReturn());
UNIMPLEMENTED();
return StubReturn();
NOISY_RETURN_IF_NULL(context, false);

return context->HasPath() && CGPathContainsPoint(context->Path(), nullptr, point, (mode & kCGPathEOFill));
}
#pragma endregion

Expand Down Expand Up @@ -1329,6 +1391,7 @@ static void __CGContextDrawGeometry(CGContextRef context, ID2D1Geometry* geometr

/**
@Status Interoperable
@Notes The current path is cleared as a side effect of this function.
*/
void CGContextStrokeRect(CGContextRef context, CGRect rect) {
NOISY_RETURN_IF_NULL(context);
Expand All @@ -1346,6 +1409,7 @@ void CGContextStrokeRect(CGContextRef context, CGRect rect) {

/**
@Status Interoperable
@Notes The current path is cleared as a side effect of this function.
*/
void CGContextStrokeRectWithWidth(CGContextRef context, CGRect rect, CGFloat width) {
NOISY_RETURN_IF_NULL(context);
Expand All @@ -1357,6 +1421,7 @@ void CGContextStrokeRectWithWidth(CGContextRef context, CGRect rect, CGFloat wid

/**
@Status Interoperable
@Notes The current path is cleared as a side effect of this function.
*/
void CGContextFillRect(CGContextRef context, CGRect rect) {
NOISY_RETURN_IF_NULL(context);
Expand All @@ -1374,6 +1439,7 @@ void CGContextFillRect(CGContextRef context, CGRect rect) {

/**
@Status Interoperable
@Notes The current path is cleared as a side effect of this function.
*/
void CGContextStrokeEllipseInRect(CGContextRef context, CGRect rect) {
NOISY_RETURN_IF_NULL(context);
Expand All @@ -1393,6 +1459,7 @@ void CGContextStrokeEllipseInRect(CGContextRef context, CGRect rect) {

/**
@Status Interoperable
@Notes The current path is cleared as a side effect of this function.
*/
void CGContextFillEllipseInRect(CGContextRef context, CGRect rect) {
NOISY_RETURN_IF_NULL(context);
Expand All @@ -1412,6 +1479,7 @@ void CGContextFillEllipseInRect(CGContextRef context, CGRect rect) {

/**
@Status Interoperable
@Notes The current path is cleared as a side effect of this function.
*/
void CGContextStrokeLineSegments(CGContextRef context, const CGPoint* points, unsigned count) {
NOISY_RETURN_IF_NULL(context);
Expand All @@ -1432,6 +1500,7 @@ void CGContextStrokeLineSegments(CGContextRef context, const CGPoint* points, un

/**
@Status Interoperable
@Notes The current path is cleared as a side effect of this function.
*/
void CGContextFillRects(CGContextRef context, const CGRect* rects, size_t count) {
NOISY_RETURN_IF_NULL(context);
Expand All @@ -1448,34 +1517,39 @@ void CGContextFillRects(CGContextRef context, const CGRect* rects, size_t count)
#pragma region Drawing Operations - Paths
/**
@Status Interoperable
@Notes The current path is cleared as a side effect of this function.
*/
void CGContextDrawPath(CGContextRef context, CGPathDrawingMode mode) {
NOISY_RETURN_IF_NULL(context);
UNIMPLEMENTED();
context->ClearPath();
}

/**
@Status Interoperable
@Notes The current path is cleared as a side effect of this function.
*/
void CGContextStrokePath(CGContextRef context) {
NOISY_RETURN_IF_NULL(context);
CGContextDrawPath(context, kCGPathStroke);
CGContextDrawPath(context, kCGPathStroke); // Clears path.
}

/**
@Status Interoperable
@Notes The current path is cleared as a side effect of this function.
*/
void CGContextFillPath(CGContextRef context) {
NOISY_RETURN_IF_NULL(context);
CGContextDrawPath(context, kCGPathFill);
CGContextDrawPath(context, kCGPathFill); // Clears path.
}

/**
@Status Interoperable
@Notes The current path is cleared as a side effect of this function.
*/
void CGContextEOFillPath(CGContextRef context) {
NOISY_RETURN_IF_NULL(context);
CGContextDrawPath(context, kCGPathEOFill);
CGContextDrawPath(context, kCGPathEOFill); // Clears path.
}
#pragma endregion

Expand Down