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

I18N: Always call BiDi algorithm for U32String #2572

Merged
merged 2 commits into from Nov 4, 2020

Conversation

@ZvikaZ
Copy link
Contributor

@ZvikaZ ZvikaZ commented Oct 27, 2020

U32String can contain characters from all languages, without relation
to the GUI's language. Therefore, we need to always call it.

Use case:
In Hebrew SQ3, when using the game's original save/restore dialogs, it's
possible to give save games Hebrew names.
What happens when we then use ScummVM's load dialog?

  • If ScummVM's GUI language is Hebrew, it's calling BiDi algo, and
    the text direction is OK.
  • However, if ScummVM's GUI language is English, before this commit it
    didn't call the BiDi algo, and the text was reversed.
@criezy
Copy link
Member

@criezy criezy commented Oct 30, 2020

The change seems simple and logical.
But maybe we should just remove convertBiDiString from TranslationManager altogether and always use Common:: convertBiDiString?

I am wondering though, what is the performance impact of always doing the conversion?
I guess if this is an issue on some low end platforms, we should just compile ScummVM without fribidi for those anyway as performance would be an issue when using hebrew GUI otherwise anyway.

@ZvikaZ
Copy link
Contributor Author

@ZvikaZ ZvikaZ commented Oct 31, 2020

Removing convertBiDiString from TranslationManager and always using Common:: convertBiDiString makes sense; however, note that TranslationManager::convertBiDiString returns Common::convertBiDiU32String(input).visual. I think that the .visual suffix isn't very intuitive (honestly, I don't even remember why is it there :-) ), and it might be easily forgotten.

Agree, I don't think that the performance penalty is of issue here, as you described it.

@phcoder
Copy link
Contributor

@phcoder phcoder commented Oct 31, 2020

I am wondering though, what is the performance impact of always doing the conversion?
I guess if this is an issue on some low end platforms, we should just compile ScummVM without fribidi for those anyway as performance would be an issue when using hebrew GUI otherwise anyway.

If performance is a concern then the code should first check if the string contains any bidi-relevant characters like strong RTL, weak RTL and so on and if it doesn't, then just pass the string through, ideally in a zero-copy way

@ZvikaZ
Copy link
Contributor Author

@ZvikaZ ZvikaZ commented Nov 3, 2020

So, let me summarize.
We have 2 points here:

  1. Maybe we should remove convertBiDiString from TranslationManager.
    @criezy - what do you think? should we do that? or maybe it's better not because of the .visual suffix I've mentioned above?

  2. Possible performance problem on slower device.
    We have here @phcoder 's suggestion - but I don't think we want to to do it:

  • if it's a "strong" machine, there isn't performance issue to solve
  • if it's a "weak" machine, it'll probably be compiled without FriBiDi support anyway, so again, there isn't performance issue to solve
@criezy
Copy link
Member

@criezy criezy commented Nov 4, 2020

For your first point I would say yes. And from what I can see it is only used in one place anyway (in ThemeEngine.cpp).
If we don't like the .visual suffix we can always add a Common::convertBidiString overload that takes a U32String and returns a U32String in common/unicode-bidi.h. But I don't think the TranslationManager is the right place for such a function.

ZvikaZ added 2 commits Oct 27, 2020
U32String can contain characters from all languages, without relation
to the GUI's language. Therefore, we need to always call it.

Use case:
In Hebrew SQ3, when using the game's original save/restore dialogs, it's
possible to give save games Hebrew names.
What happens when we then use ScummVM's load dialog?
- If ScummVM's GUI language is Hebrew, it's calling BiDI algo, and
  the text direction is OK.
- However, if ScummVM's GUI language is English, before this commit it
  didn't call the BiDI algo, and the text was reversed.
@ZvikaZ ZvikaZ force-pushed the ZvikaZ:z_gui_load_rtl branch from d00e0a6 to 0ebe16b Nov 4, 2020
@ZvikaZ
Copy link
Contributor Author

@ZvikaZ ZvikaZ commented Nov 4, 2020

As you suggested, I've removed it from TranslationManager.
And regarding the .visual - I tried without it, and the Hebrew GUI looks great.
It's indeed not needed.

@criezy
Copy link
Member

@criezy criezy commented Nov 4, 2020

And regarding the .visual - I tried without it, and the Hebrew GUI looks great.
It's indeed not needed.

I am actually surprised about that. But after double checking, there is indeed a U32String constructor that takes a UnicodeBiDiText, so it indeed works, but at the cost of making an additional copy of the string.

@criezy
Copy link
Member

@criezy criezy commented Nov 4, 2020

I will merge this now, but I may change the way we convert from UnicodeBiDiText to U32String afterward.

@criezy criezy merged commit 606eeae into scummvm:master Nov 4, 2020
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deepcode-ci-bot Well done, no issues found!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.