Conversation
SomeoneToIgnore
left a comment
There was a problem hiding this comment.
Thank you, needs some more work.
c14a7cf to
8f38a40
Compare
|
@SomeoneToIgnore thanks for the review! I believe I have addressed all your comments. There is an open question I have around the names for the setting values, but thankfully it shouldn't be a huge lift if we decide to go with different nomenclature. |
SomeoneToIgnore
left a comment
There was a problem hiding this comment.
Getting closer, thank you.
|
Thanks for the second review! I won't be able to address these comments until next week because I'll be at RustConf. |
This allows callers to specify how indentation for soft-wrapped lines should function. For now this uses the existing default of same-level indentation.
Also add soft_wrap_indent tests.
This is a 1:1 refactor of the existing calculation. A subsequent commit will introduce the change that necessitated this refactor.
Because soft-wrapped display rows now can start on a *lower* indent column than the first display row, we cannot rely on the first row's indent for code action placement, and instead need to check to see if there's enough space to display the code action glyph. If not, use the buffer's first display row for the calculating code action placement.
8f38a40 to
cd6417c
Compare
|
I've addressed all additional feedback as discrete commits and rebased this over |
|
SomeoneToIgnore
left a comment
There was a problem hiding this comment.
Seems that we need to deal with line_wrapper code better, also added a few more notes.
| IndentAdjustment::ExtraColumns(extra) => { | ||
| Self::MAX_INDENT.min(base_indent + extra) | ||
| } |
There was a problem hiding this comment.
SameIndent is self-bounding: base_indent is measured inside a row that fit, so it never exceeds the wrap column count.
ExtraColumns adds up to 2 × tab_size on top with only the MAX_INDENT clamp, so indent can exceed the wrap width.
After that, width = item_width + indent × space_width > wrap_width holds for every following character, and the line degrades to one character per row, each row wider than the wrap width.
Repro (EditorTestContext, soft_wrap: bounded, preferred_line_length: 10, extra_two, tab size 4, text " ab cd ef gh"):
| ab |
| c|
| d|
| |
| e|
| f|
| |
| g|
| h|
9 rows of 15 columns in a 10-column wrap; same gives 3 rows of ≤ 10 columns.
The overflow is bounded by 2 × tab_size columns, with one row per remaining character, so a 200-character line becomes ~200 rows.
This is reachable with extra_two for any line whose indent is within 8 columns of the pane width, e.g. deeply nested code in a narrow split.
VS Code guards exactly this in computeWrappedTextIndentLength: if (wrappedTextIndentLength + columnsForFullWidthChar > firstLineBreakColumn) wrappedTextIndentLength = 0.
Please add the same guard here (fall back to 0, or to base_indent) and cover it with a unit test in this file.
| self.buffer.read(cx).language_settings(cx).soft_wrap_indent | ||
| } | ||
|
|
||
| pub(super) fn apply_soft_wrap_indent(&mut self, cx: &mut Context<Self>) { |
There was a problem hiding this comment.
The existing precedent for a settings-driven wrap parameter is soft_wrap: element.rs calls editor.soft_wrap_mode(cx) + set_wrap_width on every prepaint (lines 8579-8589 and 11286-11290), and set_wrap_width early-returns when unchanged.
set_soft_wrap_indent already early-returns the same way.
Reading self.soft_wrap_indent(cx) inside Editor::set_wrap_width below and forwarding it to whichever map is picked there gives the indent the same freshness as soft_wrap/preferred_line_length, and removes the Editor::new, is_minimap, and five event-handler call sites.
It also closes the remaining gap: an empty multibuffer (project search before the first result) whose global soft_wrap_indent changes emits no buffer SettingsChanged, and fetch_applicable_language_settings compares two empty maps, so placeholder_display_map stays stale until an excerpt arrives.
The new tests keep working unmodified: in test mode App::flush_effects draws dirty windows (gpui/src/app.rs:1817-1830), so the poll runs before the next editor.update.
Cost is one extra MultiBuffer::language_settings per frame, which soft_wrap_mode already pays on the same frame.
| } | ||
|
|
||
| #[gpui::test] | ||
| async fn test_soft_wrap_indent_returns_some_for_zero_indent(cx: &mut gpui::TestAppContext) { |
There was a problem hiding this comment.
The None case in test_soft_wrap_indent above already produces a zero-indent wrap.
One assert_eq!(wrap_snapshot.soft_wrap_indent(WrapRow(1)), Some(0)) there covers this, so this test and its comment block can go.
The same 30 lines of setup are also copied into test_soft_wrap_indent_updates_on_tab_size_change; please share a helper if it stays.
| #[ztracing::instrument(skip_all)] | ||
| fn indent_adjustment(&self, tab_size: std::num::NonZeroU32) -> gpui::IndentAdjustment { |
There was a problem hiding this comment.
Nit: gpui::IndentAdjustment and std::num::NonZeroU32 are spelled out inline throughout this file, please import them.
The ztracing instrument on a one-line match is noise.
Objective
Solution
same):none,extra_one, andextra_two(where they are multiples of the indent column width).none-level wrap indentation (preceded by a refactor of the function used to calculate its display row).WrapSnapshot::soft_wrap_indentwhere querying a 0-indent wrap returnedNoneinstead ofSome(0)Testing
wrap_map.rsfor all indentation levels.Self-Review Checklist:
Showcase
Changes are grouped by commit to ease review.
Screenshots below!
Click to view showcase
sameindent (current Zed behavior)noneindentextra_oneindentextra_twoindentthe setting

Release Notes:
soft_wrap_indentsetting ("none","same","extra_one","extra_two") to configure indentation for soft-wrapped continuation lines.IndentAdjustmentenum toLineWrapperto control indentation of soft-wrapped continuation lines.