Add extractFontFamily method#705
Conversation
|
Warning Review limit reached
More reviews will be available in 10 minutes and 43 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new ChangesFont Family Extraction Utility
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![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
🤖 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/PDFontDescriptor.java`:
- Around line 58-63: The STYLE_SUFFIXES array in PDFontDescriptor includes the
overly broad "It" entry which causes valid family names ending in "it" (e.g.,
"Summit") to be truncated; remove the "It" string from STYLE_SUFFIXES (or
replace it with an unambiguous token like "Ital" only if needed) and update any
tests that relied on removing that short suffix; ensure the code path that trims
name suffixes (the logic that iterates STYLE_SUFFIXES when normalizing family
names) still covers all intended style markers such as "Italic", "Oblique",
"Bold", etc., without the ambiguous "It".
- Around line 132-133: extractFontFamily currently returns null for a null/empty
fontName which breaks its fallback contract; change the initial guard in
PDFontDescriptor.extractFontFamily so that when fontName is null or empty it
returns the CSS fallback "sans-serif" instead of null, ensuring callers always
receive a valid font-family string and preserving the method's fallback
behavior.
🪄 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: d3848ea0-03f5-4c20-aff6-7c36eee64da4
📒 Files selected for processing (1)
src/main/java/org/verapdf/pd/font/PDFontDescriptor.java
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/main/java/org/verapdf/tools/FontConstants.java (1)
7-77:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
"It"suffix is too broad and corrupts valid family names.The
"It"entry on line 27 will incorrectly strip valid font names ending init, such as"Summit"→"Summ". The list already includes more specific alternatives like"Italic"and"Ita". Remove"It"to prevent false normalization.🛡️ Proposed fix
// ---- Slope ---- - "Italic", "Ita", "It", + "Italic", "Ita", "Oblique", "Obl", "Caps", "CapsI",🤖 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/tools/FontConstants.java` around lines 7 - 77, STYLE_SUFFIXES contains a too-broad "It" suffix that wrongly trims valid family names (e.g., "Summit" → "Summ"); remove the "It" entry from the STYLE_SUFFIXES list in FontConstants to stop accidental normalization, keeping the more specific "Italic" and "Ita" entries intact; update any unit tests or normalization logic that assumed "It" to rely on the remaining suffixes or add a targeted rule if genuinely needed.src/main/java/org/verapdf/pd/font/PDFontDescriptor.java (1)
125-126:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn
"sans-serif"instead ofnullfor empty input.Returning
nullfor null or empty input breaks the method's fallback contract. Line 155 already returns"sans-serif"when the processed name is empty, so the initial null check should do the same for consistency. This forces unnecessary null checks at call sites.🛡️ Proposed fix
public static String extractFontFamilyFromFontName(String fontNameWithoutSubset) { - if (fontNameWithoutSubset == null || fontNameWithoutSubset.isEmpty()) return null; + if (fontNameWithoutSubset == null || fontNameWithoutSubset.isEmpty()) return "sans-serif";🤖 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/PDFontDescriptor.java` around lines 125 - 126, The null/empty guard in PDFontDescriptor.extractFontFamilyFromFontName currently returns null for a null or empty fontNameWithoutSubset which breaks the method's fallback contract; change that guard to return "sans-serif" instead so it matches the later empty-name fallback (and avoids forcing callers to null-check). Update the behavior in extractFontFamilyFromFontName to return "sans-serif" when fontNameWithoutSubset is null or empty, keeping the rest of the name-processing logic unchanged.
🧹 Nitpick comments (1)
src/main/java/org/verapdf/pd/font/PDFontDescriptor.java (1)
29-29: 💤 Low valueConsider explicit imports over wildcard.
Wildcard imports (
java.util.*) can obscure dependencies and cause conflicts if types are added to the package in the future. Explicit imports improve clarity.♻️ Suggested refactor
-import java.util.*; +import java.util.Iterator; +import java.util.Map;🤖 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/PDFontDescriptor.java` at line 29, Replace the wildcard import at the top of PDFontDescriptor (the "import java.util.*;" statement) with explicit imports for only the types actually used by the class (e.g., List, Map, Set, Optional, Collection, Iterator, etc. as applicable); update the import list to include those specific java.util classes referenced anywhere in PDFontDescriptor to improve clarity and avoid accidental conflicts.
🤖 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/PDFontDescriptor.java`:
- Line 129: The replacement removing "*<digits>" in
PDFontDescriptor.extractFontFamilyFromFontName is inconsistent with this
project's subset format and the fact the method is unused; update the method to
either (A) remove the name.replaceAll("\\*\\d+", "") line and rely on
caller-side subset stripping, or (B) change the normalization to handle this
repo's subset marker by detecting PDFont.SUBSET_REGEX (or applying substring(7)
when it matches) before further processing, and add a short comment explaining
why subset stripping is done here if you keep it; ensure the change is made
inside extractFontFamilyFromFontName and reference PDFont.SUBSET_REGEX or
substring(7) as appropriate.
---
Duplicate comments:
In `@src/main/java/org/verapdf/pd/font/PDFontDescriptor.java`:
- Around line 125-126: The null/empty guard in
PDFontDescriptor.extractFontFamilyFromFontName currently returns null for a null
or empty fontNameWithoutSubset which breaks the method's fallback contract;
change that guard to return "sans-serif" instead so it matches the later
empty-name fallback (and avoids forcing callers to null-check). Update the
behavior in extractFontFamilyFromFontName to return "sans-serif" when
fontNameWithoutSubset is null or empty, keeping the rest of the name-processing
logic unchanged.
In `@src/main/java/org/verapdf/tools/FontConstants.java`:
- Around line 7-77: STYLE_SUFFIXES contains a too-broad "It" suffix that wrongly
trims valid family names (e.g., "Summit" → "Summ"); remove the "It" entry from
the STYLE_SUFFIXES list in FontConstants to stop accidental normalization,
keeping the more specific "Italic" and "Ita" entries intact; update any unit
tests or normalization logic that assumed "It" to rely on the remaining suffixes
or add a targeted rule if genuinely needed.
---
Nitpick comments:
In `@src/main/java/org/verapdf/pd/font/PDFontDescriptor.java`:
- Line 29: Replace the wildcard import at the top of PDFontDescriptor (the
"import java.util.*;" statement) with explicit imports for only the types
actually used by the class (e.g., List, Map, Set, Optional, Collection,
Iterator, etc. as applicable); update the import list to include those specific
java.util classes referenced anywhere in PDFontDescriptor to improve clarity and
avoid accidental conflicts.
🪄 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: 731047db-bc46-4e31-bf1e-b9a3d7565fc4
📒 Files selected for processing (2)
src/main/java/org/verapdf/pd/font/PDFontDescriptor.javasrc/main/java/org/verapdf/tools/FontConstants.java
2 participants
Summary by CodeRabbit