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 CTFramesetter and CTTypeSetter with code improvements #2437

Merged
merged 2 commits into from
Apr 11, 2017

Conversation

msft-Jeyaram
Copy link
Contributor

  • Implement CTFramesetter and CTTypeSetter via Runtimebase
  • Remove member access of CTFramesetter & CTTypeSetter
  • Code improvements

Fixes #2355 and #2358

@@ -17,7 +17,7 @@

#import <CoreText/CoreText.h>
#import <CoreText/CTParagraphStyle.h>

#import <CFCppBase.h>
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.

this can go away #Resolved

if (range.length == 0L) {
range.length = [typesetter->_string length] - range.location;
range.length = CFStringGetLength(CFAttributedStringGetString(attributedString)) - range.location;
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.

why not CFAttributedStringGetLength? Technically, an NSAttributedString subclass can override -length, and with the way you've implemented this, it will not get called. #Resolved

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.

The initial implementation is this, so the bug pretains. get the string out of the attributedString then get the length.
you are correct, needs to be updated.


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


// Call _DWriteWrapper to get _CTLine object list that makes up this frame
_CTTypesetter* typesetter = static_cast<_CTTypesetter*>(framesetter->_typesetter);
CFAttributedStringRef attributedString = __CTTypesetterGetAttributedString(framesetter->Typesetter());
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.

Double underscores are for file-private static functions in the CT convention. #Resolved

return ([frame->_lines count] > 0) ? static_cast<_CTLine*>([frame->_lines firstObject])->_strRange.length : 0;
__CTTypesetter* typesetter = const_cast<__CTTypesetter*>(ts);
_CTFrame* frame =
_DWriteGetFrame(__CTTypesetterGetAttributedString(typesetter),
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.

this one can use the internal function ->AttributedString() #Resolved

__CTTypesetter* typesetter = const_cast<__CTTypesetter*>(ts);
_CTFrame* frame =
_DWriteGetFrame(__CTTypesetterGetAttributedString(typesetter),
CFRangeMake(index, CFStringGetLength(CFAttributedStringGetString(typesetter->AttributedString())) - index),
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.

same as in CTFramesetter #Resolved

CGRectMake(offset, 0, width, FLT_MAX));
return ([frame->_lines count] > 0) ? static_cast<_CTLine*>([frame->_lines firstObject])->_strRange.length : 0;
if ([frame->_lines count] > 0) {
Copy link
Contributor

@rajsesh rajsesh Apr 7, 2017

Choose a reason for hiding this comment

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

[frame->_lines count] [](start = 8, length = 21)

These objc calls can already be changed to CFCalls due to bridging so you dont have to touch this again when you fix CTLines #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.

I've already done them in a separate branch, i'll merge that in when it's done.
Main reason it's not part of this cr is that i would have to add extra castings, which would get removed in the final sweep. so waiting till then.


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

Copy link
Contributor

@aballway aballway left a comment

Choose a reason for hiding this comment

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

I'm gonna miss my ternaries 😿

@msft-Jeyaram msft-Jeyaram changed the title Remove the Foundation dependency from CTFramesetter and CTTypeSetter with code improvements [CoreText] Remove the Foundation dependency from CTFramesetter and CTTypeSetter with code improvements Apr 7, 2017
@DHowett-MSFT
Copy link

nit: can you reword this to be <72 characters

- Removed the dependency on Foundation
- Removed the private member access of CTTypeSetter
- Code improvements
Fixes microsoft#2355
- Removed the dependency on Foundation
- Removed the private member access of CTFramesetter
- Code improvements
Fixes microsoft#2358
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.

7 participants