From 54260809773f9b9996857753d9f31647b5ff6232 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Wed, 25 Oct 2017 17:52:04 -0700 Subject: [PATCH 1/3] Replace onDidChange with a shimmed version of onDidChangeText --- spec/marker-layer-spec.coffee | 2 +- spec/text-buffer-io-spec.js | 43 ++++------------ spec/text-buffer-spec.coffee | 77 +++++++++++++++++----------- src/helpers.js | 9 ++++ src/text-buffer.coffee | 95 +++++++++++++++-------------------- 5 files changed, 109 insertions(+), 117 deletions(-) diff --git a/spec/marker-layer-spec.coffee b/spec/marker-layer-spec.coffee index a9202f1f08..06659400cd 100644 --- a/spec/marker-layer-spec.coffee +++ b/spec/marker-layer-spec.coffee @@ -183,7 +183,7 @@ describe "MarkerLayer", -> displayLayerDidChange = false changeCount = 0 - buffer.onDidChangeText -> + buffer.onDidChange -> changeCount++ updateCount = 0 diff --git a/spec/text-buffer-io-spec.js b/spec/text-buffer-io-spec.js index 056b1aaf6f..dcbfe8b7e7 100644 --- a/spec/text-buffer-io-spec.js +++ b/spec/text-buffer-io-spec.js @@ -193,18 +193,12 @@ describe('TextBuffer IO', () => { fs.writeFileSync(filePath, 'abcdXefg', 'utf8') { - const subscription = buffer.onDidChange(() => { - subscription.dispose() - buffer.setText('') - }) - } - - { - const subscription = buffer.onDidChangeText(({changes}) => { + const subscription = buffer.onDidChange(({changes}) => { subscription.dispose() expect(changes.length).toBe(1) - expect(changes[0].oldText).toBe('abcdefg') - expect(changes[0].newText).toBe('') + expect(changes[0].oldText).toBe('') + expect(changes[0].newText).toBe('X') + buffer.setText('') }) } @@ -218,6 +212,9 @@ describe('TextBuffer IO', () => { expect(buffer.getText()).toBe('') + buffer.undo() + expect(buffer.getText()).toBe('abcdXefg') + buffer.undo() expect(buffer.getText()).toBe('abcdefg') @@ -257,7 +254,6 @@ describe('TextBuffer IO', () => { const changeEvents = [] buffer.onWillChange(() => changeEvents.push(['will-change'])) buffer.onDidChange((event) => changeEvents.push(['did-change', event])) - buffer.onDidChangeText((event) => changeEvents.push(['did-change-text', event])) buffer.save().then(() => { expect(buffer.isModified()).toBe(false) @@ -881,11 +877,7 @@ describe('TextBuffer IO', () => { expect(buffer.getText()).toEqual('abcde') events.push(['will-change']) }) - buffer.onDidChange((event) => { - expect(buffer.getText()).toEqual(newText) - events.push(['did-change', event]) - }) - buffer.onDidChangeText((event) => events.push(['did-change-text', event])) + buffer.onDidChange((event) => events.push(['did-change', event])) buffer.onDidReload((event) => events.push(['did-reload'])) const markerB = buffer.markRange(Range(Point(0, 1), Point(0, 2))) @@ -915,12 +907,6 @@ describe('TextBuffer IO', () => { 'did-change', { oldRange: Range(Point(0, 0), Point(0, 5)), newRange: Range(Point(0, 0), Point(0, newText.length)), - oldText: 'abcde', - newText: newText - } - ], - [ - 'did-change-text', { changes: [ { oldRange: Range(Point.ZERO, Point.ZERO), @@ -952,7 +938,6 @@ describe('TextBuffer IO', () => { const events = [] buffer.onWillChange(() => events.push(['will-change'])) buffer.onDidChange((event) => events.push(['did-change', event])) - buffer.onDidChangeText((event) => events.push(['did-change-text', event])) const subscription = buffer.onDidReload(() => { subscription.dispose() @@ -967,12 +952,6 @@ describe('TextBuffer IO', () => { 'did-change', { oldRange: Range(Point(0, 3), Point(0, 5)), newRange: Range(Point(0, 3), Point(0, 7)), - oldText: 'de', - newText: ' de ' - } - ], - [ - 'did-change-text', { changes: [ { oldRange: Range(Point(0, 3), Point(0, 3)), @@ -1000,7 +979,6 @@ describe('TextBuffer IO', () => { buffer.onWillReload((event) => events.push(event)) buffer.onDidReload((event) => events.push(event)) buffer.onDidChange((event) => events.push(event)) - buffer.onDidChangeText((event) => events.push(event)) buffer.onDidConflict((event) => events.push(event)) fs.writeFileSync(buffer.getPath(), 'abcde') @@ -1019,7 +997,6 @@ describe('TextBuffer IO', () => { const changeEvents = [] buffer.onWillChange(() => changeEvents.push('will-change')) buffer.onDidChange((event) => changeEvents.push('did-change')) - buffer.onDidChangeText((event) => changeEvents.push('did-change-text')) // We debounce file system change events to avoid redundant loads. But // for large files, another file system change event may occur *after* the @@ -1049,9 +1026,9 @@ describe('TextBuffer IO', () => { }, buffer.fileChangeDelay + 50) }, buffer.fileChangeDelay + 50) - const subscription = buffer.onDidChangeText(() => { + const subscription = buffer.onDidChange(() => { if (buffer.getText() === 'abcdef') { - expect(changeEvents).toEqual(['will-change', 'did-change', 'did-change-text']) + expect(changeEvents).toEqual(['will-change', 'did-change']) subscription.dispose() done() } diff --git a/spec/text-buffer-spec.coffee b/spec/text-buffer-spec.coffee index 7bfab9b97e..da36000c2f 100644 --- a/spec/text-buffer-spec.coffee +++ b/spec/text-buffer-spec.coffee @@ -97,22 +97,27 @@ 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, ::onDidChange observers and display layer ::onDidChangeSync observers with the relevant details", -> + it "notifies, in order, decoration layers, display layers, and display layer ::onDidChangeSync observers with the relevant details", -> events = [] - textDecorationLayer1 = {bufferDidChange: (e) -> events.push({source: textDecorationLayer1, event: e})} - textDecorationLayer2 = {bufferDidChange: (e) -> events.push({source: textDecorationLayer2, event: e})} + textDecorationLayer1 = { + bufferDidChange: (e) -> events.push({source: 'decoration-layer-1', event: e}) + } + textDecorationLayer2 = { + bufferDidChange: (e) -> events.push({source: 'decoration-layer-2', event: e}), + jasmineToString: -> '' + } displayLayer1 = buffer.addDisplayLayer() displayLayer2 = buffer.addDisplayLayer() spyOn(displayLayer1, 'bufferDidChange').and.callFake (e) -> - events.push({source: displayLayer1, event: e}) + events.push({source: 'display-layer-1', event: e}) DisplayLayer.prototype.bufferDidChange.call(displayLayer1, e) spyOn(displayLayer2, 'bufferDidChange').and.callFake (e) -> - events.push({source: displayLayer2, event: e}) + events.push({source: 'display-layer-2', event: e}) DisplayLayer.prototype.bufferDidChange.call(displayLayer2, e) - buffer.onDidChange (e) -> events.push({source: buffer, event: e}) buffer.registerTextDecorationLayer(textDecorationLayer1) buffer.registerTextDecorationLayer(textDecorationLayer1) # insert a duplicate decoration layer buffer.registerTextDecorationLayer(textDecorationLayer2) + buffer.onDidChange (e) -> events.push({source: 'buffer', event: JSON.parse(JSON.stringify(e))}) disposable = displayLayer1.onDidChangeSync -> disposable.dispose() @@ -128,17 +133,31 @@ describe "TextBuffer", -> oldText: "a", newText: "abc", } expect(events).toEqual [ - {source: textDecorationLayer1, event: changeEvent1}, - {source: textDecorationLayer2, event: changeEvent1}, - {source: displayLayer1, event: changeEvent1}, - {source: displayLayer2, event: changeEvent1}, - {source: buffer, event: changeEvent1}, - - {source: textDecorationLayer1, event: changeEvent2}, - {source: textDecorationLayer2, event: changeEvent2}, - {source: displayLayer1, event: changeEvent2}, - {source: displayLayer2, event: changeEvent2}, - {source: buffer, event: changeEvent2} + {source: 'decoration-layer-1', event: changeEvent1}, + {source: 'decoration-layer-2', event: changeEvent1}, + {source: 'display-layer-1', event: changeEvent1}, + {source: 'display-layer-2', event: changeEvent1}, + + {source: 'decoration-layer-1', event: changeEvent2}, + {source: 'decoration-layer-2', event: changeEvent2}, + {source: 'display-layer-1', event: changeEvent2}, + {source: 'display-layer-2', event: changeEvent2}, + + { + source: 'buffer', + event: { + oldRange: Range(Point(0, 2), Point(2, 3)), + newRange: Range(Point(0, 2), Point(2, 4)), + changes: [ + { + oldRange: Range(Point(0, 2), Point(2, 3)), + newRange: Range(Point(0, 2), Point(2, 4)), + oldText: "llo\nworld\r\nhow", + newText: "y there\r\ncabct\nwhat" + } + ] + } + } ] it "returns the newRange of the change", -> @@ -214,8 +233,8 @@ describe "TextBuffer", -> }]) it "still emits text change events (regression)", (done) -> - didChangeTextEvents = [] - buffer.onDidChangeText (event) -> didChangeTextEvents.push(event) + didChangeEvents = [] + buffer.onDidChange (event) -> didChangeEvents.push(event) buffer.onDidStopChanging ({changes}) -> assertChangesEqual(changes, [{ @@ -227,8 +246,8 @@ describe "TextBuffer", -> done() buffer.setTextInRange([[0, 0], [0, 1]], 'y', {undo: 'skip'}) - expect(didChangeTextEvents.length).toBe(1) - assertChangesEqual(didChangeTextEvents[0].changes, [{ + expect(didChangeEvents.length).toBe(1) + assertChangesEqual(didChangeEvents[0].changes, [{ oldRange: [[0, 0], [0, 1]], newRange: [[0, 0], [0, 1]], oldText: 'h', @@ -236,8 +255,8 @@ describe "TextBuffer", -> }]) buffer.transact -> buffer.setTextInRange([[0, 0], [0, 1]], 'z', {undo: 'skip'}) - expect(didChangeTextEvents.length).toBe(2) - assertChangesEqual(didChangeTextEvents[1].changes, [{ + expect(didChangeEvents.length).toBe(2) + assertChangesEqual(didChangeEvents[1].changes, [{ oldRange: [[0, 0], [0, 1]], newRange: [[0, 0], [0, 1]], oldText: 'y', @@ -266,28 +285,28 @@ describe "TextBuffer", -> expect(changeEvents[1].newText).toBe "ms\r\ndo you\r\nlike\r\ndirt" buffer.setTextInRange([[5, 1], [5, 3]], '\r') - expect(changeEvents[2]).toEqual({ + expect(changeEvents[2].changes).toEqual([{ oldRange: [[5, 1], [5, 3]], newRange: [[5, 1], [6, 0]], oldText: 'ik', newText: '\r\n' - }) + }]) buffer.undo() - expect(changeEvents[3]).toEqual({ + expect(changeEvents[3].changes).toEqual([{ oldRange: [[5, 1], [6, 0]], newRange: [[5, 1], [5, 3]], oldText: '\r\n', newText: 'ik' - }) + }]) buffer.redo() - expect(changeEvents[4]).toEqual({ + expect(changeEvents[4].changes).toEqual([{ oldRange: [[5, 1], [5, 3]], newRange: [[5, 1], [6, 0]], oldText: 'ik', newText: '\r\n' - }) + }]) describe "when the range's start row has no line ending (because it's the last line of the buffer)", -> describe "when the buffer contains no newlines", -> diff --git a/src/helpers.js b/src/helpers.js index eb779a2716..c3f61c88ef 100644 --- a/src/helpers.js +++ b/src/helpers.js @@ -93,6 +93,15 @@ class TextChange { this.oldText = oldText this.newText = newText } + + isEqual (other) { + return ( + this.oldRange.isEqual(other.oldRange) && + this.newRange.isEqual(other.newRange) && + this.oldText === other.oldText && + this.newText === other.newText + ) + } } Object.defineProperty(TextChange.prototype, 'start', { diff --git a/src/text-buffer.coffee b/src/text-buffer.coffee index c00968610e..51f9a1ba7a 100644 --- a/src/text-buffer.coffee +++ b/src/text-buffer.coffee @@ -20,25 +20,29 @@ Grim = require 'grim' class TransactionAbortedError extends Error constructor: -> super -class CompositeChangeEvent - constructor: (buffer, patch) -> - {oldStart: compositeStart, oldEnd, newEnd} = patch.getBounds() - @oldRange = new Range(compositeStart, oldEnd) - @newRange = new Range(compositeStart, newEnd) +class ChangeEvent + constructor: (buffer, changes) -> + @changes = changes + + start = changes[0].oldRange.start + oldEnd = changes[changes.length - 1].oldRange.end + newEnd = changes[changes.length - 1].newRange.end + @oldRange = new Range(start, oldEnd).freeze() + @newRange = new Range(start, newEnd).freeze() oldText = null newText = null Object.defineProperty(this, 'oldText', { - enumerable: true, + enumerable: false, get: -> unless oldText? oldBuffer = new NativeTextBuffer(@newText) - for change in patch.getChanges() by -1 + for change in changes by -1 oldBuffer.setTextInRange( new Range( - traversal(change.newStart, compositeStart), - traversal(change.newEnd, compositeStart) + traversal(change.newRange.start, start), + traversal(change.newRange.end, start) ), change.oldText ) @@ -47,28 +51,32 @@ class CompositeChangeEvent }) Object.defineProperty(this, 'newText', { - enumerable: true, + enumerable: false, get: -> unless newText? newText = buffer.getTextInRange(@newRange) newText }) + isEqual: (other) -> + ( + @changes.length is other.changes.length and + @changes.every((change, i) -> change.isEqual(other.changes[i])) and + @oldRange.isEqual(other.oldRange) and + @newRange.isEqual(other.newRange) + ) + # Extended: A mutable text container with undo/redo support and the ability to # annotate logical regions in the text. # -# ## Working With Aggregated Changes -# -# When observing changes to the buffer's textual content, it is important to use -# change-aggregating methods such as {::onDidChangeText}, {::onDidStopChanging}, -# and {::getChangesSinceCheckpoint} in order to maintain high performance. These -# methods allows your code to respond to *sets* of changes rather than each -# individual change. +# ## Observing Changes # -# These methods report aggregated buffer updates as arrays of change objects -# containing the following fields: `oldRange`, `newRange`, `oldText`, and -# `newText`. The `oldText`, `newText`, and `newRange` fields are -# self-explanatory, but the interepretation of `oldRange` is more nuanced: +# You can observe changes in a {TextBuffer} using methods like {::onDidChange}, +# {::onDidStopChanging}, and {::getChangesSinceCheckpoint}. These methods report +# aggregated buffer updates as arrays of change objects containing the following +# fields: `oldRange`, `newRange`, `oldText`, and `newText`. The `oldText`, +# `newText`, and `newRange` fields are self-explanatory, but the interepretation +# of `oldRange` is more nuanced: # # The reported `oldRange` is the range of the replaced text in the original # contents of the buffer *irrespective of the spatial impact of any other @@ -77,7 +85,7 @@ class CompositeChangeEvent # be to apply the changes in reverse: # # ```js -# buffer1.onDidChangeText(({changes}) => { +# buffer1.onDidChange(({changes}) => { # for (const {oldRange, newText} of changes.reverse()) { # buffer2.setTextInRange(oldRange, newText) # } @@ -89,7 +97,7 @@ class CompositeChangeEvent # {::setTextInRange}, as follows: # # ```js -# buffer1.onDidChangeText(({changes}) => { +# buffer1.onDidChange(({changes}) => { # for (const {oldRange, newRange, newText} of changes) { # const rangeToReplace = Range( # newRange.start, @@ -335,35 +343,14 @@ class TextBuffer onWillChange: (callback) -> @emitter.on 'will-change', callback - # Public: Invoke the given callback synchronously when the content of the - # buffer changes. **You should probably not be using this in packages**. - # - # Because observers are invoked synchronously, it's important not to perform - # any expensive operations via this method. Consider {::onDidStopChanging} to - # delay expensive operations until after changes stop occurring, or at the - # very least use {::onDidChangeText} to invoke your callback once *per - # transaction* rather than *once per change*. This will help prevent - # performance degredation when users of your package are typing with multiple - # cursors, and other scenarios in which multiple changes occur within - # transactions. - # - # * `callback` {Function} to be called when the buffer changes. - # * `event` {Object} with the following keys: - # * `oldRange` {Range} of the old text. - # * `newRange` {Range} of the new text. - # * `oldText` {String} containing the text that was replaced. - # * `newText` {String} containing the text that was inserted. - # - # Returns a {Disposable} on which `.dispose()` can be called to unsubscribe. - onDidChange: (callback) -> - @emitter.on 'did-change', callback - # Public: Invoke the given callback synchronously when a transaction finishes # with a list of all the changes in the transaction. # # * `callback` {Function} to be called when a transaction in which textual # changes occurred is completed. # * `event` {Object} with the following keys: + # * `oldRange` The smallest combined {Range} containing all of the old text. + # * `newRange` The smallest combined {Range} containing all of the new text. # * `changes` {Array} of {Object}s summarizing the aggregated changes # that occurred during the transaction. See *Working With Aggregated # Changes* in the description of the {TextBuffer} class for details. @@ -376,9 +363,12 @@ class TextBuffer # * `newText`: A {String} representing the inserted text. # # Returns a {Disposable} on which `.dispose()` can be called to unsubscribe. - onDidChangeText: (callback) -> + onDidChange: (callback) -> @emitter.on 'did-change-text', callback + # Public: This is now identical to {onDidChange}. + onDidChangeText: (callback) -> @onDidChange(callback) + preemptDidChange: (callback) -> @emitter.preempt 'did-change', callback @@ -388,7 +378,7 @@ class TextBuffer # # This method can be used to perform potentially expensive operations that # don't need to be performed synchronously. If you need to run your callback - # synchronously, use {::onDidChangeText} instead. + # synchronously, use {::onDidChange} instead. # # * `callback` {Function} to be called when the buffer stops changing. # * `event` {Object} with the following keys: @@ -1198,9 +1188,7 @@ class TextBuffer abortTransaction: -> throw new TransactionAbortedError("Transaction aborted.") - # Public: Clear the undo stack. When calling this method within a transaction, - # the {::onDidChangeText} event will not be triggered because the information - # describing the changes is lost. + # Public: Clear the undo stack. clearUndoStack: -> @historyProvider.clearUndoStack() # Public: Create a pointer to the current state of the buffer for use @@ -1801,8 +1789,6 @@ class TextBuffer traversal(change.newEnd, change.newStart) ) - @emitDidChangeEvent(new CompositeChangeEvent(@buffer, patch)) - markersSnapshot = @createMarkerSnapshot() @historyProvider.groupChangesSinceCheckpoint(checkpoint, {markers: markersSnapshot, deleteCheckpoint: true}) @@ -1924,7 +1910,8 @@ class TextBuffer patchFromChanges(@changesSinceLastDidChangeTextEvent).getChanges() )) @changesSinceLastDidChangeTextEvent.length = 0 - @emitter.emit 'did-change-text', {changes: compactedChanges} + if compactedChanges.length > 0 + @emitter.emit 'did-change-text', new ChangeEvent(this, compactedChanges) @debouncedEmitDidStopChangingEvent() @_emittedWillChangeEvent = false From e9d084f99ed859262426a60dd894c13019645247 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Thu, 26 Oct 2017 09:58:27 -0700 Subject: [PATCH 2/3] 13.7.0-did-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 8259a784a7..600e5c9d69 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "text-buffer", - "version": "13.6.1", + "version": "13.7.0-did-change-event-1", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/package.json b/package.json index 8689438dd1..3a3c6fd31a 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "text-buffer", - "version": "13.6.1", + "version": "13.7.0-did-change-event-1", "description": "A container for large mutable strings with annotated regions", "main": "./lib/text-buffer", "scripts": { From 15ade70d439b138f5e1bad8981bae1ddbd462450 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Thu, 26 Oct 2017 14:54:13 -0700 Subject: [PATCH 3/3] Update onDidChangeText -> onDidChange in some specs --- spec/text-buffer-spec.coffee | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/spec/text-buffer-spec.coffee b/spec/text-buffer-spec.coffee index da36000c2f..1eff8c121d 100644 --- a/spec/text-buffer-spec.coffee +++ b/spec/text-buffer-spec.coffee @@ -2264,7 +2264,7 @@ describe "TextBuffer", -> buffer.setText('\n') expect(buffer.isEmpty()).toBeFalsy() - describe "::onWillChange", -> + describe "::onWillChange(callback)", -> it "notifies observers before a transaction, an undo or a redo", -> changeCount = 0 expectedText = '' @@ -2301,14 +2301,14 @@ describe "TextBuffer", -> buffer.revertToCheckpoint(checkpoint) expect(changeCount).toBe(5) - describe "::onDidChangeText(callback)", -> + describe "::onDidChange(callback)", -> beforeEach -> filePath = require.resolve('./fixtures/sample.js') buffer = TextBuffer.loadSync(filePath) it "notifies observers after a transaction, an undo or a redo", -> textChanges = [] - buffer.onDidChangeText ({changes}) -> textChanges.push(changes...) + buffer.onDidChange ({changes}) -> textChanges.push(changes...) buffer.insert([0, 0], "abc") buffer.delete([[0, 0], [0, 1]]) @@ -2402,12 +2402,12 @@ describe "TextBuffer", -> it "doesn't notify observers after an empty transaction", -> didChangeTextSpy = jasmine.createSpy() - buffer.onDidChangeText(didChangeTextSpy) + buffer.onDidChange(didChangeTextSpy) buffer.transact(->) expect(didChangeTextSpy).not.toHaveBeenCalled() it "doesn't throw an error when clearing the undo stack within a transaction", -> - buffer.onDidChangeText(didChangeTextSpy = jasmine.createSpy()) + buffer.onDidChange(didChangeTextSpy = jasmine.createSpy()) expect(-> buffer.transact(-> buffer.clearUndoStack())).not.toThrowError() expect(didChangeTextSpy).not.toHaveBeenCalled() @@ -2476,8 +2476,8 @@ describe "TextBuffer", -> ]) done() - it "provides the correct changes when the buffer is mutated in the onDidChangeText callback", (done) -> - buffer.onDidChangeText ({changes}) -> + it "provides the correct changes when the buffer is mutated in the onDidChange callback", (done) -> + buffer.onDidChange ({changes}) -> switch changes[0].newText when 'a' buffer.insert(changes[0].newRange.end, 'b')