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

[CoreText] Remove the Foundation dependency from CTFrame #2427

Merged
merged 4 commits into from
Apr 12, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
131 changes: 60 additions & 71 deletions Frameworks/CoreText/CTFrame.mm
Original file line number Diff line number Diff line change
Expand Up @@ -28,56 +28,45 @@
const CFStringRef kCTFrameClippingPathsAttributeName = CFSTR("kCTFrameClippingPathsAttributeName");
const CFStringRef kCTFramePathClippingPathAttributeName = CFSTR("kCTFramePathClippingPathAttributeName");

@implementation _CTFrame : NSObject
- (instancetype)init {
if ([super init]) {
_lines.attach([NSMutableArray new]);
}

return self;
}

@end

/**
@Status Interoperable
@Notes
*/
CFRange CTFrameGetStringRange(CTFrameRef frame) {
_CTFrame* framePtr = static_cast<_CTFrame*>(frame);
RETURN_RESULT_IF_NULL(frame, CFRangeMake(0, 0));

CFIndex count = 0;
if (framePtr) {
for (_CTLine* line in static_cast<id<NSFastEnumeration>>(framePtr->_lines)) {
count += line->_strRange.length;
}
CFIndex len = CFArrayGetCount(frame->_lines);

for (CFIndex index = 0; index < len; ++index) {
_CTLine* line = static_cast<_CTLine*>(CFArrayGetValueAtIndex(frame->_lines, index));
count += line->_strRange.length;
}

return CFRangeMake(0, count);
}

/**
@Status Interoperable
@Notes
*/
CFRange CTFrameGetVisibleStringRange(CTFrameRef frame) {
_CTFrame* framePtr = static_cast<_CTFrame*>(frame);
RETURN_RESULT_IF_NULL(frame, CFRangeMake(0, 0));

CFIndex count = 0;
if (framePtr) {
for (size_t i = 0; i < framePtr->_lineOrigins.size(); ++i) {
if (framePtr->_lineOrigins[i].y < framePtr->_frameRect.size.height) {
_CTLine* line = static_cast<_CTLine*>([framePtr->_lines objectAtIndex:i]);
count += line->_strRange.length;
}
for (size_t index = 0; index < frame->_lineOrigins.size(); ++index) {
if (frame->_lineOrigins[index].y < frame->_frameRect.size.height) {
_CTLine* line = static_cast<_CTLine*>(CFArrayGetValueAtIndex(frame->_lines, index));
count += line->_strRange.length;
}
}
return CFRangeMake(0, count);
}

/**
@Status Interoperable
@Notes
*/
CGPathRef CTFrameGetPath(CTFrameRef frame) {
return frame ? static_cast<_CTFrame*>(frame)->_path.get() : nil;
RETURN_NULL_IF(!frame);
return frame->_path;
}

/**
Expand All @@ -93,74 +82,74 @@ CFDictionaryRef CTFrameGetFrameAttributes(CTFrameRef frame) {
@Status Interoperable
*/
CFArrayRef CTFrameGetLines(CTFrameRef frame) {
return frame ? static_cast<CFArrayRef>(static_cast<_CTFrame*>(frame)->_lines.get()) : nil;
RETURN_NULL_IF(!frame);
return frame->_lines;
}

/**
@Status Interoperable
*/
void CTFrameGetLineOrigins(CTFrameRef frameRef, CFRange range, CGPoint origins[]) {
_CTFrame* frame = static_cast<_CTFrame*>(frameRef);
if (frame) {
_boundedCopy(range, frame->_lineOrigins.size(), frame->_lineOrigins.data(), origins);
}
void CTFrameGetLineOrigins(CTFrameRef frame, CFRange range, CGPoint origins[]) {
RETURN_IF(!frame);
_boundedCopy(range, frame->_lineOrigins.size(), frame->_lineOrigins.data(), origins);
}

/**
@Status Interoperable
*/
void CTFrameDraw(CTFrameRef frameRef, CGContextRef ctx) {
_CTFrame* frame = static_cast<_CTFrame*>(frameRef);
if (frame && ctx) {
std::vector<GlyphRunData> runs;

for (size_t i = 0; i < frame->_lineOrigins.size() && (frame->_lineOrigins[i].y < frame->_frameRect.size.height); ++i) {
_CTLine* line = static_cast<_CTLine*>([frame->_lines objectAtIndex:i]);
CGPoint relativePosition = frame->_lineOrigins[i];
for (size_t j = 0; j < [line->_runs count]; ++j) {
_CTRun* curRun = [line->_runs objectAtIndex:j];
if (j > 0) {
// Adjusts x position relative to the last run drawn
relativePosition.x += curRun->_relativeXOffset;
}
runs.emplace_back(GlyphRunData{ &curRun->_dwriteGlyphRun, relativePosition, (CFDictionaryRef)curRun->_attributes.get() });
void CTFrameDraw(CTFrameRef frame, CGContextRef ctx) {
RETURN_IF(!ctx);
Copy link
Contributor

@aballway aballway Apr 5, 2017

Choose a reason for hiding this comment

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

I don't think it's necessary to churn these files to add early returns #ByDesign

Copy link
Contributor Author

@msft-Jeyaram msft-Jeyaram Apr 5, 2017

Choose a reason for hiding this comment

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

Code Clarity, it makes much easier to read + follow the standards of other frameworks. #ByDesign

RETURN_IF(!frame);

std::vector<GlyphRunData> runs;

for (size_t i = 0; i < frame->_lineOrigins.size() && (frame->_lineOrigins[i].y < frame->_frameRect.size.height); ++i) {
_CTLine* line = static_cast<_CTLine*>(CFArrayGetValueAtIndex(frame->_lines, i));

CGPoint relativePosition = frame->_lineOrigins[i];

for (size_t j = 0; j < [line->_runs count]; ++j) {
Copy link
Contributor

@ms-jihua ms-jihua Apr 6, 2017

Choose a reason for hiding this comment

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

nit: might want to leave a TODO here as a reminder for when you get to CTLine #ByDesign

Copy link

@DHowett-MSFT DHowett-MSFT Apr 6, 2017

Choose a reason for hiding this comment

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

the compiler will handily totally fall over for this line when that transition is made :) #ByDesign

Copy link
Contributor

@ms-jihua ms-jihua Apr 6, 2017

Choose a reason for hiding this comment

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

o, tru #ByDesign

Copy link
Contributor Author

@msft-Jeyaram msft-Jeyaram Apr 6, 2017

Choose a reason for hiding this comment

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

also when you make it private, to track down all these places. well it really goes off ;) #ByDesign

_CTRun* curRun = [line->_runs objectAtIndex:j];
if (j > 0) {
// Adjusts x position relative to the last run drawn
relativePosition.x += curRun->_relativeXOffset;
}
runs.emplace_back(GlyphRunData{ &curRun->_dwriteGlyphRun, relativePosition, (CFDictionaryRef)curRun->_attributes.get() });
}
}

if (!runs.empty()) {
// Need to invert and translate coordinates so frame draws from top-left
CGRect boundingRect = CGPathGetBoundingBox(frame->_path.get());
CGContextTranslateCTM(ctx, 0, boundingRect.size.height);
if (!runs.empty()) {
// Need to invert and translate coordinates so frame draws from top-left
CGRect boundingRect = CGPathGetBoundingBox(frame->_path);
CGContextTranslateCTM(ctx, 0, boundingRect.size.height);

// Invert Text Matrix and CTM so glyphs are drawn in correct orientation and position
CGAffineTransform textMatrix = CGContextGetTextMatrix(ctx);
CGContextSetTextMatrix(ctx, CGAffineTransformMake(textMatrix.a, -textMatrix.b, textMatrix.c, -textMatrix.d, 0, 0));
CGContextScaleCTM(ctx, 1.0f, -1.0f);
// Invert Text Matrix and CTM so glyphs are drawn in correct orientation and position
CGAffineTransform textMatrix = CGContextGetTextMatrix(ctx);
CGContextSetTextMatrix(ctx, CGAffineTransformMake(textMatrix.a, -textMatrix.b, textMatrix.c, -textMatrix.d, 0, 0));
CGContextScaleCTM(ctx, 1.0f, -1.0f);

_CGContextPushBeginDraw(ctx);
auto popEnd = wil::ScopeExit([&]() {
// Restore CTM and Text Matrix to values before we modified them
CGContextSetTextMatrix(ctx, textMatrix);
CGContextScaleCTM(ctx, 1.0f, -1.0f);
CGContextTranslateCTM(ctx, 0, -boundingRect.size.height);
_CGContextPopEndDraw(ctx);
});
_CGContextPushBeginDraw(ctx);
auto popEnd = wil::ScopeExit([&]() {
// Restore CTM and Text Matrix to values before we modified them
CGContextSetTextMatrix(ctx, textMatrix);
CGContextScaleCTM(ctx, 1.0f, -1.0f);
CGContextTranslateCTM(ctx, 0, -boundingRect.size.height);
_CGContextPopEndDraw(ctx);
});

_CGContextDrawGlyphRuns(ctx, runs.data(), runs.size());
}
_CGContextDrawGlyphRuns(ctx, runs.data(), runs.size());
}
}

/**
@Status Stub
@Notes
@Status Interoperable
Copy link
Contributor

@ms-jihua ms-jihua Apr 6, 2017

Choose a reason for hiding this comment

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

remove the annotation in the .h also #Resolved

*/
CFTypeID CTFrameGetTypeID() {
UNIMPLEMENTED();
return StubReturn();
return __CTFrame::GetTypeID();
}

// Convenience private function for NSString+UIKitAdditions
CGSize _CTFrameGetSize(CTFrameRef frame) {
return frame ? static_cast<_CTFrame*>(frame)->_frameRect.size : CGSize{};
RETURN_RESULT_IF_NULL(frame, CGSize{});
return frame->_frameRect.size;
}
7 changes: 4 additions & 3 deletions Frameworks/CoreText/CTFramesetter.mm
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,9 @@ CTFrameRef CTFramesetterCreateFrame(CTFramesetterRef framesetter, CFRange range,
range.length = CFAttributedStringGetLength(attributedString) - range.location;
}

StrongId<_CTFrame> ret = _DWriteGetFrame(attributedString, range, frameRect);
ret->_path.reset(CGPathRetain(path));
woc::StrongCF<__CTFrame*> ret;
ret = const_cast<__CTFrame*>(_DWriteGetFrame(attributedString, range, frameRect));
ret->_path = path;
ret->_frameRect.origin = frameRect.origin;

// Trying to access attributes without any text will throw an error
Expand Down Expand Up @@ -102,7 +103,7 @@ CTFrameRef CTFramesetterCreateFrame(CTFramesetterRef framesetter, CFRange range,
(lineBreakMode == kCTLineBreakByClipping || lineBreakMode == kCTLineBreakByTruncatingHead ||
lineBreakMode == kCTLineBreakByTruncatingTail || lineBreakMode == kCTLineBreakByTruncatingMiddle)) {
for (size_t i = 0; i < ret->_lineOrigins.size(); ++i) {
_CTLine* line = [ret->_lines objectAtIndex:i];
_CTLine* line = static_cast<_CTLine*>(CFArrayGetValueAtIndex(ret->_lines, i));
if (line->_width > frameRect.size.width) {
ret->_lineOrigins[i].x -= line->_relativeXOffset;
}
Expand Down
18 changes: 10 additions & 8 deletions Frameworks/CoreText/CTTypesetter.mm
Original file line number Diff line number Diff line change
Expand Up @@ -73,19 +73,20 @@ CTLineRef CTTypesetterCreateLine(CTTypesetterRef typesetter, CFRange stringRange
@Status Interoperable
*/
CTLineRef CTTypesetterCreateLineWithOffset(CTTypesetterRef ts, CFRange range, double offset) {
_CTFrame* frame = _DWriteGetFrame(ts->AttributedString(), range, CGRectMake(offset, 0, FLT_MAX, FLT_MAX));

CTFrameRef frame = _DWriteGetFrame(ts->AttributedString(), range, CGRectMake(offset, 0, FLT_MAX, FLT_MAX));
RETURN_NULL_IF(!frame);
if ([frame->_lines count] != 1) {

CFIndex count = CFArrayGetCount(frame->_lines);
if (count != 1) {
TraceError(TAG,
L"CTTypesetterCreateLineWithOffset - range {%f, %f} did not fit on a single line, instead used %u.",
range.location,
range.length,
[frame->_lines count]);
count);
return nullptr;
}

return static_cast<CTLineRef>([[frame->_lines firstObject] retain]);
return static_cast<CTLineRef>(CFRetain(CFArrayGetValueAtIndex(frame->_lines, 0)));
}

/**
Expand All @@ -99,11 +100,12 @@ CFIndex CTTypesetterSuggestLineBreak(CTTypesetterRef typesetter, CFIndex startIn
@Status Interoperable
*/
CFIndex CTTypesetterSuggestLineBreakWithOffset(CTTypesetterRef typesetter, CFIndex index, double width, double offset) {
_CTFrame* frame = _DWriteGetFrame(typesetter->AttributedString(),
CTFrameRef frame = _DWriteGetFrame(typesetter->AttributedString(),
CFRangeMake(index, CFAttributedStringGetLength(typesetter->AttributedString()) - index),
CGRectMake(offset, 0, width, FLT_MAX));
if ([frame->_lines count] > 0) {
return static_cast<_CTLine*>([frame->_lines firstObject])->_strRange.length;

if (CFArrayGetCount(frame->_lines) > 0) {
return static_cast<_CTLine*>(CFArrayGetValueAtIndex(frame->_lines, 0))->_strRange.length;
}

return 0;
Expand Down
2 changes: 1 addition & 1 deletion Frameworks/CoreText/DWriteWrapper_CoreText.h
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ struct _DWriteGlyphRunDetails {
bool _CloneDWriteGlyphRun(_In_ DWRITE_GLYPH_RUN const* src, _Outptr_ DWRITE_GLYPH_RUN* dest);

CGSize _DWriteGetFrameSize(CFAttributedStringRef string, CFRange range, CGSize maxSize, CFRange* fitRange);
_CTFrame* _DWriteGetFrame(CFAttributedStringRef string, CFRange range, CGRect frameSize);
CTFrameRef _DWriteGetFrame(CFAttributedStringRef string, CFRange range, CGRect frameSize);
_CTLine* _DWriteGetLine(CFAttributedStringRef string);

// DWriteWrapper functions relating to CTFont, CTFontDescriptor
Expand Down
31 changes: 16 additions & 15 deletions Frameworks/CoreText/DWriteWrapper_CoreText.mm
Original file line number Diff line number Diff line change
Expand Up @@ -395,9 +395,9 @@ HRESULT STDMETHODCALLTYPE GetPixelsPerDip(_In_opt_ void* clientDrawingContext, _
*/
static _CTLine* _DWriteGetLine(CFAttributedStringRef string) {
CFRange range = CFRangeMake(0, CFAttributedStringGetLength(string));
_CTFrame* frame = _DWriteGetFrame(string, range, CGRectMake(0, 0, FLT_MAX, FLT_MAX));
if ([frame->_lines count] > 0) {
return [[frame->_lines firstObject] retain];
CTFrameRef frame = _DWriteGetFrame(string, range, CGRectMake(0, 0, FLT_MAX, FLT_MAX));
if (CFArrayGetCount(frame->_lines) > 0) {
return [static_cast<_CTLine*>(CFArrayGetValueAtIndex(frame->_lines, 0)) retain];
}

return [_CTLine new];
Expand All @@ -410,15 +410,15 @@ HRESULT STDMETHODCALLTYPE GetPixelsPerDip(_In_opt_ void* clientDrawingContext, _
* @parameter range attributed string range to use.
* @parameter frameSize size parameters of the frame to fit the text into.
*
* @return _CTFrame* created using the given parameters
* @return CTFrameRef created using the given parameters
*/
static _CTFrame* _DWriteGetFrame(CFAttributedStringRef string, CFRange range, CGRect frameSize) {
static CTFrameRef _DWriteGetFrame(CFAttributedStringRef string, CFRange range, CGRect frameSize) {
RETURN_NULL_IF(!string);

_CTFrame* frame = [[_CTFrame new] autorelease];
if (range.length <= 0) {
return frame;
}
__CTFrame* frame = __CTFrame::CreateInstance(kCFAllocatorDefault);
CFAutorelease(frame);
Copy link
Contributor

@aballway aballway Apr 7, 2017

Choose a reason for hiding this comment

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

nit: use AutoCF vs. CFAutorelease #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.

Since it's returning and auto released object, i'd rather not AutoCF it it then detach it and autorelease it.


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

Copy link

@DHowett-MSFT DHowett-MSFT Apr 7, 2017

Choose a reason for hiding this comment

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

I hate that this method returns an "un-owned" CTFrame. It should be a Create method or it should not exist. #Resolved

Copy link
Contributor Author

@msft-Jeyaram msft-Jeyaram Apr 7, 2017

Choose a reason for hiding this comment

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

The way this method does stuff does makes me sad. The frame is created in parts :(
Sadly it's not the scope of this review to fix this up real good.
#ByDesign


RETURN_RESULT_IF(range.length <= 0, frame);

ComPtr<IDWriteTextLayout> textLayout;
RETURN_NULL_IF_FAILED(__DWriteTextLayoutCreate(string, range, frameSize, &textLayout));
Expand Down Expand Up @@ -446,7 +446,8 @@ HRESULT STDMETHODCALLTYPE GetPixelsPerDip(_In_opt_ void* clientDrawingContext, _

while (i < numOfGlyphRuns) {
_CTLine* line = [[_CTLine new] autorelease];
NSMutableArray<_CTRun*>* runs = [NSMutableArray array];

auto runs = woc::MakeStrongCF<CFMutableArrayRef>(CFArrayCreateMutable(kCFAllocatorDefault, 0, &kCFTypeArrayCallBacks));
uint32_t stringRange = 0;
uint32_t glyphCount = 0;
prevXPosForDraw = 0;
Expand Down Expand Up @@ -495,18 +496,18 @@ HRESULT STDMETHODCALLTYPE GetPixelsPerDip(_In_opt_ void* clientDrawingContext, _
line->_width += glyphRunDetails._dwriteGlyphRun[j].glyphAdvances[index];
}

[runs addObject:run];
CFArrayAppendValue(runs, run);
stringRange += run->_range.length;
glyphCount += glyphRunDetails._dwriteGlyphRun[j].glyphCount;
}

// Fast-forward i to start on the next line
i = j;

if ([runs count] > 0) {
if (CFArrayGetCount(runs) > 0) {
prevYPosForDraw = yPos;
line->_runs = runs;
_CTRun* firstRun = static_cast<_CTRun*>(runs[0]);
line->_runs = static_cast<NSMutableArray*>(runs.get());
_CTRun* firstRun = static_cast<_CTRun*>(CFArrayGetValueAtIndex(runs, 0));
line->_strRange.location = firstRun->_range.location;
line->_strRange.length = stringRange;
line->_glyphCount = glyphCount;
Expand All @@ -521,7 +522,7 @@ HRESULT STDMETHODCALLTYPE GetPixelsPerDip(_In_opt_ void* clientDrawingContext, _
lineOrigin = { firstRun->_glyphOrigins[0].x, firstRun->_glyphOrigins[0].y };
}

[frame->_lines addObject:line];
CFArrayAppendValue(frame->_lines, line);
frame->_lineOrigins.emplace_back(lineOrigin);
}
}
Expand Down
18 changes: 12 additions & 6 deletions Frameworks/include/CoreTextInternal.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

#import <CoreText/CoreText.h>
#import <CoreText/CTParagraphStyle.h>
#import <CFCppBase.h>
#import "Starboard.h"
#include <COMIncludes.h>
#import <DWrite.h>
Expand Down Expand Up @@ -73,14 +74,19 @@ CFAttributedStringRef _CTTypesetterGetAttributedString(CTTypesetterRef typesette
}
@end

@interface _CTFrame : NSObject {
@public
#pragma region CTFrame
struct __CTFrame : CoreFoundation::CppBase<__CTFrame> {
__CTFrame() : _lines(CFArrayCreateMutable(kCFAllocatorDefault, 0, &kCFTypeArrayCallBacks)) {
Copy link
Contributor Author

@msft-Jeyaram msft-Jeyaram Apr 5, 2017

Choose a reason for hiding this comment

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

sadly due to the extensive usage of the internal vars outside the actual class, these had to go internal-public 😢 #ByDesign

}

CGRect _frameRect;
woc::unique_cf<CGPathRef> _path;
woc::StrongCF<CGPathRef> _path;
std::vector<CGPoint> _lineOrigins;
StrongId<NSMutableArray<_CTLine*>> _lines;
}
@end
// Hold CTLineRef
woc::StrongCF<CFMutableArrayRef> _lines;
};

#pragma endregion CTFrame

// Private helper methods for UIKit
CORETEXT_EXPORT CGSize _CTFrameGetSize(CTFrameRef frame);
Expand Down
2 changes: 1 addition & 1 deletion include/CoreText/CTFrame.h
Original file line number Diff line number Diff line change
Expand Up @@ -45,4 +45,4 @@ CORETEXT_EXPORT CFDictionaryRef CTFrameGetFrameAttributes(CTFrameRef frame) STUB
CORETEXT_EXPORT CFArrayRef CTFrameGetLines(CTFrameRef frame);
CORETEXT_EXPORT void CTFrameGetLineOrigins(CTFrameRef frame, CFRange range, CGPoint origins[]);
CORETEXT_EXPORT void CTFrameDraw(CTFrameRef frame, CGContextRef context);
CORETEXT_EXPORT CFTypeID CTFrameGetTypeID() STUB_METHOD;
CORETEXT_EXPORT CFTypeID CTFrameGetTypeID();
Loading