Skip to content

Position editor lines by their real heights when they vary - #1084

Open
webergnr wants to merge 3 commits into
lapce:mainfrom
webergnr:variable-line-height
Open

Position editor lines by their real heights when they vary#1084
webergnr wants to merge 3 commits into
lapce:mainfrom
webergnr:variable-line-height

Conversation

@webergnr

Copy link
Copy Markdown

Position editor lines by their real heights when they vary

The bug

normal_compute_screen_lines places every visual line at y_idx * line_height,
where line_height is that line's height — so the grid step comes from
whichever line is being placed. The code says as much:

// TODO: don't assume universal line height!
let line_height = style.line_height(editor.id(), 0);

With a uniform styling this is correct and O(1). As soon as Styling::line_height
varies per line — a markdown editor sizing headings, a diff view with a taller
row — lines overlap: a 31px heading followed by a 20px body line puts the body at
1 * 20 = 20, eleven pixels inside the heading. line_col_of_point, the document
height and ensure_visible divide or multiply by the same single height, so
hit-testing and caret-following drift with it.

The fix

A Styling::uniform_line_height(edid) -> bool hint, defaulting to true:
every existing styling keeps the arithmetic path exactly as it is today, and
nothing gets slower. A styling that returns false gets positions accumulated
from the real heights:

  • Editor::line_y / rvline_y / rvline_at_y / line_height_total /
    total_height walk the heights.
  • Wrapped lines are included: a line's height is line_height × rows, and the
    row count is read off the cached text layout via Lines::cached_line_count
    a line with no layout yet counts as one, the same assumption
    Lines::last_vline already makes, so no layout is forced and the accuracy
    model is unchanged.
  • normal_compute_screen_lines starts from the visual line containing y0 and
    stops at the first row past y1 instead of counting a vline span.
  • The same walk backs line_col_of_point(_with_phantom), EditorView's and the
    gutter's content height, and the ensure_visible rect.

The cost when the hint is false is a walk over the lines above the viewport,
the same order of work Lines::last_vline already does when wrapping is on.

Tests

row_at_y — the walk itself — is a free function with four tests: a uniform
document lands where division would, a tall line pushes the ones under it down,
a wrapped line owns a row per wrap, and past the last line there is no row. The
existing suite (107) stays green.

Where it is used

A markdown notes editor whose buffer holds headings and body text in one
text_editor, at the sizes their own styling gives them: before this, the body
line under every heading was drawn inside it. Happy to add before/after
screenshots, a Styling example, or a changelog entry if you'd like them here.

…pped

normal_compute_screen_lines placed every visual line at index * its own
line height, which is only correct when all lines share one height (the
in-code TODO acknowledged this). With WrapMethod::None every buffer line
is one visual line, so real heights can be summed without forcing text
layouts: screen line positions, point<->line hit testing and the total
document height now accumulate style.line_height per line. Wrapped
editors keep the uniform-grid behaviour unchanged.
Copilot AI lite review requested due to automatic review settings August 21, 2026 20:37

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds opt-in variable-height editor positioning while preserving the uniform-height fast path.

Changes:

  • Adds uniform_line_height support and cached wrapped-row counting.
  • Updates rendering, hit-testing, caret visibility, and content sizing.
  • Adds tests and changelog documentation.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Summary
src/views/editor/visual_line.rs Adds cached visual-line counts.
src/views/editor/view.rs Updates sizing and caret visibility. Moderate issue: bottom-scroll margin uses the first line’s height instead of the last line’s height.
src/views/editor/text.rs Adds the styling capability hint.
src/views/editor/mod.rs Implements variable-height positioning and hit-testing. Moderate issue: lazy layout creation can offset positions when the created line has a different height.
src/views/editor/gutter.rs Uses accumulated content height. Moderate issue: bottom-scroll margin uses the first line’s height instead of the last line’s height.
CHANGELOG.md Documents the feature.
Suppressed comments (4)

src/views/editor/mod.rs:1724

  • rvline_at_y reads cached_line_count before the cache is invalidated at line 1715. After an edit or config change, it can return a stale wrapped line_index; iter_rvlines_init then clears the cache and treats that index as past the newly unlaid line, so this branch can produce no screen rows. Validate the cache, including config_id, before calling rvline_at_y.
        let start = editor.rvline_at_y(y0);

src/views/editor/mod.rs:779

  • With this mode enabled, page_move, center_window, top_of_window, bottom_of_window, and scroll still use line_height(0) for document positions and scroll deltas (mod.rs:665-750). A variable-height editor therefore scrolls using a different coordinate system than the rows computed here, leaving the cursor at the wrong vertical position. These commands need accumulated line positions, or the hint must be explicitly limited.
    pub fn per_line_heights_active(&self) -> bool {
        !self.style().uniform_line_height(self.id())
    }

src/views/editor/mod.rs:816

  • The variable branch recalculates every buffer line on each call, but total_height() is used by both editor layout/measure paths and the gutter measure path. This makes large variable-height documents pay O(document size) repeatedly during layout, rather than the viewport-prefix walk described for this feature; unlike Lines::last_vline, there is no cached result. Cache the total/prefix heights and invalidate them with document, style, and layout-cache changes.
        if self.per_line_heights_active() {
            (0..=self.last_line())
                .map(|line| self.line_height_total(line))
                .sum()

src/views/editor/view.rs:1002

  • total_height() now accounts for per-line heights, but the scroll-beyond-last-line margin still subtracts the first line's height. When the first line is taller than the last line, the bottom margin is too small to scroll the final row to the top. Use editor.line_height(editor.last_line()) for this margin.
            let last_line_height = editor.total_height();

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@@ -225,7 +225,7 @@ impl EditorGutterView {

// Height is determined by editor content
let line_height = f64::from(editor.line_height(0));
Comment thread src/views/editor/mod.rs
Comment on lines +777 to +779
pub fn per_line_heights_active(&self) -> bool {
!self.style().uniform_line_height(self.id())
}
Comment thread src/views/editor/view.rs
};

let last_line_height = line_height * (editor.last_vline().get() + 1) as f64;
let last_line_height = editor.total_height();
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