Skip to content
This repository was archived by the owner on Mar 4, 2025. It is now read-only.

Conversation

@hmmftg
Copy link

@hmmftg hmmftg commented May 27, 2023

  1. Replace manual RTL with automatic rtl detection
  2. Handle arabic presentation form with goarabic library
  3. fix character width calculation problem for special characters with variable lengths in different presentation forms
  4. Write unit tests and add sample arabic fonts

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 91.66% and project coverage change: +0.41 🎉

Comparison is base (1f4411a) 79.40% compared to head (bfc3a7b) 79.81%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #41      +/-   ##
==========================================
+ Coverage   79.40%   79.81%   +0.41%     
==========================================
  Files          32       33       +1     
  Lines        7547     7527      -20     
==========================================
+ Hits         5993     6008      +15     
+ Misses       1228     1200      -28     
+ Partials      326      319       -7     
Impacted Files Coverage Δ
font.go 51.41% <0.00%> (-0.15%) ⬇️
fpdf.go 82.94% <85.71%> (+0.82%) ⬆️
rtl.go 100.00% <100.00%> (ø)
util.go 71.86% <100.00%> (-2.14%) ⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@sbinet sbinet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the PR (and apologies for the belated answer).

I have a couple of comments, see below, but also one here:
instead of adding the fonts that exercize RTL text in this repository, could we instead add them in a dedicated repository ?

see, e.g., what I did for other fonts:

(and, what are the LICENSE requirements of that new font ?)

finally: your PR makes Fpdf automatically detect whether a piece of text is RTL (or LTR).
this introduces inconsistency with the RTL and LTR public methods of Fpdf.
how would you think we should resolve this ?

thanks again.

@hmmftg
Copy link
Author

hmmftg commented Jun 5, 2023

Hi, thanks for your attention
I have to replace those 3rd party fonts with globally free fonts with nearly full unicode coverage I think, ie arial
As we experienced in text editors and tools that support rtl, they act smart about rtl detection and it sounds okay, they check first letter of first word and decide rtl/ltr for the whole paragraph for example if I started my paragraph with an rtl character and may be I have a few ltr words in my paragraph, then I'm writing in an rtl language and there is no doubt, and vice versa. so I think a developer who uses this library should not switch between rtl and ltr for each paragraph, you can test it with any os and any rtl keyboard layout and write something to see behavior of cursor, it comes up with years of usage and feedback and we are using it daily in sms, email, documents and else.
I'll check the comments and fix them by tomorrow.

@sbinet
Copy link
Contributor

sbinet commented Jun 5, 2023

I am definitely not against having something automatic.
what I was pointing at was that if we were to add the automatic LTR/RTL detection based on text content, then the old API would need to be adapted.

also, you hard-code a given reverse-strings function (or, rather, you replaced one hard-coded one with another one).
but (and I don't claim to be an expert there), what about all the other RTL scripts ? (e.g. Adlam, Hanifi Rohingya, Hebrew, Mandaic, Mende Kikakui, N’Ko, Old Hungarian, ...)
are those handled the same ?

shouldn't we instead provide a way for users to provide their own "reverse-strings" function ?
or a map of "script -> reverse-strings" ? (in case each script needs a particular reverse-strings function and there are multiple scripts in a given document)

@hmmftg hmmftg requested a review from sbinet June 6, 2023 13:48
@sbinet
Copy link
Contributor

sbinet commented Jun 13, 2023

I haven't forgotten about this.
(but I won't be able to properly review it before Thursday. sorry about that)

@sbinet
Copy link
Contributor

sbinet commented Sep 13, 2023

ping ?

unless I am mistaken, I don't think my comment in #41 (comment) has been addressed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants