Merge pull request #30 from github/improve-undo-manager

Improve undo manager

.atom/atom.coffee

@@ -0,0 +1,10 @@
+requireExtension 'autocomplete'
+requireExtension 'strip-trailing-whitespace'
+requireExtension 'fuzzy-finder'
+requireExtension 'tree-view'
+requireExtension 'command-panel'
+requireExtension 'keybindings-view'
+requireExtension 'snippets'
+
+# status-bar is a bit broken until webkit gets a decent flexbox implementation
+# requireExtension 'status-bar'

.atom/snippets/coffee.snippets

@@ -0,0 +1,34 @@
+snippet de "Describe block"
+describe "${1:description}", ->
+  ${2:body}
+endsnippet
+
+snippet i "It block"
+it "$1", ->
+  $2
+endsnippet
+
+snippet be "Before each"
+beforeEach ->
+  $1
+endsnippet
+
+snippet ex "Expectation"
+expect($1).to$2
+endsnippet
+
+snippet log "Console log"
+console.log $1
+endsnippet
+
+snippet ra "Range array"
+[[$1, $2], [$3, $4]]
+endsnippet
+
+snippet pt "Point array"
+[$1, $2]
+endsnippet
+
+snippet spy "Jasmine spy"
+jasmine.createSpy("${1:description}")$2
+endsnippet

Atom/src/AtomController.mm

@@ -118,11 +118,13 @@
     CefRefPtr<CefV8Value> retval;
     CefRefPtr<CefV8Exception> exception;
     CefV8ValueList arguments;
+
     global->GetValue("reload")->ExecuteFunction(global, arguments, retval, exception, true);
-    if (exception) _clientHandler->GetBrowser()->ReloadIgnoreCache();
+    if (exception.get()) {
+      _clientHandler->GetBrowser()->ReloadIgnoreCache();
+    }
     context->Exit();
     
-
     return YES;
   }
   

spec/app/buffer-spec.coffee

@@ -220,6 +220,11 @@ describe 'Buffer', ->
         expect(event.oldText).toBe oldText
         expect(event.newText).toBe "foo\nbar"
 
+    it "allows a 'change' event handler to safely undo the change", ->
+      buffer.on 'change', -> buffer.undo()
+      buffer.change([0, 0], "hello")
+      expect(buffer.lineForRow(0)).toBe "var quicksort = function () {"
+
   describe ".setText(text)", ->
     it "changes the entire contents of the buffer and emits a change event", ->
       lastRow = buffer.getLastRow()

spec/app/edit-session-spec.coffee

@@ -6,18 +6,13 @@ describe "EditSession", ->
   [buffer, editSession, lineLengths] = []
 
   beforeEach ->
-    buffer = new Buffer(require.resolve('fixtures/sample.js'))
-    editSession = new EditSession
-      buffer: buffer
-      tabText: '  '
-      autoIndent: false
-      softWrap: false
-      project: new Project()
-
+    buffer = new Buffer()
+    editSession = fixturesProject.open('sample.js', autoIndent: false)
+    buffer = editSession.buffer
     lineLengths = buffer.getLines().map (line) -> line.length
 
   afterEach ->
-    buffer.destroy()
+    fixturesProject.destroy()
 
   describe "cursor movement", ->
     describe ".setCursorScreenPosition(screenPosition)", ->
@@ -1263,6 +1258,16 @@ describe "EditSession", ->
         expect(selections[0].getBufferRange()).toEqual [[1, 6], [1, 6]]
         expect(selections[1].getBufferRange()).toEqual [[1, 18], [1, 18]]
 
+      it "restores selected ranges even when the change occurred in another edit session", ->
+        otherEditSession = fixturesProject.open(editSession.getPath())
+        otherEditSession.setSelectedBufferRange([[2, 2], [3, 3]])
+        otherEditSession.delete()
+
+        editSession.undo()
+
+        expect(editSession.getSelectedBufferRange()).toEqual [[2, 2], [3, 3]]
+        expect(otherEditSession.getSelectedBufferRange()).toEqual [[3, 3], [3, 3]]
+
     describe "when the buffer is changed (via its direct api, rather than via than edit session)", ->
       it "moves the cursor so it is in the same relative position of the buffer", ->
         expect(editSession.getCursorScreenPosition()).toEqual [0, 0]

spec/app/editor-spec.coffee

@@ -120,8 +120,8 @@ describe "Editor", ->
       expect(otherEditSession.buffer.subscriptionCount()).toBeGreaterThan 1
 
       editor.remove()
-      expect(previousEditSession.buffer.subscriptionCount()).toBe 1
-      expect(otherEditSession.buffer.subscriptionCount()).toBe 1
+      expect(previousEditSession.buffer.subscriptionCount()).toBe 0
+      expect(otherEditSession.buffer.subscriptionCount()).toBe 0
 
   describe "when 'close' is triggered", ->
     it "closes active edit session and loads next edit session", ->

spec/app/undo-manager-spec.coffee

@@ -7,7 +7,7 @@ describe "UndoManager", ->
 
   beforeEach ->
     buffer = new Buffer(require.resolve('fixtures/sample.js'))
-    undoManager = new UndoManager(buffer)
+    undoManager = buffer.undoManager
 
   afterEach ->
     buffer.destroy()
@@ -63,49 +63,37 @@ describe "UndoManager", ->
       undoManager.redo()
       expect(buffer.getText()).toContain 'qsport'
 
-  describe "startUndoBatch() / endUndoBatch()", ->
-    it "causes changes in batch to be undone simultaneously and returns an array of ranges to select from undo and redo", ->
+  describe "transact(fn)", ->
+    it "causes changes in the transaction to be undone simultaneously", ->
       buffer.insert([0, 0], "foo")
 
-      ignoredRanges = [[[666, 666], [666, 666]], [[666, 666], [666, 666]]]
-      beforeRanges = [[[1, 2], [1, 2]], [[1, 9], [1, 9]]]
-      afterRanges =[[[1, 5], [1, 5]], [[1, 12], [1, 12]]]
-
-      undoManager.startUndoBatch(beforeRanges)
-      undoManager.startUndoBatch(ignoredRanges) # calls can be nested
-      buffer.insert([1, 2], "111")
-      buffer.insert([1, 9], "222")
-      undoManager.endUndoBatch(ignoredRanges) # calls can be nested
-      undoManager.endUndoBatch(afterRanges)
+      undoManager.transact ->
+        undoManager.transact ->
+          buffer.insert([1, 2], "111")
+          buffer.insert([1, 9], "222")
 
       expect(buffer.lineForRow(1)).toBe '  111var 222sort = function(items) {'
 
-      ranges = undoManager.undo()
-      expect(ranges).toBe beforeRanges
+      undoManager.undo()
       expect(buffer.lineForRow(1)).toBe '  var sort = function(items) {'
       expect(buffer.lineForRow(0)).toContain 'foo'
 
-      ranges = undoManager.undo()
-      expect(ranges).toBeUndefined()
+      undoManager.undo()
 
       expect(buffer.lineForRow(0)).not.toContain 'foo'
 
-      ranges = undoManager.redo()
-      expect(ranges).toBeUndefined()
+      undoManager.redo()
       expect(buffer.lineForRow(0)).toContain 'foo'
 
-      ranges = undoManager.redo()
-      expect(ranges).toBe afterRanges
+      undoManager.redo()
       expect(buffer.lineForRow(1)).toBe '  111var 222sort = function(items) {'
 
-      ranges = undoManager.undo()
-      expect(ranges).toBe beforeRanges
+      undoManager.undo()
       expect(buffer.lineForRow(1)).toBe '  var sort = function(items) {'
 
-    it "does not store empty batches", ->
+    it "does not record empty transactions", ->
       buffer.insert([0,0], "foo")
-      undoManager.startUndoBatch()
-      undoManager.endUndoBatch()
+      undoManager.transact ->
 
       undoManager.undo()
       expect(buffer.lineForRow(0)).not.toContain("foo")

spec/app/window-spec.coffee

@@ -61,4 +61,4 @@ describe "Window", ->
 
       $(window).trigger 'beforeunload'
 
-      expect(editor1.getBuffer().subscriptionCount()).toBe 1 # buffer has a self-subscription for the undo manager
+      expect(editor1.getBuffer().subscriptionCount()).toBe 0

spec/extensions/snippets-spec.coffee

@@ -41,6 +41,11 @@ describe "Snippets extension", ->
         go here ${1:first} and then here ${2:second}
 
         endsnippet
+
+        snippet t5 "Caused problems with undo"
+        first line$1
+          ${2:placeholder ending second line}
+        endsnippet
       """
 
     describe "when the letters preceding the cursor trigger a snippet", ->
@@ -144,6 +149,44 @@ describe "Snippets extension", ->
         expect(buffer.lineForRow(0)).toBe "xte  var quicksort = function () {"
         expect(editor.getCursorScreenPosition()).toEqual [0, 5]
 
+    describe "when a previous snippet expansion has just been undone", ->
+      it "expands the snippet based on the current prefix rather than jumping to the old snippet's tab stop", ->
+        editor.insertText 't5\n'
+        editor.setCursorBufferPosition [0, 2]
+        editor.trigger keydownEvent('tab', target: editor[0])
+        expect(buffer.lineForRow(0)).toBe "first line"
+        editor.undo()
+        expect(buffer.lineForRow(0)).toBe "t5"
+        editor.trigger keydownEvent('tab', target: editor[0])
+        expect(buffer.lineForRow(0)).toBe "first line"
+
+    describe "when a snippet expansion is undone and redone", ->
+      it "recreates the snippet's tab stops", ->
+        editor.insertText '    t5\n'
+        editor.setCursorBufferPosition [0, 6]
+        editor.trigger keydownEvent('tab', target: editor[0])
+        expect(buffer.lineForRow(0)).toBe "    first line"
+        editor.undo()
+        editor.redo()
+
+        expect(editor.getCursorBufferPosition()).toEqual [0, 14]
+        editor.trigger keydownEvent('tab', target: editor[0])
+        expect(editor.getSelectedBufferRange()).toEqual [[1, 6], [1, 36]]
+
+      it "restores tabs stops in active edit session even when the initial expansion was in a different edit session", ->
+        anotherEditor = editor.splitRight()
+
+        editor.insertText '    t5\n'
+        editor.setCursorBufferPosition [0, 6]
+        editor.trigger keydownEvent('tab', target: editor[0])
+        expect(buffer.lineForRow(0)).toBe "    first line"
+        editor.undo()
+
+        anotherEditor.redo()
+        expect(anotherEditor.getCursorBufferPosition()).toEqual [0, 14]
+        anotherEditor.trigger keydownEvent('tab', target: anotherEditor[0])
+        expect(anotherEditor.getSelectedBufferRange()).toEqual [[1, 6], [1, 36]]
+
   describe ".loadSnippetsFile(path)", ->
     it "loads the snippets in the given file", ->
       spyOn(fs, 'read').andReturn """

src/app/buffer-change-operation.coffee

@@ -0,0 +1,53 @@
+Range = require 'range'
+
+module.exports =
+class BufferChangeOperation
+  buffer: null
+  oldRange: null
+  oldText: null
+  newRange: null
+  newText: null
+
+  constructor: ({@buffer, @oldRange, @newText}) ->
+
+  do: ->
+    @oldText = @buffer.getTextInRange(@oldRange)
+    @newRange = @calculateNewRange(@oldRange, @newText)
+    @changeBuffer
+      oldRange: @oldRange
+      newRange: @newRange
+      oldText: @oldText
+      newText: @newText
+
+  undo: ->
+    @changeBuffer
+      oldRange: @newRange
+      newRange: @oldRange
+      oldText: @newText
+      newText: @oldText
+
+  changeBuffer: ({ oldRange, newRange, newText, oldText }) ->
+    { prefix, suffix } = @buffer.prefixAndSuffixForRange(oldRange)
+
+    newTextLines = newText.split('\n')
+    if newTextLines.length == 1
+      newTextLines = [prefix + newText + suffix]
+    else
+      lastLineIndex = newTextLines.length - 1
+      newTextLines[0] = prefix + newTextLines[0]
+      newTextLines[lastLineIndex] += suffix
+
+    @buffer.replaceLines(oldRange.start.row, oldRange.end.row, newTextLines)
+    @buffer.trigger 'change', { oldRange, newRange, oldText, newText }
+    newRange
+
+  calculateNewRange: (oldRange, newText) ->
+    newRange = new Range(oldRange.start.copy(), oldRange.start.copy())
+    newTextLines = newText.split('\n')
+    if newTextLines.length == 1
+      newRange.end.column += newText.length
+    else
+      lastLineIndex = newTextLines.length - 1
+      newRange.end.row += lastLineIndex
+      newRange.end.column = newTextLines[lastLineIndex].length
+    newRange

src/app/buffer.coffee

@@ -5,10 +5,12 @@ Point = require 'point'
 Range = require 'range'
 EventEmitter = require 'event-emitter'
 UndoManager = require 'undo-manager'
+BufferChangeOperation = require 'buffer-change-operation'
 
 module.exports =
 class Buffer
   @idCounter = 1
+  undoManager: null
   modified: null
   lines: null
   file: null
@@ -129,40 +131,31 @@ class Buffer
 
   change: (oldRange, newText) ->
     oldRange = Range.fromObject(oldRange)
-    newRange = new Range(oldRange.start.copy(), oldRange.start.copy())
-    prefix = @lines[oldRange.start.row][0...oldRange.start.column]
-    suffix = @lines[oldRange.end.row][oldRange.end.column..]
-    oldText = @getTextInRange(oldRange)
+    operation = new BufferChangeOperation({buffer: this, oldRange, newText})
+    @pushOperation(operation)
 
-    newTextLines = newText.split('\n')
+  prefixAndSuffixForRange: (range) ->
+    prefix: @lines[range.start.row][0...range.start.column]
+    suffix: @lines[range.end.row][range.end.column..]
 
-    if newTextLines.length == 1
-      newRange.end.column += newText.length
-      newTextLines = [prefix + newText + suffix]
-    else
-      lastLineIndex = newTextLines.length - 1
-      newTextLines[0] = prefix + newTextLines[0]
-      newRange.end.row += lastLineIndex
-      newRange.end.column = newTextLines[lastLineIndex].length
-      newTextLines[lastLineIndex] += suffix
-
-    @lines[oldRange.start.row..oldRange.end.row] = newTextLines
+  replaceLines: (startRow, endRow, newLines) ->
+    @lines[startRow..endRow] = newLines
     @modified = true
 
-    @trigger 'change', { oldRange, newRange, oldText, newText }
-    newRange
+  pushOperation: (operation, editSession) ->
+    if @undoManager
+      @undoManager.pushOperation(operation, editSession)
+    else
+      operation.do()
 
-  startUndoBatch: (selectedBufferRanges) ->
-    @undoManager.startUndoBatch(selectedBufferRanges)
+  transact: (fn) ->
+    @undoManager.transact(fn)
 
-  endUndoBatch: (selectedBufferRanges) ->
-    @undoManager.endUndoBatch(selectedBufferRanges)
+  undo: (editSession) ->
+    @undoManager.undo(editSession)
 
-  undo: ->
-    @undoManager.undo()
-
-  redo: ->
-    @undoManager.redo()
+  redo: (editSession) ->
+    @undoManager.redo(editSession)
 
   save: ->
     @saveAs(@getPath())

src/app/edit-session.coffee

@@ -95,6 +95,7 @@ class EditSession
     new Point(row, column)
 
   getFileExtension: -> @buffer.getExtension()
+  getPath: -> @buffer.getPath()
   getEofBufferPosition: -> @buffer.getEofPosition()
   bufferRangeForBufferRow: (row) -> @buffer.rangeForRow(row)
   lineForBufferRow: (row) -> @buffer.lineForRow(row)
@@ -177,12 +178,10 @@ class EditSession
     @insertText($native.readFromPasteboard())
 
   undo: ->
-    if ranges = @buffer.undo()
-      @setSelectedBufferRanges(ranges)
+    @buffer.undo(this)
 
   redo: ->
-    if ranges = @buffer.redo()
-      @setSelectedBufferRanges(ranges)
+    @buffer.redo(this)
 
   foldSelection: ->
     selection.fold() for selection in @getSelections()
@@ -234,10 +233,23 @@ class EditSession
     @tokenizedBuffer.toggleLineCommentsInRange(range)
 
   mutateSelectedText: (fn) ->
-    selections = @getSelections()
-    @buffer.startUndoBatch(@getSelectedBufferRanges())
-    fn(selection) for selection in selections
-    @buffer.endUndoBatch(@getSelectedBufferRanges())
+    @transact => fn(selection) for selection in @getSelections()
+
+  transact: (fn) ->
+    @buffer.transact =>
+      oldSelectedRanges = @getSelectedBufferRanges()
+      @pushOperation
+        undo: (editSession) ->
+          editSession?.setSelectedBufferRanges(oldSelectedRanges)
+
+      fn()
+      newSelectedRanges = @getSelectedBufferRanges()
+      @pushOperation
+        redo: (editSession) ->
+          editSession?.setSelectedBufferRanges(newSelectedRanges)
+
+  pushOperation: (operation) ->
+    @buffer.pushOperation(operation, this)
 
   getAnchors: ->
     new Array(@anchors...)

src/app/project.coffee

@@ -75,26 +75,30 @@ class Project
   getSoftWrap: -> @softWrap
   setSoftWrap: (@softWrap) ->
 
-  open: (filePath) ->
+  open: (filePath, editSessionOptions={}) ->
     if filePath?
       filePath = @resolve(filePath)
       buffer = @bufferWithPath(filePath) ? @buildBuffer(filePath)
     else
       buffer = @buildBuffer()
 
-    editSession = new EditSession
-      project: this
-      buffer: buffer
-      tabText: @getTabText()
-      autoIndent: @getAutoIndent()
-      softTabs: @getSoftTabs()
-      softWrap: @getSoftWrap()
-
+    @buildEditSession(buffer, editSessionOptions)
 
+  buildEditSession: (buffer, editSessionOptions) ->
+    options = _.extend(@defaultEditSessionOptions(), editSessionOptions)
+    options.project = this
+    options.buffer = buffer
+    editSession = new EditSession(options)
     @editSessions.push editSession
     @trigger 'new-edit-session', editSession
     editSession
 
+  defaultEditSessionOptions: ->
+    tabText: @getTabText()
+    autoIndent: @getAutoIndent()
+    softTabs: @getSoftTabs()
+    softWrap: @getSoftWrap()
+
   destroy: ->
     for editSession in _.clone(@editSessions)
       @removeEditSession(editSession)

src/app/range.coffee

@@ -22,7 +22,7 @@ class Range
       @start = pointB
       @end = pointA
 
-  copy: (range) ->
+  copy: ->
     new Range(@start.copy(), @end.copy())
 
   isEqual: (other) ->

src/app/undo-manager.coffee

@@ -1,56 +1,46 @@
+_ = require 'underscore'
+
 module.exports =
+
 class UndoManager
   undoHistory: null
   redoHistory: null
-  currentBatch: null
-  preserveHistory: false
-  startBatchCallCount: null
+  currentTransaction: null
 
-  constructor: (@buffer) ->
+  constructor: ->
     @startBatchCallCount = 0
     @undoHistory = []
     @redoHistory = []
-    @buffer.on 'change', (op) =>
-      unless @preserveHistory
-        if @currentBatch
-          @currentBatch.push(op)
-        else
-          @undoHistory.push([op])
-        @redoHistory = []
 
-  undo: ->
+  pushOperation: (operation, editSession) ->
+    if @currentTransaction
+      @currentTransaction.push(operation)
+    else
+      @undoHistory.push([operation])
+    @redoHistory = []
+    operation.do?(editSession)
+
+  transact: (fn) ->
+    if @currentTransaction
+      fn()
+    else
+      @currentTransaction = []
+      fn()
+      @undoHistory.push(@currentTransaction) if @currentTransaction.length
+      @currentTransaction = null
+
+  undo: (editSession) ->
     if batch = @undoHistory.pop()
-      @preservingHistory =>
-        opsInReverse = new Array(batch...)
-        opsInReverse.reverse()
-        for op in opsInReverse
-          @buffer.change op.newRange, op.oldText
-        @redoHistory.push batch
+      opsInReverse = new Array(batch...)
+      opsInReverse.reverse()
+      op.undo?(editSession) for op in opsInReverse
+      @redoHistory.push batch
       batch.oldSelectionRanges
 
-  redo: ->
+  redo: (editSession) ->
     if batch = @redoHistory.pop()
-      @preservingHistory =>
-        for op in batch
-          @buffer.change op.oldRange, op.newText
-        @undoHistory.push batch
+      for op in batch
+        op.do?(editSession)
+        op.redo?(editSession)
+      @undoHistory.push(batch)
       batch.newSelectionRanges
-
-  startUndoBatch: (ranges) ->
-    @startBatchCallCount++
-    return if @startBatchCallCount > 1
-    @currentBatch = []
-    @currentBatch.oldSelectionRanges = ranges
-
-  endUndoBatch: (ranges) ->
-    @startBatchCallCount--
-    return if @startBatchCallCount > 0
-    @currentBatch.newSelectionRanges = ranges
-    @undoHistory.push(@currentBatch) if @currentBatch.length > 0
-    @currentBatch = null
-
-  preservingHistory: (fn) ->
-    @preserveHistory = true
-    fn()
-    @preserveHistory = false
-

src/extensions/snippets/snippet-expansion.coffee

@@ -5,11 +5,12 @@ class SnippetExpansion
   constructor: (snippet, @editSession) ->
     @editSession.selectToBeginningOfWord()
     startPosition = @editSession.getCursorBufferPosition()
-    @editSession.insertText(snippet.body)
-    if snippet.tabStops.length
-      @placeTabStopAnchorRanges(startPosition, snippet.tabStops)
-    if snippet.lineCount > 1
-      @indentSubsequentLines(startPosition.row, snippet)
+    @editSession.transact =>
+      @editSession.insertText(snippet.body)
+      if snippet.tabStops.length
+        @placeTabStopAnchorRanges(startPosition, snippet.tabStops)
+      if snippet.lineCount > 1
+        @indentSubsequentLines(startPosition.row, snippet)
 
   placeTabStopAnchorRanges: (startPosition, tabStopRanges) ->
     @tabStopAnchorRanges = tabStopRanges.map ({start, end}) =>
@@ -53,3 +54,8 @@ class SnippetExpansion
   destroy: ->
     anchorRange.destroy() for anchorRange in @tabStopAnchorRanges
     @editSession.snippetExpansion = null
+
+  restore: (@editSession) ->
+    @editSession.snippetExpansion = this
+    @tabStopAnchorRanges = @tabStopAnchorRanges.map (anchorRange) =>
+      @editSession.addAnchorRange(anchorRange.getBufferRange())

src/extensions/snippets/snippets.coffee

@@ -28,7 +28,12 @@ module.exports =
       editSession = editor.activeEditSession
       prefix = editSession.getLastCursor().getCurrentWordPrefix()
       if snippet = @snippetsByExtension[editSession.getFileExtension()][prefix]
-        editSession.snippetExpansion = new SnippetExpansion(snippet, editSession)
+        editSession.transact ->
+          snippetExpansion = new SnippetExpansion(snippet, editSession)
+          editSession.snippetExpansion = snippetExpansion
+          editSession.pushOperation
+            undo: -> snippetExpansion.destroy()
+            redo: (editSession) -> snippetExpansion.restore(editSession)
       else
         e.abortKeyBinding()