Wrong bbox in location for vertical font#708
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds vertical metrics support for CID fonts through new W2 parsing and metric classes, PDCIDFont vertical-width APIs, and DW2 fallback handling. It also adds null checks and standardized logging for invalid CID W-array entries. ChangesVertical Metrics for CID Fonts
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PDCIDFont
participant CIDW2Array
participant CIDW2ArrayRange
PDCIDFont->>CIDW2Array: construct from W2 array
CIDW2Array->>CIDW2Array: populate mappings and ranges
PDCIDFont->>CIDW2Array: getDisplacement(cid)
CIDW2Array->>CIDW2ArrayRange: check CID membership
CIDW2ArrayRange-->>CIDW2Array: return displacement
CIDW2Array-->>PDCIDFont: return displacement or null
PDCIDFont->>PDCIDFont: apply DW2 or constant fallback
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Jul 14, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/java/org/verapdf/pd/font/PDCIDFont.java (2)
146-157: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueCache the absence of
W2to avoid repeated lookups.If the
W2array is missing or invalid,this.verticalMetricsremainsnull. As a result, every subsequent call togetVerticalWidth()will repeat thedictionary.getKey(ASAtom.W2)lookup. Consider caching the absence of theW2array (e.g., by assigning a singleton emptyCIDW2Arrayor using a boolean flag) to avoid redundant dictionary lookups per character.Note:
getWidth()exhibits the exact same pattern for theWarray, so this optimization could be applied to both.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/verapdf/pd/font/PDCIDFont.java` around lines 146 - 157, Update getVerticalWidth so missing or invalid W2 data is cached instead of leaving verticalMetrics null; use the existing CIDW2Array mechanism or a dedicated flag, while preserving the default width result. Apply the same absence-caching optimization to getWidth for the W array if it has the equivalent lazy lookup pattern.
175-186: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache default vertical metrics and fix indentation.
getDefaultVMetrics()performs dictionary lookups and potentially allocatesnew Double[2]on every call. Since it is invoked bygetVerticalWidth()per character, this can introduce a significant performance overhead. Consider caching the parsedDW2metrics during initialization, similar to what is done forverticalMetrics.Additionally:
- Returning the mutable static array
DEFAULT_CID_FONT_VERTICAL_WIDTHas a fallback risks accidental global state corruption if a downstream caller modifies the returned array. Caching a read-only or cloned copy can prevent this.- Lines 179, 181, and 183 inconsistently use tabs for indentation instead of spaces.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/verapdf/pd/font/PDCIDFont.java` around lines 175 - 186, Cache the parsed DW2 metrics during PDCIDFont initialization and have getDefaultVMetrics() reuse that cached value instead of repeating dictionary lookups and allocations. Ensure fallback results do not expose the mutable DEFAULT_CID_FONT_VERTICAL_WIDTH array by returning a safe immutable or cloned representation, while preserving the existing default-metrics behavior. Normalize the indentation of the affected assignments and null checks to spaces.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/org/verapdf/pd/font/CIDW2Array.java`:
- Line 41: Update the logger declaration in CIDW2Array to use CIDW2Array.class,
then review the invalid W2-entry logging sites in the relevant methods and
change Level.FINE to Level.SEVERE if these errors should follow the PR’s unified
severity; keep the severity unchanged only if the existing behavior is
explicitly intentional.
In `@src/main/java/org/verapdf/pd/font/CIDWArray.java`:
- Around line 57-59: Update the cidBegin null-check in CIDWArray parsing to
return immediately after logging the invalid W array, matching the existing
width == null handling. This must prevent subsequent cidBegin.intValue() calls
and abort parsing before the token sequence becomes desynchronized.
---
Nitpick comments:
In `@src/main/java/org/verapdf/pd/font/PDCIDFont.java`:
- Around line 146-157: Update getVerticalWidth so missing or invalid W2 data is
cached instead of leaving verticalMetrics null; use the existing CIDW2Array
mechanism or a dedicated flag, while preserving the default width result. Apply
the same absence-caching optimization to getWidth for the W array if it has the
equivalent lazy lookup pattern.
- Around line 175-186: Cache the parsed DW2 metrics during PDCIDFont
initialization and have getDefaultVMetrics() reuse that cached value instead of
repeating dictionary lookups and allocations. Ensure fallback results do not
expose the mutable DEFAULT_CID_FONT_VERTICAL_WIDTH array by returning a safe
immutable or cloned representation, while preserving the existing
default-metrics behavior. Normalize the indentation of the affected assignments
and null checks to spaces.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c8dcd7ac-10fd-405e-969e-673d8cf832fa
📒 Files selected for processing (5)
src/main/java/org/verapdf/pd/font/CIDVerticalMetrics.javasrc/main/java/org/verapdf/pd/font/CIDW2Array.javasrc/main/java/org/verapdf/pd/font/CIDW2ArrayRange.javasrc/main/java/org/verapdf/pd/font/CIDWArray.javasrc/main/java/org/verapdf/pd/font/PDCIDFont.java
MaximPlusov
force-pushed
the
vertical-font-support
branch
from
July 15, 2026 13:50
2db26a8 to
8870bc3
Compare
MaximPlusov
force-pushed
the
vertical-font-support
branch
from
July 16, 2026 18:55
8870bc3 to
9032a2a
Compare
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
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.
Closes: https://trello.com/c/SCBNY4Sg
pdf.js PR: veraPDF/pdf.js#30
veraPDF-validation PR: veraPDF/veraPDF-validation#735
Summary by CodeRabbit
New Features
Bug Fixes