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

Remove Foundation from CTRun #2511

Merged
merged 5 commits into from
Apr 19, 2017

Conversation

msft-Jeyaram
Copy link
Contributor

  • Implement CTRun via runtimebase
  • Implement CTLine's internal holder of CTRun via CoreFoundation
  • General code improvements

fixes #2356

@msft-Jeyaram
Copy link
Contributor Author

msft-Jeyaram commented Apr 13, 2017

*Note: The NSAttribute changes will be part of the CTLine code review #ByDesign

@@ -109,12 +109,12 @@ void CTFrameDraw(CTFrameRef frame, CGContextRef ctx) {
CGPoint relativePosition = frame->_lineOrigins[i];

for (size_t j = 0; j < [line->_runs count]; ++j) {
_CTRun* curRun = [line->_runs objectAtIndex:j];
__CTRun* curRun = static_cast<__CTRun*>([line->_runs objectAtIndex:j]);
Copy link

@DHowett-MSFT DHowett-MSFT Apr 13, 2017

Choose a reason for hiding this comment

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

since you've done the conversion of line->_runs to a CFArray, you can make these two changes to CFArray as well! #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.

It's already changed as part of CTLine, that will be out soon.


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

// struct __CTRun has to be made public via CoreTextInternal.h (Frameworks\include). Therefore __CTRun(const CTRunRef& other)'s
// implementation needs to be moved here.
__CTRun::__CTRun(const CTRunRef& other)
: _attributes(CFDictionaryCreateMutableCopy(kCFAllocatorSystemDefault, CFDictionaryGetCount(other->_attributes), other->_attributes)),
Copy link

@DHowett-MSFT DHowett-MSFT Apr 13, 2017

Choose a reason for hiding this comment

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

do you need to use woc::TakeOwnership here? (is _attributes a StrongCF?) #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I forgot that the bug is now fixed :p


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

_glyphAdvances(other->_glyphAdvances),
_relativeXOffset(other->_relativeXOffset),
_relativeYOffset(other->_relativeYOffset),
_stringFragment(CFStringCreateCopy(kCFAllocatorDefault, other->_stringFragment)) {
Copy link

@DHowett-MSFT DHowett-MSFT Apr 13, 2017

Choose a reason for hiding this comment

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

ut supra #Resolved

*/
const CGGlyph* CTRunGetGlyphsPtr(CTRunRef run) {
_CTRun* runPtr = static_cast<_CTRun*>(run);
return (runPtr && runPtr->_dwriteGlyphRun.glyphCount) ? runPtr->_dwriteGlyphRun.glyphIndices : nullptr;
RETURN_NULL_IF(!run);
Copy link

@DHowett-MSFT DHowett-MSFT Apr 13, 2017

Choose a reason for hiding this comment

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

you can fold these into one RETURN_NULL_IF(!x || !y) #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can do.


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

}

/**
@Status NotInPlan
@Notes this would require us to move to using bridged type implementation, seems of little value at this point
Copy link

@DHowett-MSFT DHowett-MSFT Apr 13, 2017

Choose a reason for hiding this comment

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

lol
#ByDesign

@@ -466,15 +466,19 @@ static CTFrameRef _DWriteGetFrame(CFAttributedStringRef string, CFRange range, C
// Iterate through runs on the current line
for (j = i; (j < numOfGlyphRuns) && (glyphRunDetails._baselineOriginY[j] == baselineOriginYForCurrentLine); ++j) {
// Create _CTRun objects and make them part of _CTLine
_CTRun* run = [[_CTRun new] autorelease];
__CTRun* run = const_cast<__CTRun*>(_CTRunCreate());
CFAutorelease(run);
Copy link

@DHowett-MSFT DHowett-MSFT Apr 13, 2017

Choose a reason for hiding this comment

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

really really don't prefer CFAutorelease; this is getting put into an array that will own the reference! #Resolved

#pragma region CTRun
struct __CTRun : CoreFoundation::CppBase<__CTRun> {
__CTRun()
: _attributes(CFDictionaryCreateMutable(kCFAllocatorSystemDefault,
Copy link

@DHowett-MSFT DHowett-MSFT Apr 13, 2017

Choose a reason for hiding this comment

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

takeownership? #Resolved

delete[] _dwriteGlyphRun.glyphOffsets;
}

static CFTypeID GetTypeID() {
Copy link

@DHowett-MSFT DHowett-MSFT Apr 13, 2017

Choose a reason for hiding this comment

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

The point of CppBase is that you do not have to think about this and the only reason to override it is to set different parameters.
Are we doing that? #ByDesign

Copy link

@DHowett-MSFT DHowett-MSFT Apr 13, 2017

Choose a reason for hiding this comment

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

Oh, I see. We overrode copy.

Copying is ill-defined for CF, and it doesn't look like anything ever calls it... #Resolved

@@ -50,4 +50,4 @@ CORETEXT_EXPORT CGRect CTRunGetImageBounds(CTRunRef run, CGContextRef context, C
CORETEXT_EXPORT void CTRunDraw(CTRunRef run, CGContextRef context, CFRange range);
CORETEXT_EXPORT CGAffineTransform CTRunGetTextMatrix(CTRunRef run);
CORETEXT_EXPORT CFTypeID CTRunGetTypeID() STUB_METHOD;
CORETEXT_EXPORT CFTypeID CTRunGetTypeID() NOTINPLAN_METHOD;
CORETEXT_EXPORT CFTypeID CTRunGetTypeID();
Copy link

@DHowett-MSFT DHowett-MSFT Apr 13, 2017

Choose a reason for hiding this comment

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

there are two of these? l o l #Resolved

@@ -109,12 +109,12 @@ void CTFrameDraw(CTFrameRef frame, CGContextRef ctx) {
CGPoint relativePosition = frame->_lineOrigins[i];

for (size_t j = 0; j < [line->_runs count]; ++j) {
_CTRun* curRun = [line->_runs objectAtIndex:j];
__CTRun* curRun = static_cast<__CTRun*>([line->_runs objectAtIndex:j]);
Copy link
Contributor

@aballway aballway Apr 13, 2017

Choose a reason for hiding this comment

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

did clang-format do this? #ByDesign

@@ -33,7 +33,7 @@
@implementation _CTLine : NSObject
- (instancetype)init {
if (self = [super init]) {
_runs.attach([NSMutableArray new]);
_runs = CFArrayCreateMutable(kCFAllocatorDefault, 0, &kCFTypeArrayCallBacks);
Copy link
Contributor

@aballway aballway Apr 13, 2017

Choose a reason for hiding this comment

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

This will over-retain #Resolved

@@ -46,7 +46,7 @@ - (instancetype)copyWithZone:(NSZone*)zone {
ret->_descent = _descent;
ret->_leading = _leading;
ret->_glyphCount = _glyphCount;
ret->_runs.attach([_runs copy]);
ret->_runs = CFArrayCreateMutableCopy(kCFAllocatorDefault, 0, _runs);
Copy link
Contributor

@aballway aballway Apr 13, 2017

Choose a reason for hiding this comment

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

same as above #Resolved

_CTRun* curRun = [line->_runs objectAtIndex:i];
CFIndex count = CFArrayGetCount(line->_runs);
for (CFIndex i = 0; i < count; ++i) {
__CTRun* curRun = const_cast<__CTRun*>(static_cast<CTRunRef>(CFArrayGetValueAtIndex(line->_runs, i)));
Copy link
Contributor

@aballway aballway Apr 13, 2017

Choose a reason for hiding this comment

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

nit: just use CTRunRef for these #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.

can't due to the address of usage below.
&curRun->_dwriteGlyphRun


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

ret->_relativeXOffset = _relativeXOffset;
ret->_relativeYOffset = _relativeYOffset;
return ret;
CTRunRef _CTRunCreate() {
Copy link
Contributor

@aballway aballway Apr 13, 2017

Choose a reason for hiding this comment

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

nit: is this necessary? #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.

rather not expose the actual creation in another file. Let's say we decide to change how it's being created in the future :p


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

Copy link
Contributor

@aballway aballway Apr 13, 2017

Choose a reason for hiding this comment

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

I think this is an unnecessary abstraction, especially since we're calling CreateInstance directly for the other CppBase structs #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.

abstractions make life easier for maintainers.
Just because it's already bad, i don't think it should go on :p
I do realize that i introduced one and that will get fixed up.


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

Copy link
Contributor

@aballway aballway Apr 14, 2017

Choose a reason for hiding this comment

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

abstractions don't necessarily make life easier, they just abstract :P
It's not a huge deal, just seems to exist for the sake of existing #ByDesign

// struct __CTRun has to be made public via CoreTextInternal.h (Frameworks\include). Therefore __CTRun(const CTRunRef& other)'s
// implementation needs to be moved here.
__CTRun::__CTRun(const CTRunRef& other)
: _attributes(CFDictionaryCreateMutableCopy(kCFAllocatorSystemDefault, CFDictionaryGetCount(other->_attributes), other->_attributes)),
Copy link
Contributor

@aballway aballway Apr 13, 2017

Choose a reason for hiding this comment

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

does this need to use TakeOwnershipT? #Resolved

_glyphAdvances(other->_glyphAdvances),
_relativeXOffset(other->_relativeXOffset),
_relativeYOffset(other->_relativeYOffset),
_stringFragment(CFStringCreateCopy(kCFAllocatorDefault, other->_stringFragment)) {
Copy link
Contributor

@aballway aballway Apr 13, 2017

Choose a reason for hiding this comment

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

same as above #Resolved

*/
CTRunStatus CTRunGetStatus(CTRunRef runRef) {
RETURN_RESULT_IF_NULL(runRef, kCTRunStatusNoStatus);

__CTRun* run = const_cast<__CTRun*>(runRef);
Copy link
Contributor

@aballway aballway Apr 13, 2017

Choose a reason for hiding this comment

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

nit: no need to cast, can use CTRunRef directly #Resolved

@@ -466,15 +466,19 @@ static CTFrameRef _DWriteGetFrame(CFAttributedStringRef string, CFRange range, C
// Iterate through runs on the current line
for (j = i; (j < numOfGlyphRuns) && (glyphRunDetails._baselineOriginY[j] == baselineOriginYForCurrentLine); ++j) {
// Create _CTRun objects and make them part of _CTLine
_CTRun* run = [[_CTRun new] autorelease];
__CTRun* run = const_cast<__CTRun*>(_CTRunCreate());
Copy link
Contributor

@aballway aballway Apr 13, 2017

Choose a reason for hiding this comment

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

nit: just use CreateInstance and CTRunRef #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.

if you look below, you are accessing members of the struct, if it's CTRunRef, it would end up being a const ref.
can't assign to a const ref :p


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

@interface _CTLine : NSObject {
@public
CFRange _strRange;
CGFloat _relativeXOffset;
CGFloat _width;
NSUInteger _glyphCount;
StrongId<NSMutableArray<_CTRun*>> _runs;
woc::StrongCF<CFMutableArrayRef> _runs;
Copy link
Contributor

@aballway aballway Apr 13, 2017

Choose a reason for hiding this comment

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

nit: this should be part of the CTLineRef fix. #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.

not really, I don't want to do extra work to make the usage of _runs work and just kill it in CTLine :p


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

_textMatrix(CGAffineTransformIdentity) {
}

__CTRun(const CTRunRef& other);
Copy link
Contributor

@ms-jihua ms-jihua Apr 13, 2017

Choose a reason for hiding this comment

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

isn't CTRunRef already const? is this redundant then? #Resolved

Copy link
Contributor

@aballway aballway Apr 13, 2017

Choose a reason for hiding this comment

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

It's also a pointer so no need to take as reference #Resolved

Copy link

@DHowett-MSFT DHowett-MSFT Apr 13, 2017

Choose a reason for hiding this comment

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

yeah to be a copy ctor this must be const __CTRun& #Resolved

@msft-Jeyaram
Copy link
Contributor Author

@ms-jihua @DHowett-MSFT @aballway knock knock

@@ -33,7 +33,7 @@
@implementation _CTLine : NSObject
- (instancetype)init {
if (self = [super init]) {
_runs.attach([NSMutableArray new]);
_runs = woc::MakeStrongCF(CFArrayCreateMutable(kCFAllocatorDefault, 0, &kCFTypeArrayCallBacks));
Copy link

@DHowett-MSFT DHowett-MSFT Apr 13, 2017

Choose a reason for hiding this comment

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

y'could just use .attach :P #Resolved

@@ -46,7 +46,7 @@ - (instancetype)copyWithZone:(NSZone*)zone {
ret->_descent = _descent;
ret->_leading = _leading;
ret->_glyphCount = _glyphCount;
ret->_runs.attach([_runs copy]);
ret->_runs = woc::MakeStrongCF(CFArrayCreateMutableCopy(kCFAllocatorDefault, 0, _runs));
Copy link

@DHowett-MSFT DHowett-MSFT Apr 13, 2017

Choose a reason for hiding this comment

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

ut supra #Resolved

0,
&kCFCopyStringDictionaryKeyCallBacks,
&kCFTypeDictionaryValueCallBacks)),
: _attributes(woc::MakeStrongCF(CFDictionaryCreateMutable(kCFAllocatorSystemDefault,
Copy link

@DHowett-MSFT DHowett-MSFT Apr 13, 2017

Choose a reason for hiding this comment

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

It is slightly more concise and involves fewer temporary objects to simply use woc::TakeOwnership #Resolved

if (!line || [line->_runs count] == 0) {
return 0;
}
RETURN_RESULT_IF((!line || CFArrayGetCount(line->_runs) == 0), 0);
Copy link
Contributor

@ms-jihua ms-jihua Apr 13, 2017

Choose a reason for hiding this comment

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

nit: parenthesize the two sides of || rather than, or in addition to, both in one
as is, my brain keeps wanting to read it as
( (!line || CFArrayGetCount(line->_runs)) == 0 ) #Resolved

@@ -49,5 +49,4 @@ CORETEXT_EXPORT double CTRunGetTypographicBounds(CTRunRef run, CFRange range, CG
CORETEXT_EXPORT CGRect CTRunGetImageBounds(CTRunRef run, CGContextRef context, CFRange range);
CORETEXT_EXPORT void CTRunDraw(CTRunRef run, CGContextRef context, CFRange range);
CORETEXT_EXPORT CGAffineTransform CTRunGetTextMatrix(CTRunRef run);
CORETEXT_EXPORT CFTypeID CTRunGetTypeID() STUB_METHOD;
Copy link
Contributor

@ms-jihua ms-jihua Apr 13, 2017

Choose a reason for hiding this comment

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

we had it twice?! #ByDesign

}

/**
@Status Interoperable
*/
CFDictionaryRef CTRunGetAttributes(CTRunRef run) {
return run ? static_cast<CFDictionaryRef>(static_cast<_CTRun*>(run)->_attributes.get()) : nil;
RETURN_NULL_IF(!run);
Copy link
Contributor

@aballway aballway Apr 13, 2017

Choose a reason for hiding this comment

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

you sure you wouldn't prefer a nice clean ternary here? 😉
return run ? run->_attributes : nil; #ByDesign

@@ -466,15 +466,19 @@ static CTFrameRef _DWriteGetFrame(CFAttributedStringRef string, CFRange range, C
// Iterate through runs on the current line
for (j = i; (j < numOfGlyphRuns) && (glyphRunDetails._baselineOriginY[j] == baselineOriginYForCurrentLine); ++j) {
// Create _CTRun objects and make them part of _CTLine
_CTRun* run = [[_CTRun new] autorelease];
auto runRef = woc::MakeStrongCF<CTRunRef>(_CTRunCreate());
Copy link
Contributor

@aballway aballway Apr 14, 2017

Choose a reason for hiding this comment

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

or if you're gonna stick with _CTRunCreate make it return a __CTRun* so you don't need to do this cast #ByDesign

run->_stringFragment = [static_cast<NSString*>(CFAttributedStringGetString(string))
substringWithRange:NSMakeRange(range.location + run->_range.location, run->_range.length)];

run->_stringFragment = CFStringCreateWithSubstring(nullptr,
Copy link
Contributor

@aballway aballway Apr 14, 2017

Choose a reason for hiding this comment

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

this will overretain #Resolved

0,
&kCFCopyStringDictionaryKeyCallBacks,
&kCFTypeDictionaryValueCallBacks)),
_textMatrix(CGAffineTransformIdentity) {
Copy link
Contributor

@aballway aballway Apr 14, 2017

Choose a reason for hiding this comment

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

nit: could this just be moved down into where it's declared? #ByDesign

@msft-Jeyaram
Copy link
Contributor Author

@DHowett-MSFT @aballway @ms-jihua ping

_CTRun* curRun = [line->_runs objectAtIndex:j];
CFIndex len = CFArrayGetCount(line->_runs);
for (size_t j = 0; j < len; ++j) {
__CTRun* curRun = const_cast<__CTRun*>(static_cast<CTRunRef>(CFArrayGetValueAtIndex(line->_runs, j)));

Choose a reason for hiding this comment

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

wait, why does this run need to be non-const? we aren't manipulating it, so it's actually better that it's const.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

&curRun->_dwriteGlyphRun
is holding it via a pointer which isn't non-const.
I can downcast it here (GlyphRunData{ &curRun->_dwriteGlyphRun but i'll just be safe and leave it very similar to how it was before.

@@ -102,8 +102,8 @@ CTLineRef CTLineCreateTruncatedLine(CTLineRef sourceLine, double width, CTLineTr
for (int i = 0; i < numberOfRuns; ++i) {
CTRunRef run = static_cast<CTRunRef>(CFArrayGetValueAtIndex(tokenRuns, i));
CFDictionaryRef attribs = CTRunGetAttributes(run);
NSAttributedString* string =
[[NSAttributedString alloc] initWithString:(static_cast<_CTRun*>(run))->_stringFragment attributes:(NSDictionary*)attribs];
NSAttributedString* string = [[NSAttributedString alloc] initWithString:static_cast<NSString*>(run->_stringFragment.get())

Choose a reason for hiding this comment

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

nit: get should not be necessary

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CTRun should be implemented via CFRuntimeBase (not via an NSObject)
5 participants