Repository navigation
fix(extractor): keep sparse columns from sharing a baseline - #602
mangeshraut712 wants to merge 8 commits into
Conversation
Same-style column clauses and a far right-aligned tag were joined when the gutter was too narrow for column detection.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Confidence score: 4/5
- In
src/extractor/layout.rs, distant same-baseline runs starting with a letter are split into separate lines even when they contain only one word, which may create incorrect line breaks in extracted text; add a wordiness or prose gate.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/extractor/layout.rs">
<violation number="1" location="src/extractor/layout.rs:3081">
P2: This void split has no wordiness or prose gate: any same-baseline run that starts with an alphabetic character and sits more than 8em/100pt away becomes its own line, even a one-word token. The test comment claims page numbers "never reach this branch", but only *digit* page numbers do — `page_number_value` recognizes ASCII digits only, so a right-aligned Roman-numeral TOC folio (`Preface` … `iii` starts with 'i', alphabetic) or a spelled-out number now splits off the entry line it previously joined (the old code merged it because `incoming_wordy` was false). Short same-style rows are likewise only protected up to the 100pt floor, so the "label rows stay one line" invariant in the PR description holds only for gaps under ~100pt, as the tests only exercise the 50pt case.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Fix all with cubic | Re-trigger cubic
| // void apart. Table header cells on this scale are | ||
| // closer than 8em and 100pt, and digit page numbers | ||
| // never reach this branch. | ||
| if gap > (item.font_size.max(last_item.font_size) * 8.0).max(100.0) { |
There was a problem hiding this comment.
P2: This void split has no wordiness or prose gate: any same-baseline run that starts with an alphabetic character and sits more than 8em/100pt away becomes its own line, even a one-word token. The test comment claims page numbers "never reach this branch", but only digit page numbers do — page_number_value recognizes ASCII digits only, so a right-aligned Roman-numeral TOC folio (Preface … iii starts with 'i', alphabetic) or a spelled-out number now splits off the entry line it previously joined (the old code merged it because incoming_wordy was false). Short same-style rows are likewise only protected up to the 100pt floor, so the "label rows stay one line" invariant in the PR description holds only for gaps under ~100pt, as the tests only exercise the 50pt case.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/extractor/layout.rs, line 3081:
<comment>This void split has no wordiness or prose gate: any same-baseline run that starts with an alphabetic character and sits more than 8em/100pt away becomes its own line, even a one-word token. The test comment claims page numbers "never reach this branch", but only *digit* page numbers do — `page_number_value` recognizes ASCII digits only, so a right-aligned Roman-numeral TOC folio (`Preface` … `iii` starts with 'i', alphabetic) or a spelled-out number now splits off the entry line it previously joined (the old code merged it because `incoming_wordy` was false). Short same-style rows are likewise only protected up to the 100pt floor, so the "label rows stay one line" invariant in the PR description holds only for gaps under ~100pt, as the tests only exercise the 50pt case.</comment>
<file context>
@@ -3064,7 +3066,19 @@ fn group_single_column(
+ // void apart. Table header cells on this scale are
+ // closer than 8em and 100pt, and digit page numbers
+ // never reach this branch.
+ if gap > (item.font_size.max(last_item.font_size) * 8.0).max(100.0) {
return false;
}
</file context>
A right-to-left stream still splits on the same gutter, a roman folio stays on its entry, and a small hole in one justified line stays one line.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
A zero or negative measured width was pushing the run's right edge the wrong way and splitting one line. Front-matter folios stay on their entry; a roman-shaped word such as "mix" still splits.
There was a problem hiding this comment.
1 existing issue remains and no new issues found across 1 file (changes from recent commits).
Confidence score: 3/5
- In
src/extractor/layout.rs, treating standalone Roman-numeral or small-number tokens beside alphabetic text as TOC folios can leave genuine sparse-column rows such aschecksum./iiifused, reducing extraction accuracy; require TOC or front-matter evidence before exempting these tokens.
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 1 unresolved issue from previous reviews.
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.
Re-trigger cubic
A roman token stays on the entry only across a contents-leader gap. The sample column gutter beside "iii" still splits.
There was a problem hiding this comment.
1 existing issue remains and no new issues found across 1 file (changes from recent commits).
Confidence score: 3/5
- In
src/extractor/layout.rs, gaps between 100 and 300pt can send genuine front-matter roman folios onto their own line, disrupting extracted layout; adjust the gap handling so these folios remain attached.
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
A contents entry keeps its roman folio below 300pt. A wrapped fragment such as "checksum." still splits from "iii".
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
A title such as "Why?" stays with its roman page number. A short sentence fragment such as "checksum." still splits from "iii".
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 1 unresolved P0–P2 issue from previous reviews.
View guided diff | Re-trigger cubic
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 2 potential issues.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| if gap > (item.font_size.max(last_item.font_size) * 8.0).max(100.0) | ||
| && continues_line | ||
| && !folio_on_entry | ||
| { | ||
| return false; |
There was a problem hiding this comment.
🟡 Wide table rows split into lines
When table cells sit more than 100pt apart, group_single_column splits a same-baseline row into separate lines. The new three-cell test produces three lines, losing the row structure in single-column output.
Learn more
group_single_column also handles pages whose table-shaped text does not enter a validated column layout. The new wide-table test exercises three cells at the same Y and accepts three output lines. Each gap exceeds the new void threshold, so the second and third cells start new lines despite belonging to one row. This changes the line structure consumed by single-column output and downstream Markdown processing.
Example: At Y=459, Status at x=47, Description at x=200, and Owner at x=400 form one header row. The test now returns three lines instead of one row.
Recommended fix: Distinguish isolated cross-column fragments from aligned table cells before applying the void split. Preserve a row when neighboring same-baseline cells have row or table evidence; add a row-preservation regression test.
Was this helpful? React with 👍 or 👎 to provide feedback.
| fn is_wrapped_fragment(entry: &TextItem, entry_text: &str) -> bool { | ||
| if entry.is_bold { | ||
| return false; | ||
| } | ||
| let trimmed = entry_text.trim(); | ||
| if trimmed.ends_with(['?', '!']) { | ||
| return false; | ||
| } | ||
| trimmed.ends_with('.') && !trimmed.ends_with("...") && trimmed.split_whitespace().count() <= 3 | ||
| } |
There was a problem hiding this comment.
🟡 Punctuated contents entries lose page numbers
When a nonbold contents title such as 1. Introduction. ends in a period, is_wrapped_fragment rejects its roman folio. A gap over 100pt then moves iii to a separate line.
Learn more
The void split is intended to separate neighboring columns while preserving contents entries and their page numbers. roman_folio_beside_entry refuses the folio exception whenever is_wrapped_fragment sees a short nonbold title ending with a period. Common numbered or punctuated contents titles have exactly that shape, and their roman page numbers are often right-aligned across a gap exceeding 100pt.
Example: 1. Introduction. at x=72, width=110, and iii at x=500 share Y=700. The title has two words and ends with a period, so iii starts a second line instead of staying on the contents entry.
Recommended fix: Identify actual contents entries using layout or neighboring entries rather than treating every short period-terminated entry as a wrapped sentence; cover numbered, period-terminated titles with roman folios.
Was this helpful? React with 👍 or 👎 to provide feedback.
This branch has not been deployed
1 participant
Fixes #482.
Sparse two-column lines were fused because the gutter is too narrow for column detection, and the same-baseline split only fired for a lowercase continuation or a bold/regular mismatch. Same-style clauses, a one-word wrap sitting a column-width void away, and a right-aligned tag beside a title all stayed on one line.
Checked against the sample in that issue (
en-22-mixed-layout.pdf):Left column: record source type, page count, andat (42, 584) andRight column: inspect tables, images, links, and readingat (318, 584), both 10pt. The gap is about 36pt, under a column split and over a word space. Both runs are full clauses, so they now become two lines. A short same-style label row (two words, ~50pt apart) stays one line.checksum.at (42, 569) andorder.at (318, 569) are one word each, about 230pt apart. A void wider than 8em and 100pt is no longer treated as a word space. Table header cells in the same file (Status/Metric/Value, about 70pt apart) stay one row.Mixed layout reportat (42, 755), 16pt, andEN-22at (544, 756), 9pt, are 1pt apart in y and hundreds of points apart in x. The tag is its own line. A digit page number on a title line stays joined.cargo test --offline --lib layout::tests— 44 passed.cargo clippy --offline -- -D warningsis clean.Summary by cubic
Fixes #482: sparse two-column lines sharing a baseline were fused when the gutter was too narrow for column detection. The same-baseline split now fires for same-style full clauses, wide voids between wrapped leftovers, and far right-aligned tags, and also works for right-to-left streams.
Why?; a wrapped fragment besideiiiand a roman-shaped word likemixstill split across a column void.Written for commit 8ecc52f. Summary will update on new commits.