Merge pull request #274 from atom/mb-one-display-layer-event-per-tx

Only emit one display layer change event per buffer transaction

Author: Max Brunsfeld

package-lock.json

@@ -1,6 +1,6 @@
 {
   "name": "text-buffer",
-  "version": "13.7.2",
+  "version": "13.8.0-display-layer-change-event-2",
   "description": "A container for large mutable strings with annotated regions",
   "main": "./lib/text-buffer",
   "scripts": {

package.json

@@ -1989,14 +1989,13 @@ describe('DisplayLayer', () => {
       displayLayer.setTextDecorationLayer(decorationLayer)
       const allChanges = []
 
-      displayLayer.onDidChangeSync((changes) => allChanges.push(...changes))
+      displayLayer.onDidChange((changes) => allChanges.push(...changes))
 
       decorationLayer.emitInvalidateRangeEvent([[2, 1], [3, 2]])
 
       expect(allChanges).toEqual([{
-        start: Point(1, 0),
-        oldExtent: Point(2, 0),
-        newExtent: Point(2, 0)
+        oldRange: Range(Point(1, 0), Point(3, 0)),
+        newRange: Range(Point(1, 0), Point(3, 0))
       }])
     })
 
@@ -2167,6 +2166,128 @@ describe('DisplayLayer', () => {
     })
   })
 
+  describe('.onDidChange', () => {
+    it('calls the given callback when the display layer\'s content changes', () => {
+      const buffer = new TextBuffer({
+        text: 'abc\ndef\nghi\njkl\nmno'
+      })
+
+      const displayLayer = buffer.addDisplayLayer({tabLength: 4})
+
+      const events = []
+      displayLayer.onDidChange((changes) => events.push(...changes))
+
+      displayLayer.foldBufferRange(Range(Point(1, 1), Point(2, 2)))
+      expect(events).toEqual([
+        {
+          oldRange: Range(Point(1, 0), Point(3, 0)),
+          newRange: Range(Point(1, 0), Point(2, 0))
+        }
+      ])
+
+      events.length = 0
+      displayLayer.foldBufferRange(Range(Point(3, 1), Point(4, 2)))
+      expect(events).toEqual([
+        {
+          oldRange: Range(Point(2, 0), Point(4, 0)),
+          newRange: Range(Point(2, 0), Point(3, 0))
+        }
+      ])
+
+      events.length = 0
+      displayLayer.destroyAllFolds()
+      expect(events).toEqual([
+        {
+          oldRange: Range(Point(1, 0), Point(3, 0)),
+          newRange: Range(Point(1, 0), Point(5, 0))
+        }
+      ])
+
+      // When multiple changes occur in a transaction, the changes are combined.
+      events.length = 0
+      buffer.transact(() => {
+        displayLayer.foldBufferRange(Range(Point(1, 1), Point(2, 2)))
+        displayLayer.foldBufferRange(Range(Point(3, 1), Point(4, 2)))
+      })
+      expect(events).toEqual([
+        {
+          oldRange: Range(Point(1, 0), Point(5, 0)),
+          newRange: Range(Point(1, 0), Point(3, 0))
+        }
+      ])
+    })
+
+    it('calls the callback only one time per text buffer transaction', () => {
+      const buffer = new TextBuffer({
+        text: 'abc\ndef\nghi\njkl\nmno'
+      })
+
+      const displayLayer = buffer.addDisplayLayer({tabLength: 4})
+
+      const events = []
+      displayLayer.onDidChange((changes) => events.push(changes))
+
+      const checkpoint = buffer.createCheckpoint()
+
+      buffer.transact(() => {
+        buffer.setTextInRange([[0, 1], [0, 1]], '\n')
+        buffer.setTextInRange([[4, 2], [4, 2]], '\n')
+        buffer.setTextInRange([[4, 2], [4, 2]], '.')
+        buffer.setTextInRange([[4, 3], [4, 3]], '.')
+        buffer.setTextInRange([[4, 4], [4, 4]], '.')
+      })
+      expect(events).toEqual([[
+        {
+          oldRange: Range(Point(0, 0), Point(1, 0)),
+          newRange: Range(Point(0, 0), Point(2, 0))
+        },
+        {
+          oldRange: Range(Point(3, 0), Point(4, 0)),
+          newRange: Range(Point(4, 0), Point(6, 0))
+        }
+      ]])
+
+      events.length = 0
+      buffer.undo()
+      expect(events).toEqual([[
+        {
+          oldRange: Range(Point(0, 0), Point(2, 0)),
+          newRange: Range(Point(0, 0), Point(1, 0))
+        },
+        {
+          oldRange: Range(Point(4, 0), Point(6, 0)),
+          newRange: Range(Point(3, 0), Point(4, 0))
+        }
+      ]])
+
+      events.length = 0
+      buffer.redo()
+      expect(events).toEqual([[
+        {
+          oldRange: Range(Point(0, 0), Point(1, 0)),
+          newRange: Range(Point(0, 0), Point(2, 0))
+        },
+        {
+          oldRange: Range(Point(3, 0), Point(4, 0)),
+          newRange: Range(Point(4, 0), Point(6, 0))
+        }
+      ]])
+
+      events.length = 0
+      buffer.revertToCheckpoint(checkpoint)
+      expect(events).toEqual([[
+        {
+          oldRange: Range(Point(0, 0), Point(2, 0)),
+          newRange: Range(Point(0, 0), Point(1, 0))
+        },
+        {
+          oldRange: Range(Point(4, 0), Point(6, 0)),
+          newRange: Range(Point(3, 0), Point(4, 0))
+        }
+      ]])
+    })
+  })
+
   describe('.getApproximateScreenLineCount()', () => {
     it('estimates the screen line count based on the currently-indexed portion of the buffer', () => {
       const buffer = new TextBuffer({
@@ -2458,18 +2579,18 @@ function verifyChangeEvent (displayLayer, fn) {
   displayLayerCopy.setTextDecorationLayer(displayLayer.getTextDecorationLayer())
   const previousTokenLines = getTokens(displayLayerCopy)
   displayLayerCopy.destroy()
-  let lastChanges = null
+  let changes = []
 
-  const disposable = displayLayer.onDidChangeSync(function (changes) {
-    lastChanges = changes
+  const disposable = displayLayer.onDidChange((event) => {
+    changes.push(...event)
   })
 
   fn()
   disposable.dispose()
   displayLayerCopy = displayLayer.copy()
   displayLayerCopy.setTextDecorationLayer(displayLayer.getTextDecorationLayer())
   const expectedTokenLines = getTokens(displayLayerCopy)
-  updateTokenLines(previousTokenLines, displayLayerCopy, lastChanges)
+  updateTokenLines(previousTokenLines, displayLayerCopy, changes)
   displayLayerCopy.destroy()
   expect(previousTokenLines).toEqual(expectedTokenLines)
 }
@@ -2700,9 +2821,9 @@ function getTokenBoundaries (displayLayer, startRow = 0, endRow = displayLayer.g
 }
 
 function updateTokenLines (tokenLines, displayLayer, changes) {
-  for (const {start, oldExtent, newExtent} of changes || []) {
-    const newTokenLines = getTokens(displayLayer, start.row, start.row + newExtent.row)
-    tokenLines.splice(start.row, oldExtent.row, ...newTokenLines)
+  for (const {oldRange, newRange} of changes || []) {
+    const newTokenLines = getTokens(displayLayer, newRange.start.row, newRange.end.row)
+    tokenLines.splice(newRange.start.row, oldRange.end.row - oldRange.start.row, ...newTokenLines)
   }
 }
 

spec/display-layer-spec.js

@@ -219,7 +219,7 @@ describe "MarkerLayer", ->
       expect(updateCount).toBe(6)
 
       # update events happen after updating display layers when there is no parent transaction.
-      displayLayer.onDidChangeSync ->
+      displayLayer.onDidChange ->
         displayLayerDidChange = true
       buffer.undo()
       expect(updateCount).toBe(7)

spec/marker-layer-spec.coffee

@@ -165,7 +165,7 @@ describe('TextBuffer IO', () => {
       const events = []
 
       const displayLayer = buffer.addDisplayLayer()
-      displayLayer.onDidChangeSync((event) => events.push(['display-layer', event]))
+      displayLayer.onDidChange((event) => events.push(['display-layer', event]))
 
       buffer.registerTextDecorationLayer({
         bufferDidChange ({oldRange, newRange}) { events.push(['decoration-layer', {oldRange, newRange}]) }
@@ -174,7 +174,7 @@ describe('TextBuffer IO', () => {
       buffer.reload().then(() => {
         expect(events).toEqual([
           ['decoration-layer', {oldRange: Range(Point(0, 7), Point(0, 7)), newRange: Range(Point(0, 7), Point(0, 11))}],
-          ['display-layer', [{start: Point(0, 0), oldExtent: Point(1, 0), newExtent: Point(1, 0)}]]
+          ['display-layer', [{oldRange: Range(Point(0, 0), Point(1, 0)), newRange: Range(Point(0, 0), Point(1, 0))}]]
         ])
         done()
       })

spec/text-buffer-io-spec.js

@@ -97,7 +97,9 @@ describe "TextBuffer", ->
       expect(buffer.getText()).toEqual "hey\nyou're old\r\nhow are you doing?"
 
     describe "after a change", ->
-      it "notifies, in order, decoration layers, display layers, and display layer ::onDidChangeSync observers with the relevant details", ->
+      it "notifies, in order, decoration layers, display layers, and display layer ::onDidChange observers with the relevant details", ->
+        buffer = new TextBuffer("hello\nworld\r\nhow are you doing?")
+
         events = []
         textDecorationLayer1 = {
           bufferDidChange: (e) -> events.push({source: 'decoration-layer-1', event: e})
@@ -118,11 +120,11 @@ describe "TextBuffer", ->
         buffer.registerTextDecorationLayer(textDecorationLayer1) # insert a duplicate decoration layer
         buffer.registerTextDecorationLayer(textDecorationLayer2)
         buffer.onDidChange (e) -> events.push({source: 'buffer', event: JSON.parse(JSON.stringify(e))})
+        displayLayer1.onDidChange (e) -> events.push({source: 'display-layer-event', event: e})
 
-        disposable = displayLayer1.onDidChangeSync ->
-          disposable.dispose()
+        buffer.transact ->
+          buffer.setTextInRange([[0, 2], [2, 3]], "y there\r\ncat\nwhat", normalizeLineEndings: false)
           buffer.setTextInRange([[1, 1], [1, 2]], "abc", normalizeLineEndings: false)
-        buffer.setTextInRange([[0, 2], [2, 3]], "y there\r\ncat\nwhat", normalizeLineEndings: false)
 
         changeEvent1 = {
           oldRange: [[0, 2], [2, 3]], newRange: [[0, 2], [2, 4]]
@@ -157,6 +159,13 @@ describe "TextBuffer", ->
                 }
               ]
             }
+          },
+          {
+            source: 'display-layer-event',
+            event: [{
+              oldRange: Range(Point(0, 0), Point(3, 0)),
+              newRange: Range(Point(0, 0), Point(3, 0))
+            }]
           }
         ]
 

spec/text-buffer-spec.coffee

@@ -24,6 +24,7 @@ class DisplayLayer {
     this.textDecorationLayer = new EmptyDecorationLayer()
     this.displayMarkerLayersById = new Map()
     this.destroyed = false
+    this.changesSinceLastEvent = new Patch()
 
     this.invisibles = params.invisibles != null ? params.invisibles : {}
     this.tabLength = params.tabLength != null ? params.tabLength : 4
@@ -153,11 +154,7 @@ class DisplayLayer {
         const endRow = this.translateBufferPositionWithSpatialIndex(Point(endBufferRow, 0), 'backward').row
         const extent = Point(endRow - startRow, 0)
         spliceArray(this.cachedScreenLines, startRow, extent.row, new Array(extent.row))
-        this.emitDidChangeSyncEvent([{
-          start: Point(startRow, 0),
-          oldExtent: extent,
-          newExtent: extent
-        }])
+        this.didChange({start: Point(startRow, 0), oldExtent: extent, newExtent: extent})
       })
     }
   }
@@ -185,8 +182,8 @@ class DisplayLayer {
     this.displayMarkerLayersById.delete(id)
   }
 
-  onDidChangeSync (callback) {
-    return this.emitter.on('did-change-sync', callback)
+  onDidChange (callback) {
+    return this.emitter.on('did-change', callback)
   }
 
   onDidReset (callback) {
@@ -207,9 +204,7 @@ class DisplayLayer {
     if (containingFoldMarkers.length === 0) {
       const foldStartRow = bufferRange.start.row
       const foldEndRow = bufferRange.end.row + 1
-      this.emitDidChangeSyncEvent([
-        this.updateSpatialIndex(foldStartRow, foldEndRow, foldEndRow, Infinity)
-      ])
+      this.didChange(this.updateSpatialIndex(foldStartRow, foldEndRow, foldEndRow, Infinity))
       this.notifyObserversIfMarkerScreenPositionsChanged()
     }
     return foldId
@@ -269,12 +264,12 @@ class DisplayLayer {
       foldMarker.destroy()
     }
 
-    this.emitDidChangeSyncEvent([this.updateSpatialIndex(
+    this.didChange(this.updateSpatialIndex(
       combinedRangeStart.row,
       combinedRangeEnd.row + 1,
       combinedRangeEnd.row + 1,
       Infinity
-    )])
+    ))
     this.notifyObserversIfMarkerScreenPositionsChanged()
 
     return foldedRanges
@@ -807,10 +802,8 @@ class DisplayLayer {
       }
     }
 
-    const combinedChanges = new Patch()
     this.indexedBufferRowCount += newEndRow - oldEndRow
-    const {start, oldExtent, newExtent} = this.updateSpatialIndex(startRow, oldEndRow + 1, newEndRow + 1, Infinity)
-    combinedChanges.splice(start, oldExtent, newExtent)
+    this.didChange(this.updateSpatialIndex(startRow, oldEndRow + 1, newEndRow + 1, Infinity))
 
     for (let bufferRange of this.textDecorationLayer.getInvalidatedRanges()) {
       bufferRange = Range.fromObject(bufferRange)
@@ -821,20 +814,25 @@ class DisplayLayer {
       const endRow = this.translateBufferPositionWithSpatialIndex(Point(endBufferRow, 0), 'backward').row
       const extent = Point(endRow - startRow, 0)
       spliceArray(this.cachedScreenLines, startRow, extent.row, new Array(extent.row))
-      combinedChanges.splice(Point(startRow, 0), extent, extent)
+      this.didChange({start: Point(startRow, 0), oldExtent: extent, newExtent: extent})
     }
+  }
 
-    return Object.freeze(combinedChanges.getChanges().map((hunk) => {
-      return {
-        start: Point.fromObject(hunk.newStart),
-        oldExtent: traversal(hunk.oldEnd, hunk.oldStart),
-        newExtent: traversal(hunk.newEnd, hunk.newStart)
-      }
-    }))
+  didChange ({start, oldExtent, newExtent}) {
+    this.changesSinceLastEvent.splice(start, oldExtent, newExtent)
+    if (this.buffer.transactCallDepth === 0) this.emitDeferredChangeEvents()
   }
 
-  emitDidChangeSyncEvent (event) {
-    this.emitter.emit('did-change-sync', event)
+  emitDeferredChangeEvents () {
+    if (this.changesSinceLastEvent.getChangeCount() > 0) {
+      this.emitter.emit('did-change', this.changesSinceLastEvent.getChanges().map((change) => {
+        return {
+          oldRange: new Range(change.oldStart, change.oldEnd),
+          newRange: new Range(change.newStart, change.newEnd)
+        }
+      }))
+      this.changesSinceLastEvent = new Patch()
+    }
   }
 
   notifyObserversIfMarkerScreenPositionsChanged () {