fix: read each line from the baseline its letters are set on - #238
Conversation
Rows were made from the boxes of the letters, and a row's box grew with every letter it took: a bracket or a comma is taller than the letters around it, and a row grown by a few of those reached the line above. The two were then read as one, with their letters shuffled together.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: QUIET Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe change adds PDFium character baseline extraction and optional Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3747073ebc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (off <= OFF_BASELINE * shorter && off < nearest) { | ||
| row = r; | ||
| nearest = off; |
There was a problem hiding this comment.
When tightly spaced lines surround a superscript, the top-down traversal can assign it to the preceding line before its actual base line has been processed. For example, with 8-point-high lines at baselines 106 and 100 and a 5-point-high superscript at 103, this condition attaches the superscript to baseline 106 because 3 <= 0.6 * 5; the later base text cannot join that row because its offset is 6. Depending on horizontal overlap, the exponent is then either left on the preceding line or beside() combines both lines into one corrupted row. Consider deferring raised characters or comparing them against both neighboring base lines.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
apps/worker/src/layout.ts-320-326 (1)
320-326: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExclude whitespace characters from the collision test.
collidescompares every character of a row, including the boxless spaces thatplacedinserts. Such a space hasleft === right, soMath.min(width, other.right - other.left)is0and the threshold becomes0. Any strict horizontal overlap then counts as a collision. A trailing placed space sits at the previous character's right edge, and a kerned character on the other line can start just left of that point. The merge inbesideis then blocked and the two lines stay separate rows.
edgesOfandcolumnGapalready ignore whitespace. Apply the same rule here.♻️ Proposed fix
function collides(row: { chars: readonly Char[] }, char: Char): boolean { + if (!char.text.trim()) return false; const width = char.right - char.left; return row.chars.some( (other) => + other.text.trim() !== "" && Math.min(other.right, char.right) - Math.max(other.left, char.left) > Math.min(width, other.right - other.left) * COLLIDES, ); }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Advanced
Run ID: da77241e-e887-4735-b4ff-984833ff9929
📒 Files selected for processing (3)
apps/worker/src/layout.tsapps/worker/src/render.tsapps/worker/test/layout.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.
A figure raised from its line stands between two baselines, and where the lines are close together it is nearer the line above than its own. Its letters are shorter than either line's, so which line it belongs to is not in where it is set: the text writes it next to its own letter.
Change
PDFium gives every character the point it is set from, which is its baseline, and a line of text is the characters set on one. The converter did not use it: rows were made from the boxes of the letters, and a row's box grew with every letter it took. A bracket or a comma is taller than the letters around it, and a row grown by a few of those reached the line above, so the two were read as one with their letters shuffled together. An Energizer Solar manual read "Phosphate (P1o-)l,y Hetehxyaleflnueo ro-" for "Phosphate (1-), Hexafluoro-" under "Polyethylene"; NOCO's guides lost paragraphs of German and Chinese that way, and a Victron booklet its default charge voltages, "(adjustableDweiftahu: lrto steatrtyi nswg: it1c4h,4, d/ i2sp8,l8ay /, 4V3E,.2D /i r5e7c,t6 oVr Bluetooth)" for "Default setting: 14,4 / 28,8 / 43,2 / 57,6 V".
A line is now the characters on one baseline. How far off it a character may be set is a share of the height of the shorter of the two lines' letters, and every character is measured by the tallest letter on its own baseline, so a line is taken into another whole or not at all. Measured by the taller, "40-60A" set 47 points high took in "Battery Voltage Model" beneath it; measured by each letter's own height, a short "c" could not reach the line its "V" and "i" were on and Victron's name came out as "Vi" on one line and "ctron" on the next. Characters PDFium gives no baseline for, which is everything printed sideways, are placed by their boxes exactly as before.
A figure raised from its line is a case of its own: its letters are shorter than the line's, and where the lines of a page are close together it stands nearer the line above than its own, so where it is set cannot say which it belongs to. The text can, since it writes the figure next to the letter it goes with, "m" then "2". The lines are made tallest first so that each is whole before any raised figure joins one, and a figure beside its letter is read on that letter's line; one standing off at the end of a line is written after that line too and belongs to neither. Without this, "area 5 m2" under a line six points above it came out as "area 5 m" with the exponent on the line above, "upper line2 of text".
Two further things follow from having baselines. PDFium writes a space of its own wherever the pen jumps and gives it no box; those were dropped, so Victron read "maybe present" and a French manual "publiées parle fabricant". Each is now put where the text puts it and kept only where the gap is a space's width: much wider than the gaps between the line's letters, and near as wide as the spaces the run writes itself. PDFium also guesses them inside words, "The g reen LED s ta tu s" and "F lange nut", and those two measures reject them. Finally, two lines that are cells of one row are put together where every cell of each stands a column clear of the other's: NOCO sets a wrapped value half a line above its label and half below, and read as three lines "Accutypes" is left with no value at all.
This changes the converter, so it reaches the data only where a document is converted again.
Validation
just checkpasses: 464 tests, records validated, two dataset builds matching across 111 files.Against main over 46 local PDFs, 2,828 pages: 1,189 pages change, and no word of five letters or more that main holds is lost. Every one of the 58 candidates was read by hand and is one of three things: main's own shuffled text put right ("vrieprpteler", "BPahtoteernyix", "cOa LrgEaD tdotea cl."), a character main dropped and this keeps (VE_REG_DEVICE_STATE for "VE REG DEVICE STATE", "l'installation", "„Power Delivery""), or a missing space put back ("may be", "Shut down", "par le fabricant"). The other direction was checked too: of the words the branch gains, 3,076 are alphabetic and 19 look shuffled, and each of those is a line main shuffled as badly or worse. Lines with a cell repeated three times or more go from 4,875 to 5,010 and the text is the same length, ×1.00.
The four documents named on #230 are repaired: Energizer p16 reads "Phosphate (1-), Hexafluoro-, Lithium", the NOCO AX65 guide reads its German p28 and Chinese p426 in order, the Victron booklet prints its charge voltages, and NOCO's "Accutypes" carries its value.
The nine new tests were mutation-tested: twelve of thirteen mutations of this change are caught, including rows made by boxes, the tolerance measured by the taller line, each letter measured by its own height, lines taken from the top down rather than tallest first, a raised figure read by where it is set or taken from any line it is written by, guessed spaces left off their line or not weighed against written ones, and each of the three conditions on putting two lines beside each other. The thirteenth, taking box-placed characters from the top of the box as main does rather than the bottom, is covered only by the comparison above, where it is what keeps a Victron wiring diagram's sideways labels as main reads them; no page small enough for a test tells the two apart.
Converting is about a sixteenth slower, 25.7s to 27.3s over those 2,828 pages, which is the cost of asking PDFium for each character's origin.