Skip to content

Fix for finding the longest word in terms of size instead of characte…#95

Closed
davevdveen wants to merge 1 commit into
TextureGroup:masterfrom
davevdveen:ASTextKitFontSizeAdjuster_fix
Closed

Fix for finding the longest word in terms of size instead of characte…#95
davevdveen wants to merge 1 commit into
TextureGroup:masterfrom
davevdveen:ASTextKitFontSizeAdjuster_fix

Conversation

@davevdveen

Copy link
Copy Markdown

…r count

Issue #94

@CLAassistant

CLAassistant commented May 1, 2017

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@ghost

ghost commented May 1, 2017

Copy link
Copy Markdown
1 Warning
⚠️ Any source code changes should have an entry in CHANGELOG.md or have #trivial in their title.

Generated by 🚫 Danger

@Adlai-Holler Adlai-Holler left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hey thanks for the diff. We've seen thread-safety issues in the past with boundingRectWithSize: despite the claims of the documentation, and the performance penalty here is likely to be tremendous.

One improvement we can make off the bat is to avoid using rangeOfString: and instead use enumerateSubstrings which will give us the ranges directly. Beyond that, I'll try to find time to profile this change in the near future, but of course you're right that this algorithm is correct.

@nguyenhuy

nguyenhuy commented Nov 28, 2017

Copy link
Copy Markdown
Member

@Adlai-Holler @davevdveen Please follow up on this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants