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 component: ICU 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 Oct 28, 2018
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
@nielsmh nielsmh added this to Wanted features in Release 1.9.0 Oct 31, 2018
@michicc michicc requested a review from andythenorth Nov 17, 2018
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

@LordAro LordAro Nov 25, 2018

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

@LordAro LordAro Nov 25, 2018

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

@michicc michicc Nov 25, 2018

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

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
6 checks passed
Release 1.9.0 automation moved this from Wanted features to Done Dec 8, 2018
@michicc michicc deleted the pr/osx_icu branch Dec 8, 2018
@LordAro LordAro removed this from Done in Release 1.9.0 Jan 23, 2019
@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 OS: MacOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants