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

Fix #6666: brackets in arabic strings #7480

Merged
merged 1 commit into from Jul 7, 2019
Merged

Fix #6666: brackets in arabic strings #7480

merged 1 commit into from Jul 7, 2019

Conversation

spnda
Copy link
Contributor

@spnda spnda commented Apr 6, 2019

No description provided.

@spnda spnda changed the title Fixed arabic town directory strings (Issue #6666) Fix arabic town directory strings (Fix Issue #6666) Apr 6, 2019
@spnda spnda changed the title Fix arabic town directory strings (Fix Issue #6666) Fix #6666: arabic town directory strings Apr 6, 2019
LordAro
LordAro previously approved these changes Apr 6, 2019
@LordAro LordAro added the backport requested This PR should be backport to current release (RC / stable) label Apr 6, 2019
@PeterN
Copy link
Member

PeterN commented Apr 8, 2019

STR_SORT_BY_ENGINE_ID and STR_STATION_LIST_SELECT_ALL_TYPES both have mismatched parenthesis as well.

@spnda spnda changed the title Fix #6666: arabic town directory strings Fix #6666: brackets in arabic strings Apr 9, 2019
@spnda
Copy link
Contributor Author

spnda commented Apr 9, 2019

I've made myself a list of strings which maybe have this error aswell.
The number infront of each string resembles the line in the file, where the string is located.
Strings which have been found to not be affected are striked out.

  • 285 STR_SORT_BY_ENGINE_ID
  • 603 STR_PERFORMANCE_DETAIL_MIN_PROFIT_TOOLTIP
  • 1063 STR_CONFIG_SETTING_TYPE_DROPDOWN_CLIENT
  • 1064 STR_CONFIG_SETTING_TYPE_DROPDOWN_GAME_MENU
  • 1065 STR_CONFIG_SETTING_TYPE_DROPDOWN_GAME_INGAME
  • 1066 STR_CONFIG_SETTING_TYPE_DROPDOWN_COMPANY_MENU
  • 1067 STR_CONFIG_SETTING_TYPE_DROPDOWN_COMPANY_INGAME
  • 1150 STR_CONFIG_SETTING_ROUGHNESS_OF_TERRAIN
  • 1218 STR_CONFIG_SETTING_DEFAULT_RAIL_TYPE
  • 1944 STR_RAIL_TOOLBAR_TOOLTIP_BUILD_TRAIN_DEPOT_FOR_BUILDING
  • 2025 STR_ROAD_TOOLBAR_TOOLTIP_BUILD_ROAD_VEHICLE_DEPOT
  • 2026 STR_ROAD_TOOLBAR_TOOLTIP_BUILD_TRAM_VEHICLE_DEPOT
  • 2533 STR_NEWGRF_COMPATIBLE_LOADED
  • 2588 STR_TOWN_DIRECTORY_TOWN
  • 2589 STR_TOWN_DIRECTORY_CITY
  • 2604 STR_TOWN_GROWS_EVERY_FOUNDED
  • 2677 STR_STATION_LIST_SELECT_ALL_TYPES
  • 2897 STR_PURCHASE_INFO_REFITTABLE
  • 3195 STR_REFIT_CAPTION
  • 3272 STR_ORDER_CONDITIONAL_REMAINING_LIFETIME
  • 3378 STR_TIMETABLE_NOT_TIMETABLEABLE
  • 3379 STR_TIMETABLE_TRAVEL_NOT_TIMETABLED
  • 3383 STR_TIMETABLE_TRAVEL_FOR_ESTIMATED
  • 3384 STR_TIMETABLE_TRAVEL_FOR_SPEED_ESTIMATED
  • 3391 STR_TIMETABLE_TOTAL_TIME_INCOMPLETE
  • 3417 STR_TIMETABLE_AUTOFILL_TOOLTIP
  • 3541 STR_ERROR_PNGMAP_MISC

@frosch123
Copy link
Member

Instead of {BLACK} there should be one of the RTL-codes, see: strgen_tables.h#L146
https://github.com/OpenTTD/OpenTTD/blob/master/src/table/strgen_tables.h#L146

@spnda
Copy link
Contributor Author

spnda commented Apr 9, 2019

Instead of {BLACK} there should be one of the RTL-codes, see: strgen_tables.h#L146

If I use the following, the problem is still there.

{BLACK}{LRM} .(بناء ورشة صيانة لعربات الطرق (لشراء و صيانة العربات

Using {LRE} changes this, it does actually fix this problem. I will push a commit to change all strings to use {LRE} soon™️. Thanks

@michicc
Copy link
Member

michicc commented Apr 9, 2019

I'm not sure you're really doing the right thing here. LRE marks the whole string as being left-to-right (until a PDF at least), and that doesn't sound right.

Doing a back-and-forth Google Translate round leads me to this bracketing:

-STR_SORT_BY_ENGINE_ID :نوع المحرك (قياسي(
+STR_SORT_BY_ENGINE_ID :نوع المحرك (قياسي)

OpenTTD (on Windows using Uniscribe) displays it properly.

@spnda
Copy link
Contributor Author

spnda commented Apr 9, 2019

Well, we want the whole string to be left to right, otherwise the bracket will jump to the end of the string. Or am I wrong there?

Within VS, the bracket, if you type it at the front of the string, will jump to the back. Probably the same what seems to occur with Google translate.

Tomorrow I sure can test what difference it makes using a different char or ending the left-to-right after the bracket closes (Or if anyone wants to do that now, sure, I am not on my PC anymore).

@michicc
Copy link
Member

michicc commented Apr 9, 2019

Pre-script: My previous comment had missing - and +. The second string line is the line I meant. Please note the direction of the bracket.

If I paste the string into Google Translate, it is rendered as I would expect it to render (with proper brackets). See https://translate.google.com/#view=home&op=translate&sl=ar&tl=en&text=نوع%20المحرك%20(قياسي)

OpenTTD will render it the same as Google, even if the string is displayed differently in the language file. In general, declaring a string that is in a right-to-left language as has having a base direction of left-to-right seems wrong and would probably fail if the string is included in another string or mixed with LTR content. Unfortunately, I can't read arabic, so take anything with a grain of salt. If something looks right to me, a native speaker might still think it a total abomination.

@michicc
Copy link
Member

michicc commented Apr 9, 2019

The link to Google Translate link you sent, renders completely fine, and as expected, on my end.

Yeah, and copying the arabic text and pasting in in Visual Studio into the lang .txt results in the formatting in my previous comment. I.e. Visual Studio (and maybe/likely the WebTranslator as well) display the string differently to how it is display in game or with a proper renderer like in Google Translate. Which of course makes the translator's job harder than it should be.

@spnda
Copy link
Contributor Author

spnda commented Apr 9, 2019

If there's something like {BLACK} inbetween the words, the brackets will always render correctly. If there's a {LRE} infront of all, it still renders correctly. Otherwise the bracket will render at the back of the string. Therefore yes, they do get displayed differently, and maybe WebTranslator should get an update for this.

@michicc
Copy link
Member

michicc commented Apr 10, 2019

The following diff makes the string show properly on Windows/Uniscribe:
fix_arabic_brackets.txt

@michicc
Copy link
Member

michicc commented Apr 10, 2019

Okay, to get the town directory to render properly in OSX, some control codes are required:

STR_TOWN_DIRECTORY_TOWN :{ORANGE}{RLE}{TOWN}{BLACK} {RLM}({COMMA})
STR_TOWN_DIRECTORY_CITY :{ORANGE}{RLE}{TOWN}{YELLOW} (مدينة){BLACK} ({COMMA})

@spnda
Copy link
Contributor Author

spnda commented Apr 10, 2019

Ok, I'd assume that's the same for Linux then, or are there differences there too?
Thanks a lot for all the info!

@michicc
Copy link
Member

michicc commented Apr 10, 2019

@frosch123 had on IRC:

STR_TOWN_DIRECTORY_TOWN :{ORANGE}{TOWN}{BLACK} {RLM}({COMMA})
STR_TOWN_DIRECTORY_CITY :{ORANGE}{TOWN}{YELLOW} (مدينة){BLACK} {RLM}({COMMA})

A {RLM} more for the city string, which doesn't harm on OSX. I'd hope the {RLE} don't harm on Linux , but didn't test this.

Combined result:

STR_TOWN_DIRECTORY_TOWN :{ORANGE}{RLE}{TOWN}{BLACK} {RLM}({COMMA})
STR_TOWN_DIRECTORY_CITY :{ORANGE}{RLE}{TOWN}{YELLOW} (مدينة){BLACK} {RLM}({COMMA})

Additionally, there's probably also a patch needed for ICU.

icu::ParagraphLayout *p = new icu::ParagraphLayout(buff, length, &runs, NULL, NULL, NULL, _current_text_dir == TD_RTL ? UBIDI_DEFAULT_RTL : UBIDI_DEFAULT_LTR, false, status);

From IRC: The _current_text_dir == TD_RTL ? UBIDI_DEFAULT_RTL : UBIDI_DEFAULT_LTR has to become _current_text_dir == TD_RTL ? 1 : 0.

@michicc
Copy link
Member

michicc commented Apr 10, 2019

I pushed two commits to your PR containing the string changes and the suggested change in gfx_layout.cpp. This is not directly for using. You should squash the lang fixes and it conflicts with the latest trunk change regarding NULL and nullptr.

@frosch123
Copy link
Member

frosch123 commented Apr 10, 2019

On linux with icu:
With the current code in gfx_layout.cpp:211 the same {RLE} are required.
Changing gfx_layout.cpp:211 to "? 1: 0" allows to skip the {RLE} in the front.
In reverse: When adding those {RLE}, we do not need to touch gfx_layout.cpp.

@stale
Copy link

stale bot commented May 14, 2019

This pull request has been automatically marked as stale because it has not had any activity in the last month.
Please feel free to give a status update now, ping for review, or re-open when it's ready.
It will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@stale stale bot added the stale Stale issues label May 14, 2019
@michicc michicc merged commit a35b43c into OpenTTD:master Jul 7, 2019
michicc pushed a commit to michicc/OpenTTD that referenced this pull request Jul 7, 2019
@michicc michicc added backported This PR is backported to a current release (RC / stable) and removed backport requested This PR should be backport to current release (RC / stable) labels Jul 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported This PR is backported to a current release (RC / stable) stale Stale issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants