From b3234dd55b7f5d7a0148f3a2283f6a9a391ae2ac Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 20 Apr 2016 14:08:17 +0200 Subject: [PATCH 01/11] Provide created, destroyed, updated markers in markerLayer.onDidUpdate --- spec/marker-layer-spec.coffee | 37 ++++++++++++++++++++++++++++----- src/display-marker-layer.coffee | 4 ++-- src/marker-layer.coffee | 15 +++++++++++-- src/marker.coffee | 2 +- 4 files changed, 48 insertions(+), 10 deletions(-) diff --git a/spec/marker-layer-spec.coffee b/spec/marker-layer-spec.coffee index 9a2df528a9..bf47724a46 100644 --- a/spec/marker-layer-spec.coffee +++ b/spec/marker-layer-spec.coffee @@ -6,7 +6,14 @@ describe "MarkerLayer", -> beforeEach -> jasmine.addCustomEqualityTester(require("underscore-plus").isEqual) - buffer = new TextBuffer(text: "abcdefghijklmnopqrstuvwxyz") + buffer = new TextBuffer(text: """ + Lorem ipsum dolor sit amet, + consectetur adipisicing elit, + sed do eiusmod tempor incididunt + ut labore et dolore magna aliqua. + Ut enim ad minim veniam, quis + nostrud exercitation ullamco laboris. + """) layer1 = buffer.addMarkerLayer() layer2 = buffer.addMarkerLayer() @@ -171,18 +178,38 @@ describe "MarkerLayer", -> describe "::onDidUpdate", -> it "notifies observers asynchronously when markers are created, updated, or destroyed", (done) -> updateCount = 0 - layer1.onDidUpdate -> + layer1.onDidUpdate ({created, destroyed, updated}) -> updateCount++ if updateCount is 1 + expect(destroyed.size).toBe(0) + expect(updated.size).toBe(0) + expect(created.has(marker1.id)).toBe(true) + expect(created.has(marker2.id)).toBe(true) + marker1.setRange([[1, 2], [3, 4]]) - marker2.setRange([[4, 5], [6, 7]]) + marker2.setRange([[3, 10], [4, 5]]) else if updateCount is 2 - buffer.insert([0, 1], "xxx") - buffer.insert([0, 1], "yyy") + expect(created.size).toBe(0) + expect(destroyed.size).toBe(0) + expect(updated.has(marker1.id)).toBe(true) + expect(updated.has(marker2.id)).toBe(true) + + buffer.insert([1, 3], "xxx") + buffer.insert([2, 0], "yyy") else if updateCount is 3 + expect(created.size).toBe(0) + expect(destroyed.size).toBe(0) + expect(updated.has(marker1.id)).toBe(true) + expect(updated.has(marker2.id)).toBe(false) + marker1.destroy() marker2.destroy() else if updateCount is 4 + expect(created.size).toBe(0) + expect(updated.size).toBe(0) + expect(destroyed.has(marker1.id)).toBe(true) + expect(destroyed.has(marker2.id)).toBe(true) + done() marker1 = layer1.markRange([[0, 2], [0, 4]]) diff --git a/src/display-marker-layer.coffee b/src/display-marker-layer.coffee index e1dc18aba6..2098239449 100644 --- a/src/display-marker-layer.coffee +++ b/src/display-marker-layer.coffee @@ -291,8 +291,8 @@ class DisplayMarkerLayer translateScreenRange: (screenRange, options) -> @displayLayer.translateScreenRange(screenRange, options) - emitDidUpdate: -> - @emitter.emit('did-update') + emitDidUpdate: (event) -> + @emitter.emit('did-update', event) emitDidDestroy: -> @emitter.emit('did-destroy') diff --git a/src/marker-layer.coffee b/src/marker-layer.coffee index ce07e2f787..c481c70b52 100644 --- a/src/marker-layer.coffee +++ b/src/marker-layer.coffee @@ -39,6 +39,9 @@ class MarkerLayer @index = new MarkerIndex @markersById = {} @markersIdsWithChangeSubscriptions = new Set + @createdMarkers = new Set + @destroyedMarkers = new Set + @updatedMarkers = new Set @destroyed = false @emitCreateMarkerEvents = false @@ -255,6 +258,7 @@ class MarkerLayer marker.destroy() else marker.valid = false + @updatedMarkers.add(id) @scheduleUpdateEvent() restoreFromSnapshot: (snapshots) -> @@ -313,8 +317,9 @@ class MarkerLayer Section: Private - Marker interface ### - markerUpdated: -> + markerUpdated: (id) -> @delegate.markersUpdated(this) + @updatedMarkers.add(id) @scheduleUpdateEvent() destroyMarker: (id) -> @@ -323,6 +328,7 @@ class MarkerLayer @markersIdsWithChangeSubscriptions.delete(id) @index.delete(id) @delegate.markersUpdated(this) + @destroyedMarkers.add(id) @scheduleUpdateEvent() getMarkerRange: (id) -> @@ -352,6 +358,7 @@ class MarkerLayer marker = @addMarker(id, range, params) @delegate.markerCreated(this, marker) @delegate.markersUpdated(this) + @createdMarkers.add(id) @scheduleUpdateEvent() @emitter.emit 'did-create-marker', marker if @emitCreateMarkerEvents marker @@ -371,7 +378,11 @@ class MarkerLayer @didUpdateEventScheduled = true process.nextTick => @didUpdateEventScheduled = false - @emitter.emit 'did-update' + event = {created: @createdMarkers, destroyed: @destroyedMarkers, updated: @updatedMarkers} + @createdMarkers = new Set + @destroyedMarkers = new Set + @updatedMarkers = new Set + @emitter.emit 'did-update', event filterSet = (set1, set2) -> if set1 diff --git a/src/marker.coffee b/src/marker.coffee index 2a8dd5650a..0aecf89f4a 100644 --- a/src/marker.coffee +++ b/src/marker.coffee @@ -370,7 +370,7 @@ class Marker updated = true @emitChangeEvent(range ? oldRange, textChanged, propertiesChanged) - @layer.markerUpdated() if updated and not textChanged + @layer.markerUpdated(@id) if updated and not textChanged updated getSnapshot: (range) -> From 85ce3bd41e2589460e62980debabe4d07adeb64f Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 20 Apr 2016 14:10:13 +0200 Subject: [PATCH 02/11] Deprecate calling marker.onDidChange --- src/marker.coffee | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/marker.coffee b/src/marker.coffee index 0aecf89f4a..00ba05e425 100644 --- a/src/marker.coffee +++ b/src/marker.coffee @@ -102,6 +102,11 @@ class Marker # # Returns a {Disposable} on which `.dispose()` can be called to unsubscribe. onDidChange: (callback) -> + Grim.deprecate(""" + Subscribing to marker change events is deprecated. Please, consider using + MarkerLayer.prototype.onDidUpdate instead. + """) + unless @hasChangeObservers @previousEventState = @getSnapshot(@getRange()) @hasChangeObservers = true From ae17f3bb66c5d1cb1fedd6558d4af4a77513b07b Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 20 Apr 2016 15:10:47 +0200 Subject: [PATCH 03/11] Emit markerLayer.onDidUpdate event at the end of the transaction --- spec/display-marker-layer-spec.coffee | 40 +++++--- spec/marker-layer-spec.coffee | 138 +++++++++++++++++++------- src/marker-layer.coffee | 38 +++---- src/text-buffer.coffee | 88 +++++++++------- 4 files changed, 200 insertions(+), 104 deletions(-) diff --git a/spec/display-marker-layer-spec.coffee b/spec/display-marker-layer-spec.coffee index 4a886c7689..25afea9739 100644 --- a/spec/display-marker-layer-spec.coffee +++ b/spec/display-marker-layer-spec.coffee @@ -131,24 +131,38 @@ describe "DisplayMarkerLayer", -> marker.destroy() expect(destroyEventCount).toBe 1 - it "emits update events when markers are created, updated directly, updated indirectly, or destroyed", (done) -> - buffer = new TextBuffer(text: 'hello world') + it "emits update events when markers are created, updated directly, updated indirectly, or destroyed", -> + buffer = new TextBuffer(text: 'hello world\nhow are you?') displayLayer = buffer.addDisplayLayer(tabLength: 4) markerLayer = displayLayer.addMarkerLayer() - updateEventCount = 0 - markerLayer.onDidUpdate -> - updateEventCount++ - if updateEventCount is 1 - marker.setScreenRange([[0, 5], [1, 0]]) - else if updateEventCount is 2 - buffer.insert([0, 0], '\t') - else if updateEventCount is 3 - marker.destroy() - else if updateEventCount is 4 - done() + events = [] + markerLayer.onDidUpdate (event) -> events.push(event) + events = [] marker = markerLayer.markScreenRange([[0, 4], [1, 4]]) + expect(events.length).toBe(1) + expect(Array.from(events[0].created)).toEqual [marker.id] + expect(Array.from(events[0].updated)).toEqual [] + expect(Array.from(events[0].destroyed)).toEqual [] + + events = [] + marker.setScreenRange([[0, 5], [1, 0]]) + expect(events.length).toBe(1) + expect(Array.from(events[0].created)).toEqual [] + expect(Array.from(events[0].updated)).toEqual [marker.id] + expect(Array.from(events[0].destroyed)).toEqual [] + + events = [] + buffer.insert([0, 0], '\t') + expect(events.length).toBe(0) + + events = [] + marker.destroy() + expect(events.length).toBe(1) + expect(Array.from(events[0].created)).toEqual [] + expect(Array.from(events[0].updated)).toEqual [] + expect(Array.from(events[0].destroyed)).toEqual [marker.id] it "allows markers to be copied", -> buffer = new TextBuffer(text: '\ta\tbc\tdef\tg\n\th') diff --git a/spec/marker-layer-spec.coffee b/spec/marker-layer-spec.coffee index bf47724a46..f79fe85568 100644 --- a/spec/marker-layer-spec.coffee +++ b/spec/marker-layer-spec.coffee @@ -176,44 +176,106 @@ describe "MarkerLayer", -> expect(layer2.findMarkers(containsPoint: [0, 4])).toEqual [layer2Marker] describe "::onDidUpdate", -> - it "notifies observers asynchronously when markers are created, updated, or destroyed", (done) -> - updateCount = 0 - layer1.onDidUpdate ({created, destroyed, updated}) -> - updateCount++ - if updateCount is 1 - expect(destroyed.size).toBe(0) - expect(updated.size).toBe(0) - expect(created.has(marker1.id)).toBe(true) - expect(created.has(marker2.id)).toBe(true) - - marker1.setRange([[1, 2], [3, 4]]) - marker2.setRange([[3, 10], [4, 5]]) - else if updateCount is 2 - expect(created.size).toBe(0) - expect(destroyed.size).toBe(0) - expect(updated.has(marker1.id)).toBe(true) - expect(updated.has(marker2.id)).toBe(true) - - buffer.insert([1, 3], "xxx") - buffer.insert([2, 0], "yyy") - else if updateCount is 3 - expect(created.size).toBe(0) - expect(destroyed.size).toBe(0) - expect(updated.has(marker1.id)).toBe(true) - expect(updated.has(marker2.id)).toBe(false) - - marker1.destroy() - marker2.destroy() - else if updateCount is 4 - expect(created.size).toBe(0) - expect(updated.size).toBe(0) - expect(destroyed.has(marker1.id)).toBe(true) - expect(destroyed.has(marker2.id)).toBe(true) - - done() - - marker1 = layer1.markRange([[0, 2], [0, 4]]) - marker2 = layer1.markRange([[0, 6], [0, 8]]) + it "notifies observers synchronously or at the end of a transaction when markers are created, updated, or destroyed", -> + layer = buffer.addMarkerLayer({maintainHistory: true}) + events = [] + [marker1, marker2, marker3] = [] + layer.onDidUpdate (event) -> events.push(event) + + buffer.transact -> + marker1 = layer.markRange([[0, 2], [0, 4]]) + marker2 = layer.markRange([[0, 6], [0, 8]]) + expect(events.length).toBe(0) + + marker3 = layer.markRange([[4, 0], [4, 5]]) + + expect(events.length).toBe(2) + expect(Array.from(events[0].created)).toEqual [marker1.id, marker2.id] + expect(Array.from(events[0].updated)).toEqual [] + expect(Array.from(events[0].destroyed)).toEqual [] + expect(Array.from(events[1].created)).toEqual [marker3.id] + expect(Array.from(events[1].updated)).toEqual [] + expect(Array.from(events[1].destroyed)).toEqual [] + + events = [] + buffer.transact -> + marker1.setRange([[1, 2], [3, 4]]) + marker2.setRange([[3, 10], [4, 5]]) + marker3.destroy() + + expect(events.length).toBe(1) + expect(Array.from(events[0].created)).toEqual [] + expect(Array.from(events[0].updated)).toEqual [marker1.id, marker2.id] + expect(Array.from(events[0].destroyed)).toEqual [marker3.id] + + events = [] + buffer.transact -> + buffer.insert([1, 3], "xxx") + buffer.insert([2, 0], "yyy") + buffer.insert([1, 5], 'zzz') + + expect(events.length).toBe(2) + expect(Array.from(events[0].created)).toEqual [] + expect(Array.from(events[0].updated)).toEqual [marker1.id] + expect(Array.from(events[0].destroyed)).toEqual [] + expect(Array.from(events[1].created)).toEqual [] + expect(Array.from(events[1].updated)).toEqual [marker1.id] + expect(Array.from(events[1].destroyed)).toEqual [] + + events = [] + buffer.undo() + buffer.undo() + + expect(events.length).toBe(2) + expect(Array.from(events[0].created)).toEqual [] + expect(Array.from(events[0].updated)).toEqual [marker1.id] + expect(Array.from(events[0].destroyed)).toEqual [] + expect(Array.from(events[1].created)).toEqual [] + expect(Array.from(events[1].updated)).toEqual [marker1.id] + expect(Array.from(events[1].destroyed)).toEqual [] + + events = [] + buffer.transact -> + buffer.insert([1, 3], 'aaa') + buffer.insert([3, 11], 'bbb') + buffer.transact -> + buffer.insert([1, 9], 'ccc') + buffer.insert([1, 12], 'ddd') + buffer.insert([4, 0], 'eee') + buffer.insert([4, 3], 'fff') + + expect(events.length).toBe(2) + expect(Array.from(events[0].created)).toEqual [] + expect(Array.from(events[0].updated)).toEqual [marker1.id, marker2.id] + expect(Array.from(events[0].destroyed)).toEqual [] + expect(Array.from(events[1].created)).toEqual [] + expect(Array.from(events[1].updated)).toEqual [marker2.id] + expect(Array.from(events[1].destroyed)).toEqual [] + + events = [] + buffer.transact -> + buffer.insert([3, 11], 'ggg') + buffer.undo() + marker1.clearTail() + marker2.clearTail() + + expect(events.length).toBe(2) + expect(Array.from(events[0].created)).toEqual [] + expect(Array.from(events[0].updated)).toEqual [marker2.id] + expect(Array.from(events[0].destroyed)).toEqual [] + expect(Array.from(events[1].created)).toEqual [] + expect(Array.from(events[1].updated)).toEqual [marker1.id, marker2.id] + expect(Array.from(events[1].destroyed)).toEqual [] + + events = [] + buffer.transact -> + marker1.destroy() + marker2.destroy() + + expect(events.length).toBe(1) + expect(Array.from(events[0].created)).toEqual [] + expect(Array.from(events[0].updated)).toEqual [] + expect(Array.from(events[0].destroyed)).toEqual [marker1.id, marker2.id] describe "::copy", -> it "creates a new marker layer with markers in the same states", -> diff --git a/src/marker-layer.coffee b/src/marker-layer.coffee index c481c70b52..2b9df732bf 100644 --- a/src/marker-layer.coffee +++ b/src/marker-layer.coffee @@ -42,6 +42,7 @@ class MarkerLayer @createdMarkers = new Set @destroyedMarkers = new Set @updatedMarkers = new Set + @setDisableDidUpdateEvent(false) @destroyed = false @emitCreateMarkerEvents = false @@ -259,7 +260,7 @@ class MarkerLayer else marker.valid = false @updatedMarkers.add(id) - @scheduleUpdateEvent() + @emitDidUpdateEvent() restoreFromSnapshot: (snapshots) -> return unless snapshots? @@ -318,18 +319,18 @@ class MarkerLayer ### markerUpdated: (id) -> - @delegate.markersUpdated(this) @updatedMarkers.add(id) - @scheduleUpdateEvent() + @emitDidUpdateEvent() + @delegate.markersUpdated(this) destroyMarker: (id) -> if @markersById.hasOwnProperty(id) delete @markersById[id] @markersIdsWithChangeSubscriptions.delete(id) @index.delete(id) - @delegate.markersUpdated(this) @destroyedMarkers.add(id) - @scheduleUpdateEvent() + @emitDidUpdateEvent() + @delegate.markersUpdated(this) getMarkerRange: (id) -> Range.fromObject(@index.getRange(id)) @@ -356,11 +357,11 @@ class MarkerLayer createMarker: (range, params) -> id = @delegate.getNextMarkerId() marker = @addMarker(id, range, params) - @delegate.markerCreated(this, marker) - @delegate.markersUpdated(this) @createdMarkers.add(id) - @scheduleUpdateEvent() + @emitDidUpdateEvent() @emitter.emit 'did-create-marker', marker if @emitCreateMarkerEvents + @delegate.markerCreated(this, marker) + @delegate.markersUpdated(this) marker ### @@ -373,16 +374,17 @@ class MarkerLayer @index.insert(id, range.start, range.end) @markersById[id] = new Marker(id, this, range, params) - scheduleUpdateEvent: -> - unless @didUpdateEventScheduled - @didUpdateEventScheduled = true - process.nextTick => - @didUpdateEventScheduled = false - event = {created: @createdMarkers, destroyed: @destroyedMarkers, updated: @updatedMarkers} - @createdMarkers = new Set - @destroyedMarkers = new Set - @updatedMarkers = new Set - @emitter.emit 'did-update', event + setDisableDidUpdateEvent: (@didUpdateEventDisabled) -> + + emitDidUpdateEvent: -> + return if @didUpdateEventDisabled + + if @createdMarkers.size > 0 or @destroyedMarkers.size > 0 or @updatedMarkers.size > 0 + event = {created: @createdMarkers, destroyed: @destroyedMarkers, updated: @updatedMarkers} + @createdMarkers = new Set + @destroyedMarkers = new Set + @updatedMarkers = new Set + @emitter.emit 'did-update', event filterSet = (set1, set2) -> if set1 diff --git a/src/text-buffer.coffee b/src/text-buffer.coffee index 2238f82835..3b96501858 100644 --- a/src/text-buffer.coffee +++ b/src/text-buffer.coffee @@ -108,6 +108,7 @@ class TextBuffer @markerLayers = params?.markerLayers ? {} @markerLayers[@defaultMarkerLayer.id] = @defaultMarkerLayer @nextMarkerId = params?.nextMarkerId ? 1 + @markerLayerDidUpdateEventDisabled = false @setEncoding(params?.encoding) @setPreferredLineEnding(params?.preferredLineEnding) @@ -963,22 +964,24 @@ class TextBuffer # Public: Undo the last operation. If a transaction is in progress, aborts it. undo: -> if pop = @history.popUndoStack() - @applyChange(change) for change in pop.patch.getChanges() - @restoreFromMarkerSnapshot(pop.snapshot) - @emitMarkerChangeEvents(pop.snapshot) - @emitDidChangeTextEvent(pop.patch) - true + @emitMarkerLayersDidUpdateEvent => + @applyChange(change) for change in pop.patch.getChanges() + @restoreFromMarkerSnapshot(pop.snapshot) + @emitMarkerChangeEvents(pop.snapshot) + @emitDidChangeTextEvent(pop.patch) + true else false # Public: Redo the last operation redo: -> if pop = @history.popRedoStack() - @applyChange(change) for change in pop.patch.getChanges() - @restoreFromMarkerSnapshot(pop.snapshot) - @emitMarkerChangeEvents(pop.snapshot) - @emitDidChangeTextEvent(pop.patch) - true + @emitMarkerLayersDidUpdateEvent => + @applyChange(change) for change in pop.patch.getChanges() + @restoreFromMarkerSnapshot(pop.snapshot) + @emitMarkerChangeEvents(pop.snapshot) + @emitDidChangeTextEvent(pop.patch) + true else false @@ -1000,27 +1003,28 @@ class TextBuffer fn = groupingInterval groupingInterval = 0 - checkpointBefore = @history.createCheckpoint(@createMarkerSnapshot(), true) + @emitMarkerLayersDidUpdateEvent => + checkpointBefore = @history.createCheckpoint(@createMarkerSnapshot(), true) - try - @transactCallDepth++ - result = fn() - catch exception - @revertToCheckpoint(checkpointBefore, true) - throw exception unless exception instanceof TransactionAbortedError - return - finally - @transactCallDepth-- - - endMarkerSnapshot = @createMarkerSnapshot() - compactedChanges = @history.groupChangesSinceCheckpoint(checkpointBefore, endMarkerSnapshot, true) - global.atom?.assert compactedChanges, "groupChangesSinceCheckpoint() returned false.", (error) => - error.metadata = {history: @history.toString()} - @history.applyGroupingInterval(groupingInterval) - @history.enforceUndoStackSizeLimit() - @emitMarkerChangeEvents(endMarkerSnapshot) - @emitDidChangeTextEvent(compactedChanges) if compactedChanges - result + try + @transactCallDepth++ + result = fn() + catch exception + @revertToCheckpoint(checkpointBefore, true) + throw exception unless exception instanceof TransactionAbortedError + return + finally + @transactCallDepth-- + + endMarkerSnapshot = @createMarkerSnapshot() + compactedChanges = @history.groupChangesSinceCheckpoint(checkpointBefore, endMarkerSnapshot, true) + global.atom?.assert compactedChanges, "groupChangesSinceCheckpoint() returned false.", (error) => + error.metadata = {history: @history.toString()} + @history.applyGroupingInterval(groupingInterval) + @history.enforceUndoStackSizeLimit() + @emitMarkerChangeEvents(endMarkerSnapshot) + @emitDidChangeTextEvent(compactedChanges) if compactedChanges + result abortTransaction: -> throw new TransactionAbortedError("Transaction aborted.") @@ -1048,11 +1052,12 @@ class TextBuffer # Returns a {Boolean} indicating whether the operation succeeded. revertToCheckpoint: (checkpoint) -> if truncated = @history.truncateUndoStack(checkpoint) - @applyChange(change) for change in truncated.patch.getChanges() - @restoreFromMarkerSnapshot(truncated.snapshot) - @emitter.emit 'did-update-markers' - @emitDidChangeTextEvent(truncated.patch) - true + @emitMarkerLayersDidUpdateEvent => + @applyChange(change) for change in truncated.patch.getChanges() + @restoreFromMarkerSnapshot(truncated.snapshot) + @emitter.emit 'did-update-markers' + @emitDidChangeTextEvent(truncated.patch) + true else false @@ -1544,6 +1549,19 @@ class TextBuffer for markerLayerId, markerLayer of @markerLayers markerLayer.emitChangeEvents(snapshot?[markerLayerId]) + emitMarkerLayersDidUpdateEvent: (fn) -> + wasDisabled = @markerLayerDidUpdateEventDisabled + @markerLayerDidUpdateEventDisabled = true + for id, markerLayer of @markerLayers + markerLayer.setDisableDidUpdateEvent(true) + result = fn() + @markerLayerDidUpdateEventDisabled = wasDisabled + for id, markerLayer of @markerLayers + markerLayer.setDisableDidUpdateEvent(false) + markerLayer.emitDidUpdateEvent() + markerLayer.setDisableDidUpdateEvent(wasDisabled) + result + emitDidChangeTextEvent: (patch) -> return if @transactCallDepth isnt 0 From 040adb1bacd13bcc36b62bed71b350f32ec2490d Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Mon, 25 Apr 2016 10:27:45 +0200 Subject: [PATCH 04/11] Introduce a different Set for `invalidated` markers --- spec/display-marker-layer-spec.coffee | 31 ++++++---- spec/marker-layer-spec.coffee | 82 ++++++++++++--------------- src/marker-layer.coffee | 8 ++- 3 files changed, 60 insertions(+), 61 deletions(-) diff --git a/spec/display-marker-layer-spec.coffee b/spec/display-marker-layer-spec.coffee index 25afea9739..0c18759a56 100644 --- a/spec/display-marker-layer-spec.coffee +++ b/spec/display-marker-layer-spec.coffee @@ -137,25 +137,34 @@ describe "DisplayMarkerLayer", -> markerLayer = displayLayer.addMarkerLayer() events = [] - markerLayer.onDidUpdate (event) -> events.push(event) + markerLayer.onDidUpdate (event) -> events.push({ + created: Array.from(event.created), + updated: Array.from(event.updated), + invalidated: Array.from(event.invalidated), + destroyed: Array.from(event.destroyed) + }) events = [] - marker = markerLayer.markScreenRange([[0, 4], [1, 4]]) - expect(events.length).toBe(1) - expect(Array.from(events[0].created)).toEqual [marker.id] - expect(Array.from(events[0].updated)).toEqual [] - expect(Array.from(events[0].destroyed)).toEqual [] + marker = markerLayer.markScreenRange([[0, 4], [1, 4]], {invalidate: 'inside'}) + expect(events).toEqual([ + {created: [marker.id], updated: [], invalidated: [], destroyed: []} + ]) events = [] marker.setScreenRange([[0, 5], [1, 0]]) - expect(events.length).toBe(1) - expect(Array.from(events[0].created)).toEqual [] - expect(Array.from(events[0].updated)).toEqual [marker.id] - expect(Array.from(events[0].destroyed)).toEqual [] + expect(events).toEqual([ + {created: [], updated: [marker.id], invalidated: [], destroyed: []} + ]) + + events = [] + buffer.insert([0, 8], 'foo') + expect(events).toEqual([ + {created: [], updated: [], invalidated: [marker.id], destroyed: []} + ]) events = [] buffer.insert([0, 0], '\t') - expect(events.length).toBe(0) + expect(events).toEqual([]) events = [] marker.destroy() diff --git a/spec/marker-layer-spec.coffee b/spec/marker-layer-spec.coffee index f79fe85568..37311b61f0 100644 --- a/spec/marker-layer-spec.coffee +++ b/spec/marker-layer-spec.coffee @@ -176,26 +176,28 @@ describe "MarkerLayer", -> expect(layer2.findMarkers(containsPoint: [0, 4])).toEqual [layer2Marker] describe "::onDidUpdate", -> - it "notifies observers synchronously or at the end of a transaction when markers are created, updated, or destroyed", -> + it "notifies observers synchronously or at the end of a transaction when markers are created, updated, invalidated, or destroyed", -> layer = buffer.addMarkerLayer({maintainHistory: true}) events = [] [marker1, marker2, marker3] = [] - layer.onDidUpdate (event) -> events.push(event) + layer.onDidUpdate (event) -> events.push({ + created: Array.from(event.created), + updated: Array.from(event.updated), + invalidated: Array.from(event.invalidated), + destroyed: Array.from(event.destroyed) + }) buffer.transact -> - marker1 = layer.markRange([[0, 2], [0, 4]]) - marker2 = layer.markRange([[0, 6], [0, 8]]) + marker1 = layer.markRange([[0, 2], [0, 4]], {invalidate: 'touch'}) + marker2 = layer.markRange([[0, 6], [0, 8]], {invalidate: 'touch'}) expect(events.length).toBe(0) marker3 = layer.markRange([[4, 0], [4, 5]]) - expect(events.length).toBe(2) - expect(Array.from(events[0].created)).toEqual [marker1.id, marker2.id] - expect(Array.from(events[0].updated)).toEqual [] - expect(Array.from(events[0].destroyed)).toEqual [] - expect(Array.from(events[1].created)).toEqual [marker3.id] - expect(Array.from(events[1].updated)).toEqual [] - expect(Array.from(events[1].destroyed)).toEqual [] + expect(events).toEqual([ + {created: [marker1.id, marker2.id], updated: [], invalidated: [], destroyed: []}, + {created: [marker3.id], updated: [], invalidated: [], destroyed: []} + ]) events = [] buffer.transact -> @@ -203,10 +205,9 @@ describe "MarkerLayer", -> marker2.setRange([[3, 10], [4, 5]]) marker3.destroy() - expect(events.length).toBe(1) - expect(Array.from(events[0].created)).toEqual [] - expect(Array.from(events[0].updated)).toEqual [marker1.id, marker2.id] - expect(Array.from(events[0].destroyed)).toEqual [marker3.id] + expect(events).toEqual([ + {created: [], updated: [marker1.id, marker2.id], invalidated: [], destroyed: [marker3.id]} + ]) events = [] buffer.transact -> @@ -214,25 +215,19 @@ describe "MarkerLayer", -> buffer.insert([2, 0], "yyy") buffer.insert([1, 5], 'zzz') - expect(events.length).toBe(2) - expect(Array.from(events[0].created)).toEqual [] - expect(Array.from(events[0].updated)).toEqual [marker1.id] - expect(Array.from(events[0].destroyed)).toEqual [] - expect(Array.from(events[1].created)).toEqual [] - expect(Array.from(events[1].updated)).toEqual [marker1.id] - expect(Array.from(events[1].destroyed)).toEqual [] + expect(events).toEqual([ + {created: [], updated: [], invalidated: [marker1.id], destroyed: []}, + {created: [], updated: [], invalidated: [marker1.id], destroyed: []} + ]) events = [] buffer.undo() buffer.undo() - expect(events.length).toBe(2) - expect(Array.from(events[0].created)).toEqual [] - expect(Array.from(events[0].updated)).toEqual [marker1.id] - expect(Array.from(events[0].destroyed)).toEqual [] - expect(Array.from(events[1].created)).toEqual [] - expect(Array.from(events[1].updated)).toEqual [marker1.id] - expect(Array.from(events[1].destroyed)).toEqual [] + expect(events).toEqual([ + {created: [], updated: [], invalidated: [marker1.id], destroyed: []}, + {created: [], updated: [], invalidated: [marker1.id], destroyed: []} + ]) events = [] buffer.transact -> @@ -244,13 +239,10 @@ describe "MarkerLayer", -> buffer.insert([4, 0], 'eee') buffer.insert([4, 3], 'fff') - expect(events.length).toBe(2) - expect(Array.from(events[0].created)).toEqual [] - expect(Array.from(events[0].updated)).toEqual [marker1.id, marker2.id] - expect(Array.from(events[0].destroyed)).toEqual [] - expect(Array.from(events[1].created)).toEqual [] - expect(Array.from(events[1].updated)).toEqual [marker2.id] - expect(Array.from(events[1].destroyed)).toEqual [] + expect(events).toEqual([ + {created: [], updated: [], invalidated: [marker1.id, marker2.id], destroyed: []}, + {created: [], updated: [], invalidated: [marker2.id], destroyed: []} + ]) events = [] buffer.transact -> @@ -259,23 +251,19 @@ describe "MarkerLayer", -> marker1.clearTail() marker2.clearTail() - expect(events.length).toBe(2) - expect(Array.from(events[0].created)).toEqual [] - expect(Array.from(events[0].updated)).toEqual [marker2.id] - expect(Array.from(events[0].destroyed)).toEqual [] - expect(Array.from(events[1].created)).toEqual [] - expect(Array.from(events[1].updated)).toEqual [marker1.id, marker2.id] - expect(Array.from(events[1].destroyed)).toEqual [] + expect(events).toEqual([ + {created: [], updated: [], invalidated: [marker2.id], destroyed: []}, + {created: [], updated: [marker1.id, marker2.id], invalidated: [], destroyed: []} + ]) events = [] buffer.transact -> marker1.destroy() marker2.destroy() - expect(events.length).toBe(1) - expect(Array.from(events[0].created)).toEqual [] - expect(Array.from(events[0].updated)).toEqual [] - expect(Array.from(events[0].destroyed)).toEqual [marker1.id, marker2.id] + expect(events).toEqual([ + {created: [], updated: [], invalidated: [], destroyed: [marker1.id, marker2.id]} + ]) describe "::copy", -> it "creates a new marker layer with markers in the same states", -> diff --git a/src/marker-layer.coffee b/src/marker-layer.coffee index 2b9df732bf..df72ae6868 100644 --- a/src/marker-layer.coffee +++ b/src/marker-layer.coffee @@ -42,6 +42,7 @@ class MarkerLayer @createdMarkers = new Set @destroyedMarkers = new Set @updatedMarkers = new Set + @invalidatedMarkers = new Set @setDisableDidUpdateEvent(false) @destroyed = false @emitCreateMarkerEvents = false @@ -255,11 +256,11 @@ class MarkerLayer invalidated.touch.forEach (id) => marker = @markersById[id] if invalidated[marker.getInvalidationStrategy()]?.has(id) + @invalidatedMarkers.add(id) if @destroyInvalidatedMarkers marker.destroy() else marker.valid = false - @updatedMarkers.add(id) @emitDidUpdateEvent() restoreFromSnapshot: (snapshots) -> @@ -379,10 +380,11 @@ class MarkerLayer emitDidUpdateEvent: -> return if @didUpdateEventDisabled - if @createdMarkers.size > 0 or @destroyedMarkers.size > 0 or @updatedMarkers.size > 0 - event = {created: @createdMarkers, destroyed: @destroyedMarkers, updated: @updatedMarkers} + if @createdMarkers.size > 0 or @destroyedMarkers.size > 0 or @invalidatedMarkers.size > 0 or @updatedMarkers.size > 0 + event = {created: @createdMarkers, destroyed: @destroyedMarkers, invalidated: @invalidatedMarkers, updated: @updatedMarkers} @createdMarkers = new Set @destroyedMarkers = new Set + @invalidatedMarkers = new Set @updatedMarkers = new Set @emitter.emit 'did-update', event From a81f3f57aa89b9c2873384aa86868b67d6116be0 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Mon, 25 Apr 2016 11:56:54 +0200 Subject: [PATCH 05/11] Always include markers in the invalidated set --- spec/marker-layer-spec.coffee | 4 ++-- src/marker-layer.coffee | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/marker-layer-spec.coffee b/spec/marker-layer-spec.coffee index 37311b61f0..c905a81d37 100644 --- a/spec/marker-layer-spec.coffee +++ b/spec/marker-layer-spec.coffee @@ -188,8 +188,8 @@ describe "MarkerLayer", -> }) buffer.transact -> - marker1 = layer.markRange([[0, 2], [0, 4]], {invalidate: 'touch'}) - marker2 = layer.markRange([[0, 6], [0, 8]], {invalidate: 'touch'}) + marker1 = layer.markRange([[0, 2], [0, 4]]) + marker2 = layer.markRange([[0, 6], [0, 8]]) expect(events.length).toBe(0) marker3 = layer.markRange([[4, 0], [4, 5]]) diff --git a/src/marker-layer.coffee b/src/marker-layer.coffee index df72ae6868..add089f9a0 100644 --- a/src/marker-layer.coffee +++ b/src/marker-layer.coffee @@ -255,8 +255,8 @@ class MarkerLayer invalidated = @index.splice(start, oldExtent, newExtent) invalidated.touch.forEach (id) => marker = @markersById[id] + @invalidatedMarkers.add(id) if invalidated[marker.getInvalidationStrategy()]?.has(id) - @invalidatedMarkers.add(id) if @destroyInvalidatedMarkers marker.destroy() else From 6a9562f47eff9e10cf70e93e37fd569525745cd1 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Mon, 25 Apr 2016 12:48:42 +0200 Subject: [PATCH 06/11] Replace `emitMarkerLayersDidUpdateEvent` with explicit steps in callers --- src/text-buffer.coffee | 98 +++++++++++++++++++++++------------------- 1 file changed, 53 insertions(+), 45 deletions(-) diff --git a/src/text-buffer.coffee b/src/text-buffer.coffee index 3b96501858..45e32669f8 100644 --- a/src/text-buffer.coffee +++ b/src/text-buffer.coffee @@ -964,24 +964,28 @@ class TextBuffer # Public: Undo the last operation. If a transaction is in progress, aborts it. undo: -> if pop = @history.popUndoStack() - @emitMarkerLayersDidUpdateEvent => - @applyChange(change) for change in pop.patch.getChanges() - @restoreFromMarkerSnapshot(pop.snapshot) - @emitMarkerChangeEvents(pop.snapshot) - @emitDidChangeTextEvent(pop.patch) - true + markerLayerUpdateEventWasDisabled = @disableMarkerLayersDidUpdateEvent() + @applyChange(change) for change in pop.patch.getChanges() + @restoreFromMarkerSnapshot(pop.snapshot) + @enableMarkersLayersDidUpdateEvent() + @emitMarkerChangeEvents(pop.snapshot) + @disableMarkerLayersDidUpdateEvent() if markerLayerUpdateEventWasDisabled + @emitDidChangeTextEvent(pop.patch) + true else false # Public: Redo the last operation redo: -> if pop = @history.popRedoStack() - @emitMarkerLayersDidUpdateEvent => - @applyChange(change) for change in pop.patch.getChanges() - @restoreFromMarkerSnapshot(pop.snapshot) - @emitMarkerChangeEvents(pop.snapshot) - @emitDidChangeTextEvent(pop.patch) - true + markerLayerUpdateEventWasDisabled = @disableMarkerLayersDidUpdateEvent() + @applyChange(change) for change in pop.patch.getChanges() + @restoreFromMarkerSnapshot(pop.snapshot) + @enableMarkersLayersDidUpdateEvent() + @emitMarkerChangeEvents(pop.snapshot) + @disableMarkerLayersDidUpdateEvent() if markerLayerUpdateEventWasDisabled + @emitDidChangeTextEvent(pop.patch) + true else false @@ -1003,28 +1007,30 @@ class TextBuffer fn = groupingInterval groupingInterval = 0 - @emitMarkerLayersDidUpdateEvent => - checkpointBefore = @history.createCheckpoint(@createMarkerSnapshot(), true) + markerLayerUpdateEventWasDisabled = @disableMarkerLayersDidUpdateEvent() + checkpointBefore = @history.createCheckpoint(@createMarkerSnapshot(), true) - try - @transactCallDepth++ - result = fn() - catch exception - @revertToCheckpoint(checkpointBefore, true) - throw exception unless exception instanceof TransactionAbortedError - return - finally - @transactCallDepth-- - - endMarkerSnapshot = @createMarkerSnapshot() - compactedChanges = @history.groupChangesSinceCheckpoint(checkpointBefore, endMarkerSnapshot, true) - global.atom?.assert compactedChanges, "groupChangesSinceCheckpoint() returned false.", (error) => - error.metadata = {history: @history.toString()} - @history.applyGroupingInterval(groupingInterval) - @history.enforceUndoStackSizeLimit() - @emitMarkerChangeEvents(endMarkerSnapshot) - @emitDidChangeTextEvent(compactedChanges) if compactedChanges - result + try + @transactCallDepth++ + result = fn() + catch exception + @revertToCheckpoint(checkpointBefore, true) + throw exception unless exception instanceof TransactionAbortedError + return + finally + @transactCallDepth-- + + endMarkerSnapshot = @createMarkerSnapshot() + compactedChanges = @history.groupChangesSinceCheckpoint(checkpointBefore, endMarkerSnapshot, true) + global.atom?.assert compactedChanges, "groupChangesSinceCheckpoint() returned false.", (error) => + error.metadata = {history: @history.toString()} + @history.applyGroupingInterval(groupingInterval) + @history.enforceUndoStackSizeLimit() + @enableMarkersLayersDidUpdateEvent() + @emitMarkerChangeEvents(endMarkerSnapshot) + @disableMarkerLayersDidUpdateEvent() if markerLayerUpdateEventWasDisabled + @emitDidChangeTextEvent(compactedChanges) if compactedChanges + result abortTransaction: -> throw new TransactionAbortedError("Transaction aborted.") @@ -1052,12 +1058,14 @@ class TextBuffer # Returns a {Boolean} indicating whether the operation succeeded. revertToCheckpoint: (checkpoint) -> if truncated = @history.truncateUndoStack(checkpoint) - @emitMarkerLayersDidUpdateEvent => - @applyChange(change) for change in truncated.patch.getChanges() - @restoreFromMarkerSnapshot(truncated.snapshot) - @emitter.emit 'did-update-markers' - @emitDidChangeTextEvent(truncated.patch) - true + markerLayerUpdateEventWasDisabled = @disableMarkerLayersDidUpdateEvent() + @applyChange(change) for change in truncated.patch.getChanges() + @restoreFromMarkerSnapshot(truncated.snapshot) + @enableMarkersLayersDidUpdateEvent() + @emitMarkerChangeEvents(truncated.snapshot) + @disableMarkerLayersDidUpdateEvent() if markerLayerUpdateEventWasDisabled + @emitDidChangeTextEvent(truncated.patch) + true else false @@ -1547,20 +1555,20 @@ class TextBuffer emitMarkerChangeEvents: (snapshot) -> for markerLayerId, markerLayer of @markerLayers + markerLayer.emitDidUpdateEvent() markerLayer.emitChangeEvents(snapshot?[markerLayerId]) - emitMarkerLayersDidUpdateEvent: (fn) -> + disableMarkerLayersDidUpdateEvent: -> wasDisabled = @markerLayerDidUpdateEventDisabled @markerLayerDidUpdateEventDisabled = true for id, markerLayer of @markerLayers markerLayer.setDisableDidUpdateEvent(true) - result = fn() - @markerLayerDidUpdateEventDisabled = wasDisabled + wasDisabled + + enableMarkersLayersDidUpdateEvent: -> + @markerLayerDidUpdateEventDisabled = false for id, markerLayer of @markerLayers markerLayer.setDisableDidUpdateEvent(false) - markerLayer.emitDidUpdateEvent() - markerLayer.setDisableDidUpdateEvent(wasDisabled) - result emitDidChangeTextEvent: (patch) -> return if @transactCallDepth isnt 0 From 011e572d5ee13fa0bc69b1c91a06f9569816acef Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Mon, 25 Apr 2016 13:15:59 +0200 Subject: [PATCH 07/11] Update documentation for new `MarkerLayer.onDidUpdate` behavior --- spec/display-marker-layer-spec.coffee | 15 ++++++------ spec/marker-layer-spec.coffee | 26 ++++++++++---------- src/display-marker-layer.coffee | 20 +++++++++------- src/marker-layer.coffee | 34 ++++++++++++++------------- 4 files changed, 49 insertions(+), 46 deletions(-) diff --git a/spec/display-marker-layer-spec.coffee b/spec/display-marker-layer-spec.coffee index 0c18759a56..0b741daebe 100644 --- a/spec/display-marker-layer-spec.coffee +++ b/spec/display-marker-layer-spec.coffee @@ -140,26 +140,26 @@ describe "DisplayMarkerLayer", -> markerLayer.onDidUpdate (event) -> events.push({ created: Array.from(event.created), updated: Array.from(event.updated), - invalidated: Array.from(event.invalidated), + touched: Array.from(event.touched), destroyed: Array.from(event.destroyed) }) events = [] marker = markerLayer.markScreenRange([[0, 4], [1, 4]], {invalidate: 'inside'}) expect(events).toEqual([ - {created: [marker.id], updated: [], invalidated: [], destroyed: []} + {created: [marker.id], updated: [], touched: [], destroyed: []} ]) events = [] marker.setScreenRange([[0, 5], [1, 0]]) expect(events).toEqual([ - {created: [], updated: [marker.id], invalidated: [], destroyed: []} + {created: [], updated: [marker.id], touched: [], destroyed: []} ]) events = [] buffer.insert([0, 8], 'foo') expect(events).toEqual([ - {created: [], updated: [], invalidated: [marker.id], destroyed: []} + {created: [], updated: [], touched: [marker.id], destroyed: []} ]) events = [] @@ -168,10 +168,9 @@ describe "DisplayMarkerLayer", -> events = [] marker.destroy() - expect(events.length).toBe(1) - expect(Array.from(events[0].created)).toEqual [] - expect(Array.from(events[0].updated)).toEqual [] - expect(Array.from(events[0].destroyed)).toEqual [marker.id] + expect(events).toEqual([ + {created: [], updated: [], touched: [], destroyed: [marker.id]} + ]) it "allows markers to be copied", -> buffer = new TextBuffer(text: '\ta\tbc\tdef\tg\n\th') diff --git a/spec/marker-layer-spec.coffee b/spec/marker-layer-spec.coffee index c905a81d37..bc3cad0a76 100644 --- a/spec/marker-layer-spec.coffee +++ b/spec/marker-layer-spec.coffee @@ -183,7 +183,7 @@ describe "MarkerLayer", -> layer.onDidUpdate (event) -> events.push({ created: Array.from(event.created), updated: Array.from(event.updated), - invalidated: Array.from(event.invalidated), + touched: Array.from(event.touched), destroyed: Array.from(event.destroyed) }) @@ -195,8 +195,8 @@ describe "MarkerLayer", -> marker3 = layer.markRange([[4, 0], [4, 5]]) expect(events).toEqual([ - {created: [marker1.id, marker2.id], updated: [], invalidated: [], destroyed: []}, - {created: [marker3.id], updated: [], invalidated: [], destroyed: []} + {created: [marker1.id, marker2.id], updated: [], touched: [], destroyed: []}, + {created: [marker3.id], updated: [], touched: [], destroyed: []} ]) events = [] @@ -206,7 +206,7 @@ describe "MarkerLayer", -> marker3.destroy() expect(events).toEqual([ - {created: [], updated: [marker1.id, marker2.id], invalidated: [], destroyed: [marker3.id]} + {created: [], updated: [marker1.id, marker2.id], touched: [], destroyed: [marker3.id]} ]) events = [] @@ -216,8 +216,8 @@ describe "MarkerLayer", -> buffer.insert([1, 5], 'zzz') expect(events).toEqual([ - {created: [], updated: [], invalidated: [marker1.id], destroyed: []}, - {created: [], updated: [], invalidated: [marker1.id], destroyed: []} + {created: [], updated: [], touched: [marker1.id], destroyed: []}, + {created: [], updated: [], touched: [marker1.id], destroyed: []} ]) events = [] @@ -225,8 +225,8 @@ describe "MarkerLayer", -> buffer.undo() expect(events).toEqual([ - {created: [], updated: [], invalidated: [marker1.id], destroyed: []}, - {created: [], updated: [], invalidated: [marker1.id], destroyed: []} + {created: [], updated: [], touched: [marker1.id], destroyed: []}, + {created: [], updated: [], touched: [marker1.id], destroyed: []} ]) events = [] @@ -240,8 +240,8 @@ describe "MarkerLayer", -> buffer.insert([4, 3], 'fff') expect(events).toEqual([ - {created: [], updated: [], invalidated: [marker1.id, marker2.id], destroyed: []}, - {created: [], updated: [], invalidated: [marker2.id], destroyed: []} + {created: [], updated: [], touched: [marker1.id, marker2.id], destroyed: []}, + {created: [], updated: [], touched: [marker2.id], destroyed: []} ]) events = [] @@ -252,8 +252,8 @@ describe "MarkerLayer", -> marker2.clearTail() expect(events).toEqual([ - {created: [], updated: [], invalidated: [marker2.id], destroyed: []}, - {created: [], updated: [marker1.id, marker2.id], invalidated: [], destroyed: []} + {created: [], updated: [], touched: [marker2.id], destroyed: []}, + {created: [], updated: [marker1.id, marker2.id], touched: [], destroyed: []} ]) events = [] @@ -262,7 +262,7 @@ describe "MarkerLayer", -> marker2.destroy() expect(events).toEqual([ - {created: [], updated: [], invalidated: [], destroyed: [marker1.id, marker2.id]} + {created: [], updated: [], touched: [], destroyed: [marker1.id, marker2.id]} ]) describe "::copy", -> diff --git a/src/display-marker-layer.coffee b/src/display-marker-layer.coffee index 2098239449..efcdceeda3 100644 --- a/src/display-marker-layer.coffee +++ b/src/display-marker-layer.coffee @@ -34,19 +34,21 @@ class DisplayMarkerLayer onDidDestroy: (callback) -> @emitter.on('did-destroy', callback) - # Public: Subscribe to be notified asynchronously whenever markers are - # created, updated, or destroyed on this layer. *Prefer this method for - # optimal performance when interacting with layers that could contain large - # numbers of markers.* + # Public: Subscribe to be notified whenever markers are created, updated, + # touched (moved because of a textual change), or destroyed on this layer. + # *Prefer this method for optimal performance when interacting with layers + # that could contain large numbers of markers.* # # * `callback` A {Function} that will be called with no arguments when changes # occur on this layer. # - # Subscribers are notified once, asynchronously when any number of changes - # occur in a given tick of the event loop. You should re-query the layer - # to determine the state of markers in which you're interested in. It may - # be counter-intuitive, but this is much more efficient than subscribing to - # events on individual markers, which are expensive to deliver. + # Subscribers are notified once when any number of changes occur in this + # {MarkerLayer}. The notification gets scheduled either at the end of a + # transaction, or synchronously when a marker changes and no transaction is + # present. You should re-query the layer to determine the state of markers in + # which you're interested in: it may be counter-intuitive, but this is much + # more efficient than subscribing to events on individual markers, which are + # expensive to deliver. # # Returns a {Disposable}. onDidUpdate: (callback) -> diff --git a/src/marker-layer.coffee b/src/marker-layer.coffee index add089f9a0..e4b42bb2e0 100644 --- a/src/marker-layer.coffee +++ b/src/marker-layer.coffee @@ -33,7 +33,7 @@ class MarkerLayer constructor: (@delegate, @id, options) -> @maintainHistory = options?.maintainHistory ? false - @destroyInvalidatedMarkers = options?.destroyInvalidatedMarkers ? false + @destroytouchedMarkers = options?.destroytouchedMarkers ? false @persistent = options?.persistent ? false @emitter = new Emitter @index = new MarkerIndex @@ -42,7 +42,7 @@ class MarkerLayer @createdMarkers = new Set @destroyedMarkers = new Set @updatedMarkers = new Set - @invalidatedMarkers = new Set + @touchedMarkers = new Set @setDisableDidUpdateEvent(false) @destroyed = false @emitCreateMarkerEvents = false @@ -208,19 +208,21 @@ class MarkerLayer Section: Event subscription ### - # Public: Subscribe to be notified asynchronously whenever markers are - # created, updated, or destroyed on this layer. *Prefer this method for - # optimal performance when interacting with layers that could contain large - # numbers of markers.* + # Public: Subscribe to be notified whenever markers are created, updated, + # touched (moved because of a textual change), or destroyed on this layer. + # *Prefer this method for optimal performance when interacting with layers + # that could contain large numbers of markers.* # # * `callback` A {Function} that will be called with no arguments when changes # occur on this layer. # - # Subscribers are notified once, asynchronously when any number of changes - # occur in a given tick of the event loop. You should re-query the layer - # to determine the state of markers in which you're interested in. It may - # be counter-intuitive, but this is much more efficient than subscribing to - # events on individual markers, which are expensive to deliver. + # Subscribers are notified once when any number of changes occur in this + # {MarkerLayer}. The notification gets scheduled either at the end of a + # transaction, or synchronously when a marker changes and no transaction is + # present. You should re-query the layer to determine the state of markers in + # which you're interested in: it may be counter-intuitive, but this is much + # more efficient than subscribing to events on individual markers, which are + # expensive to deliver. # # Returns a {Disposable}. onDidUpdate: (callback) -> @@ -255,9 +257,9 @@ class MarkerLayer invalidated = @index.splice(start, oldExtent, newExtent) invalidated.touch.forEach (id) => marker = @markersById[id] - @invalidatedMarkers.add(id) + @touchedMarkers.add(id) if invalidated[marker.getInvalidationStrategy()]?.has(id) - if @destroyInvalidatedMarkers + if @destroytouchedMarkers marker.destroy() else marker.valid = false @@ -380,11 +382,11 @@ class MarkerLayer emitDidUpdateEvent: -> return if @didUpdateEventDisabled - if @createdMarkers.size > 0 or @destroyedMarkers.size > 0 or @invalidatedMarkers.size > 0 or @updatedMarkers.size > 0 - event = {created: @createdMarkers, destroyed: @destroyedMarkers, invalidated: @invalidatedMarkers, updated: @updatedMarkers} + if @createdMarkers.size > 0 or @destroyedMarkers.size > 0 or @touchedMarkers.size > 0 or @updatedMarkers.size > 0 + event = {created: @createdMarkers, destroyed: @destroyedMarkers, touched: @touchedMarkers, updated: @updatedMarkers} @createdMarkers = new Set @destroyedMarkers = new Set - @invalidatedMarkers = new Set + @touchedMarkers = new Set @updatedMarkers = new Set @emitter.emit 'did-update', event From d8ec2f8d1d8d2f9c408a454b7e3acdadd2cbebe5 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Mon, 25 Apr 2016 13:39:40 +0200 Subject: [PATCH 08/11] :art: --- spec/marker-layer-spec.coffee | 2 +- src/marker-layer.coffee | 4 ++-- src/marker.coffee | 10 +++++----- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/spec/marker-layer-spec.coffee b/spec/marker-layer-spec.coffee index bc3cad0a76..ce42e02755 100644 --- a/spec/marker-layer-spec.coffee +++ b/spec/marker-layer-spec.coffee @@ -176,7 +176,7 @@ describe "MarkerLayer", -> expect(layer2.findMarkers(containsPoint: [0, 4])).toEqual [layer2Marker] describe "::onDidUpdate", -> - it "notifies observers synchronously or at the end of a transaction when markers are created, updated, invalidated, or destroyed", -> + it "notifies observers synchronously or at the end of a transaction when markers are created, updated directly, updated indirectly, or destroyed", -> layer = buffer.addMarkerLayer({maintainHistory: true}) events = [] [marker1, marker2, marker3] = [] diff --git a/src/marker-layer.coffee b/src/marker-layer.coffee index e4b42bb2e0..cafc10edd8 100644 --- a/src/marker-layer.coffee +++ b/src/marker-layer.coffee @@ -33,7 +33,7 @@ class MarkerLayer constructor: (@delegate, @id, options) -> @maintainHistory = options?.maintainHistory ? false - @destroytouchedMarkers = options?.destroytouchedMarkers ? false + @destroyInvalidatedMarkers = options?.destroyInvalidatedMarkers ? false @persistent = options?.persistent ? false @emitter = new Emitter @index = new MarkerIndex @@ -259,7 +259,7 @@ class MarkerLayer marker = @markersById[id] @touchedMarkers.add(id) if invalidated[marker.getInvalidationStrategy()]?.has(id) - if @destroytouchedMarkers + if @destroyInvalidatedMarkers marker.destroy() else marker.valid = false diff --git a/src/marker.coffee b/src/marker.coffee index 00ba05e425..a6f05b78c3 100644 --- a/src/marker.coffee +++ b/src/marker.coffee @@ -102,12 +102,12 @@ class Marker # # Returns a {Disposable} on which `.dispose()` can be called to unsubscribe. onDidChange: (callback) -> - Grim.deprecate(""" - Subscribing to marker change events is deprecated. Please, consider using - MarkerLayer.prototype.onDidUpdate instead. - """) - unless @hasChangeObservers + Grim.deprecate(""" + Subscribing to marker change events is deprecated. Please, consider using + `MarkerLayer.prototype.onDidUpdate` instead. + """) + @previousEventState = @getSnapshot(@getRange()) @hasChangeObservers = true @layer.markersIdsWithChangeSubscriptions.add(@id) From 63d47f3ac2037956ccf1a2fdb0da11f94eb7c710 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 4 May 2016 15:22:05 +0200 Subject: [PATCH 09/11] Make MarkerLayer.prototype.getMarkerCount() faster --- src/marker-layer.coffee | 55 +++++++++++++++++++++++------------------ 1 file changed, 31 insertions(+), 24 deletions(-) diff --git a/src/marker-layer.coffee b/src/marker-layer.coffee index cafc10edd8..a25c45234a 100644 --- a/src/marker-layer.coffee +++ b/src/marker-layer.coffee @@ -37,7 +37,8 @@ class MarkerLayer @persistent = options?.persistent ? false @emitter = new Emitter @index = new MarkerIndex - @markersById = {} + @markersById = new Map + @markers = [] @markersIdsWithChangeSubscriptions = new Set @createdMarkers = new Set @destroyedMarkers = new Set @@ -51,7 +52,7 @@ class MarkerLayer # locations. copy: -> copy = @delegate.addMarkerLayer({@maintainHistory}) - for markerId, marker of @markersById + @markersById.forEach (marker, id) => snapshot = marker.getSnapshot(null) copy.createMarker(marker.getRange(), marker.getSnapshot()) copy @@ -78,19 +79,21 @@ class MarkerLayer # # Returns a {Marker}. getMarker: (id) -> - @markersById[id] + @markersById.get(id) # Public: Get all existing markers on the marker layer. # # Returns an {Array} of {Marker}s. getMarkers: -> - marker for id, marker of @markersById + markers = [] + @markersById.forEach (marker) -> markers.push(marker) + markers # Public: Get the number of markers in the marker layer. # # Returns a {Number}. getMarkerCount: -> - Object.keys(@markersById).length + @markersById.size # Public: Find markers in the layer conforming to the given parameters. # @@ -129,13 +132,17 @@ class MarkerLayer continue delete params[key] - markerIds ?= new Set(Object.keys(@markersById)) - result = [] - markerIds.forEach (markerId) => - marker = @markersById[markerId] - return unless marker.matchesParams(params) - result.push(marker) + if markerIds? + markerIds.forEach (markerId) => + marker = @markersById.get(markerId) + if marker.matchesParams(params) + result.push(marker) + else + @markersById.forEach (marker) => + if marker.matchesParams(params) + result.push(marker) + result.sort (a, b) -> a.compare(b) ### @@ -256,7 +263,7 @@ class MarkerLayer splice: (start, oldExtent, newExtent) -> invalidated = @index.splice(start, oldExtent, newExtent) invalidated.touch.forEach (id) => - marker = @markersById[id] + marker = @markersById.get(id) @touchedMarkers.add(id) if invalidated[marker.getInvalidationStrategy()]?.has(id) if @destroyInvalidatedMarkers @@ -269,17 +276,17 @@ class MarkerLayer return unless snapshots? snapshotIds = Object.keys(snapshots) - existingMarkerIds = Object.keys(@markersById) + existingMarkerIds = Array.from(@markersById.keys()) for id in snapshotIds snapshot = snapshots[id] - if marker = @markersById[id] + if marker = @markersById.get(parseInt(id)) marker.update(marker.getRange(), snapshot, true) else newMarker = @createMarker(snapshot.range, snapshot) for id in existingMarkerIds - if (marker = @markersById[id]) and (not snapshots[id]?) + if (marker = @markersById.get(parseInt(id))) and (not snapshots[id]?) marker.destroy() @delegate.markersUpdated(this) @@ -287,22 +294,20 @@ class MarkerLayer createSnapshot: -> result = {} ranges = @index.dump() - for id in Object.keys(@markersById) - marker = @markersById[id] + @markersById.forEach (marker, id) => result[id] = marker.getSnapshot(Range.fromObject(ranges[id]), false) result emitChangeEvents: (snapshot) -> @markersIdsWithChangeSubscriptions.forEach (id) => - if marker = @markersById[id] # event handlers could destroy markers + if marker = @markersById.get(id) # event handlers could destroy markers marker.emitChangeEvent(snapshot?[id]?.range, true, false) @delegate.markersUpdated(this) serialize: -> ranges = @index.dump() markersById = {} - for id in Object.keys(@markersById) - marker = @markersById[id] + @markersById.forEach (marker, id) => markersById[id] = marker.getSnapshot(Range.fromObject(ranges[id]), false) {@id, @maintainHistory, @persistent, markersById, version: SerializationVersion} @@ -314,7 +319,7 @@ class MarkerLayer for id, markerState of state.markersById range = Range.fromObject(markerState.range) delete markerState.range - @addMarker(id, range, markerState) + @addMarker(parseInt(id), range, markerState) return ### @@ -327,8 +332,8 @@ class MarkerLayer @delegate.markersUpdated(this) destroyMarker: (id) -> - if @markersById.hasOwnProperty(id) - delete @markersById[id] + if @markersById.has(id) + @markersById.delete(id) @markersIdsWithChangeSubscriptions.delete(id) @index.delete(id) @destroyedMarkers.add(id) @@ -375,7 +380,9 @@ class MarkerLayer Point.assertValid(range.start) Point.assertValid(range.end) @index.insert(id, range.start, range.end) - @markersById[id] = new Marker(id, this, range, params) + marker = new Marker(id, this, range, params) + @markersById.set(id, marker) + marker setDisableDidUpdateEvent: (@didUpdateEventDisabled) -> From f87f806092aacbee35c7384e0de472ddf6a40fe1 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 4 May 2016 15:23:11 +0200 Subject: [PATCH 10/11] Introduce MarkerLayer.prototype.getLastMarker() --- spec/marker-layer-spec.coffee | 20 ++++++++++++++++++++ src/display-marker-layer.coffee | 6 ++++++ src/marker-layer.coffee | 22 ++++++++++++++++++++++ 3 files changed, 48 insertions(+) diff --git a/spec/marker-layer-spec.coffee b/spec/marker-layer-spec.coffee index ce42e02755..6dc9427961 100644 --- a/spec/marker-layer-spec.coffee +++ b/spec/marker-layer-spec.coffee @@ -75,6 +75,26 @@ describe "MarkerLayer", -> expect(createEventCount).toBe 1 expect(updateEventCount).toBe 2 + describe "::getLastMarker()", -> + it "returns the last (in terms of time) non-destroyed marker added to the layer", -> + marker1 = layer1.markRange([[0, 0], [0, 3]]) + marker2 = layer1.markRange([[0, 2], [0, 6]]) + marker3 = layer1.markRange([[0, 8], [0, 10]]) + marker4 = layer1.markRange([[1, 0], [1, 10]]) + expect(layer1.getLastMarker()).toBe(marker4) + + marker4.destroy() + expect(layer1.getLastMarker()).toBe(marker3) + + marker1.destroy() + expect(layer1.getLastMarker()).toBe(marker3) + + marker3.destroy() + expect(layer1.getLastMarker()).toBe(marker2) + + marker2.destroy() + expect(layer1.getLastMarker()).toBeFalsy() + describe "when destroyInvalidatedMarkers is enabled for the layer", -> it "destroys markers when they are invalidated via a splice", -> layer3 = buffer.addMarkerLayer(destroyInvalidatedMarkers: true) diff --git a/src/display-marker-layer.coffee b/src/display-marker-layer.coffee index efcdceeda3..1597364cd0 100644 --- a/src/display-marker-layer.coffee +++ b/src/display-marker-layer.coffee @@ -236,6 +236,12 @@ class DisplayMarkerLayer getMarkers: -> @bufferMarkerLayer.getMarkers().map ({id}) => @getMarker(id) + # Public: Get the last (in terms of time) non-destroyed marker added to this layer. + # + # Returns a {DisplayMarker}. + getLastMarker: -> + @getMarker(@bufferMarkerLayer.getLastMarker()?.id) + # Public: Get the number of markers in the marker layer. # # Returns a {Number}. diff --git a/src/marker-layer.coffee b/src/marker-layer.coffee index a25c45234a..a98f7e6f5f 100644 --- a/src/marker-layer.coffee +++ b/src/marker-layer.coffee @@ -81,6 +81,12 @@ class MarkerLayer getMarker: (id) -> @markersById.get(id) + # Public: Get the last (in terms of time) non-destroyed marker added to this layer. + # + # Returns a {Marker}. + getLastMarker: -> + @getMarker(@markers[@markers.length - 1]) + # Public: Get all existing markers on the marker layer. # # Returns an {Array} of {Marker}s. @@ -334,6 +340,8 @@ class MarkerLayer destroyMarker: (id) -> if @markersById.has(id) @markersById.delete(id) + index = @indexForMarkerId(id) + @markers.splice(index, 1) if index isnt -1 @markersIdsWithChangeSubscriptions.delete(id) @index.delete(id) @destroyedMarkers.add(id) @@ -382,8 +390,22 @@ class MarkerLayer @index.insert(id, range.start, range.end) marker = new Marker(id, this, range, params) @markersById.set(id, marker) + @markers.push(id) marker + indexForMarkerId: (id) -> + low = 0 + high = @markers.length - 1 + while low <= high + index = low + ((high - low) >> 1) + if id < @markers[index] + high = index - 1 + else if id is @markers[index] + return index + else + low = index + 1 + -1 + setDisableDidUpdateEvent: (@didUpdateEventDisabled) -> emitDidUpdateEvent: -> From 816bc753cc397d3e766bcf1a76a8fd71ff0cbb25 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 4 May 2016 16:19:11 +0200 Subject: [PATCH 11/11] :shirt: Fix linter errors --- src/marker-layer.coffee | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/marker-layer.coffee b/src/marker-layer.coffee index a98f7e6f5f..0a79e3cf0a 100644 --- a/src/marker-layer.coffee +++ b/src/marker-layer.coffee @@ -52,7 +52,7 @@ class MarkerLayer # locations. copy: -> copy = @delegate.addMarkerLayer({@maintainHistory}) - @markersById.forEach (marker, id) => + @markersById.forEach (marker, id) -> snapshot = marker.getSnapshot(null) copy.createMarker(marker.getRange(), marker.getSnapshot()) copy @@ -145,7 +145,7 @@ class MarkerLayer if marker.matchesParams(params) result.push(marker) else - @markersById.forEach (marker) => + @markersById.forEach (marker) -> if marker.matchesParams(params) result.push(marker) @@ -300,7 +300,7 @@ class MarkerLayer createSnapshot: -> result = {} ranges = @index.dump() - @markersById.forEach (marker, id) => + @markersById.forEach (marker, id) -> result[id] = marker.getSnapshot(Range.fromObject(ranges[id]), false) result @@ -313,7 +313,7 @@ class MarkerLayer serialize: -> ranges = @index.dump() markersById = {} - @markersById.forEach (marker, id) => + @markersById.forEach (marker, id) -> markersById[id] = marker.getSnapshot(Range.fromObject(ranges[id]), false) {@id, @maintainHistory, @persistent, markersById, version: SerializationVersion}