Merge pull request #770 from jacekkopecky/ensure-cursor-on-line

use onDidChangeCursorPosition for keeping cursor in line

Author: Max Brunsfeld

lib/motions/general-motions.coffee

@@ -88,15 +88,6 @@ class Motion
   moveSelection: (selection, count, options) ->
     selection.modifySelection => @moveCursor(selection.cursor, count, options)
 
-  ensureCursorIsWithinLine: (cursor) ->
-    return if @vimState.mode is 'visual' or not cursor.selection.isEmpty()
-    {goalColumn} = cursor
-    {row, column} = cursor.getBufferPosition()
-    lastColumn = cursor.getCurrentLineBufferRange().end.column
-    if column >= lastColumn - 1
-      cursor.setBufferPosition([row, Math.max(lastColumn - 1, 0)])
-    cursor.goalColumn ?= goalColumn
-
   isComplete: -> true
 
   isRecordable: -> false
@@ -142,9 +133,8 @@ class MoveLeft extends Motion
   operatesInclusively: false
 
   moveCursor: (cursor, count=1) ->
-    _.times count, =>
+    _.times count, ->
       cursor.moveLeft() if not cursor.isAtBeginningOfLine() or settings.wrapLeftRightMotion()
-      @ensureCursorIsWithinLine(cursor)
 
 class MoveRight extends Motion
   operatesInclusively: false
@@ -159,16 +149,14 @@ class MoveRight extends Motion
 
       cursor.moveRight() unless cursor.isAtEndOfLine()
       cursor.moveRight() if wrapToNextLine and cursor.isAtEndOfLine()
-      @ensureCursorIsWithinLine(cursor)
 
 class MoveUp extends Motion
   operatesLinewise: true
 
   moveCursor: (cursor, count=1) ->
-    _.times count, =>
+    _.times count, ->
       unless cursor.getScreenRow() is 0
         cursor.moveUp()
-        @ensureCursorIsWithinLine(cursor)
 
 class MoveDown extends Motion
   operatesLinewise: true
@@ -177,7 +165,6 @@ class MoveDown extends Motion
     _.times count, =>
       unless cursor.getScreenRow() is @editor.getLastScreenRow()
         cursor.moveDown()
-        @ensureCursorIsWithinLine(cursor)
 
 class MoveToPreviousWord extends Motion
   operatesInclusively: false
@@ -328,10 +315,9 @@ class MoveToLastCharacterOfLine extends Motion
   operatesInclusively: false
 
   moveCursor: (cursor, count=1) ->
-    _.times count, =>
+    _.times count, ->
       cursor.moveToEndOfLine()
       cursor.goalColumn = Infinity
-      @ensureCursorIsWithinLine(cursor)
 
 class MoveToLastNonblankCharacterOfLineAndDown extends Motion
   operatesInclusively: true

lib/vim-state.coffee

@@ -39,6 +39,9 @@ class VimState
         @activateVisualMode('characterwise') if @mode is 'normal'
     , 100)
 
+    @subscriptions.add @editor.onDidChangeCursorPosition ({cursor}) => @ensureCursorIsWithinLine(cursor)
+    @subscriptions.add @editor.onDidAddCursor @ensureCursorIsWithinLine
+
     @editorElement.classList.add("vim-mode")
     @setupNormalMode()
     if settings.startInInsertMode()
@@ -212,28 +215,33 @@ class VimState
   # it.
   pushOperations: (operations) ->
     return unless operations?
-    operations = [operations] unless _.isArray(operations)
-
-    for operation in operations
-      # Motions in visual mode perform their selections.
-      if @mode is 'visual' and (operation instanceof Motions.Motion or operation instanceof TextObjects.TextObject)
-        operation.execute = operation.select
-
-      # if we have started an operation that responds to canComposeWith check if it can compose
-      # with the operation we're going to push onto the stack
-      if (topOp = @topOperation())? and topOp.canComposeWith? and not topOp.canComposeWith(operation)
-        @resetNormalMode()
-        @emitter.emit('failed-to-compose')
-        break
+    try
+      @processing = true
+      operations = [operations] unless _.isArray(operations)
+
+      for operation in operations
+        # Motions in visual mode perform their selections.
+        if @mode is 'visual' and (operation instanceof Motions.Motion or operation instanceof TextObjects.TextObject)
+          operation.execute = operation.select
+
+        # if we have started an operation that responds to canComposeWith check if it can compose
+        # with the operation we're going to push onto the stack
+        if (topOp = @topOperation())? and topOp.canComposeWith? and not topOp.canComposeWith(operation)
+          @resetNormalMode()
+          @emitter.emit('failed-to-compose')
+          break
 
-      @opStack.push(operation)
+        @opStack.push(operation)
 
-      # If we've received an operator in visual mode, mark the current
-      # selection as the motion to operate on.
-      if @mode is 'visual' and operation instanceof Operators.Operator
-        @opStack.push(new Motions.CurrentSelection(@editor, this))
+        # If we've received an operator in visual mode, mark the current
+        # selection as the motion to operate on.
+        if @mode is 'visual' and operation instanceof Operators.Operator
+          @opStack.push(new Motions.CurrentSelection(@editor, this))
 
-      @processOpStack()
+        @processOpStack()
+    finally
+      @processing = false
+      @ensureCursorIsWithinLine(cursor) for cursor in @editor.getCursors()
 
   onDidFailToCompose: (fn) ->
     @emitter.on('failed-to-compose', fn)
@@ -654,6 +662,16 @@ class VimState
     text = @getRegister(name)?.text
     @editor.insertText(text) if text?
 
+  ensureCursorIsWithinLine: (cursor) =>
+    return if @processing or @mode isnt 'normal'
+
+    {goalColumn} = cursor
+    if cursor.isAtEndOfLine() and not cursor.isAtBeginningOfLine()
+      @processing = true # to ignore the cursor change (and recursion) caused by the next line
+      cursor.moveLeft()
+      @processing = false
+    cursor.goalColumn = goalColumn
+
 # This uses private APIs and may break if TextBuffer is refactored.
 # Package authors - copy and paste this code at your own risk.
 getChangesSinceCheckpoint = (buffer, checkpoint) ->

spec/motions-spec.coffee

@@ -145,30 +145,21 @@ describe "Motions", ->
         keydown('w')
         expect(editor.getCursorScreenPosition()).toEqual [2, 0]
 
-        # FIXME: The definition of Cursor#getEndOfCurrentWordBufferPosition,
-        # means that the end of the word can't be the current cursor
-        # position (even though it is when your cursor is on a new line).
-        #
-        # Therefore it picks the end of the next word here (which is [3,3])
-        # to start looking for the next word, which is also the end of the
-        # buffer so the cursor never advances.
-        #
-        # See atom/vim-mode#3
         keydown('w')
         expect(editor.getCursorScreenPosition()).toEqual [3, 0]
 
         keydown('w')
-        expect(editor.getCursorScreenPosition()).toEqual [3, 3]
+        expect(editor.getCursorScreenPosition()).toEqual [3, 2]
 
-        # After cursor gets to the EOF, it should stay there.
+        # When the cursor gets to the EOF, it should stay there.
         keydown('w')
-        expect(editor.getCursorScreenPosition()).toEqual [3, 3]
+        expect(editor.getCursorScreenPosition()).toEqual [3, 2]
 
       it "moves the cursor to the end of the word if last word in file", ->
         editor.setText("abc")
         editor.setCursorScreenPosition([0, 0])
         keydown('w')
-        expect(editor.getCursorScreenPosition()).toEqual([0, 3])
+        expect(editor.getCursorScreenPosition()).toEqual([0, 2])
 
     describe "as a selection", ->
       describe "within a word", ->
@@ -464,7 +455,7 @@ describe "Motions", ->
         editor.setCursorScreenPosition([1, 10])
         keydown('y')
         keydown('B', shift: true)
-        expect(vimState.getRegister('"').text).toBe 'xyz-123'
+        expect(vimState.getRegister('"').text).toBe 'xyz-12' # because cursor is on the `3`
 
       it "doesn't go past the beginning of the file", ->
         editor.setCursorScreenPosition([0, 0])
@@ -966,7 +957,7 @@ describe "Motions", ->
       beforeEach -> keydown('G', shift: true)
 
       it "moves the cursor to the last line after whitespace", ->
-        expect(editor.getCursorScreenPosition()).toEqual [3, 1]
+        expect(editor.getCursorScreenPosition()).toEqual [3, 0]
 
     describe "as a repeated motion", ->
       beforeEach ->
@@ -1263,14 +1254,9 @@ describe "Motions", ->
 
         it "doesn't move cursor unless next match has exact word ending", ->
           editor.setText("abc\n@def\nabc\n@def1\n")
-          # FIXME: I suspect there is a bug laying around
-          # Cursor#getEndOfCurrentWordBufferPosition, this function
-          # is returning '@' as a word, instead of returning the whole
-          # word '@def', this behavior is avoided in this test, when we
-          # execute the '*' command when cursor is on character after '@'
-          # (in this particular example, the 'd' char)
           editor.setCursorBufferPosition([1, 1])
           keydown("*")
+          # this is because of the default isKeyword value of vim-mode that includes @
           expect(editor.getCursorBufferPosition()).toEqual [1, 0]
 
         # FIXME: This behavior is different from the one found in

spec/scroll-spec.coffee

@@ -176,7 +176,7 @@ describe "Scrolling", ->
         expect(editor.getCursorBufferPosition()).toEqual [0, 1]
         pos10 = zsPos(10)
         expect(pos10).toEqual(startPosition)
-        expect(editor.getCursorBufferPosition()).toEqual [0, 5]
+        expect(editor.getCursorBufferPosition()).toEqual [0, 4]
 
 
     describe "the ze keybinding", ->
@@ -227,4 +227,4 @@ describe "Scrolling", ->
         expect(editor.getCursorBufferPosition()).toEqual [0, 1]
         pos10 = zePos(10)
         expect(pos10).toEqual(startPosition)
-        expect(editor.getCursorBufferPosition()).toEqual [0, 5]
+        expect(editor.getCursorBufferPosition()).toEqual [0, 4]

spec/vim-state-spec.coffee

@@ -154,8 +154,7 @@ describe "VimState", ->
     describe "with content", ->
       beforeEach -> editor.setText("012345\n\nabcdef")
 
-      # FIXME: See atom/vim-mode#2
-      xdescribe "on a line with content", ->
+      describe "on a line with content", ->
         beforeEach -> editor.setCursorScreenPosition([0, 6])
 
         it "does not allow the cursor to be placed on the \n character", ->