feat: add g_ motion (move to last non-blank character) - #262
Conversation
Implements the g_ motion from vim, which moves to the last non-whitespace character on the line. Supports counts (e.g. 2g_ moves to the last non-blank of the next line) and works correctly with operators (dg_ deletes to last non-blank, preserving trailing whitespace).
There was a problem hiding this comment.
Pull request overview
Adds Vim’s g_ motion to move the cursor to the last non-blank character on a line (inclusive), including count support and operator compatibility, aligning behavior with existing whitespace-aware motions.
Changes:
- Register
g_as an inclusive motion mapped tomoveToLastNonWhiteSpaceCharacter. - Implement
moveToLastNonWhiteSpaceCharacterandfindLastNonWhiteSpaceCharacter()helper. - Add automated tests covering basic movement, count usage, and
dg_behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/vim.js |
Adds the g_ keybinding, implements the motion, and introduces a helper to find the last non-whitespace character. |
test/vim_test.js |
Adds a new test case for g_ movement, count behavior, and delete-operator interaction. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| var lineText = cm.getLine(line); | ||
| var lastNonWS = findLastNonWhiteSpaceCharacter(lineText); | ||
| return new Pos(line, Math.max(0, lastNonWS)); |
There was a problem hiding this comment.
moveToLastNonWhiteSpaceCharacter computes line = head.line + motionArgs.repeat - 1 and immediately calls cm.getLine(line) without clipping the line number to the document bounds. With a large count (e.g. 100g_ near EOF) this can produce an out-of-range line and return a Pos with an invalid line, which then flows into operator selections (dg_, etc.) without being clipped and can break selection logic. Clamp the target line to [cm.firstLine(), cm.lastLine()] (or use cm.clipPos(new Pos(line, 0)).line) before calling getLine and before returning the Pos.
| var lineText = cm.getLine(line); | |
| var lastNonWS = findLastNonWhiteSpaceCharacter(lineText); | |
| return new Pos(line, Math.max(0, lastNonWS)); | |
| var clippedLine = cm.clipPos(new Pos(line, 0)).line; | |
| var lineText = cm.getLine(clippedLine); | |
| var lastNonWS = findLastNonWhiteSpaceCharacter(lineText); | |
| return new Pos(clippedLine, Math.max(0, lastNonWS)); |
| testVim('g_', function(cm, vim, helpers) { | ||
| // Test basic g_ on line with trailing spaces | ||
| cm.setCursor(0, 0); | ||
| helpers.doKeys('g', '_'); | ||
| helpers.assertCursorAt(0, 2); | ||
| // Test g_ on line without trailing spaces | ||
| cm.setCursor(1, 0); | ||
| helpers.doKeys('g', '_'); | ||
| helpers.assertCursorAt(1, 2); | ||
| // Test g_ with count (2g_ goes to last non-whitespace of next line) | ||
| cm.setCursor(0, 0); | ||
| helpers.doKeys('2', 'g', '_'); | ||
| helpers.assertCursorAt(1, 2); | ||
| // Test dg_ deletes to last non-whitespace (preserving trailing spaces) | ||
| cm.setCursor(0, 0); | ||
| helpers.doKeys('d', 'g', '_'); | ||
| eq(' \nfoo', cm.getValue()); | ||
| }, { value: 'foo \nfoo' }); |
There was a problem hiding this comment.
The new g_ tests cover a small count (2g_) but don’t cover boundary behavior when the count would move past the end of the document. Since g_ is countable, adding a test like 100g_ on a short buffer (and asserting it lands on the last line’s last non-blank without throwing) would help lock in correct clipping behavior.
| var line = head.line + motionArgs.repeat - 1; | ||
| var lineText = cm.getLine(line); | ||
| var lastNonWS = findLastNonWhiteSpaceCharacter(lineText); | ||
| return new Pos(line, Math.max(0, lastNonWS)); |
There was a problem hiding this comment.
question: do we need the conditional linewise case for counted operators here? In Vim, d2g_ from the first nonblank of a\nlast \nthird deletes the first two complete lines, while this implementation leaves the trailing spaces from the second line. Starting inside the first line remains characterwise, so this seems to need a conditional rather than linewise: true on the mapping.
~ written by Zerg 👾 (valiant-disruptor-aba4)
| return 0; | ||
| } | ||
| var index = text.search(/\s+$/); | ||
| return index == -1 ? text.length - 1 : index - 1; |
There was a problem hiding this comment.
question: can this return the start of the final code point? With x😀 from column 0, this returns the low-surrogate index. Cursor clipping then lands on the first trailing space, so vg_ includes that space; Vim stops on the emoji.
~ written by Zerg 👾 (valiant-disruptor-aba4)
Manual Review
I tested using this motion in the replit link -- it worked.
Adds the
g_motion, which moves the cursor to the last non-whitespace character on the line.Behavior
g_moves to the last non-blank character of the current line2g_moves to the last non-blank of the next line (count - 1 lines down)dg_deletes to the last non-blank, preserving trailing whitespaceThis is the counterpart to
^(first non-blank), which is already implemented. Currentlyg_is unmapped and does nothing.Implementation
findLastNonWhiteSpaceCharacter()mirrors the existingfindFirstNonWhiteSpaceCharacter()moveToLastNonWhiteSpaceCharacterregistered withinclusive: trueTest plan
vim_test.jsOpen web-demo @ d6eeff49181ef1b2a41ff8f40aba9e6ee427cc96