From 9aec992693b025ae151338fe0437661560f19711 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Tue, 30 Apr 2013 17:37:28 -0600 Subject: [PATCH] Hold marker [in]validation events until after buffer change events For a while, the goal has been to prevent marker update events from firing until after the buffer change event. But at the same time, we want all the markers to be updated when the buffer change fires. This commit irons out some issues for markers that are invalidated or revalidated by the change, making their behavior consistent with the rest of marker updates. --- spec/app/display-buffer-spec.coffee | 124 ++++++++++++++++++------- src/app/buffer-change-operation.coffee | 19 ++-- src/app/text-buffer.coffee | 8 +- 3 files changed, 108 insertions(+), 43 deletions(-) diff --git a/spec/app/display-buffer-spec.coffee b/spec/app/display-buffer-spec.coffee index 69d5061dd..754a4e176 100644 --- a/spec/app/display-buffer-spec.coffee +++ b/spec/app/display-buffer-spec.coffee @@ -555,16 +555,16 @@ describe "DisplayBuffer", -> expect(marker.setTailBufferPosition([1, 0])).toBeFalsy() describe "marker change events", -> - [changedHandler, marker] = [] + [markerChangedHandler, marker] = [] beforeEach -> marker = displayBuffer.markScreenRange([[5, 4], [5, 10]]) - marker.on 'changed', changedHandler = jasmine.createSpy("changedHandler") + marker.on 'changed', markerChangedHandler = jasmine.createSpy("markerChangedHandler") it "triggers the 'changed' event whenever the markers head's screen position changes in the buffer or on screen", -> marker.setHeadScreenPosition([8, 20]) - expect(changedHandler).toHaveBeenCalled() - expect(changedHandler.argsForCall[0][0]).toEqual { + expect(markerChangedHandler).toHaveBeenCalled() + expect(markerChangedHandler.argsForCall[0][0]).toEqual { oldHeadScreenPosition: [5, 10] oldHeadBufferPosition: [8, 10] newHeadScreenPosition: [8, 20] @@ -576,11 +576,11 @@ describe "DisplayBuffer", -> bufferChanged: false valid: true } - changedHandler.reset() + markerChangedHandler.reset() buffer.insert([11, 0], '...') - expect(changedHandler).toHaveBeenCalled() - expect(changedHandler.argsForCall[0][0]).toEqual { + expect(markerChangedHandler).toHaveBeenCalled() + expect(markerChangedHandler.argsForCall[0][0]).toEqual { oldHeadScreenPosition: [8, 20] oldHeadBufferPosition: [11, 20] newHeadScreenPosition: [8, 23] @@ -592,11 +592,11 @@ describe "DisplayBuffer", -> bufferChanged: true valid: true } - changedHandler.reset() + markerChangedHandler.reset() displayBuffer.destroyFoldsContainingBufferRow(4) - expect(changedHandler).toHaveBeenCalled() - expect(changedHandler.argsForCall[0][0]).toEqual { + expect(markerChangedHandler).toHaveBeenCalled() + expect(markerChangedHandler.argsForCall[0][0]).toEqual { oldHeadScreenPosition: [8, 23] oldHeadBufferPosition: [11, 23] newHeadScreenPosition: [11, 23] @@ -608,11 +608,11 @@ describe "DisplayBuffer", -> bufferChanged: false valid: true } - changedHandler.reset() + markerChangedHandler.reset() displayBuffer.createFold(4, 7) - expect(changedHandler).toHaveBeenCalled() - expect(changedHandler.argsForCall[0][0]).toEqual { + expect(markerChangedHandler).toHaveBeenCalled() + expect(markerChangedHandler.argsForCall[0][0]).toEqual { oldHeadScreenPosition: [11, 23] oldHeadBufferPosition: [11, 23] newHeadScreenPosition: [8, 23] @@ -627,8 +627,8 @@ describe "DisplayBuffer", -> it "triggers the 'changed' event whenever the marker tail's position changes in the buffer or on screen", -> marker.setTailScreenPosition([8, 20]) - expect(changedHandler).toHaveBeenCalled() - expect(changedHandler.argsForCall[0][0]).toEqual { + expect(markerChangedHandler).toHaveBeenCalled() + expect(markerChangedHandler.argsForCall[0][0]).toEqual { oldHeadScreenPosition: [5, 10] oldHeadBufferPosition: [8, 10] newHeadScreenPosition: [5, 10] @@ -640,11 +640,11 @@ describe "DisplayBuffer", -> bufferChanged: false valid: true } - changedHandler.reset() + markerChangedHandler.reset() buffer.insert([11, 0], '...') - expect(changedHandler).toHaveBeenCalled() - expect(changedHandler.argsForCall[0][0]).toEqual { + expect(markerChangedHandler).toHaveBeenCalled() + expect(markerChangedHandler.argsForCall[0][0]).toEqual { oldHeadScreenPosition: [5, 10] oldHeadBufferPosition: [8, 10] newHeadScreenPosition: [5, 10] @@ -659,8 +659,8 @@ describe "DisplayBuffer", -> it "triggers the 'changed' event whenever the marker is invalidated or revalidated", -> buffer.deleteRow(8) - expect(changedHandler).toHaveBeenCalled() - expect(changedHandler.argsForCall[0][0]).toEqual { + expect(markerChangedHandler).toHaveBeenCalled() + expect(markerChangedHandler.argsForCall[0][0]).toEqual { oldHeadScreenPosition: [5, 10] oldHeadBufferPosition: [8, 10] newHeadScreenPosition: [5, 10] @@ -673,11 +673,11 @@ describe "DisplayBuffer", -> valid: false } - changedHandler.reset() + markerChangedHandler.reset() buffer.undo() - expect(changedHandler).toHaveBeenCalled() - expect(changedHandler.argsForCall[0][0]).toEqual { + expect(markerChangedHandler).toHaveBeenCalled() + expect(markerChangedHandler.argsForCall[0][0]).toEqual { oldHeadScreenPosition: [5, 10] oldHeadBufferPosition: [8, 10] newHeadScreenPosition: [5, 10] @@ -692,26 +692,86 @@ describe "DisplayBuffer", -> it "does not call the callback for screen changes that don't change the position of the marker", -> displayBuffer.createFold(10, 11) - expect(changedHandler).not.toHaveBeenCalled() + expect(markerChangedHandler).not.toHaveBeenCalled() - it "updates the position of markers before emitting buffer change events, but does not notify their observers until the change event", -> - displayBuffer.on 'changed', changeHandler = jasmine.createSpy("changeHandler").andCallFake -> + it "updates markers before emitting buffer change events, but does not notify their observers until the change event", -> + marker2 = displayBuffer.markBufferRange([[8, 1], [8, 1]]) + marker2.on 'changed', marker2ChangedHandler = jasmine.createSpy("marker2ChangedHandler") + displayBuffer.on 'changed', changeHandler = jasmine.createSpy("changeHandler").andCallFake -> onDisplayBufferChange() + + # New change ---- + + onDisplayBufferChange = -> # calls change handler first - expect(changedHandler).not.toHaveBeenCalled() + expect(markerChangedHandler).not.toHaveBeenCalled() + expect(marker2ChangedHandler).not.toHaveBeenCalled() # but still updates the markers expect(marker.getScreenRange()).toEqual [[5, 7], [5, 13]] expect(marker.getHeadScreenPosition()).toEqual [5, 13] expect(marker.getTailScreenPosition()).toEqual [5, 7] + expect(marker2.isValid()).toBeFalsy() - buffer.insert([8, 1], "...") - + buffer.change([[8, 0], [8, 2]], ".....") expect(changeHandler).toHaveBeenCalled() - expect(changedHandler).toHaveBeenCalled() + expect(markerChangedHandler).toHaveBeenCalled() + expect(marker2ChangedHandler).toHaveBeenCalled() + + # Undo change ---- + + changeHandler.reset() + markerChangedHandler.reset() + marker2ChangedHandler.reset() + + marker3 = displayBuffer.markBufferRange([[8, 1], [8, 2]]) + marker3.on 'changed', marker3ChangedHandler = jasmine.createSpy("marker3ChangedHandler") + + onDisplayBufferChange = -> + # calls change handler first + expect(markerChangedHandler).not.toHaveBeenCalled() + expect(marker2ChangedHandler).not.toHaveBeenCalled() + expect(marker3ChangedHandler).not.toHaveBeenCalled() + # but still updates the markers + expect(marker.getScreenRange()).toEqual [[5, 4], [5, 10]] + expect(marker.getHeadScreenPosition()).toEqual [5, 10] + expect(marker.getTailScreenPosition()).toEqual [5, 4] + expect(marker2.isValid()).toBeTruthy() + expect(marker3.isValid()).toBeFalsy() + + buffer.undo() + expect(changeHandler).toHaveBeenCalled() + expect(markerChangedHandler).toHaveBeenCalled() + expect(marker2ChangedHandler).toHaveBeenCalled() + expect(marker3ChangedHandler).toHaveBeenCalled() + + # Redo change ---- + + changeHandler.reset() + markerChangedHandler.reset() + marker2ChangedHandler.reset() + marker3ChangedHandler.reset() + + onDisplayBufferChange = -> + # calls change handler first + expect(markerChangedHandler).not.toHaveBeenCalled() + expect(marker2ChangedHandler).not.toHaveBeenCalled() + expect(marker3ChangedHandler).not.toHaveBeenCalled() + # but still updates the markers + expect(marker.getScreenRange()).toEqual [[5, 7], [5, 13]] + expect(marker.getHeadScreenPosition()).toEqual [5, 13] + expect(marker.getTailScreenPosition()).toEqual [5, 7] + expect(marker2.isValid()).toBeFalsy() + expect(marker3.isValid()).toBeTruthy() + + buffer.redo() + expect(changeHandler).toHaveBeenCalled() + expect(markerChangedHandler).toHaveBeenCalled() + expect(marker2ChangedHandler).toHaveBeenCalled() + expect(marker3ChangedHandler).toHaveBeenCalled() it "updates the position of markers before emitting change events that aren't caused by a buffer change", -> displayBuffer.on 'changed', changeHandler = jasmine.createSpy("changeHandler").andCallFake -> # calls change handler first - expect(changedHandler).not.toHaveBeenCalled() + expect(markerChangedHandler).not.toHaveBeenCalled() # but still updates the markers expect(marker.getScreenRange()).toEqual [[8, 4], [8, 10]] expect(marker.getHeadScreenPosition()).toEqual [8, 10] @@ -720,7 +780,7 @@ describe "DisplayBuffer", -> displayBuffer.destroyFoldsContainingBufferRow(4) expect(changeHandler).toHaveBeenCalled() - expect(changedHandler).toHaveBeenCalled() + expect(markerChangedHandler).toHaveBeenCalled() describe ".findMarkers(attributes)", -> it "allows the startBufferRow and endBufferRow to be specified", -> diff --git a/src/app/buffer-change-operation.coffee b/src/app/buffer-change-operation.coffee index b39bd3d4d..c0910a7ce 100644 --- a/src/app/buffer-change-operation.coffee +++ b/src/app/buffer-change-operation.coffee @@ -19,19 +19,21 @@ class BufferChangeOperation @options ?= {} do: -> + @pauseMarkerObservation() @oldText = @buffer.getTextInRange(@oldRange) @newRange = @calculateNewRange(@oldRange, @newText) @markersToRestoreOnUndo = @invalidateMarkers(@oldRange) - @changeBuffer + newRange = @changeBuffer oldRange: @oldRange newRange: @newRange oldText: @oldText newText: @newText - - redo: -> - @restoreMarkers(@markersToRestoreOnRedo) + @restoreMarkers(@markersToRestoreOnRedo) if @markersToRestoreOnRedo + @resumeMarkerObservation() + newRange undo: -> + @pauseMarkerObservation() @markersToRestoreOnRedo = @invalidateMarkers(@newRange) @changeBuffer oldRange: @newRange @@ -39,6 +41,7 @@ class BufferChangeOperation oldText: @newText newText: @oldText @restoreMarkers(@markersToRestoreOnUndo) + @resumeMarkerObservation() splitLines: (text) -> lines = text.split('\n') @@ -74,13 +77,10 @@ class BufferChangeOperation @buffer.cachedMemoryContents = null @buffer.conflict = false if @buffer.conflict and !@buffer.isModified() - @pauseMarkerObservation() event = { oldRange, newRange, oldText, newText } @updateMarkers(event) @buffer.trigger 'changed', event @buffer.scheduleModifiedEvents() - @resumeMarkerObservation() - @buffer.trigger 'markers-updated' newRange @@ -99,10 +99,11 @@ class BufferChangeOperation _.compact(@buffer.getMarkers().map (marker) -> marker.tryToInvalidate(oldRange)) pauseMarkerObservation: -> - marker.pauseEvents() for marker in @buffer.getMarkers() + marker.pauseEvents() for marker in @buffer.getMarkers(includeInvalid: true) resumeMarkerObservation: -> - marker.resumeEvents() for marker in @buffer.getMarkers() + marker.resumeEvents() for marker in @buffer.getMarkers(includeInvalid: true) + @buffer.trigger 'markers-updated' updateMarkers: (bufferChange) -> marker.handleBufferChange(bufferChange) for marker in @buffer.getMarkers() diff --git a/src/app/text-buffer.coffee b/src/app/text-buffer.coffee index b72bb4d1b..36d2da6c8 100644 --- a/src/app/text-buffer.coffee +++ b/src/app/text-buffer.coffee @@ -425,8 +425,12 @@ class Buffer isEmpty: -> @lines.length is 1 and @lines[0].length is 0 # Returns all valid {BufferMarker}s on the buffer. - getMarkers: -> - _.values(@validMarkers) + getMarkers: ({includeInvalid} = {}) -> + markers = _.values(@validMarkers) + if includeInvalid + markers.concat(_.values(@invalidMarkers)) + else + markers # Returns the {BufferMarker} with the given id. getMarker: (id) ->