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

add Hebrew support #3420

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

vraja-pro
Copy link

Following the discussion in stack overflow, can you please add this code?

Also opened an issue #3419

Test instruction:
render to pdf the following code:

<div style="font-family:firefly, DejaVu Sans, sans-serif;direction:rtl;text-align:right">בדיקה– 123</div>

With this PR, Check you get in the pdf the following result:

בדיקה- 123

Without this PR the letters would appear in the reversed order.

@bsweeney bsweeney added this to the 3.1.0 milestone Mar 17, 2024
@bsweeney
Copy link
Member

bsweeney commented Mar 17, 2024

I'll review but a better solution would probably be to finalize the work started in #2107. Parsing your sample through the changes proposed in that PR also produce the desired result.

@vraja-pro
Copy link
Author

Hey @bsweeney! Sure, what is there to finalize? Can I help in any way?

@bsweeney
Copy link
Member

It's been a while since I last looked at 2107, so I don't have any definitive guidance at the moment. I need to review the functionality from the ground up to be sure that specific implementation makes the most sense.

That being said there are two things that it needs:

  1. Rebasing on master so that it can be more directly merged after the change is complete.
  2. Multi-frame, multi-line support. I'm pretty sure lines with multiple frames (e.g., text broken up by inline elements like span tags) or text that spans more than one line are not yet rendered correctly.

@vraja-pro
Copy link
Author

I checked out the branch in #2107 and rebased, the solution doesn't seem to work for me.
To be honest, my PR seems more straight forward and it actually does the work. Could be improved by adding a check for Arabic though.

@bsweeney
Copy link
Member

I checked out the branch in #2107 and rebased, the solution doesn't seem to work for me.

That change is working OK for me with your sample. Still, it has a lot of deficiencies and, as I said, more work would be required before I'd consider releasing it.

Since that branch is pretty far behind master, I recreated the changes in the rtl branch.

To be honest, my PR seems more straight forward and it actually does the work. Could be improved by adding a check for Arabic though.

Straight forward, sure, but it's not exactly a robust implementation even discounting that it only supports Hebrew.

A big deficiency is that this change performs the work within the context of a single frame, but a string of text can be made of multiple frames, so a more complex example won't render as expected. Try the following:

<p style="font-family:DejaVu Sans, sans-serif;direction:rtl;">Lorem Ipsum הוא פשוט טקסט <strong>גולמי של תעשיית</strong> ההדפסה וההקלדה.</p>

Additionally, this technique handles one aspect of complex text layout (directionality), but some languages require additional features (e.g., Arabic requires shaping).

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.

None yet

2 participants