Fix cursor relocation when inside removed text by masmu · Pull Request #3977 · micro-editor/micro

masmu · 2026-01-27T09:56:07Z

This PR fixes a bug in the buffer event handler where cursors positioned strictly inside a removed text range (start < cursor < end) were not correctly updated.

Previously, the DoTextEvent logic for TextEventRemove handled cursors after the removed range and cursors at the end of the range, but fell through for cursors residing inside the range. This caused such cursors to retain their original (x, y) coordinates. Since the text before them was deleted, retaining the old coordinates meant the cursor effectively "jumped" forward relative to the content.

Reproduction:
Without the Lua plugin system, it is difficult to trigger the bug, but it can be easily reproduced with the following code:

VERSION = "1.0.0"

local micro = import("micro")
local buffer = import("micro/buffer")

function postinit()
    local bp = micro.CurPane()
    if bp.Buf:Size() > 0 then
        return
    end
    testCase1(bp)
end

function testCase1(bp)
    bp.Buf:Insert(buffer.Loc(0, 0), "AABBCC")

    -- Place cursor at (3, 0) - Between the two Bs
    -- A A B | B C C
    -- 0 1 2 3 4 5 6
    local c = bp.Buf:GetActiveCursor()
    c.Loc = buffer.Loc(3, 0)

    micro.Log("Start: Cursor at " .. c.Loc.X .. "," .. c.Loc.Y)
    bp.Buf:Remove(buffer.Loc(2, 0), buffer.Loc(4, 0))

    -- Without the fix (missing lines 97-98 in eventhandler.go):
    -- The cursor is "greater than start (2)" BUT NOT "greater than end (4)".
    -- The logic falls through, and the cursor position is NOT updated.
    -- It remains at (3,0).

    -- With the fix:
    -- The cursor is detected as being inside the removed range and clamped to start (2,0).

    if c.Loc.X == 3 then
        micro.InfoBar():Message(
            "BUG DETECTED: Cursor stayed at index 3 (Should be 2)")
    elseif c.Loc.X == 2 then
        micro.InfoBar():Message(
            "NO BUG DETECTED: Cursor correctly moved to index 2")
    else
        micro.InfoBar():Message("Unexpected: Cursor at " .. c.Loc.X)
    end

    micro.Log("End: Cursor at " .. c.Loc.X .. "," .. c.Loc.Y)
end

Andriamanitra · 2026-01-27T11:26:01Z

Similar bugs exist for replace/replaceall commands. This PR doesn't fix those because they don't use DoTextEvent (they go through eh.MultipleReplace -> eh.Execute -> ExecuteTextEvent).

masmu · 2026-01-27T12:27:51Z

Thanks for the review. You are correct that Replace / ReplaceAll (via MultipleReplace) bypass DoTextEvent and thus aren't covered by this fix.

However, the primary goal of this PR was to fix the cursor desync issues causing annoying cursor jumps.

Fixing MultipleReplace (which supports multiple deltas and skips DoTextEvent entire logic) seems like a larger, separate task. Would you agree?

Andriamanitra · 2026-01-27T13:18:44Z

That's fair. I pointed it out because that's the issue I first assumed this PR was fixing, and I was surprised to see no change in behavior when I went to test it.

masmu · 2026-01-27T13:41:52Z

No worries, that makes sense.

Would you be able to review this PR? I was hoping to get this merged soon, as this fix is a prerequisite for some more bug fixes I have already prepared.

JoeKar · 2026-01-29T06:20:36Z

I had no time to do the review yet, but absolutely great to see an official test for this! 👍

dmaluka · 2026-10-30T19:02:15Z

 				} else if loc.Y == end.Y && loc.GreaterEqual(end) {
 					loc = loc.MoveLA(-DiffLA(start, end, eh.buf.LineArray), eh.buf.LineArray)
+				} else if loc.GreaterThan(start) {
+					loc = start


The entire fix is this 2-line change, the rest of this commit is just adding tests?
So move those tests to a separate commit (so that it is immediately seen what exactly the fix is)?

...Also nit: could you wrap lines in the commit message, e.g. with 72 characters limit?

Yes, just those 2 lines.

...Also nit: could you wrap lines in the commit message, e.g. with 72 characters limit?

Sure.

Thank you.
Shall we switch the two commits to introduce the fix first before we add the regression test?

You are welcome.

That has been holding up a refactoring for almost a year now. No offense to you guys. I know you are doing your best.

Shall we switch the two commits to introduce the fix first before we add the regression test?

Whatever works best for you.

Whatever works best for you.

This is how I would do it: First the fix, then the test.
We're just about to cross the finish line. Let's cross it. 😅

Yep, if the test is introduced first, it causes a temporary regression of make test in the 1st commit (fixed only by the 2nd commit). That is not a huge problem, but still potentially might cause unnecessary troubles when bisecting.

dmaluka · 2026-10-30T19:03:45Z

Apologies for long delayed review.

The fix looks good to me, thanks. Please just split the 2nd commit to separate the actual fix from the tests, as I asked above.

I haven't reviewed the tests yet.

dmaluka · 2026-10-30T19:05:11Z

BTW a simple lua function for testing the fix (can be bound to a key via bind Alt-r lua:initlua.removeAround or whatever):

function removeAround(bp)
    bp.Buf:Remove(bp.Cursor.Loc:Move(-2, bp.Buf), bp.Cursor.Loc:Move(2, bp.Buf))
end

dmaluka · 2026-11-10T01:49:33Z

+	}
+}
+
+func TestReplaceMultiLineCursorMovement(t *testing.T) {


Why MultiLine if it only deals with a single line?

True. It should be probably named something like TestReplaceSingleLineCursorAndSelectionShift.

Is that okay?

AFAICS what this test does is: replace text which overlaps with the selection (but doesn't contain the cursor position). The name TestReplaceSingleLineCursorAndSelectionShift doesn't quite reflect that? The selection is not just shifted, it is modified?

Also why need to emphasize SingleLine? E.g. the above and below test names TestReplaceWithSelection and TestReplaceSelectedTextCursorPosition don't emphasize it?

Yep, good point. What do think of TestReplaceOverlappingSelection?

dmaluka · 2026-11-10T01:56:30Z

 				} else if loc.Y == end.Y && loc.GreaterEqual(end) {
 					loc = loc.MoveLA(-DiffLA(start, end, eh.buf.LineArray), eh.buf.LineArray)
+				} else if loc.GreaterThan(start) {
+					loc = start


Yep, if the test is introduced first, it causes a temporary regression of make test in the 1st commit (fixed only by the 2nd commit). That is not a huge problem, but still potentially might cause unnecessary troubles when bisecting.

Previously, if a cursor was positioned strictly inside a range of text being removed (start < cursor < end), the event handler failed to update its position. This resulted in the cursor retaining its old X/Y coordinates, effectively "jumping" forward into subsequent text. The fix ensures that any cursor inside the removed region is clamped to the start of the deletion.

dmaluka · 2026-11-11T19:47:10Z

+	expectedLoc1 := Loc{5, 0}
+	if cursor1.Loc != expectedLoc1 {
+		t.Errorf("Expected cursor1 at %v, got %v", expectedLoc1, cursor1.Loc)
+	}


Any reason why not check cursor1.Selection here?

Probably just missing, I am gonna add the selection assertion for cursor1. 👍

dmaluka · 2026-11-11T21:36:06Z

+	cursor.Num = 0
+	buf.SetCursors([]*Cursor{cursor})
+
+	buf.Remove(Loc{11, 0}, Loc{13, 0})


12..13 would be a more interesting test?

Yes. I would vote to add a new test for this and update the name of the current test to mirror the names of RemoveAtEOLNoTrailingWs / RemoveAtEOLLeavingTrailingWs.

So:

TestUpdateTrailingWsRemoveWsOnlyAtEOLLeavingNoTrailingWs

TestUpdateTrailingWsRemoveWsOnlyAtEOLLeavingTrailingWs

The names get long though.

dmaluka · 2026-11-11T21:41:18Z

+
+func TestUpdateTrailingWsLineShiftInsert(t *testing.T) {
+	buf := NewBufferFromString("line1\nline2  \nline3", "", BTDefault)
+	cursor := NewCursor(buf, Loc{8, 1})


We don't have as many as 8 characters at line 1?

That's right, good catch! It was actually supposed to be {7, 1}. I'll fix that.

dmaluka · 2026-11-11T21:41:46Z

+
+func TestUpdateTrailingWsLineShiftRemove(t *testing.T) {
+	buf := NewBufferFromString("line1\nline2\nline3  \nline4", "", BTDefault)
+	cursor := NewCursor(buf, Loc{8, 2})


We don't have as many as 8 characters at line 2?

Here was well! It was actually supposed to be {7, 2}. I'll fix that.

dmaluka · 2026-11-11T22:05:47Z

+	buf.Replace(Loc{6, 0}, Loc{11, 0}, "test")
+
+	if cursor.NewTrailingWsY != -1 {
+		t.Errorf("Expected NewTrailingWsY to be -1 (cursor not at EOL after replace), got %d", cursor.NewTrailingWsY)


Cursor is actually still at EOL after replace in this test?

Ehm ... yes? 🤔

Before the replacement: Test string's width = 13, we place the cursor to EOL {13, 0}.
We replace world with test, which is one character shorter than before.
So the new EOL is {12, 0}. The cursor remains there.

Have I completely misunderstood something here?

Ahh, that's not clear from the message. Should we maybe change the text to (cursor not at insert end)?

- Basic insert/remove/replace operations - Trailing whitespace updates - Multiple cursors inside, before and after removed regions - Cursors spanning across removal boundaries - Multiline replacements

JoeKar approved these changes Jan 29, 2026

View reviewed changes

JoeKar requested a review from dmaluka July 15, 2026 17:59

dmaluka reviewed Oct 30, 2026

View reviewed changes

masmu force-pushed the fix/cursor-relocation-removed-text branch from 8dcf9a8 to a5baf1c Compare November 6, 2026 08:45

dmaluka reviewed Nov 10, 2026

View reviewed changes

masmu force-pushed the fix/cursor-relocation-removed-text branch from a5baf1c to 5348393 Compare November 10, 2026 13:17

dmaluka reviewed Nov 11, 2026

View reviewed changes

test: add regression tests for buffer event handler logic

7d5e0df

- Basic insert/remove/replace operations - Trailing whitespace updates - Multiple cursors inside, before and after removed regions - Cursors spanning across removal boundaries - Multiline replacements

masmu force-pushed the fix/cursor-relocation-removed-text branch from 5348393 to 7d5e0df Compare November 13, 2026 08:17

Conversation

masmu commented Jan 27, 2026

Uh oh!

Andriamanitra commented Jan 27, 2026

Uh oh!

masmu commented Jan 27, 2026

Uh oh!

Andriamanitra commented Jan 27, 2026

Uh oh!

masmu commented Jan 27, 2026

Uh oh!

JoeKar commented Jan 29, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmaluka commented Oct 30, 2026

Uh oh!

dmaluka commented Oct 30, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants