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

PINK: support hebrew version #2236

Merged
merged 14 commits into from May 12, 2020
Merged

Conversation

@BLooperZ
Copy link
Contributor

BLooperZ commented May 9, 2020

Based on #2217
Changes include:

  • BiDi text in macmenu and mactext
  • RTL layout for macmenu (which is activated when running the hebrew version)
  • Accelerator underline position
  • Fix encoding for text in pages

I have also created a new TextRenderer as suggested in the opening comment of font.h.
this was made to allow to only think about the logical string and let the renderer abstract the conversion to visual text (so no need to track if currently working with logical string or visual, which I found very confusing).

Copy link
Member

sev- left a comment

Looks like a step in the right direction, but I do not understand couple of API changes.

namespace TextRenderer {

void drawString(Surface *dst, const Graphics::Font &font, const Common::String &str, const Common::CodePage page, int x, int y, int w, uint32 color, TextAlign align, int deltax, bool useEllipsis) {
font.drawString(dst, convertBiDiString(str, page), x, y, w, color, align, deltax, useEllipsis);

This comment has been minimized.

Copy link
@sev-

sev- May 9, 2020

Member

I do not understand the need for these methods, and they're also poorly named. It makes much more thing to put convertBiDiString() right in place where it is called.

This comment has been minimized.

Copy link
@BLooperZ

BLooperZ May 9, 2020

Author Contributor

I have found it hard to track if I'm using the string in logical order (before conversion) or visual order (after conversion) so I needed abstraction, and moving it directly to Font::drawString might introduce unexpected effects... I think this "high-level draw bidi string method" might help using bidi in other places (only have to think about logical order as the drawing abstract it away)... please tell me if you are still not convinced, and I will revert this API change.

* For LTR (Left To Right) languages, returns the original input
* For RTL (Right To Left) languages, returns visual representation of a logical single-line input
*/
U32String convertBiDiU32String(const U32String &input);

This comment has been minimized.

Copy link
@sev-

sev- May 9, 2020

Member

No, I am with @criezy here, please move these to str.cpp and ustr.cpp respectively.

This comment has been minimized.

Copy link
@BLooperZ

BLooperZ May 9, 2020

Author Contributor

Ok, I will change it on #2217 and rebase

@BLooperZ BLooperZ force-pushed the BLooperZ:pink_bidi_macgui_flipped branch from caa907a to b70513f May 10, 2020
@BLooperZ BLooperZ requested a review from sev- May 10, 2020
@BLooperZ BLooperZ force-pushed the BLooperZ:pink_bidi_macgui_flipped branch from 129d0bb to b70513f May 10, 2020
@BLooperZ
Copy link
Contributor Author

BLooperZ commented May 10, 2020

Introduced Common::UnicodeBiDiText which helps
making it clear if using visual or logical string order,
and to be able to use additional info from FriBiDi conversion
(e.g. logical to visual position, which is used in underlineAccelerator)

If the API is ok, #2217 can be closed in favour of this change.

@BLooperZ BLooperZ force-pushed the BLooperZ:pink_bidi_macgui_flipped branch 3 times, most recently from efab864 to b7baf62 May 10, 2020
@BLooperZ BLooperZ force-pushed the BLooperZ:pink_bidi_macgui_flipped branch from b7baf62 to 07a74cb May 11, 2020
@sev-
Copy link
Member

sev- commented May 12, 2020

Thank you, now it is a good solution. Merging.

@sev- sev- merged commit ec83715 into scummvm:master May 12, 2020
2 checks passed
2 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
ZvikaZ added a commit to ZvikaZ/scummvm that referenced this pull request May 12, 2020
Continuing the work at scummvm#2236,
which moved `convertBiDiString(..page)` to unicode-bidi.h,
now moving to there also the `(..lang)` flavour.

Thus, translation.h has only the SVM-GUI related function, and the
two util functions page+code are in unicode-bidi.
ZvikaZ added a commit to ZvikaZ/scummvm that referenced this pull request May 19, 2020
Continuing the work at scummvm#2236,
which moved `convertBiDiString(..page)` to unicode-bidi.h,
now moving to there also the `(..lang)` flavour.

Thus, translation.h has only the SVM-GUI related function, and the
two util functions page+code are in unicode-bidi.
sev- added a commit that referenced this pull request May 19, 2020
Continuing the work at #2236,
which moved `convertBiDiString(..page)` to unicode-bidi.h,
now moving to there also the `(..lang)` flavour.

Thus, translation.h has only the SVM-GUI related function, and the
two util functions page+code are in unicode-bidi.
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

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