Skip to content

editor: Align selections on rendered position rather than byte offsets - #62305

Closed
4ktLuffy wants to merge 1 commit into
zed-industries:mainfrom
4ktLuffy:fix/align-selections-rendered-position
Closed

editor: Align selections on rendered position rather than byte offsets#62305
4ktLuffy wants to merge 1 commit into
zed-industries:mainfrom
4ktLuffy:fix/align-selections-rendered-position

Conversation

@4ktLuffy

@4ktLuffy 4ktLuffy commented Aug 7, 2026

Copy link
Copy Markdown

Objective

Fixes #60192.

Editor::align_selections computed padding from Point::column, which is a byte offset. Any
row that is wider in bytes than it is on screen reports a larger column than it occupies, so
the wrong number of spaces is inserted.

Using the example from the issue, where is 3 bytes and π is 2:

a ← 1  # one          prefix:  9 bytes,  7 columns
bc ← π  # two         prefix: 11 bytes,  8 columns

The target is taken as max(9, 11) = 11 bytes, so the first row gains two spaces and lands
at column 9 while the second stays at column 8.

This turned out to be broader than the report. The underlying fault is
bytes-versus-rendered-position, and multi-byte characters are one of three ways to reach it:

Hard tabs. a\t is two bytes but renders out to the tab stop, so byte offsets treat it as
no wider than bb and insert no padding at all:

before          before this PR         after this PR
a→x             a→x                    a→x
bbx             bbx     (unaligned)    bb  x    (aligned)

Rows that were already aligned get pulled apart. ãa and bb both render three columns
wide but are 4 and 3 bytes, so the previous behaviour padded the second row and broke a
correct alignment:

before          before this PR         after this PR
ãa x            ãa x                   ãa x
bb x            bb  x   (broken)       bb x     (unchanged)

Solution

Measure where each cursor actually renders, using x_for_display_point, and convert the gap
back into spaces with the space advance.

This is the same approach as #57097, which fixed the identical problem for columnar selection.
Because padding is always spaces, a row's accumulated shift is exactly
space_count * space_width, so the per-column running offset stays exact.

Testing

test_align_selections_multibyte covers, in order:

  1. A pure-ASCII control, where byte offsets and rendered columns agree. It passes with and
    without the change — it is there to show the test is not simply red.
  2. A 2-byte character before the cursor.
  3. The exact case from the issue.
  4. Rows that are already aligned, which must be left alone.
  5. Several cursors per row, exercising the running per-row offset across more than one column.
  6. Hard tabs.

Reverting only editor.rs and re-running gives:

test editor_tests::test_align_selections_multibyte ... FAILED

Diff < left / right > :
<ãa x
>ãa  x
 bbb x

The two existing align tests (test_align_selections, test_align_selections_multicolumn)
pass unchanged in both directions, which is consistent with them being ASCII-only and never
having exercised this path.

Tested on macOS (Apple Silicon). cargo fmt and cargo clippy -p editor --tests are clean.

Notes for review

  • Monospace assumption. space_width comes from em_layout_width, the em advance rather
    than the measured width of U+0020. They are equal in a monospace font and differ in a
    proportional one — though padding with spaces cannot align proportional text in any case.
    editor: Fix columnar selection alignment on rows with multi-byte chars #57097 makes the same assumption via the same call.
  • Rounding. (gap / space_width).round() is exact when every glyph advance is a whole
    multiple of the space advance, which holds for monospace. A fallback glyph of unusual width
    would round to the nearest space rather than fail loudly.
  • Cost. x_for_display_point is now called once per cursor, where the previous code was
    integer arithmetic. LineLayoutCache (crates/gpui/src/text_system.rs:365) means repeated
    layouts of the same row are cached, so this should not be a per-cursor re-layout.
  • Not covered by tests: folded regions and block rows. Soft wrap was checked manually and
    showed no regression — the previous behaviour was wrong there too — but I did not confirm
    the line had actually wrapped, so I have not shipped that as a test.

One question for maintainers

The tab case now aligns by inserting spaces. Where hard_tabs is enabled, spaces may not
be what you want, even though it matches this action's existing behaviour. Happy to change it
to tabs, or to drop the tab case from this PR and raise it separately — whichever you prefer.

Self-Review Checklist:

  • I've reviewed my own diff for quality, security, and reliability
  • Unsafe blocks (if any) have justifying comments — none added
  • The content adheres to Zed's UI standards — no UI change
  • Tests cover the new/changed behavior
  • Performance impact has been considered and is acceptable

Showcase

Aligning the # symbols in the issue's example:

before                  before this PR          after this PR
a ← 1  # one            a ← 1    # one          a ← 1   # one
bc ← π  # two           bc ← π  # two           bc ← π  # two

`align_selections` computed padding from `Point::column`, which is a byte
offset, so any row wider in bytes than it is on screen reported a larger
column than it occupied and received the wrong number of spaces.

Using the example from zed-industries#60192, where `←` is 3 bytes and `π` is 2:

    a ← 1  # one          prefix:  9 bytes,  7 columns
    bc ← π  # two         prefix: 11 bytes,  8 columns

The target was taken as max(9, 11) = 11 bytes, so the first row gained two
spaces and landed at column 9 while the second stayed at column 8.

Measure where each cursor actually renders instead, the same approach
zed-industries#57097 used for columnar selection. Padding is always spaces, so a row's
accumulated shift is exactly space_count * space_width and the per-column
running offset stays exact.

This is not only a multi-byte problem. Two further cases were found while
testing and are covered by the new test:

- Hard tabs. "a\t" is two bytes but renders out to the tab stop, so byte
  offsets treated it as no wider than "bb" and inserted no padding at all.
- Rows that were already aligned were pulled apart. "ãa " and "bb " both
  render three columns wide but are 4 and 3 bytes, so the second row was
  padded and a correct alignment was broken.

test_align_selections_multibyte covers a pure-ASCII control that passes
with and without the change, a 2-byte character, the reported case, the
already-aligned case, several cursors per row, and hard tabs. Reverting
only editor.rs makes it fail; the two existing align tests are unaffected
in both directions.
@cla-bot cla-bot Bot added the cla-signed The user has signed the Contributor License Agreement label Aug 7, 2026
@zed-community-bot zed-community-bot Bot added the first contribution the author's first pull request to Zed. NOTE: the label application is automated via github actions label Aug 7, 2026
@smitbarmase smitbarmase added the area:editor Feedback for code editing, formatting, editor iterations, etc label Aug 7, 2026
@SomeoneToIgnore

Copy link
Copy Markdown
Contributor

Thank you, closing in favor of already created #61997

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

Labels

area:editor Feedback for code editing, formatting, editor iterations, etc cla-signed The user has signed the Contributor License Agreement first contribution the author's first pull request to Zed. NOTE: the label application is automated via github actions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"Align selections" fails with multi-byte characters

3 participants