[
https://issues.apache.org/jira/browse/PDFBOX-6292?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18135114#comment-18135114
]
Tilman Hausherr commented on PDFBOX-6292:
-----------------------------------------
[~tallison] assuming you're building PDFBox from source, and you having a very
different machine than mine, consider adding your own problematic files locally
in the directory "pdfbox/src/test/resources/input". After the build fails, get
the output file pairs in "pdfbox/target/test-output" and add them to the input
directory.
Here's what Claude Sonnet 5.5 wrote, I'm including it for the record. The space
width is an old problem, especially with fonts that don't have space at all.
I'm not really persuaded by the comments, partly because it wasn't able to
retrieve the code properly. Item 4 is just wrong because there are a lot of
space regression tests in the repository and some more on my machine.
===
The PR page has no description beyond a note that it was AI-generated, so I
need the actual diff. I’ll try the “Files changed” tab, which the page links to.
I have the diff now. Before I judge the logic, I want to see the surrounding
code in {{getSpaceWidth()}} and {{{}PDType0Font.encode(){}}}, since the diff
hunks are cut off.
h2. Review of apache/pdfbox#550: don’t derive the space width from a
substitute’s encoding
*Verdict:* the fix is correct for the case it targets, but I’d want a few
things addressed or clarified before merging. I only read the diff and the
surrounding code in {{PDFont}} and {{{}PDType0Font{}}}. I did not check out or
run anything, and I couldn’t see the CI results.
h3. What it does
The PDFBOX-5920 change made {{getSpaceWidth()}} call {{getStringWidth(" ")}} so
that the encoding supplies the right code. For a non-embedded Identity-H
CIDFontType2 with no ToUnicode, {{encode()}} returns the GID of whichever
substitute font happens to be installed. The code is then looked up in the
PDF’s {{/W}} array, so the space width depends on the machine’s fonts.
The PR adds a protected hook, {{{}PDFont.isEncodedViaSubstitute(){}}}, which
defaults to {{{}false{}}}. {{PDType0Font}} overrides it for that case, and
{{getSpaceWidth()}} skips the {{getStringWidth(" ")}} attempt when the hook
returns true. It then falls through to the existing {{getWidth(32)}} fallback.
The change is small, and the hook is a sensible way to scope it.
h3. Concerns
# *The predicate may be too narrow.* From memory of
{{PDCIDFontType2.encode()}} (I did not read it here), a non-embedded font gets
its code from the substitute’s cmap regardless of the CMap name. If so, a
non-embedded CIDFontType2 with a predefined CMap such as {{GBK-EUC-H}} or
{{UniGB-UCS2-H}} and no ToUnicode has the same machine-dependent problem. The
{{startsWith("Identity-")}} check would miss it. Please confirm against
{{{}PDCIDFontType2.encode(){}}}, and either widen the check or say in a comment
why it is limited to Identity.
# *The test is circular.* It asserts {{{}font.getWidth(32) ==
font.getSpaceWidth(){}}}, which is exactly what the fallback computes. It
proves the result no longer depends on the substitute, which is useful, but it
doesn’t show that {{getWidth(32)}} is the right answer. For Identity-H with an
Adobe-GB1 ROS, code 32 is CID 32, not the space glyph. I believe CID 1 is the
space in all four Adobe CJK collections, but please verify that. If so,
{{getWidth(1)}} for those collections would be more accurate than
{{{}getWidth(32){}}}. That would be a follow-up, since the existing fallback
already behaves this way, but a comment saying that code 32 is a guess would
help.
# *The test depends on the machine.* It uses {{assumeTrue}} on the
substitute’s space GID, so on a machine where the GID falls outside 1–31 it
silently skips rather than fails. That’s acceptable, but CI should be checked
to make sure it actually runs there and isn’t always skipped. I also can’t
confirm that the test compiles against the current {{COSName}} constants and
{{{}PDCIDFontType2.getTrueTypeFont(){}}}.
# *There is no regression coverage for behavior changes.* {{getSpaceWidth()}}
drives word-break detection in {{{}PDFTextStripper{}}}. Documents that fit this
profile will now extract with different spacing. Please confirm that the
existing text-extraction tests pass, and ideally add a real-world sample, since
the test uses a synthetic dictionary.
# *Process.* The PR description is only “This was 100% 🤖. Beware.” It gives no
problem statement, no JIRA link text, and no description of how it was
verified. PDFBOX-6292 is in the title, but a reviewer shouldn’t have to go and
find the ticket. An AI-assisted change also needs a human to vouch for
correctness and licensing, which the author’s note doesn’t do.
h3. Minor
* The Javadoc {{{@link #encode(int)}}} points at a protected method. That’s
fine, but it will render oddly.
* Everything else is consistent with the code around it, including the comment
style and the {{PDFBOX-xxxx}} references.
The first two points are the ones I’d press on. I can write up inline comments
you could post on the PR, or read {{PDCIDFontType2.encode()}} and settle point
1 properly.
> Space width of non-embedded Identity-H CIDFontType2 fonts depends on the
> installed substitute font
> ---------------------------------------------------------------------------------------------------
>
> Key: PDFBOX-6292
> URL: https://issues.apache.org/jira/browse/PDFBOX-6292
> Project: PDFBox
> Issue Type: Bug
> Components: Text extraction
> Reporter: Tim Allison
> Priority: Trivial
> Fix For: 2.0.38, 3.0.9 PDFBox, 4.0.0
>
>
> While working on the ms fonts issue in Tika, my :robot: and I stumbled on
> this.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
