-
Notifications
You must be signed in to change notification settings - Fork 806
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
Conversation
*Note: The NSAttribute changes will be part of the CTLine code review #ByDesign |
Frameworks/CoreText/CTFrame.mm
Outdated
@@ -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]); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
Frameworks/CoreText/CTRun.mm
Outdated
// 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)), |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Frameworks/CoreText/CTRun.mm
Outdated
_glyphAdvances(other->_glyphAdvances), | ||
_relativeXOffset(other->_relativeXOffset), | ||
_relativeYOffset(other->_relativeYOffset), | ||
_stringFragment(CFStringCreateCopy(kCFAllocatorDefault, other->_stringFragment)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ut supra #Resolved
Frameworks/CoreText/CTRun.mm
Outdated
*/ | ||
const CGGlyph* CTRunGetGlyphsPtr(CTRunRef run) { | ||
_CTRun* runPtr = static_cast<_CTRun*>(run); | ||
return (runPtr && runPtr->_dwriteGlyphRun.glyphCount) ? runPtr->_dwriteGlyphRun.glyphIndices : nullptr; | ||
RETURN_NULL_IF(!run); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | ||
|
||
/** | ||
@Status NotInPlan | ||
@Notes this would require us to move to using bridged type implementation, seems of little value at this point |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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() { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
include/CoreText/CTRun.h
Outdated
@@ -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(); |
There was a problem hiding this comment.
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
Frameworks/CoreText/CTFrame.mm
Outdated
@@ -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]); |
There was a problem hiding this comment.
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
Frameworks/CoreText/CTLine.mm
Outdated
@@ -33,7 +33,7 @@ | |||
@implementation _CTLine : NSObject | |||
- (instancetype)init { | |||
if (self = [super init]) { | |||
_runs.attach([NSMutableArray new]); | |||
_runs = CFArrayCreateMutable(kCFAllocatorDefault, 0, &kCFTypeArrayCallBacks); |
There was a problem hiding this comment.
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
Frameworks/CoreText/CTLine.mm
Outdated
@@ -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); |
There was a problem hiding this comment.
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))); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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() { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
Frameworks/CoreText/CTRun.mm
Outdated
// 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)), |
There was a problem hiding this comment.
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
Frameworks/CoreText/CTRun.mm
Outdated
_glyphAdvances(other->_glyphAdvances), | ||
_relativeXOffset(other->_relativeXOffset), | ||
_relativeYOffset(other->_relativeYOffset), | ||
_stringFragment(CFStringCreateCopy(kCFAllocatorDefault, other->_stringFragment)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above #Resolved
Frameworks/CoreText/CTRun.mm
Outdated
*/ | ||
CTRunStatus CTRunGetStatus(CTRunRef runRef) { | ||
RETURN_RESULT_IF_NULL(runRef, kCTRunStatusNoStatus); | ||
|
||
__CTRun* run = const_cast<__CTRun*>(runRef); |
There was a problem hiding this comment.
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()); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
@ms-jihua @DHowett-MSFT @aballway knock knock |
Frameworks/CoreText/CTLine.mm
Outdated
@@ -33,7 +33,7 @@ | |||
@implementation _CTLine : NSObject | |||
- (instancetype)init { | |||
if (self = [super init]) { | |||
_runs.attach([NSMutableArray new]); | |||
_runs = woc::MakeStrongCF(CFArrayCreateMutable(kCFAllocatorDefault, 0, &kCFTypeArrayCallBacks)); |
There was a problem hiding this comment.
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
Frameworks/CoreText/CTLine.mm
Outdated
@@ -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)); |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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
Frameworks/CoreText/CTLine.mm
Outdated
if (!line || [line->_runs count] == 0) { | ||
return 0; | ||
} | ||
RETURN_RESULT_IF((!line || CFArrayGetCount(line->_runs) == 0), 0); |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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()); |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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
_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))); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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()) |
There was a problem hiding this comment.
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
1dfd611
to
829f9ad
Compare
fixes #2356