Skip to content

ENH: Add ActualText tag when we produce RTL appearance stream#3901

Open
PJBrs wants to merge 5 commits into
py-pdf:mainfrom
PJBrs:fontwork
Open

ENH: Add ActualText tag when we produce RTL appearance stream#3901
PJBrs wants to merge 5 commits into
py-pdf:mainfrom
PJBrs:fontwork

Conversation

@PJBrs

@PJBrs PJBrs commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Copying text from a PDF copies text in the order that it is encoded in the PDF. For RTL text, this means that the copied text is in reverse. To remedy that problem, this PR tags such text with /Span /ActualText, which overrides text copying to the direction and encoding in which the text was originally added.

I note that #3514 was just closed. Perhaps this PR should be seen as the epilogue to that closure ;-)

This was assisted by Google Gemini, especially the code that produces that Span in AppearanceStream. But lightly!

PJBrs added 3 commits June 25, 2026 17:38
This patch passes both the encoded and unencoded text to the code
that produces the actual appearance stream data.

Also fix silly comment typo
When copying text from a PDF, it will return the characters in the
order that they are encoded. Withe RTL text, this actually is in
reverse. Add /ActualText to enable copying text in the correct
direction.
@codecov

codecov Bot commented Jun 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.87%. Comparing base (efc511b) to head (2f786a6).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3901   +/-   ##
=======================================
  Coverage   97.86%   97.87%           
=======================================
  Files          57       57           
  Lines       10738    10755   +17     
  Branches     2006     2010    +4     
=======================================
+ Hits        10509    10526   +17     
  Misses        127      127           
  Partials      102      102           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@stefan6419846 stefan6419846 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I have left some direct remarks.

Please note that it might take a few weeks for me to do a follow-up review as per the pinned issue.

Comment thread pypdf/generic/_appearance_stream.py Outdated
Comment thread tests/test_appearance_stream.py Outdated
layout=layout, text=text, font_size=font_size, is_multiline=is_multiline
)
assert b"9.8 Tf" in appearance_stream.get_data()
assert b"9.6 Tf" in appearance_stream.get_data()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why does this change? Isn't the change about new dictionary elements only, and not about the content stream operators itself?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I added one line of bidi text to increase test coverage here. Since the test is mainly intended to assert that text scaling works properly, and since I added a line, the font size has decreased as a side effect. Hence this change.

PJBrs added 2 commits June 26, 2026 13:28
This patch introduces and uses a named tuple for the combination of
an unscaled word width, the word itself and the glyphs that encode
that word with the current font.
@PJBrs

PJBrs commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

I have left some direct remarks.

Thanks, I think I addressed all your points.

Please note that it might take a few weeks for me to do a follow-up review as per the pinned issue.

Have a good time!

@PJBrs PJBrs requested a review from stefan6419846 June 26, 2026 11:53
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.

2 participants