MAINT: Converge on one shared Font class for text extraction and appearance streams #3583
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3583 +/- ##
==========================================
+ Coverage 97.30% 97.31% +0.01%
==========================================
Files 56 55 -1
Lines 9838 9770 -68
Branches 1790 1780 -10
==========================================
- Hits 9573 9508 -65
+ Misses 157 155 -2
+ Partials 108 107 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
PJBrs
force-pushed
the
fontwork
branch
8 times, most recently
from
93accb3 to
3be4e39
Compare
PJBrs
force-pushed
the
fontwork
branch
5 times, most recently
from
eeb8357 to
2db3744
Compare
PJBrs
commented
Jan 2, 2026
This patch ports the AppearanceStream to the new Font class. This is in hardly any way different from the original code, except making sure that a default width is set in the character widths for the 14 Adobe core fonts. This is not in fact necessary at this point, but will be when the Font class sets default width itself, and other code begins to depend on that.
This patch ensures that character widths are collected correctly also for fonts that have encoding defined as a string.
Previously, character widths were not computed for type1 and TrueType fonts when encoding was a string. For one test, that is, test_text_extraction_layout_mode in tests/test_workflows.py, this meant that all character widths were treated as one space width. Now, the real widths are used, which changes the output of the test significantly, but in keeping with the intended output. This patch implements the new output. To be sure, I counted the number of newlines and words in both versions, and they are exactly the same, so no spaces were accidentally omitted between words in the new version, nor were they added, since the new version has fewer spaces than the old one.
This patch makes sure that the Font class has a way to compute space_width.
The FontDescriptor code deals with fonts by type. After having dealt with Type1, MMType1, Type3 and TrueType fonts, it is not necessary to check if the remaining fonts are CID or composite fonts, they all are.
This patch ports the layout mode text extraction code to the new font class.
This introduces one test failure, which itself appears to derive from a
misconception about space width in the original Font class.
Previously, a layout mode font was initialized in _page.py as follows:
fonts[font_name] = _layout_mode.Font(*cmap, font_dict)
*cmap, in this case was the return value of build_char_map, which consists of:
- Font sub-type;
- Space_width criteria (50% of width);
- Encoding map;
- Character-map; and
- Font-dictionary
Notice that build_char_map does _not_ return the width of a space, but the
width of _half_ a space. However, if we look at the arguments to the layout
mode Font class, clearly the class expects to be passed the full width of a
space. This is also clear from the word_width method in the layout mode Font
class, which substitutes a missing width with 2 * space_width. It follows
that the layout mode Font class _expected_ to be passed a full space width,
but really was only passed the width of half a space.
When porting to the new Font class, this becomes problematic when calculating
text width, because the new Font class uses self.character_widths["default"]
as a fallback for a missing width, which is approximately (and in many cases
exactly) the width of two spaces. This in turn causes problems with text
extraction in cases where the width of a space itself is missing ("The Crazy
Ones"), and cases where a font with a missing character width is calculated
wider than before.
For the first issue, this patch introduces a work-around that also exists in
the conventional text extraction code, that is, dealing with missing space
width separately.
For the second issue - this causes one test to fail:
the test_layout_mode_text_state in tests/test_text_extraction.py. This is
entirely due to the existence of a unicode private range character in the
file.
…t_mode_text_state
For several reasons, the output of the test_layout_mode_text_state test has changed
significantly with changing to the new Font class. Here's why:
1. The original layout mode Font class set a space width that was actually half a
space wide in reality. In computing word with, a default fallback value was used of
"self.space_width * 2", which in reality was just the width of one space.
2. The new Font class uses "self.character_widths["default"]" as a fallback value
for calculating word width. This value is calculated as follows:
- If a missing width is defined in a Font's font descriptor, set that as default
width
- Else if the width of a space is defined in a Font's character widths and it
is not zero, set the width of two spaces as default width
- Else calculate the average of all character widths and set that as default
width
For the document in test_layout_mode_text_state, this results in very different
default character widths. In the original Font class, it set a space width of
125, and used 250 as a fallback widht. With the new Font class, it reads a value
of 1000 from missing width in a font descriptor.
The document contains one character from a private unicode range, the width of
which is not defined. This character appears a number of times throughout the
document. As a result, this character's width is calculated much wider with the
new code than with the old code. In all other respects, though, the output is the
same. So, the test_layout_mode_text_state's test goal - seeing whether a font
change within a q context is addressed correctly - still holds.
The expected output of this test is stored as a user attachment on github.
Instead of replacing the document, just remove the space characters from the
rendered output and check the result. This makes the test pass while keeping
its intended purpose.
The compute_font_width method is no longer used and therefore obsolete.
This adds a warning to FontDescriptor that replicates a warning originally in the build_font_width_map method in cmap.py. In tests/test_cmap.py, test_function_in_font_widths specificallly tests for this warning. Adding this warning to FontDescriptor for the same problem case, the test keeps fulfilling its purpose, but now for the new Font class.
This patch stops collecting character maps, space widths and encodings to the TextExtractor, keeping only the font resource that is necessary in the TextExtractor class. All the other aspects are now covered with the Font class. Incidentally, this should reduce the number of times that font widths are collected during text extraction, which used to be once for every font resource (for collecting space width) and again during text extraction. Now it is only once, when the fonts are collected in page.py.
After moving the text extraction code to the font class, which collects its own font width map, this code is not needed anymore.
This removes three methods that have become obsolete since porting the non-layout text extraction code to the Font class.
The test for iss1533 was based on the old build_char_map code. Now that that code is removed, port the test to the new Font class, which should cover the underlying issue just the same.
This does not cause a circular import anymore after refactoring.
Contributor
Author
|
@stefan6419846 Thanks for your review! I addressed most of your points. I did make a couple of additional changes. In the Font class, I changed this: The underlying logic is as follows - when a default character width is missing, then you should fall back to default width, not space width. As a rule, assume that default width is roughly the width of two spaces. This causes problems in the layout mode font class, which, as I mentioned earlier, assumed that what was passed as half a space's width actually represented a full space width. That's why the original (wrong) code worked, while the new caused a bit of trouble. So I added the following, which also exists in the non-layout mode code: This fixes all tests, except one, where I think the changed rendering is actually correct. See this commit's message for explanation: 89cc310. In short, there is one important way in which the new layout mode code deviates from the old code, and that's the fallback width in the Font text_width code. This changes rendering in case we encounter a character with unknown width that is not a space. Finally, in the non-layout extraction code, I noticed two things:
I removed all that passing space width around, and added the following in _page.py, to restore the old behaviour: |
Contributor
Author
|
Forget my comment that this is faster. It's just the same as it was. |
This reverts "ENH: TextExtractor: Separate old and new text for width calculation" and embeds the font widths calculation within the _handle_tj() method in _text_extractor.py and in get_display_str() in _text_extraction/__init__.py instead. This way, we get the character widths within the same loop in which we collect the unicode characters, without the need to keep track of old and new text, and having to add or separate these later on. Also, it actually takes so little code that this hardly justified the _get_actual_text_widths that did this before.
Contributor
Author
|
@stefan6419846 Thanks for your additional comments! I think that I have addressed all of them. Please let me know if you have any other comments. |
stefan6419846
approved these changes
Jan 7, 2026
Collaborator
stefan6419846
left a comment
stefan6419846
left a comment
There was a problem hiding this comment.
Thanks for your patience.
michelcrypt4d4mus
pushed a commit
to michelcrypt4d4mus/pdfalyzer
that referenced
this pull request
Jan 15, 2026
…py-pdf/pypdf#3583) with `_cmap.build_char_map()` from
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
EDITED ON 2 JANUARY
This PR introduces one dedicated Font class for all text extraction code and appearance streams. This should be a basis for adding new font resources to appearance streams, in relation to bug #3514
Changes:
This PR got rather big, because, first, I noticed how the original font class did not parse widths for some string encoded fonts, then I noticed that it didn't set a default width either. However, this resulted in rather low coverage. To resolve low coverage I also ported the non-layout mode text extraction code to the new Font class. This was a lot more work, but now coverage is very good, without the need to add more tests! The new font class works fine with the current appearance stream code and both the original and the layout mode text extraction code.
The current version - January 2 - is ready for review. I'm specifically looking for feedback on:
Small note for reviewing - the diff stat for this PR is rather big, but the relevant changes are mostly limited to _font.py and the _get_actual_text_widths method in pypdf/_text_extraction/_text_extractor.py (012826d). The rest is only restructuring, mostly renaming and re-typing variables.
Some further advantages of this PR: