Repository navigation
Conversation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Paragraph text extraction and regression coverage crates/pdf/src/lib.rs, crates/pdf/src/tests_line_breaks.rs, ROADMAP.md |
master_text_pieces and text_layer now obtain paragraph text through Paragraph::text(). Tests cover line breaks, character ranges, source-model immutability, and exported Notes and Handouts text. The roadmap records the PDF export change. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix · Severity of issue fixed: Low
Merge Risk: 
83cc4
-
The text-layer change preserves alignment across paragraph line breaks. No issue requiring a fix before merge was identified.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: preserving paragraph line breaks in the PDF text layer during export. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
The PDF text layer cut each laid-out run's characters out of a paragraph string built from run.text. A Shift+Enter break run has empty text but occupies one character in the layout, so every character after a break was read from one position too early and the searchable text came out scrambled (BETA became ETAG). Build the string with Paragraph::text(), which counts a break as one character like the layout does, at both sites: the slide text layer and master_text_pieces (notes and handout masters). Adds two regression tests, which fail without the change.
81a07db to
83cc4c5
Compare
|
Keeping line breaks aligned in PDF text improves search and copy results for exported presentations. I checked this change against #1470: the vector PDF work is separate from this text-layer line-break fix. If you would rather not, I can open a PR that credits you, keeps your commits and shows a git range-diff against this one, so you'll still get credit and have visible work. |
FredApps
left a comment
FredApps
left a comment
There was a problem hiding this comment.
ReviewerA — source-read COMMENT (not an approval)
I read the head of the diff: PDF export keeps line breaks in the text layer's paragraph text. The two places that joined a paragraph's runs (runs.iter().map(|r| r.text).collect::<String>()) now call p.text(), so a line break inside a paragraph (Shift+Enter) counts as one character in the hidden text layer and offsets stay aligned with the drawn glyphs; the ROADMAP records it and a test module tests_line_breaks is added. A text layer whose offsets drift from the drawn text breaks search and selection in the PDF, so using the model's own text accessor is the correct fix. p.text() must give a newline or a vertical tab as the character the exporter expects, and that choice should be stated in the test. I have not run it, and no exact-head CI result exists for this head. Nothing was run.
I ran nothing and have no exact-head CI result for this PR, so this is not an approval. Where a diff was long I read its code hunks rather than every generated or documentation line.
Status: waiting_author.
Problem
A paragraph with a line break (Shift+Enter) has its characters scrambled in the PDF text layer. Text after a break is read from one position too early, so search and copy give wrong text. In a table cell I saw
BETAcome out asETAG.Cause
text_layerandmaster_text_piecesincrates/pdf/src/lib.rsbuild each paragraph's string withp.runs.iter().map(|r| r.text.as_str()).collect::<String>(), then cut the laid-out run's character range out of it. ARunKind::Breakrun has emptytextbut occupies one character in the layout, becauseRun::content()returns"\u{b}"for it. The string is one character short per break, so every later range is shifted.Fix
Build the string with
Paragraph::text(), which is a sum ofRun::content()and so counts a break as one character like the layout does. This covers both sites: the slide text layer, and the notes and handout master text.Tests
crates/pdf/src/tests_line_breaks.rsadds two regression tests:line_breaks_preserve_actual_pdf_text_and_native_character_rangesexports a slide for several paragraphs with breaks (leading, consecutive, trailing, and accented or Greek text with combining marks). It checks that the layout's character ranges index the paragraph text and that the extracted PDF text equals the source with the breaks removed.line_breaks_preserve_master_pieces_and_notes_handout_pdf_textchecksmaster_text_piecesand the exported text on notes and handout pages, where the master header text has breaks.Both fail on
mainand pass with this change.cargo fmt --checkis clean and the 17 tests indeckcraft-pdfpass.Overlap with open PRs
#1192 and #1197 rewrite the same two lines for a different bug (slide-number fields, #1153): #1192 switches them to
r.content()and #1197 to a precomputedshownstring. Either also fixes this line-break bug, and I checked that applying only #1192's twocontent()hunks makes these tests pass. Neither adds a test for breaks.I opened this so the regression tests land. If #1192 or #1197 merges first, I'll rebase this down to just the tests, or close it if you'd rather add them to that PR. Happy to go either way.
Summary by CodeRabbit