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

COMMON: Base BiDi support on U32String #2217

Closed
wants to merge 3 commits into from
Closed

Conversation

@BLooperZ
Copy link
Contributor

BLooperZ commented May 2, 2020

Following #2208
Apply Fribidi convertion on U32String and use ScummVM's charset/unicode conversion rather than Fribidi's.
The BiDi convertion itself now works on U32String which is already encoded in UTF-32, so when using it, it should not require additional parameters (lang) and can be used unconditionally as-is (if conversion is not needed, it should return the original string).

This will simplify RTL support for engines using U32String, such as Director, Pink and Blade Runner. (Maybe some parts of the GUI?)

@BLooperZ BLooperZ force-pushed the BLooperZ:pink_bidi branch from ed88ddc to 914ec3b May 2, 2020
@BLooperZ BLooperZ changed the title Base BiDi support on U32String COMMON: Base BiDi support on U32String May 2, 2020
@lephilousophe
Copy link
Member

lephilousophe commented May 2, 2020

As @criezy proposed yesterday on #2215, shouldn't these functions moved to str.h and ustr.h?
That would make them independent of USE_TRANSLATION and as they are more string manipulation functions than translation things that seems more logical.

@ZvikaZ
Copy link
Contributor

ZvikaZ commented May 2, 2020

I like these changes. And also support moving out of transman, to avoid all these ifdefs.

@BLooperZ
Copy link
Contributor Author

BLooperZ commented May 2, 2020

I think it is good idea to move it out of transman, but I think putting the function on separate files (str.h+ustr.h) might be confusing.
Perhaps should put it in str-enc.h or some new file (e.g. str-bidi.h)?

@BLooperZ BLooperZ force-pushed the BLooperZ:pink_bidi branch from 1598c68 to 914ec3b May 3, 2020
@BLooperZ
Copy link
Contributor Author

BLooperZ commented May 3, 2020

Moved to str-bidi.h.
Methods which already were in transman are left there as stubs for (probably short-term) backwards compatibility.

@BLooperZ BLooperZ force-pushed the BLooperZ:pink_bidi branch from 74780ad to 5073ef4 May 3, 2020
@ZvikaZ
Copy link
Contributor

ZvikaZ commented May 4, 2020

Moved to str-bidi.h.
Methods which already were in transman are left there as stubs for (probably short-term) backwards compatibility.

I think it's better to move everything to the same place. Anyway, there is currently only one place that calls convertBiDiString (in gui/ThemeEngine.cpp), so it should be easy.
And ThemeEngine.cpp would benefit from the removal of #ifdef USE_TRANSLATION.

@ZvikaZ
Copy link
Contributor

ZvikaZ commented May 4, 2020

Moved to str-bidi.h.
Methods which already were in transman are left there as stubs for (probably short-term) backwards compatibility.

I think it's better to move everything to the same place. Anyway, there is currently only one place that calls convertBiDiString (in gui/ThemeEngine.cpp), so it should be easy.
And ThemeEngine.cpp would benefit from the removal of #ifdef USE_TRANSLATION.

On the other hand, maybe it doesn't matter now, and I will do it in my #2215 , after this PR will be accepter.

@sev-
Copy link
Member

sev- commented May 4, 2020

@criezy are you happy with having str-bidi.h or better still stuff it into str.h and ustr.h? I'm personally more inclined towards the latter.

@criezy
Copy link
Member

criezy commented May 4, 2020

I don't think we need to have all the BiDi conversion functions defined in the same place. In my opinion it would make more sense to have convertBiDiString in str.h and convertBiDiU32String in ustr.h. And keeping the one for the GUI in TranslationManager is also fine I think as it has to check the current GUI language (in in theory encoding), which is handled by the TranslationManager instance.

@bgK
Copy link
Member

bgK commented May 5, 2020

IMO the PR is fine as it is, with all the code in str-bidi.h. We should avoid adding more stuff to the String class and prefer utility fonctions so we can simply change them to take a StringView instead of a String when we have one (see https://en.cppreference.com/w/cpp/string/basic_string_view).

@bluegr
Copy link
Member

bluegr commented May 9, 2020

I also agree that the PR is fine as it is

@BLooperZ BLooperZ force-pushed the BLooperZ:pink_bidi branch from 5073ef4 to 0499697 May 9, 2020
@BLooperZ
Copy link
Contributor Author

BLooperZ commented May 9, 2020

As requested here and on #2236 I have moved the conversion to str.h and ustr.h.
But in retrospect I think it wasn't a good change and it was better to use the separate str-bidi.h for number of reason:

  1. str.h is very widely used, so every change to this file require building almost everything again (long build when updating code base).
  2. BiDi support might introduce more changes which should have visible context (for example, getVisualPosition - supporting accelerator underline in macgui needed a way to figure out the visual position of nth character of the original (logical) string)(used in #2236).
  3. it is better to use this convertion functions in specific "key point" throughout the code and working with the logical string as much as possible to avoid confusion (push conversion as close to the rendering itself as we can). putting it in a separate file helps minimizing the use of those methods to the necessary abstractions or search which files uses methods for BiDi handling by watching the #include directive.
  4. BiDi operations on strings are actually stubs using operations on U32Strings and suddenly requires codepage which otherwise String doesn't care about and wasn't needed for other string operations.

IMO it's better to revert this last commit.

@sev-
Copy link
Member

sev- commented May 12, 2020

Closing this in favor of #2238

@sev- sev- closed this May 12, 2020
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

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