fix(editor): map visual rows to source rows for softwrap inside markdown tables (#10016)#10707
fix(editor): map visual rows to source rows for softwrap inside markdown tables (#10016)#10707lonexreb wants to merge 2 commits into
Conversation
…oard copy (warpdotdev#10016) Adds a unit test exercising both `Buffer::clipboard_table_text_in_range` and `Buffer::selected_text_as_plain_text` with a 500-char plain-text table cell. The cell spans multiple internal text fragments (TEXT_FRAGMENT_SIZE = 128 in test builds), so the test guards against fragment-bounded truncation in the buffer-level clipboard pipeline used by the rendered markdown viewer. Investigation context for warpdotdev#10016 follows in the PR body.
github-actions
Bot
added
the
external-contributor
…own tables (warpdotdev#10016) `LaidOutTable::lines()` was reporting `1 + table.rows.len()` — a source-row count. When a table cell soft-wraps to multiple visual lines, the visual line count is strictly larger, so anything that consumes `lines()` (the SumTree's `LineCount` dimension, the softwrap point ↔ char-offset mapping for `BlockItem::Table`, line-range queries) was undercounting and aliasing every visual line in the wrapped row onto the row's first visual line — or worse, spilling past the table and landing inside the next block. Concretely: the `BlockItem::Table` arms of `softwrap_point_to_offset` and `offset_to_softwrap_point` treated `point.row()` (a visual line index) as if it were a source-row index. A user selecting visual line 2 of a row whose first cell wraps to 3 visual lines would have their selection start resolved to source row 2 of the table — pointing one row below the wrap, so the copy contained the wrong row. This change: - Computes `LaidOutTable::lines()` as the sum of per-row visual heights, where each row's height is the max wrapped-line count across its cells (always at least 1). - Adds private helpers `visual_row_heights`, `visual_lines_before_row`, `visual_line_to_source_row`, and `row_sub_line_source_offset` to translate between visual line indices and `(source_row, sub_line)` pairs, including the source character offset of a given sub-line within column 0 of a row. - Rewrites the `BlockItem::Table` arms of `softwrap_point_to_offset` and `offset_to_softwrap_point` to walk those mappings instead of treating `point.row()` and `cell.row` as 1:1 with source rows. The forward map uses `cell_offset_map.rendered_to_source` to translate the sub-line's rendered start back to a source offset; the inverse map uses `source_to_rendered` plus `CellLayout::line_at_char_offset` to find the visual sub-line a source offset falls on. Adds four focused unit tests in `mod_tests.rs` exercising the new mapping: - `test_table_visual_line_count_matches_wrapped_rows` pins `lines()` to the sum of per-row visual heights. - `test_table_softwrap_round_trip_inside_wrapped_cell` confirms that visual points within a wrapped body row round-trip back to the body row's first visual line (canonical row-start collapse). - `test_table_offset_to_softwrap_point_uses_visual_rows_after_wrap` confirms that a source offset in the row AFTER a wrapped row resolves to a visual line that accounts for the wrap's extra visual lines. - `test_table_lines_falls_back_to_one_per_empty_row` guards the `.max(1)` fallback so an empty cell layout cannot collapse a row to zero visual lines. The buffer-level clipboard pipeline (`Buffer::clipboard_table_text_in_range`, `Buffer::selected_text_as_plain_text`) was already correct — its regression test from the earlier commit on this branch still passes — so this fix restores accurate selection-coordinate resolution feeding into that pipeline.
|
This PR is not linked to an issue that is marked with Issue-state enforcement details:
To continue, link this PR to a same-repo issue such as Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is not linked to an issue that is marked with ready-to-implement.
Issue-state enforcement details:
-
Associated same-repo issues checked: none
-
Required readiness label:
ready-to-implement
To continue, link this PR to a same-repo issue such as Closes #123 in the PR description, and make sure that issue has ready-to-implement.
Powered by Oz
lonexreb
changed the title
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I reviewed this pull request and requested human review from: Comment Powered by Oz |
oz-for-oss
Bot
dismissed
their stale review
Oz no longer requests changes for this pull request after the latest automated review.
There was a problem hiding this comment.
Overview
This PR updates the markdown table render model so table line counts and table softwrap point/source-offset conversions account for rows whose cells occupy multiple visual lines. It also adds focused regression coverage for the render-model mapping and the existing buffer-level clipboard path.
Concerns
- No blocking correctness, security, or error-handling concerns found in the changed lines. The PR explains why end-to-end manual reproduction was not available in the contributor environment and covers the affected mapping with unit tests.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
kevinyang372
requested review from
harryalbert
and removed request for
kevinyang372
harryalbert
left a comment
harryalbert
left a comment
There was a problem hiding this comment.
Thanks for submitting a fix here @lonexreb ! I'm not able to reproduce the issue in my warp build for some reason, so I'm not able to test your solution. Could you attach a video showing:
- the original bug in production
- the fixed build working with no bug
Thank you!
|
Hey @harryalbert — thanks for the review. Recorded both states on a local build of the OSS bits at warp-oss: The bug on master The fix branch The leading-whitespace symptom is a milder surface of the same root cause @neongrau hit in #10016 (their original screenshot shows a selected row pasting The 4 unit tests in Let me know if you want me to record any other selection geometry (e.g., the dramatic "wrong row" case with a wider table) or test anything else. |
|
@lonexreb could you re-attach those demo videos? i'm not able to actually watch them (see the screenshot below for what I'm seeing), even when I copy-paste the URLs into another window |
|
Sure thing @harryalbert here it is bug:- bug.movSame fix:- fix.movIdentical fixture, identical selection. Paste returns row 2's content verbatim, no leading-whitespace contamination, matching what's visually highlighted. The leading-whitespace symptom is a milder surface of the same root cause @neongrau hit in #10016 (their original screenshot shows a selected row pasting The 4 unit tests in Let me know if you want me to record any other selection geometry (e.g., the dramatic "wrong row" case with a |
|
Hey, I'm going to close this PR because it seems like we can't get a solid repro. Feel free to re-open a PR with a solid verification video attached. Thank you! |
|
hey @harryalbert, sorry — crossed wires here. the two videos i posted right before (the 024ed249... bug clip and 2a219dd2... fix clip) are the actual verification recordings. the follow-up message saying "there are no videos" was me confusing it with an older comment thread and shouldn't have been sent, please ignore that one. if the videos still aren't playing on github's side, lmk and i can re-host them as a gist or direct mp4 links. would really appreciate a reopen — happy to record any other selection geometry too if it helps nail it down. |
2 participants
Closes #10016
Summary
Fixes #10016 — copying a selection that lands inside (or after) a soft-wrapped markdown-table cell returned the wrong row's content. The buffer-level clipboard pipeline was already correct; the bug was in the render-model layer that maps visual selection coordinates to source character offsets.
Refs: #10016
Root cause
LaidOutTable::lines()returned1 + table.rows.len()— a source-row count. The SumTree'sLineCountdimension and theBlockItem::Tablearms ofsoftwrap_point_to_offset/offset_to_softwrap_pointtreatedSoftWrapPoint::row()(a visual line index) as if it were a source-row index. When a cell soft-wrapped to multiple visual lines:start_line + source_row, ignoring wrap height contributed by earlier rows.This is what produced the reporter's screenshot, where the visible selection pasted content from a different row.
What changed
crates/editor/src/render/model/mod.rs:LaidOutTable::lines()(around line 1375): returns the sum of per-row visual heights. Each row's visual height ismax(cell.line_heights.len(), 1)across its cells.visual_row_heights,visual_lines_before_row,visual_line_to_source_row, androw_sub_line_source_offsethelpers onLaidOutTableto translate between visual line indices and(source_row, sub_line)pairs, and to resolve the source-character offset for a sub-line within column 0 of a row.BlockItem::Tablearm ofsoftwrap_point_to_offset: walks the per-row visual heights to find(source_row, sub_line), then usescell_offset_map.rendered_to_sourceoncell_layouts[row][0].line_char_ranges[sub_line].startto produce the correct source offset. Falls back toend_char_offset()when the visual line is past the table.BlockItem::Tablearm ofoffset_to_softwrap_point: finds the source(row, col, offset_in_cell), converts the source offset into a rendered offset, usesCellLayout::line_at_char_offsetto find the visual sub-line, then sums prior rows' visual heights to produce the correct visual row.crates/editor/src/render/model/mod_tests.rs— four focused unit tests:test_table_visual_line_count_matches_wrapped_rows—lines()equalsΣ max(cell visual line counts).test_table_softwrap_round_trip_inside_wrapped_cell— visual points within a wrapped row round-trip back to the row's first visual line (canonical row-start collapse, sincesoftwrap_point_to_offsetresolves to col-0 start).test_table_offset_to_softwrap_point_uses_visual_rows_after_wrap— a source offset in the row after a wrapped row resolves to a visual line that accounts for the wrap's extra visual lines.test_table_lines_falls_back_to_one_per_empty_row— an empty cell layout cannot collapse a row to zero visual lines.The previous-commit regression test (
test_clipboard_table_copy_preserves_long_cell_spanning_text_fragments) is still green — it proves the buffer-level pipeline is unaffected.Test plan
cargo check -p warp_editor --testsclean.cargo fmt -p warp_editor --checkclean.cargo test -p warp_editor --lib test_table— 14/14 pass, including the 4 new tests and the previously added clipboard regression test.cargo test -p warp_editor --lib render::model::tests— 34/34 pass.cargo test -p warp_editor --lib -- --test-threads=1) green; the 8 failures observed under parallel run are an unrelatedparking_lot::Oncepoisoning race in therender::teststest infra that reproduces on master and is not caused by this change (each test passes in isolation and with--test-threads=1).Notes for reviewer
BlockItem::Tablearms andLaidOutTable. No otherBlockItemvariant is touched.softwrap_point_to_offset) only resolves to column 0's content — same as the previous behavior. The column handling for inside-cell hit testing is already done elsewhere (coordinate_to_offset)..max(1)invisual_row_heightmatches the existingrow_heightssemantics: every laid-out row has at least one visual line.Manual testing note
End-to-end manual verification on macOS requires the Xcode
metalshader toolchain, which is not available in my contribution environment (Command Line Tools only —crates/warpui/build.rspanics on missingmetal). I verified the change via:cargo check/cargo teston the affected crate(s) — all green (see PR body for specific counts).Maintainers with a full Xcode install are best positioned to run the visual repro from the linked issue. Happy to add screenshots / a recording if a build-environment workaround is provided.