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

Feature: [Linux] RTL support with modern versions of ICU #10747

Merged
merged 2 commits into from May 1, 2023

Conversation

TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Apr 30, 2023

Motivation / Problem

With ICU 58+, we no longer have RTL support on Linux. This is becoming a bit annoying, as we can't keep using ICU 50 to build our releases.

Debian, Ubuntu, Flatpak, and many others already have this problem, and their versions do not support RTL. Our official Linux release does, but purely as we use a very old ICU to build against.

So it is about time. Switch to ICU + harfbuzz, meaning we no longer need ICU-lx (which supplied the RTL support).

Fixes #6922. A 5 year old ticket.
Fixes #6666.

Description

This means we have RTL support again with ICU 58+. It makes use of:

  • ICU for bidi-itemization
  • ICU for script-itemization
  • OpenTTD for style-itemization
  • harfbuzz for shaping

image
image
image
image
image

Work-in-progress

So far the Windows (Uniscribe) and Linux (ICU + harfbuzz) look identical for single-line entries.

  • Support multi-line
  • IME input (the char-to-glyph mapping returns nullptr atm)
  • Crashlog showing harfbuzz version
  • Remove ICU from comments where it is no longer ICU-only
  • Split fallback layouter to its own file
  • Support custom OpenTTD icons (like train-icon, etc)

Limitations

Some translations aren't perfect, which means some things look really odd. So often it seems like the shaping goes wrong, but in reality it is the translation who is to blame. So for now, we are comparing Windows (uniscribe) with this PR, and accept it if both render things the same.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR touches english.txt or translations? Check the guidelines
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@TrueBrain TrueBrain force-pushed the linux-harfbuzz branch 2 times, most recently from 8a176ec to e56fd46 Compare April 30, 2023 19:02
src/gfx_layout_icu.cpp Outdated Show resolved Hide resolved
src/gfx_layout_icu.cpp Outdated Show resolved Hide resolved
src/3rdparty/icu/scriptrun.h Fixed Show fixed Hide fixed
src/3rdparty/icu/scriptrun.h Fixed Show fixed Hide fixed
src/3rdparty/icu/scriptrun.h Fixed Show fixed Hide fixed
@TrueBrain TrueBrain force-pushed the linux-harfbuzz branch 4 times, most recently from 0a5f22f to 15a2a6f Compare May 1, 2023 13:04
@TrueBrain TrueBrain marked this pull request as ready for review May 1, 2023 13:04
src/CMakeLists.txt Outdated Show resolved Hide resolved
src/gfx_layout_icu.cpp Outdated Show resolved Hide resolved
This means we have RTL support again with ICU 58+. It makes use of:
- ICU for bidi-itemization
- ICU for script-itemization
- OpenTTD for style-itemization
- harfbuzz for shaping
Copy link
Member

@michicc michicc left a comment

Choose a reason for hiding this comment

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

In the end, nobody here can really verify the layout results, so if it looks alright enough, it is definitely better than nothing at all.

@TrueBrain TrueBrain enabled auto-merge (rebase) May 1, 2023 19:19
@TrueBrain TrueBrain merged commit 81d4fa6 into OpenTTD:master May 1, 2023
19 checks passed
@TrueBrain TrueBrain deleted the linux-harfbuzz branch January 18, 2024 18:44
@matthijskooijman
Copy link
Contributor

Seems this PR got omitted from the changelog, but it is quite a relevant change IMHO. Maybe good to add it retroactively?

@TrueBrain
Copy link
Member Author

In the changelog we tend to only mention things that have an impact on users. In this case, for them nothing changed: RTL worked with 13.0 and with 14.0, with little observable difference.

The only change really is for someone like yourself. And we informed you by closing your ticket about this :D we don't really have a good way to "talk" to distro maintainer in that sense.

So personally I don't see a reason to add this to the changelog itself. And as this was the last outdated package that we depended on, I also hope this is the last time it would have been interesting to distro maintainer :D

@matthijskooijman
Copy link
Contributor

In the changelog we tend to only mention things that have an impact on users. In this case, for them nothing changed: RTL worked with 13.0 and with 14.0, with little observable difference.

Ah, that makes sense. Not super-convenient for me and maybe worth reconsidering in the future (mentioning dependency changes or licensing changes/additions of 3rd party code is probably not problematic for regular users, but would help packagers), but no need to retroactively fix anything then.

And we informed you by closing your ticket about this

Yeah, so I put a cryptic note on my own TODO list, but then was a bit confused when it was not mentioned in the changelog. Now I know why :-)

And as this was the last outdated package that we depended on, I also hope this is the last time it would have been interesting to distro maintainer :D

I don't want to burst your bubble, but I'm pretty sure this is not going to be the last dependency change ;-p

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.

Replace ICU ParagraphLayout with something else? Town name list in Arabic
4 participants