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

Eliminate ICU for OSX #6949

Merged
merged 4 commits into from Dec 8, 2018
Merged

Eliminate ICU for OSX #6949

merged 4 commits into from Dec 8, 2018

Conversation

michicc
Copy link
Member

@michicc michicc commented Oct 28, 2018

This PR provides native OSX implementations for string sorting, caret handling, and text layout. While it is still possible to use ICU for these on OSX, the default build will not depend on it anymore.

@michicc michicc added OS: MacOS This issue is related to a Mac OS problem component: ICU Issue caused by problems with ICU layout labels Oct 28, 2018
src/fontcache.cpp Outdated Show resolved Hide resolved
@michicc michicc force-pushed the pr/osx_icu branch 2 times, most recently from 339aa77 to ab9d28c Compare October 28, 2018 23:42
src/fontcache.cpp Outdated Show resolved Hide resolved
src/gfx_layout.cpp Outdated Show resolved Hide resolved
src/os/macosx/string_osx.cpp Outdated Show resolved Hide resolved
config.lib Outdated Show resolved Hide resolved
LordAro
LordAro previously approved these changes Nov 25, 2018
src/os/macosx/string_osx.cpp Outdated Show resolved Hide resolved
CFRelease(cf1);
CFRelease(cf2);

return (int)res + 2;
Copy link
Member

Choose a reason for hiding this comment

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

given the only usage of this is to just do -2 on the result again, why not make the actual return value consistent with what people expect from a sorting function (-1, 0, 1)? Seems to me like the supported check should be done elsewhere

Copy link
Member

Choose a reason for hiding this comment

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

Oh, OTTDStringCompare does the same. bleh.

Copy link
Member Author

Choose a reason for hiding this comment

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

The supported check is platform-specific, the call site is not. As such, we need a value for failed as well.

By default, the native API will be used instead of ICU, but if ICU is
forced in using configure, it will take precedence.
Copy link
Contributor

@andythenorth andythenorth left a comment

Choose a reason for hiding this comment

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

Tested on macOS 10.13.6. Can't see any issues.

  • tested font display, list sorting and text caret behaviour
  • tested roman (original sprite font), left-to-right

Also tested

  • Arabic (Egypt) right-to-left with a system font
  • Korean, left-to-right with a system font
    No obvious issues, but I don't know what's expected behaviour there.

@michicc michicc merged commit 32ce1ce into OpenTTD:master Dec 8, 2018
@michicc michicc deleted the pr/osx_icu branch December 8, 2018 19:13
@LordAro LordAro added this to the 1.9.0 milestone Jan 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: ICU Issue caused by problems with ICU layout OS: MacOS This issue is related to a Mac OS problem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants