Skip to content

Fix word wrap measurement resuming inside a wrapped word - #987

Open
user77 (hexbinoct) wants to merge 1 commit into
microsoft:mainfrom
hexbinoct:fix-wrap-opp-lookahead
Open

user77 (hexbinoct) wants to merge 1 commit into
microsoft:mainfrom
hexbinoct:fix-wrap-opp-lookahead

Conversation

@hexbinoct

Copy link
Copy Markdown
Contributor

Problem

When measure_forward stops on its target in the middle of a word, the lookahead checks whether that word still fits on the row. If it doesn't, the cursor moves to the next row along with the word, but wrap_opp stays true, even though the new row has no wrap opportunity before the cursor. The main loop clears it when it wraps; the lookahead doesn't.

A measurement that continues from such a cursor then thinks the row can still be broken, and when the rest of the word reaches the wrap column it moves it down another row instead of hard wrapping it.

For example 01234 x56789abcdefghijklmnop at width 20:

|01234               |
|x56789abcdefghijklmn|
|op                  |

With the cursor after the x (row 1, column 1), cursor_move_to_offset to the end of the line returns row 3, column 1, a row that doesn't exist. From the line start it returns row 2, column 2.

Since #979, edit_word_wrap_layout_finish measures the end of the edited line from the cursor, so this also reaches visual_line_count(). The example above is what you get by typing x after 01234 in 0123456789abcdefghijklmnop at width 20. After that edit the line has 3 rows, but the count says 4. After undo it says 3 instead of 2, and after redo 5 instead of 3. assert_layout from #980 fails on each of those steps.

Fix

Clear wrap_opp in the lookahead branch too, like the main loop does when it wraps.

test_measure_forward_word_wrap expected wrap_opp: true for the cursor after the b in foo bar, which the lookahead moved to row 1. That row has no wrap opportunity before the cursor, and the same test already expects false for the cursor at the start of that row (goto_visual to {0, 1}), so it now expects false there too.

Testing

  • New test_measure_forward_word_wrap_resume: measuring to the end of the line from a cursor inside a word that the lookahead wrapped gives the same cursor as measuring from the start. Without the fix it ends at {5, 2} instead of {1, 2}.
  • cargo test --workspace and cargo clippy -p edit --all-features --all-targets -- --no-deps --deny warnings pass (Rust 1.95), and nightly rustfmt is clean on the changed file.
  • I also ran a randomized undo/redo test on TextBuffer (3000 seeds of 40 edits, a third of them with word wrap on, checking the layout from scratch after every step like assert_layout), with and without this change. It fixes 19 failing seeds (visual line count, visual cursor position, and one cursor.visual_pos.y <= self.stats.visual_lines debug assert) and adds no new failures. Some word wrap layout failures remain; they have other causes, at least one of them involving tabs.

#845 and #957 also change this lookahead loop. Neither conflicts with this change, and the measurement tests pass with #957's change merged in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant