Skip to content

editor: keep wrapped search matches visible with context - #3013

Open
hlcfan wants to merge 5 commits into
longbridge:mainfrom
hlcfan:surrounding-lines
Open

editor: keep wrapped search matches visible with context#3013
hlcfan wants to merge 5 commits into
longbridge:mainfrom
hlcfan:surrounding-lines

Conversation

@hlcfan

@hlcfan hlcfan commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Description

Keep search matches visible with surrounding context when long lines wrap. Resolve matches to their display rows, including offscreen matches, and calculate padding from viewport capacity instead of visible logical-line count.

Screenshot

Before After
Screenshot 2026-09-08 at 7 33 40 PM Screenshot 2026-09-08 at 7 34 56 PM

How to Test

  • cargo test -p gpui-base --lib input:: --offline — 220 tests passed, including the wrapped-match regression test.
  • cargo run -p example-editor --offline — built and launched on macOS.
  • With soft wrapping enabled, search for a word near the end of a long offscreen line. Navigate to the match and verify it stays visible with surrounding rows. E.g. "path": "relative_path:asdfasfafadsf[?(@['username'] == 237842985798579834534536363365")]"

Checklist

  • I have read the CONTRIBUTING document and followed the guidelines.
  • Reviewed the changes in this PR and confirmed AI generated code (If any) is accurate.
  • Passed cargo run for story tests related to the changes.
  • Tested macOS, Windows and Linux platforms performance (if the change is platform-specific)

@hlcfan hlcfan changed the title fix(editor): keep wrapped search matches visible with context editor: keep wrapped search matches visible with context Sep 8, 2026
@huacnlee

huacnlee commented Sep 8, 2026

Copy link
Copy Markdown
Member

Code review

No blocking issues found. Checked for bugs, CLAUDE.md compliance, git history context, and comment consistency.

The root cause fix looks right: visible_range counts logical lines (element.rs documents it as "based on unwrapped lines"), so one long wrapped line collapsed it to 1-2 and starved the cursor_surrounding_lines margin. Deriving capacity from bounds.size.height / line_height fixes the unit mismatch, and switching to buffer_pos_to_display_pos resolves the wrapped sub-row through the DisplayMap wrap cache rather than last_layout, so offscreen matches work. Dropping row_offset_y += pos.y is correct — the new call already includes the sub-row offset. This generalizes #2955, whose regression test used non-wrapping lines where logical and display rows coincide.

Two optional, non-blocking notes:

  1. (bounds.size.height / line_height) as usize is now duplicated verbatim at both call sites. The coding guides ask to centralize shared geometry, and the adjacent comment already says the two paths must agree — a small shared helper would enforce that.

let top_bottom_margin = cursor_surrounding_padding(
state.mode.is_auto_grow(),
state.cursor_surrounding_lines,
(bounds.size.height / line_height) as usize,
line_height,
);

super::element::cursor_surrounding_padding(
self.mode.is_auto_grow(),
self.cursor_surrounding_lines,
(bounds.size.height / line_height) as usize,
line_height,
)

  1. Pre-existing, but two lines above a modified line: the comment references TextElement::layout_cursor, renamed to layout_cursors in f6e2ed6. Cheap to fix while here.

// Scroll the row into view. Use the same edge clearance helper as
// `TextElement::layout_cursor` so both scroll-into-view paths agree
// (a mismatch flickered on `Down` at end-of-buffer with a small
// `cursor_surrounding_lines` override).

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

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.

2 participants