From 7a87adc3fd4397fe8d0b39e002573e2d9ac84bb0 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Tue, 24 Oct 2017 17:24:03 -0700 Subject: [PATCH 1/5] Remove argument passed to TextBuffer.onWillChange listeners --- spec/text-buffer-io-spec.js | 26 +++++---------- spec/text-buffer-spec.coffee | 10 +++--- src/text-buffer.coffee | 64 +++++++++++------------------------- 3 files changed, 31 insertions(+), 69 deletions(-) diff --git a/spec/text-buffer-io-spec.js b/spec/text-buffer-io-spec.js index 249e0c8ee7..056b1aaf6f 100644 --- a/spec/text-buffer-io-spec.js +++ b/spec/text-buffer-io-spec.js @@ -255,7 +255,7 @@ describe('TextBuffer IO', () => { expect(buffer.isModified()).toBe(true) const changeEvents = [] - buffer.onWillChange((event) => changeEvents.push(['will-change', event])) + buffer.onWillChange(() => changeEvents.push(['will-change'])) buffer.onDidChange((event) => changeEvents.push(['did-change', event])) buffer.onDidChangeText((event) => changeEvents.push(['did-change-text', event])) @@ -877,9 +877,9 @@ describe('TextBuffer IO', () => { const events = [] buffer.onWillReload((event) => events.push(['will-reload'])) - buffer.onWillChange((event) => { + buffer.onWillChange(() => { expect(buffer.getText()).toEqual('abcde') - events.push(['will-change', event]) + events.push(['will-change']) }) buffer.onDidChange((event) => { expect(buffer.getText()).toEqual(newText) @@ -909,12 +909,7 @@ describe('TextBuffer IO', () => { 'will-reload' ], [ - 'will-change', { - oldRange: Range(Point(0, 0), Point(0, 5)), - newRange: Range(Point(0, 0), Point(0, newText.length)), - oldText: 'abcde', - newText: newText - } + 'will-change' ], [ 'did-change', { @@ -951,11 +946,11 @@ describe('TextBuffer IO', () => { }) }) - it('passes the smallest possible change event to onWillChange and onDidChange listeners', (done) => { + it('passes the smallest possible change event to onDidChange listeners', (done) => { fs.writeFileSync(buffer.getPath(), 'abc de ') const events = [] - buffer.onWillChange((event) => events.push(['will-change', event])) + buffer.onWillChange(() => events.push(['will-change'])) buffer.onDidChange((event) => events.push(['did-change', event])) buffer.onDidChangeText((event) => events.push(['did-change-text', event])) @@ -966,12 +961,7 @@ describe('TextBuffer IO', () => { expect(toPlainObject(events)).toEqual(toPlainObject([ [ - 'will-change', { - oldRange: Range(Point(0, 3), Point(0, 5)), - newRange: Range(Point(0, 3), Point(0, 7)), - oldText: 'de', - newText: ' de ' - } + 'will-change' ], [ 'did-change', { @@ -1027,7 +1017,7 @@ describe('TextBuffer IO', () => { it('does not fire duplicate change events when multiple changes happen on disk', (done) => { const changeEvents = [] - buffer.onWillChange((event) => changeEvents.push('will-change')) + buffer.onWillChange(() => changeEvents.push('will-change')) buffer.onDidChange((event) => changeEvents.push('did-change')) buffer.onDidChangeText((event) => changeEvents.push('did-change-text')) diff --git a/spec/text-buffer-spec.coffee b/spec/text-buffer-spec.coffee index 2089a7113f..a87652585b 100644 --- a/spec/text-buffer-spec.coffee +++ b/spec/text-buffer-spec.coffee @@ -97,16 +97,14 @@ describe "TextBuffer", -> expect(buffer.getText()).toEqual "hey\nyou're old\r\nhow are you doing?" describe "before a change", -> - it "notifies ::onWillChange observers with the relevant details", -> - changes = [] + it "notifies ::onWillChange observers", -> + changeCount = 0 buffer.onWillChange (change) -> expect(buffer.getText()).toBe "hello\nworld\r\nhow are you doing?" - changes.push(change) + changeCount++ buffer.setTextInRange([[0, 2], [2, 3]], "y there\r\ncat\nwhat", normalizeLineEndings: false) - expect(changes).toEqual [{ - oldRange: [[0, 2], [2, 3]] - }] + expect(changeCount).toBe(1) describe "after a change", -> it "notifies, in order, decoration layers, display layers, ::onDidChange observers and display layer ::onDidChangeSync observers with the relevant details", -> diff --git a/src/text-buffer.coffee b/src/text-buffer.coffee index d7aaeeaecc..102f3d3d94 100644 --- a/src/text-buffer.coffee +++ b/src/text-buffer.coffee @@ -29,29 +29,20 @@ class CompositeChangeEvent oldText = null newText = null - Object.defineProperty(this, 'didChange', { - enumerable: false, - writable: true, - value: false - }) - Object.defineProperty(this, 'oldText', { enumerable: true, get: -> unless oldText? - if @didChange - oldBuffer = new NativeTextBuffer(@newText) - for change in patch.getChanges() by -1 - oldBuffer.setTextInRange( - new Range( - traversal(change.newStart, compositeStart), - traversal(change.newEnd, compositeStart) - ), - change.oldText - ) - oldText = oldBuffer.getText() - else - oldText = buffer.getTextInRange(@oldRange) + oldBuffer = new NativeTextBuffer(@newText) + for change in patch.getChanges() by -1 + oldBuffer.setTextInRange( + new Range( + traversal(change.newStart, compositeStart), + traversal(change.newEnd, compositeStart) + ), + change.oldText + ) + oldText = oldBuffer.getText() oldText }) @@ -59,19 +50,7 @@ class CompositeChangeEvent enumerable: true, get: -> unless newText? - if @didChange - newText = buffer.getTextInRange(@newRange) - else - newBuffer = new NativeTextBuffer(@oldText) - for change in patch.getChanges() by -1 - newBuffer.setTextInRange( - new Range( - traversal(change.oldStart, compositeStart), - traversal(change.oldEnd, compositeStart) - ), - change.newText - ) - newText = newBuffer.getText() + newText = buffer.getTextInRange(@newRange) newText }) @@ -897,7 +876,7 @@ class TextBuffer changeEvent = {oldRange, newRange, oldText, newText} for id, displayLayer of @displayLayers displayLayer.bufferWillChange(changeEvent) - @emitter.emit 'will-change', {oldRange} + @emitter.emit('will-change') @buffer.setTextInRange(oldRange, newText) @@ -1730,17 +1709,15 @@ class TextBuffer Grim.deprecate('The .loadSync instance method is deprecated. Create a loaded buffer using TextBuffer.loadSync(filePath) instead.') checkpoint = null - changeEvent = null try patch = @buffer.loadSync( @getPath(), @getEncoding(), (percentDone, patch) => if patch and patch.getChangeCount() > 0 - changeEvent = new CompositeChangeEvent(@buffer, patch) checkpoint = @historyProvider.createCheckpoint(markers: @createMarkerSnapshot(), isBarrier: true) @emitter.emit('will-reload') - @emitter.emit('will-change', changeEvent) + @emitter.emit('will-change') ) catch error if not options.mustExist and error.code is 'ENOENT' @@ -1749,7 +1726,7 @@ class TextBuffer else throw error - @finishLoading(changeEvent, checkpoint, patch) + @finishLoading(checkpoint, patch) load: (options) -> unless options?.internal @@ -1761,7 +1738,6 @@ class TextBuffer @file.createReadStream() checkpoint = null - changeEvent = null loadCount = ++@loadCount @buffer.load( source, @@ -1774,14 +1750,13 @@ class TextBuffer return false if @loadCount > loadCount if patch if patch.getChangeCount() > 0 - changeEvent = new CompositeChangeEvent(@buffer, patch) checkpoint = @historyProvider.createCheckpoint(markers: @createMarkerSnapshot(), isBarrier: true) @emitter.emit('will-reload') - @emitter.emit('will-change', changeEvent) + @emitter.emit('will-change') else if options?.discardChanges @emitter.emit('will-reload') ).then((patch) => - @finishLoading(changeEvent, checkpoint, patch, options) + @finishLoading(checkpoint, patch, options) ).catch((error) => if not options?.mustExist and error.code is 'ENOENT' @emitter.emit('will-reload') @@ -1791,8 +1766,8 @@ class TextBuffer throw error ) - finishLoading: (changeEvent, checkpoint, patch, options) -> - if @isDestroyed() or (@loaded and not changeEvent? and patch?) + finishLoading: (checkpoint, patch, options) -> + if @isDestroyed() or (@loaded and not checkpoint? and patch?) if options?.discardChanges @emitter.emit('did-reload') return null @@ -1824,8 +1799,7 @@ class TextBuffer traversal(change.newEnd, change.newStart) ) - changeEvent.didChange = true - @emitDidChangeEvent(changeEvent) + @emitDidChangeEvent(new CompositeChangeEvent(@buffer, patch)) markersSnapshot = @createMarkerSnapshot() @historyProvider.groupChangesSinceCheckpoint(checkpoint, {markers: markersSnapshot, deleteCheckpoint: true}) From 44e54be4859b79ee37296e4e77422f4c03284c24 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Tue, 24 Oct 2017 17:41:19 -0700 Subject: [PATCH 2/5] Only call onWillChange listeners once per transaction --- spec/text-buffer-spec.coffee | 37 ++++++++++++++++++++++++++---------- src/text-buffer.coffee | 12 +++++++++--- 2 files changed, 36 insertions(+), 13 deletions(-) diff --git a/spec/text-buffer-spec.coffee b/spec/text-buffer-spec.coffee index a87652585b..909ebd24a7 100644 --- a/spec/text-buffer-spec.coffee +++ b/spec/text-buffer-spec.coffee @@ -96,16 +96,6 @@ describe "TextBuffer", -> buffer.setTextInRange([[0, 2], [1, 3]], "y\nyou're o", normalizeLineEndings: false) expect(buffer.getText()).toEqual "hey\nyou're old\r\nhow are you doing?" - describe "before a change", -> - it "notifies ::onWillChange observers", -> - changeCount = 0 - buffer.onWillChange (change) -> - expect(buffer.getText()).toBe "hello\nworld\r\nhow are you doing?" - changeCount++ - - buffer.setTextInRange([[0, 2], [2, 3]], "y there\r\ncat\nwhat", normalizeLineEndings: false) - expect(changeCount).toBe(1) - describe "after a change", -> it "notifies, in order, decoration layers, display layers, ::onDidChange observers and display layer ::onDidChangeSync observers with the relevant details", -> events = [] @@ -2255,6 +2245,33 @@ describe "TextBuffer", -> buffer.setText('\n') expect(buffer.isEmpty()).toBeFalsy() + describe "::onWillChange", -> + it "notifies observers before a transaction, an undo or a redo", -> + changeCount = 0 + expectedText = '' + + buffer = new TextBuffer() + buffer.onWillChange (change) -> + expect(buffer.getText()).toBe expectedText + changeCount++ + + buffer.append('a') + expect(changeCount).toBe(1) + expectedText = 'a' + + buffer.transact -> + buffer.append('b') + buffer.append('c') + expect(changeCount).toBe(2) + expectedText = 'abc' + + buffer.undo() + expect(changeCount).toBe(3) + expectedText = 'a' + + buffer.redo() + expect(changeCount).toBe(4) + describe "::onDidChangeText(callback)", -> beforeEach -> filePath = require.resolve('./fixtures/sample.js') diff --git a/src/text-buffer.coffee b/src/text-buffer.coffee index 102f3d3d94..46b09b146c 100644 --- a/src/text-buffer.coffee +++ b/src/text-buffer.coffee @@ -876,7 +876,6 @@ class TextBuffer changeEvent = {oldRange, newRange, oldText, newText} for id, displayLayer of @displayLayers displayLayer.bufferWillChange(changeEvent) - @emitter.emit('will-change') @buffer.setTextInRange(oldRange, newText) @@ -1136,6 +1135,7 @@ class TextBuffer # Public: Undo the last operation. If a transaction is in progress, aborts it. undo: -> if pop = @historyProvider.undo() + @emitWillChangeEvent() @applyChange(change) for change in pop.textUpdates @restoreFromMarkerSnapshot(pop.markers) @emitDidChangeTextEvent() @@ -1147,6 +1147,7 @@ class TextBuffer # Public: Redo the last operation redo: -> if pop = @historyProvider.redo() + @emitWillChangeEvent() @applyChange(change) for change in pop.textUpdates @restoreFromMarkerSnapshot(pop.markers) @emitDidChangeTextEvent() @@ -1173,6 +1174,7 @@ class TextBuffer fn = groupingInterval groupingInterval = 0 + @emitWillChangeEvent() checkpointBefore = @historyProvider.createCheckpoint(markers: @createMarkerSnapshot(), isBarrier: true) try @@ -1220,6 +1222,7 @@ class TextBuffer # Returns a {Boolean} indicating whether the operation succeeded. revertToCheckpoint: (checkpoint, options) -> if truncated = @historyProvider.revertToCheckpoint(checkpoint, options) + @emitWillChangeEvent() @applyChange(change) for change in truncated.textUpdates @restoreFromMarkerSnapshot(truncated.markers) @emitDidChangeTextEvent() @@ -1717,7 +1720,7 @@ class TextBuffer if patch and patch.getChangeCount() > 0 checkpoint = @historyProvider.createCheckpoint(markers: @createMarkerSnapshot(), isBarrier: true) @emitter.emit('will-reload') - @emitter.emit('will-change') + @emitWillChangeEvent() ) catch error if not options.mustExist and error.code is 'ENOENT' @@ -1752,7 +1755,7 @@ class TextBuffer if patch.getChangeCount() > 0 checkpoint = @historyProvider.createCheckpoint(markers: @createMarkerSnapshot(), isBarrier: true) @emitter.emit('will-reload') - @emitter.emit('will-change') + @emitWillChangeEvent() else if options?.discardChanges @emitter.emit('will-reload') ).then((patch) => @@ -1911,6 +1914,9 @@ class TextBuffer for markerLayerId, markerLayer of @markerLayers markerLayer.emitChangeEvents(snapshot?[markerLayerId]) + emitWillChangeEvent: -> + @emitter.emit('will-change') + emitDidChangeTextEvent: -> if @transactCallDepth is 0 and @changesSinceLastDidChangeTextEvent.length > 0 compactedChanges = Object.freeze(normalizePatchChanges( From a0ae9504f7f39de79fe1ffb9526d3f0570029d82 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Wed, 25 Oct 2017 12:56:43 -0700 Subject: [PATCH 3/5] 13.6.0-will-change-event-1 --- package-lock.json | 2 +- package.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/package-lock.json b/package-lock.json index 5f7f995d96..11da66674f 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "text-buffer", - "version": "13.5.8", + "version": "13.6.0-will-change-event-1", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/package.json b/package.json index 88d1a461ba..c7bb227bfb 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "text-buffer", - "version": "13.5.8", + "version": "13.6.0-will-change-event-1", "description": "A container for large mutable strings with annotated regions", "main": "./lib/text-buffer", "scripts": { From d1b6d4c46708438c016c328b85d10598fd7bfff2 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Wed, 25 Oct 2017 13:51:35 -0700 Subject: [PATCH 4/5] Avoid calling .onWillChange listeners for empty transactions --- spec/text-buffer-spec.coffee | 10 ++++++++++ src/text-buffer.coffee | 10 ++++++---- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/spec/text-buffer-spec.coffee b/spec/text-buffer-spec.coffee index 909ebd24a7..7bfab9b97e 100644 --- a/spec/text-buffer-spec.coffee +++ b/spec/text-buffer-spec.coffee @@ -2251,6 +2251,8 @@ describe "TextBuffer", -> expectedText = '' buffer = new TextBuffer() + checkpoint = buffer.createCheckpoint() + buffer.onWillChange (change) -> expect(buffer.getText()).toBe expectedText changeCount++ @@ -2265,12 +2267,20 @@ describe "TextBuffer", -> expect(changeCount).toBe(2) expectedText = 'abc' + # Empty transactions do not cause onWillChange listeners to be called + buffer.transact -> + expect(changeCount).toBe(2) + buffer.undo() expect(changeCount).toBe(3) expectedText = 'a' buffer.redo() expect(changeCount).toBe(4) + expectedText = 'abc' + + buffer.revertToCheckpoint(checkpoint) + expect(changeCount).toBe(5) describe "::onDidChangeText(callback)", -> beforeEach -> diff --git a/src/text-buffer.coffee b/src/text-buffer.coffee index 46b09b146c..c00968610e 100644 --- a/src/text-buffer.coffee +++ b/src/text-buffer.coffee @@ -160,6 +160,7 @@ class TextBuffer @nextMarkerId = 1 @outstandingSaveCount = 0 @loadCount = 0 + @_emittedWillChangeEvent = false @setEncoding(params?.encoding) @setPreferredLineEnding(params?.preferredLineEnding) @@ -329,8 +330,6 @@ class TextBuffer # any expensive operations via this method. # # * `callback` {Function} to be called when the buffer changes. - # * `event` {Object} with the following keys: - # * `oldRange` {Range} of the old text. # # Returns a {Disposable} on which `.dispose()` can be called to unsubscribe. onWillChange: (callback) -> @@ -877,6 +876,7 @@ class TextBuffer for id, displayLayer of @displayLayers displayLayer.bufferWillChange(changeEvent) + @emitWillChangeEvent() @buffer.setTextInRange(oldRange, newText) if @markerLayers? @@ -1174,7 +1174,6 @@ class TextBuffer fn = groupingInterval groupingInterval = 0 - @emitWillChangeEvent() checkpointBefore = @historyProvider.createCheckpoint(markers: @createMarkerSnapshot(), isBarrier: true) try @@ -1915,7 +1914,9 @@ class TextBuffer markerLayer.emitChangeEvents(snapshot?[markerLayerId]) emitWillChangeEvent: -> - @emitter.emit('will-change') + unless @_emittedWillChangeEvent + @emitter.emit('will-change') + @_emittedWillChangeEvent = true emitDidChangeTextEvent: -> if @transactCallDepth is 0 and @changesSinceLastDidChangeTextEvent.length > 0 @@ -1925,6 +1926,7 @@ class TextBuffer @changesSinceLastDidChangeTextEvent.length = 0 @emitter.emit 'did-change-text', {changes: compactedChanges} @debouncedEmitDidStopChangingEvent() + @_emittedWillChangeEvent = false # Identifies if the buffer belongs to multiple editors. # From d9a074ed3a42acb26ec2e17474bfed77c888f38a Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Wed, 25 Oct 2017 14:46:59 -0700 Subject: [PATCH 5/5] 13.6.0-will-change-event-2 --- package-lock.json | 2 +- package.json | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/package-lock.json b/package-lock.json index 11da66674f..d12d50394d 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "text-buffer", - "version": "13.6.0-will-change-event-1", + "version": "13.6.0-will-change-event-2", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/package.json b/package.json index c7bb227bfb..f6d415148f 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "text-buffer", - "version": "13.6.0-will-change-event-1", + "version": "13.6.0-will-change-event-2", "description": "A container for large mutable strings with annotated regions", "main": "./lib/text-buffer", "scripts": {