3
25
Fork
You've already forked fpdf
10

Appy rtl #41

Open
hmmftg wants to merge 8 commits from hmmftg/appy-rtl into main
pull from: hmmftg/appy-rtl
merge into: go-pdf:main
go-pdf:main
hmmftg commented 2023年05月27日 11:22:07 +02:00 (Migrated from github.com)
Copy link
  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
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 commented 2023年05月27日 14:23:53 +02:00 (Migrated from github.com)
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.

## [Codecov](https://app.codecov.io/gh/go-pdf/fpdf/pull/41?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-pdf) Report Patch coverage: **`91.66`**% and project coverage change: **`+0.41`** :tada: > Comparison is base [(`1f4411a`)](https://app.codecov.io/gh/go-pdf/fpdf/commit/1f4411a8f4e37e0e9f875621026ac30c1a6d507d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-pdf) 79.40% compared to head [(`bfc3a7b`)](https://app.codecov.io/gh/go-pdf/fpdf/pull/41?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-pdf) 79.81%. :exclamation: 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](https://github.com/apps/codecov) for your organization. [Read more](https://about.codecov.io/blog/codecov-is-updating-its-github-integration/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-pdf). <details><summary>Additional details and impacted files</summary> ```diff @@ 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](https://app.codecov.io/gh/go-pdf/fpdf/pull/41?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-pdf) | Coverage Δ | | |---|---|---| | [font.go](https://app.codecov.io/gh/go-pdf/fpdf/pull/41?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-pdf#diff-Zm9udC5nbw==) | `51.41% <0.00%> (-0.15%)` | :arrow_down: | | [fpdf.go](https://app.codecov.io/gh/go-pdf/fpdf/pull/41?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-pdf#diff-ZnBkZi5nbw==) | `82.94% <85.71%> (+0.82%)` | :arrow_up: | | [rtl.go](https://app.codecov.io/gh/go-pdf/fpdf/pull/41?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-pdf#diff-cnRsLmdv) | `100.00% <100.00%> (ø)` | | | [util.go](https://app.codecov.io/gh/go-pdf/fpdf/pull/41?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-pdf#diff-dXRpbC5nbw==) | `71.86% <100.00%> (-2.14%)` | :arrow_down: | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/go-pdf/fpdf/pull/41/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-pdf) </details> [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/go-pdf/fpdf/pull/41?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-pdf). :loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-pdf).
sbinet (Migrated from github.com) requested changes 2023年06月05日 12:03:43 +02:00
sbinet (Migrated from github.com) left a comment
Copy link

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.

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: - https://github.com/go-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.
sbinet (Migrated from github.com) commented 2023年06月05日 11:45:23 +02:00
Copy link

please remove

please remove
@ -2426,6 +2428,11 @@ func (f *Fpdf) Bookmark(txtStr string, level int, y float64) {
f.outlines=append(f.outlines,outlineType{text:txtStr,level:level,y:y,p:f.PageNo(),prev:-1,last:-1,next:-1,first:-1})
sbinet (Migrated from github.com) commented 2023年06月05日 11:46:04 +02:00
Copy link

please document this new function.

also, do we really need to export it ?

please document this new function. also, do we really need to export it ?
sbinet (Migrated from github.com) commented 2023年06月05日 11:46:14 +02:00
Copy link

please remove.

please remove.
sbinet (Migrated from github.com) commented 2023年06月05日 11:47:29 +02:00
Copy link

please keep this section.

please keep this section.
sbinet (Migrated from github.com) commented 2023年06月05日 11:49:18 +02:00
Copy link

please document this new function.

also: do we need to export it ?
also: acronyms in Go are upper case. (ie: it should read isRTL)

please document this new function. also: do we need to export it ? also: acronyms in Go are upper case. (_ie:_ it should read `isRTL`)
@ -0,0 +13,4 @@
*ACTIONOFCONTRACT,NEGLIGENCEOROTHERTORTIOUSACTION,ARISINGOUTOF
*ORINCONNECTIONWITHTHEUSEORPERFORMANCEOFTHISSOFTWARE.
*/
packagefpdf
sbinet (Migrated from github.com) commented 2023年06月05日 11:47:58 +02:00
Copy link

please add the needed LICENSE blurb.

please add the needed LICENSE blurb.
sbinet (Migrated from github.com) commented 2023年06月05日 11:50:55 +02:00
Copy link

that could be simplified to:

bomState:=unicode.IgnoreBOMifbom{bomState=unicode.UseBOM}
that could be simplified to: ```go bomState := unicode.IgnoreBOM if bom { bomState = unicode.UseBOM } ```
sbinet (Migrated from github.com) commented 2023年06月05日 11:51:16 +02:00
Copy link

please remove the commented out code.

please remove the commented out code.
hmmftg commented 2023年06月05日 15:42:07 +02:00 (Migrated from github.com)
Copy link

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.

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 commented 2023年06月05日 15:57:06 +02:00 (Migrated from github.com)
Copy link

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)

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)
sbinet commented 2023年06月13日 08:55:42 +02:00 (Migrated from github.com)
Copy link

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

I haven't forgotten about this. (but I won't be able to properly review it before Thursday. sorry about that)
sbinet (Migrated from github.com) reviewed 2023年06月21日 09:37:42 +02:00
sbinet (Migrated from github.com) commented 2023年06月21日 09:28:46 +02:00
Copy link

please remove this file and its .vscode parent directory.

please remove this file and its `.vscode` parent directory.
sbinet (Migrated from github.com) commented 2023年06月21日 09:29:08 +02:00
Copy link
				// TODO(hmmftg): check why different presentations of the same arabic character have equal widths

but I must say I am not sure to understand what the comment is supposed to convey :)

```suggestion // TODO(hmmftg): check why different presentations of the same arabic character have equal widths ``` but I must say I am not sure to understand what the comment is supposed to convey :)
@ -2438,3 +2443,2 @@
x-=f.GetStringWidth(txtStr)
}
x-=f.GetStringWidth(txtStr)
txt2=f.escape(utf8toutf16(txtStr,false))
sbinet (Migrated from github.com) commented 2023年06月21日 09:33:40 +02:00
Copy link

instead of hard-coding this to use goarabic.FixArabic, couldn't we rather let users register their own RTL function ?

this would also fix my previous comment from https://github.com/go-pdf/fpdf/pull/41#issuecomment-1576857551
(even if we would still have to fix the API discrepancy)

WDYT ?

instead of hard-coding this to use `goarabic.FixArabic`, couldn't we rather let users register their own RTL function ? this would also fix my previous comment from https://github.com/go-pdf/fpdf/pull/41#issuecomment-1576857551 (even if we would still have to fix the API discrepancy) WDYT ?
sbinet (Migrated from github.com) commented 2023年06月21日 09:34:30 +02:00
Copy link
	f.isRTL = isRTL(txtStr)
```suggestion f.isRTL = isRTL(txtStr) ```
hmmftg (Migrated from github.com) reviewed 2023年06月26日 07:43:32 +02:00
@ -2438,3 +2443,2 @@
x-=f.GetStringWidth(txtStr)
}
x-=f.GetStringWidth(txtStr)
txt2=f.escape(utf8toutf16(txtStr,false))
hmmftg (Migrated from github.com) commented 2023年06月26日 07:43:32 +02:00
Copy link

Ok, know I get what do you mean! we elastically let other each user with each rtl mechanisem choose it's own way

Ok, know I get what do you mean! we elastically let other each user with each rtl mechanisem choose it's own way
hmmftg (Migrated from github.com) reviewed 2023年06月26日 10:03:36 +02:00
hmmftg (Migrated from github.com) commented 2023年06月26日 10:03:36 +02:00
Copy link

it's been fixed and comment was forgotten sorry

it's been fixed and comment was forgotten sorry
sbinet commented 2023年09月13日 14:32:01 +02:00 (Migrated from github.com)
Copy link

ping ?

unless I am mistaken, I don't think my comment in https://github.com/go-pdf/fpdf/pull/41#discussion_r1236508693 has been addressed.

_ping ?_ unless I am mistaken, I don't think my comment in https://github.com/go-pdf/fpdf/pull/41#discussion_r1236508693 has been addressed.
This pull request has changes conflicting with the target branch.
  • fpdf.go
  • go.mod
  • go.sum
  • pdf/reference/Fpdf_AddUTF8Font.pdf
View command line instructions

Manual merge helper

Use this merge commit message when completing the merge manually.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin hmmftg/appy-rtl:hmmftg/appy-rtl
git switch hmmftg/appy-rtl

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch main
git merge --no-ff hmmftg/appy-rtl
git switch hmmftg/appy-rtl
git rebase main
git switch main
git merge --ff-only hmmftg/appy-rtl
git switch hmmftg/appy-rtl
git rebase main
git switch main
git merge --no-ff hmmftg/appy-rtl
git switch main
git merge --squash hmmftg/appy-rtl
git switch main
git merge --ff-only hmmftg/appy-rtl
git switch main
git merge hmmftg/appy-rtl
git push origin main
Sign in to join this conversation.
No reviewers
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
go-pdf/fpdf!41
Reference in a new issue
go-pdf/fpdf
No description provided.
Delete branch "hmmftg/appy-rtl"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?