Skip to content

fix(editor): scroll jumps to end after IME commit (Pinyin / Rime)#1019

Merged
datlechin merged 1 commit intomainfrom
fix/ime-commit-scroll-jump
May 6, 2026
Merged

fix(editor): scroll jumps to end after IME commit (Pinyin / Rime)#1019
datlechin merged 1 commit intomainfrom
fix/ime-commit-scroll-jump

Conversation

@datlechin
Copy link
Copy Markdown
Member

Summary

Fixes #1012. Typing a Chinese word (Pinyin or Rime) in the SQL editor scrolled to the end of the script after committing the candidate. Same shape happens for any IME-backed input: option-e then é, Japanese kana, etc.

TextView.insertText(_:replacementRange:) was doing two edits per IME commit:

  1. unmarkText()replaceCharacters(in: markedRanges, with: "") to wipe the marked text.
  2. _insertTextreplaceCharacters(in: replacementRange, with: "测试") with the same range AppKit had supplied at commit time.

By the time edit #2 ran, the document had shrunk by markedRanges.totalLength. The range AppKit gave us pointed at characters that no longer existed:

  • Mid-document marked text → edit [WIP] Improve system keyboard shortcut functionality #2 ate whatever happened to follow the wiped marked range. Test imeCommitInTheMiddleDoesNotCorruptText reproduces: typed marked "ceshi" at offset 6 of "alpha\n\nbeta" then committed "测试" → string came out as "alpha\\n测试" instead of "alpha\\n测试\\nbeta" (the \\nbeta portion was eaten).
  • End-of-document marked text → edit [WIP] Improve system keyboard shortcut functionality #2's range was out of bounds; TextStory raised Range invalid for string. The test runner caught it as a fatal error. In the released app the failure mode is "cursor lands at documentLength, scroll jumps there" — same root cause.

insertText now resolves the effective range(s) before clearing marked-text bookkeeping and performs one replaceCharacters call. Multi-cursor IME (where each cursor gets its own marked range) replaces every range in one pass.

Latent bug exposed by the fix

TextSelectionManager.didReplaceCharacters had the wrong delta formula:

```swift
let delta = replacementLength == 0 ? -range.length : replacementLength // before
let delta = replacementLength - range.length // after
```

Selections at offsets past range.max shift by delta. The old formula dropped the chars-removed term, so a same-length replace (e.g. "´" → "é" in the multi-cursor IME path) over-shifted later selections by range.length. The unmark-then-insert flow happened to mask this because all of its replaces had replacementLength == 0. With the single-edit IME path, the bug surfaced and broke the existing test_markedTextMultipleSelection. Same-length replaces now leave selections past the replace point unchanged.

Test plan

  • New IMEInputTests — 5 cases covering middle-of-doc commit, end-of-doc commit, NSNotFound replacement range, marked-state cleanup, and the plain Latin path. All fail on `main`; all pass on this branch.
  • All 6 existing MarkedTextTests (multi-cursor IME) still pass.
  • Full `swift test` suite: 81 swift-testing + 44 XCTest, all green.
  • `xcodebuild ... build` succeeds.
  • `swiftlint --strict` clean on all touched files.
  • Manual on the test fixture: open a SQL script, place caret on a blank line in the middle, type "ceshi" via Pinyin, press space to commit "测试". Caret stays at the inserted text; scroll position stays put.

@chatgpt-codex-connector
Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@datlechin datlechin merged commit 12aff48 into main May 6, 2026
2 checks passed
@datlechin datlechin deleted the fix/ime-commit-scroll-jump branch May 6, 2026 04:53
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.

SQL editor scroll to end after inputing Chinese character

1 participant