From f4bbd8ea107d687ba31eb906e62180d0d3f8dc54 Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 23 Feb 2017 11:27:19 +0100 Subject: [PATCH 1/4] Use dirty state rather events to avoid O(N^2) behaviour --- .../public/coffee/ide/editor/Document.coffee | 5 +- .../track-changes/TrackChangesManager.coffee | 154 +++++++++--------- .../ide/review-panel/RangesTracker.coffee | 72 +++++--- 3 files changed, 124 insertions(+), 107 deletions(-) diff --git a/services/web/public/coffee/ide/editor/Document.coffee b/services/web/public/coffee/ide/editor/Document.coffee index 1de01b5467..91c05e6e39 100644 --- a/services/web/public/coffee/ide/editor/Document.coffee +++ b/services/web/public/coffee/ide/editor/Document.coffee @@ -353,11 +353,12 @@ define [ @ranges.applyOp op, { user_id: track_changes_as } if old_id_seed? @ranges.setIdSeed(old_id_seed) + @emit "ranges:dirty" _catchUpRanges: (changes = [], comments = []) -> # We've just been given the current server's ranges, but need to apply any local ops we have. # Reset to the server state then apply our local ops again. - @ranges.emit "clear" + @emit "ranges:clear" @ranges.changes = changes @ranges.comments = comments @ranges.track_changes = @doc.track_changes @@ -367,4 +368,4 @@ define [ for op in @doc.getPendingOp() or [] @ranges.setIdSeed(@doc.track_changes_id_seeds.pending) @ranges.applyOp(op, { user_id: @track_changes_as }) - @ranges.emit "redraw" + @emit "ranges:redraw" diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor/track-changes/TrackChangesManager.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor/track-changes/TrackChangesManager.coffee index 38b3e5678c..17c10e7f71 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor/track-changes/TrackChangesManager.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor/track-changes/TrackChangesManager.coffee @@ -14,11 +14,11 @@ define [ return if !track_changes? @setTrackChanges(track_changes) - @$scope.$watch "sharejsDoc", (doc) => + @$scope.$watch "sharejsDoc", (doc, oldDoc) => return if !doc? - @disconnectFromRangesTracker() - @rangesTracker = doc.ranges - @connectToRangesTracker() + if oldDoc? + @disconnectFromDoc(oldDoc) + @connectToDoc(doc) @$scope.$on "comment:add", (e, thread_id, offset, length) => @addCommentToSelection(thread_id, offset, length) @@ -36,10 +36,10 @@ define [ @removeCommentId(comment_id) @$scope.$on "comment:resolve_threads", (e, thread_ids) => - @resolveCommentByThreadIds(thread_ids) + @hideCommentsByThreadIds(thread_ids) @$scope.$on "comment:unresolve_thread", (e, thread_id) => - @unresolveCommentByThreadId(thread_id) + @showCommentByThreadId(thread_id) @$scope.$on "review-panel:recalculate-screen-positions", () => @recalculateReviewEntriesScreenPositions() @@ -92,18 +92,11 @@ define [ else unbindFromAce() - disconnectFromRangesTracker: () -> + disconnectFromDoc: (doc) -> @changeIdToMarkerIdMap = {} - - if @rangesTracker? - @rangesTracker.off "insert:added" - @rangesTracker.off "insert:removed" - @rangesTracker.off "delete:added" - @rangesTracker.off "delete:removed" - @rangesTracker.off "changes:moved" - @rangesTracker.off "comment:added" - @rangesTracker.off "comment:moved" - @rangesTracker.off "comment:removed" + doc.off "ranges:clear" + doc.off "ranges:redraw" + doc.off "ranges:dirty" setTrackChanges: (value) -> if value @@ -111,56 +104,15 @@ define [ else @$scope.sharejsDoc?.track_changes_as = null - connectToRangesTracker: () -> + connectToDoc: (doc) -> + @rangesTracker = doc.ranges @setTrackChanges(@$scope.trackChanges) - # Add a timeout because on remote ops, we get these notifications before - # ace has updated - @rangesTracker.on "insert:added", (change) => - sl_console.log "[insert:added]", change - setTimeout () => - @_onInsertAdded(change) - @broadcastChange() - @rangesTracker.on "insert:removed", (change) => - sl_console.log "[insert:removed]", change - setTimeout () => - @_onInsertRemoved(change) - @broadcastChange() - @rangesTracker.on "delete:added", (change) => - sl_console.log "[delete:added]", change - setTimeout () => - @_onDeleteAdded(change) - @broadcastChange() - @rangesTracker.on "delete:removed", (change) => - sl_console.log "[delete:removed]", change - setTimeout () => - @_onDeleteRemoved(change) - @broadcastChange() - @rangesTracker.on "changes:moved", (changes) => - sl_console.log "[changes:moved]", changes - setTimeout () => - @_onChangesMoved(changes) - @broadcastChange() - - @rangesTracker.on "comment:added", (comment) => - sl_console.log "[comment:added]", comment - setTimeout () => - @_onCommentAdded(comment) - @broadcastChange() - @rangesTracker.on "comment:moved", (comment) => - sl_console.log "[comment:moved]", comment - setTimeout () => - @_onCommentMoved(comment) - @broadcastChange() - @rangesTracker.on "comment:removed", (comment) => - sl_console.log "[comment:removed]", comment - setTimeout () => - @_onCommentRemoved(comment) - @broadcastChange() - - @rangesTracker.on "clear", () => + doc.on "ranges:dirty", () => + @updateAnnotations() + doc.on "ranges:clear", () => @clearAnnotations() - @rangesTracker.on "redraw", () => + doc.on "ranges:redraw", () => @redrawAnnotations() clearAnnotations: () -> @@ -181,6 +133,55 @@ define [ @_onCommentAdded(comment) @broadcastChange() + + _doneUpdateThisLoop: false + _pendingUpdates: false + updateAnnotations: () -> + # Doc updates with multiple ops, like search/replace or block comments + # will call this with every individual op in a single event loop. So only + # do the first this loop, then schedule an update for the next loop for the rest. + if !@_doneUpdateThisLoop + @_doUpdateAnnotations() + @_doneUpdateThisLoop = true + setTimeout () => + if @_pendingUpdates + @_doUpdateAnnotations() + @_doneUpdateThisLoop = false + @_pendingUpdates = false + else + @_pendingUpdates = true + + _doUpdateAnnotations: () -> + dirty = @rangesTracker.getDirtyState() + + updateMarkers = false + + for id, change of dirty.change.added + if change.op.i? + @_onInsertAdded(change) + else if change.op.d? + @_onDeleteAdded(change) + for id, change of dirty.change.removed + if change.op.i? + @_onInsertRemoved(change) + else if change.op.d? + @_onDeleteRemoved(change) + for id, change of dirty.change.moved + updateMarkers = true + @_onChangeMoved(change) + + for id, comment of dirty.comment.added + @_onCommentAdded(comment) + for id, comment of dirty.comment.removed + @_onCommentRemoved(comment) + for id, comment of dirty.comment.moved + updateMarkers = true + @_onCommentMoved(comment) + + @rangesTracker.resetDirtyState() + if updateMarkers + @editor.renderer.updateBackMarkers() + @broadcastChange() addComment: (offset, content, thread_id) -> op = { c: content, p: offset, t: thread_id } @@ -200,6 +201,7 @@ define [ acceptChangeId: (change_id) -> @rangesTracker.removeChangeId(change_id) + @updateAnnotations() rejectChangeId: (change_id) -> change = @rangesTracker.getChange(change_id) @@ -221,8 +223,9 @@ define [ removeCommentId: (comment_id) -> @rangesTracker.removeCommentId(comment_id) + @updateAnnotations() - resolveCommentByThreadIds: (thread_ids) -> + hideCommentsByThreadIds: (thread_ids) -> resolve_ids = {} for id in thread_ids resolve_ids[id] = true @@ -231,7 +234,7 @@ define [ @_onCommentRemoved(comment) @broadcastChange() - unresolveCommentByThreadId: (thread_id) -> + showCommentByThreadId: (thread_id) -> for comment in @rangesTracker?.comments or [] if comment.op.t == thread_id @_onCommentAdded(comment) @@ -421,23 +424,18 @@ define [ lines = @editor.getSession().getDocument().getAllLines() return AceShareJsCodec.shareJsOffsetToAcePosition(offset, lines) - _onChangesMoved: (changes) -> - # TODO: PERFORMANCE: Only run through the Ace lines once, and calculate all - # change positions as we go. - for change in changes - start = @_shareJsOffsetToAcePosition(change.op.p) - if change.op.i? - end = @_shareJsOffsetToAcePosition(change.op.p + change.op.i.length) - else - end = start - @_updateMarker(change.id, start, end) - @editor.renderer.updateBackMarkers() + _onChangeMoved: (change) -> + start = @_shareJsOffsetToAcePosition(change.op.p) + if change.op.i? + end = @_shareJsOffsetToAcePosition(change.op.p + change.op.i.length) + else + end = start + @_updateMarker(change.id, start, end) _onCommentMoved: (comment) -> start = @_shareJsOffsetToAcePosition(comment.op.p) end = @_shareJsOffsetToAcePosition(comment.op.p + comment.op.c.length) @_updateMarker(comment.id, start, end) - @editor.renderer.updateBackMarkers() _updateMarker: (change_id, start, end) -> return if !@changeIdToMarkerIdMap[change_id]? diff --git a/services/web/public/coffee/ide/review-panel/RangesTracker.coffee b/services/web/public/coffee/ide/review-panel/RangesTracker.coffee index 865ecf4ef6..1f32f19d3a 100644 --- a/services/web/public/coffee/ide/review-panel/RangesTracker.coffee +++ b/services/web/public/coffee/ide/review-panel/RangesTracker.coffee @@ -1,5 +1,5 @@ -load = (EventEmitter) -> - class RangesTracker extends EventEmitter +load = () -> + class RangesTracker # The purpose of this class is to track a set of inserts and deletes to a document, like # track changes in Word. We store these as a set of ShareJs style ranges: # {i: "foo", p: 42} # Insert 'foo' at offset 42 @@ -36,6 +36,7 @@ load = (EventEmitter) -> # middle of a previous insert by the first user, the original insert will be split into two. constructor: (@changes = [], @comments = []) -> @setIdSeed(RangesTracker.generateIdSeed()) + @resetDirtyState() getIdSeed: () -> return @id_seed @@ -75,7 +76,7 @@ load = (EventEmitter) -> comment = @getComment(comment_id) return if !comment? @comments = @comments.filter (c) -> c.id != comment_id - @emit "comment:removed", comment + @_markAsDirty comment, "comment", "removed" getChange: (change_id) -> change = null @@ -103,7 +104,11 @@ load = (EventEmitter) -> @addComment(op, metadata) else throw new Error("unknown op type") - + + applyOps: (ops, metadata = {}) -> + for op in ops + @applyOp(op, metadata) + addComment: (op, metadata) -> # TODO: Don't allow overlapping comments? @comments.push comment = { @@ -114,18 +119,18 @@ load = (EventEmitter) -> t: op.t metadata } - @emit "comment:added", comment + @_markAsDirty comment, "comment", "added" return comment applyInsertToComments: (op) -> for comment in @comments if op.p <= comment.op.p comment.op.p += op.i.length - @emit "comment:moved", comment + @_markAsDirty comment, "comment", "moved" else if op.p < comment.op.p + comment.op.c.length offset = op.p - comment.op.p comment.op.c = comment.op.c[0..(offset-1)] + op.i + comment.op.c[offset...] - @emit "comment:moved", comment + @_markAsDirty comment, "comment", "moved" applyDeleteToComments: (op) -> op_start = op.p @@ -138,7 +143,7 @@ load = (EventEmitter) -> if op_end <= comment_start # delete is fully before comment comment.op.p -= op_length - @emit "comment:moved", comment + @_markAsDirty comment, "comment", "moved" else if op_start >= comment_end # delete is fully after comment, nothing to do else @@ -161,7 +166,7 @@ load = (EventEmitter) -> comment.op.p = Math.min(comment_start, op_start) comment.op.c = remaining_before + remaining_after - @emit "comment:moved", comment + @_markAsDirty comment, "comment", "moved" applyInsertToChanges: (op, metadata) -> op_start = op.p @@ -206,12 +211,12 @@ load = (EventEmitter) -> # If this is an insert op at the end of an existing insert with a delete following, and it cancels out the following # delete then we shouldn't append it to this insert, but instead only cancel the following delete. # E.g. - # foo|<--- about to insert 'b' here + # foo|<--- about to insert 'bar' here # inserted 'foo' --^ ^-- deleted 'bar' - # should become just 'foo' not 'foob' (with the delete marker becoming just 'ar'), . + # should become just 'foo' not 'foobar' (with the delete marker disappearing), . next_change = @changes[i+1] is_op_adjacent_to_next_delete = next_change? and next_change.op.d? and op.p == change_end and next_change.op.p == op.p - will_op_cancel_next_delete = is_op_adjacent_to_next_delete and next_change.op.d.slice(0, op.i.length) == op.i + will_op_cancel_next_delete = is_op_adjacent_to_next_delete and next_change.op.d == op.i # If there is a delete at the start of the insert, and we're inserting # at the start, we SHOULDN'T merge since the delete acts as a partition. @@ -281,8 +286,8 @@ load = (EventEmitter) -> for change in remove_changes @_removeChange change - if moved_changes.length > 0 - @emit "changes:moved", moved_changes + for change in moved_changes + @_markAsDirty change, "change", "moved" applyDeleteToChanges: (op, metadata) -> op_start = op.p @@ -406,8 +411,8 @@ load = (EventEmitter) -> @_removeChange change moved_changes = moved_changes.filter (c) -> c != change - if moved_changes.length > 0 - @emit "changes:moved", moved_changes + for change in moved_changes + @_markAsDirty change, "change", "moved" _addOp: (op, metadata) -> change = { @@ -427,17 +432,11 @@ load = (EventEmitter) -> else return -1 - if op.d? - @emit "delete:added", change - else if op.i? - @emit "insert:added", change + @_markAsDirty(change, "change", "added") _removeChange: (change) -> @changes = @changes.filter (c) -> c.id != change.id - if change.op.d? - @emit "delete:removed", change - else if change.op.i? - @emit "insert:removed", change + @_markAsDirty change, "change", "removed" _applyOpModifications: (content, op_modifications) -> # Put in descending position order, with deleting first if at the same offset @@ -486,13 +485,32 @@ load = (EventEmitter) -> previous_change = change return { moved_changes, remove_changes } + resetDirtyState: () -> + @_dirtyState = { + comment: { + moved: {} + removed: {} + added: {} + } + change: { + moved: {} + removed: {} + added: {} + } + } + + getDirtyState: () -> + return @_dirtyState + + _markAsDirty: (object, type, action) -> + @_dirtyState[type][action][object.id] = object + _clone: (object) -> clone = {} (clone[k] = v for k,v of object) return clone if define? - define ["utils/EventEmitter"], load + define [], load else - EventEmitter = require("events").EventEmitter - module.exports = load(EventEmitter) \ No newline at end of file + module.exports = load() \ No newline at end of file From 7418d12bf8602aa7eb5233014ec895145bdae589 Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 23 Feb 2017 11:34:58 +0100 Subject: [PATCH 2/4] Defer updates of remote ops until Ace has been updated --- services/web/public/coffee/ide/editor/Document.coffee | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/services/web/public/coffee/ide/editor/Document.coffee b/services/web/public/coffee/ide/editor/Document.coffee index 91c05e6e39..a321b85049 100644 --- a/services/web/public/coffee/ide/editor/Document.coffee +++ b/services/web/public/coffee/ide/editor/Document.coffee @@ -353,7 +353,12 @@ define [ @ranges.applyOp op, { user_id: track_changes_as } if old_id_seed? @ranges.setIdSeed(old_id_seed) - @emit "ranges:dirty" + if remote_op + # With remote ops, Ace hasn't been updated when we receive this op, + # so defer updating track changes until it has + setTimeout () => @emit "ranges:dirty" + else + @emit "ranges:dirty" _catchUpRanges: (changes = [], comments = []) -> # We've just been given the current server's ranges, but need to apply any local ops we have. From dd0271e7994a856b1505382ce7966dedbd3f4f9b Mon Sep 17 00:00:00 2001 From: James Allen Date: Fri, 24 Feb 2017 14:20:26 +0100 Subject: [PATCH 3/4] Only cancel deletes with inserts on undo and reject --- .../track-changes/TrackChangesManager.coffee | 4 +++ .../editor/sharejs/vendor/client/ace.coffee | 10 +++--- .../sharejs/vendor/types/text-api.coffee | 14 +++++--- .../editor/sharejs/vendor/types/text.coffee | 33 +++++++++++-------- .../ide/review-panel/RangesTracker.coffee | 18 +++++----- 5 files changed, 50 insertions(+), 29 deletions(-) diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor/track-changes/TrackChangesManager.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor/track-changes/TrackChangesManager.coffee index 17c10e7f71..f51d1ed41e 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor/track-changes/TrackChangesManager.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor/track-changes/TrackChangesManager.coffee @@ -210,14 +210,18 @@ define [ if change.op.d? content = change.op.d position = @_shareJsOffsetToAcePosition(change.op.p) + session.$fromReject = true # Tell track changes to cancel out delete session.insert(position, content) + session.$fromReject = false else if change.op.i? start = @_shareJsOffsetToAcePosition(change.op.p) end = @_shareJsOffsetToAcePosition(change.op.p + change.op.i.length) editor_text = session.getDocument().getTextRange({start, end}) if editor_text != change.op.i throw new Error("Op to be removed (#{JSON.stringify(change.op)}), does not match editor text, '#{editor_text}'") + session.$fromReject = true session.remove({start, end}) + session.$fromReject = false else throw new Error("unknown change: #{JSON.stringify(change)}") diff --git a/services/web/public/coffee/ide/editor/sharejs/vendor/client/ace.coffee b/services/web/public/coffee/ide/editor/sharejs/vendor/client/ace.coffee index 9526f9826c..b382a439ff 100644 --- a/services/web/public/coffee/ide/editor/sharejs/vendor/client/ace.coffee +++ b/services/web/public/coffee/ide/editor/sharejs/vendor/client/ace.coffee @@ -3,7 +3,7 @@ Range = ace.require("ace/range").Range # Convert an ace delta into an op understood by share.js -applyToShareJS = (editorDoc, delta, doc) -> +applyToShareJS = (editorDoc, delta, doc, fromUndo) -> # Get the start position of the range, in no. of characters getStartOffsetPosition = (start) -> # This is quite inefficient - getLines makes a copy of the entire @@ -27,11 +27,11 @@ applyToShareJS = (editorDoc, delta, doc) -> switch delta.action when 'insert' text = delta.lines.join('\n') - doc.insert pos, text + doc.insert pos, text, fromUndo when 'remove' text = delta.lines.join('\n') - doc.del pos, text.length + doc.del pos, text.length, fromUndo else throw new Error "unknown action: #{delta.action}" @@ -78,8 +78,10 @@ window.sharejs.extendDoc 'attach_ace', (editor, keepEditorContents, maxDocLength if maxDocLength? and editorDoc.getValue().length > maxDocLength doc.emit "error", new Error("document length is greater than maxDocLength") return + + fromUndo = !!(editor.getSession().$fromUndo or editor.getSession().$fromReject) - applyToShareJS editorDoc, change, doc + applyToShareJS editorDoc, change, doc, fromUndo check() diff --git a/services/web/public/coffee/ide/editor/sharejs/vendor/types/text-api.coffee b/services/web/public/coffee/ide/editor/sharejs/vendor/types/text-api.coffee index 274b6019c5..98bb3fd503 100644 --- a/services/web/public/coffee/ide/editor/sharejs/vendor/types/text-api.coffee +++ b/services/web/public/coffee/ide/editor/sharejs/vendor/types/text-api.coffee @@ -11,14 +11,20 @@ text.api = # Get the text contents of a document getText: -> @snapshot - insert: (pos, text, callback) -> - op = [{p:pos, i:text}] + insert: (pos, text, fromUndo, callback) -> + op = {p:pos, i:text} + if fromUndo + op.u = true + op = [op] @submitOp op, callback op - del: (pos, length, callback) -> - op = [{p:pos, d:@snapshot[pos...(pos + length)]}] + del: (pos, length, fromUndo, callback) -> + op = {p:pos, d:@snapshot[pos...(pos + length)]} + if fromUndo + op.u = true + op = [op] @submitOp op, callback op diff --git a/services/web/public/coffee/ide/editor/sharejs/vendor/types/text.coffee b/services/web/public/coffee/ide/editor/sharejs/vendor/types/text.coffee index 2a3b79997d..ee7bf57043 100644 --- a/services/web/public/coffee/ide/editor/sharejs/vendor/types/text.coffee +++ b/services/web/public/coffee/ide/editor/sharejs/vendor/types/text.coffee @@ -56,6 +56,13 @@ text.apply = (snapshot, op) -> throw new Error "Unknown op type" snapshot +cloneAndModify = (op, modifications) -> + newOp = {} + for k,v of op + newOp[k] = v + for k,v of modifications + newOp[k] = v + return newOp # Exported for use by the random op generator. # @@ -69,10 +76,10 @@ text._append = append = (newOp, c) -> last = newOp[newOp.length - 1] # Compose the insert into the previous insert if possible - if last.i? && c.i? and last.p <= c.p <= (last.p + last.i.length) - newOp[newOp.length - 1] = {i:strInject(last.i, c.p - last.p, c.i), p:last.p} - else if last.d? && c.d? and c.p <= last.p <= (c.p + c.d.length) - newOp[newOp.length - 1] = {d:strInject(c.d, last.p - c.p, last.d), p:c.p} + if last.i? && c.i? and last.p <= c.p <= (last.p + last.i.length) and last.u == c.u + newOp[newOp.length - 1] = cloneAndModify(last, {i:strInject(last.i, c.p - last.p, c.i)}) + else if last.d? && c.d? and c.p <= last.p <= (c.p + c.d.length) and last.u == c.u + newOp[newOp.length - 1] = cloneAndModify(last, {d:strInject(c.d, last.p - c.p, last.d), p: c.p}) else newOp.push c @@ -150,25 +157,25 @@ text._tc = transformComponent = (dest, c, otherC, side) -> checkValidOp [otherC] if c.i? - append dest, {i:c.i, p:transformPosition(c.p, otherC, side == 'right')} + append dest, cloneAndModify(c, {p:transformPosition(c.p, otherC, side == 'right')}) else if c.d? # Delete if otherC.i? # delete vs insert s = c.d if c.p < otherC.p - append dest, {d:s[...otherC.p - c.p], p:c.p} + append dest, cloneAndModify(c, {d:s[...otherC.p - c.p]}) s = s[(otherC.p - c.p)..] if s != '' - append dest, {d:s, p:c.p + otherC.i.length} + append dest, cloneAndModify(c, {d:s, p:c.p + otherC.i.length}) else if otherC.d? # Delete vs delete if c.p >= otherC.p + otherC.d.length - append dest, {d:c.d, p:c.p - otherC.d.length} + append dest, cloneAndModify(c, {p:c.p - otherC.d.length}) else if c.p + c.d.length <= otherC.p append dest, c else # They overlap somewhere. - newC = {d:'', p:c.p} + newC = cloneAndModify(c, {d:''}) if c.p < otherC.p newC.d = c.d[...(otherC.p - c.p)] if c.p + c.d.length > otherC.p + otherC.d.length @@ -198,18 +205,18 @@ text._tc = transformComponent = (dest, c, otherC, side) -> if c.p < otherC.p < c.p + c.c.length offset = otherC.p - c.p new_c = (c.c[0..(offset-1)] + otherC.i + c.c[offset...]) - append dest, {c:new_c, p:c.p, t: c.t} + append dest, cloneAndModify(c, {c:new_c}) else - append dest, {c:c.c, p:transformPosition(c.p, otherC, true), t: c.t} + append dest, cloneAndModify(c, {p:transformPosition(c.p, otherC, true)}) else if otherC.d? if c.p >= otherC.p + otherC.d.length - append dest, {c:c.c, p:c.p - otherC.d.length, t: c.t} + append dest, cloneAndModify(c, {p:c.p - otherC.d.length}) else if c.p + c.c.length <= otherC.p append dest, c else # Delete overlaps comment # They overlap somewhere. - newC = {c:'', p:c.p, t: c.t} + newC = cloneAndModify(c, {c:''}) if c.p < otherC.p newC.c = c.c[...(otherC.p - c.p)] if c.p + c.c.length > otherC.p + otherC.d.length diff --git a/services/web/public/coffee/ide/review-panel/RangesTracker.coffee b/services/web/public/coffee/ide/review-panel/RangesTracker.coffee index 1f32f19d3a..a9c43e9816 100644 --- a/services/web/public/coffee/ide/review-panel/RangesTracker.coffee +++ b/services/web/public/coffee/ide/review-panel/RangesTracker.coffee @@ -172,6 +172,7 @@ load = () -> op_start = op.p op_length = op.i.length op_end = op.p + op_length + undoing = !!op.u already_merged = false @@ -189,8 +190,9 @@ load = () -> change.op.p += op_length moved_changes.push change else if op_start == change_start - # If the insert matches the start of the delete, just remove it from the delete instead - if change.op.d.length >= op.i.length and change.op.d.slice(0, op.i.length) == op.i + # If we are undoing, then we want to cancel any existing delete ranges if we can. + # Check if the insert matches the start of the delete, and just remove it from the delete instead if so. + if undoing and change.op.d.length >= op.i.length and change.op.d.slice(0, op.i.length) == op.i change.op.d = change.op.d.slice(op.i.length) change.op.p += op.i.length if change.op.d == "" @@ -208,15 +210,15 @@ load = () -> # Only merge inserts if they are from the same user is_same_user = metadata.user_id == change.metadata.user_id - # If this is an insert op at the end of an existing insert with a delete following, and it cancels out the following - # delete then we shouldn't append it to this insert, but instead only cancel the following delete. + # If we are undoing, then our changes will be removed from any delete ops just after. In that case, if there is also + # an insert op just before, then we shouldn't append it to this insert, but instead only cancel the following delete. # E.g. - # foo|<--- about to insert 'bar' here + # foo|<--- about to insert 'b' here # inserted 'foo' --^ ^-- deleted 'bar' - # should become just 'foo' not 'foobar' (with the delete marker disappearing), . + # should become just 'foo' not 'foob' (with the delete marker becoming just 'ar'), . next_change = @changes[i+1] is_op_adjacent_to_next_delete = next_change? and next_change.op.d? and op.p == change_end and next_change.op.p == op.p - will_op_cancel_next_delete = is_op_adjacent_to_next_delete and next_change.op.d == op.i + will_op_cancel_next_delete = undoing and is_op_adjacent_to_next_delete and next_change.op.d.slice(0, op.i.length) == op.i # If there is a delete at the start of the insert, and we're inserting # at the start, we SHOULDN'T merge since the delete acts as a partition. @@ -513,4 +515,4 @@ load = () -> if define? define [], load else - module.exports = load() \ No newline at end of file + module.exports = load() From a6679a1aeb1f568def6644d938385e506e8d597d Mon Sep 17 00:00:00 2001 From: James Allen Date: Wed, 1 Mar 2017 16:33:04 +0000 Subject: [PATCH 4/4] Stop local and remote ops being batched together in the undo manager if they happen in the same flush --- .../ide/editor/directives/aceEditor.coffee | 4 -- .../aceEditor/undo/UndoManager.coffee | 67 ++++++++++++------- .../editor/sharejs/vendor/client/ace.coffee | 36 +++++++++- 3 files changed, 77 insertions(+), 30 deletions(-) diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor.coffee index faa1159e00..2a633e24f8 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor.coffee @@ -322,10 +322,6 @@ define [ doc = session.getDocument() doc.on "change", onChange - sharejs_doc.on "remoteop.recordRemote", (op, oldSnapshot, msg) -> - undoManager.nextUpdateIsRemote = true - trackChangesManager.nextUpdateMetaData = msg?.meta - editor.initing = true sharejs_doc.attachToAce(editor) editor.initing = false diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor/undo/UndoManager.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor/undo/UndoManager.coffee index a4348bed98..1f53fffe52 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor/undo/UndoManager.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor/undo/UndoManager.coffee @@ -11,10 +11,10 @@ define [ show_remote_warning: false @reset() - @nextUpdateIsRemote = false @editor.on "changeSession", (e) => @reset() + @session = e.session e.session.setUndoManager(@) showUndoConflictWarning: () -> @@ -38,20 +38,44 @@ define [ @firstUpdate = false return aceDeltaSets = options.args[0] - @session = options.args[1] return if !aceDeltaSets? + @session = options.args[1] - lines = @session.getDocument().getAllLines() - linesBeforeChange = @_revertAceDeltaSetsOnDocLines(aceDeltaSets, lines) - simpleDeltaSets = @_aceDeltaSetsToSimpleDeltaSets(aceDeltaSets, linesBeforeChange) - @undoStack.push( - deltaSets: simpleDeltaSets - remote: @nextUpdateIsRemote - ) + # We need to split the delta sets into local or remote groups before pushing onto + # the undo stack, since these are treated differently. + splitDeltaSets = [] + currentDeltaSet = null # Make global to this function + do newDeltaSet = () -> + currentDeltaSet = {group: "doc", deltas: []} + splitDeltaSets.push currentDeltaSet + currentRemoteState = null + + for deltaSet in aceDeltaSets or [] + if deltaSet.group == "doc" # ignore code folding etc. + for delta in deltaSet.deltas + if currentDeltaSet.remote? and currentDeltaSet.remote != !!delta.remote + newDeltaSet() + currentDeltaSet.deltas.push delta + currentDeltaSet.remote = !!delta.remote + + # The lines are currently as they are after applying all these deltas, but to turn into simple deltas, + # we need the lines before each delta group. + docLines = @session.getDocument().getAllLines() + docLines = @_revertAceDeltaSetsOnDocLines(aceDeltaSets, docLines) + for deltaSet in splitDeltaSets + {simpleDeltaSet, docLines} = @_aceDeltaSetToSimpleDeltaSet(deltaSet, docLines) + frame = { + deltaSets: [simpleDeltaSet] + remote: deltaSet.remote + } + @undoStack.push frame @redoStack = [] - @nextUpdateIsRemote = false undo: (dontSelect) -> + # We rely on the doclines being in sync with the undo stack, so make sure + # any pending undo deltas are processed. + @session.$syncInformUndoManager() + localUpdatesMade = @_shiftLocalChangeToTopOfUndoStack() return if !localUpdatesMade @@ -206,19 +230,16 @@ define [ throw "Unknown delta type" return doc.split("\n") - _aceDeltaSetsToSimpleDeltaSets: (aceDeltaSets, docLines) -> - simpleDeltaSets = [] - for deltaSet in aceDeltaSets - if deltaSet.group == "doc" # ignore fold changes - simpleDeltas = [] - for delta in deltaSet.deltas - simpleDeltas.push @_aceDeltaToSimpleDelta(delta, docLines) - docLines = @_applyAceDeltasToDocLines([delta], docLines) - simpleDeltaSets.push { - deltas: simpleDeltas - group: deltaSet.group - } - return simpleDeltaSets + _aceDeltaSetToSimpleDeltaSet: (deltaSet, docLines) -> + simpleDeltas = [] + for delta in deltaSet.deltas + simpleDeltas.push @_aceDeltaToSimpleDelta(delta, docLines) + docLines = @_applyAceDeltasToDocLines([delta], docLines) + simpleDeltaSet = { + deltas: simpleDeltas + group: deltaSet.group + } + return {simpleDeltaSet, docLines} _simpleDeltaSetsToAceDeltaSets: (simpleDeltaSets, docLines) -> for deltaSet in simpleDeltaSets diff --git a/services/web/public/coffee/ide/editor/sharejs/vendor/client/ace.coffee b/services/web/public/coffee/ide/editor/sharejs/vendor/client/ace.coffee index b382a439ff..ac0db65b1a 100644 --- a/services/web/public/coffee/ide/editor/sharejs/vendor/client/ace.coffee +++ b/services/web/public/coffee/ide/editor/sharejs/vendor/client/ace.coffee @@ -110,16 +110,46 @@ window.sharejs.extendDoc 'attach_ace', (editor, keepEditorContents, maxDocLength row:row, column:offset + # We want to insert a remote:true into the delta if the op comes from the + # underlying sharejs doc (which means it is from a remote op), so we have to do + # the work of editorDoc.insert and editorDoc.remove manually. These methods are + # copied from ace.js doc#insert and #remove, and then inject the remote:true + # flag into the delta. doc.on 'insert', (pos, text) -> + if (editorDoc.getLength() <= 1) + editorDoc.$detectNewLine(text) + + lines = editorDoc.$split(text) + position = offsetToPos(pos) + start = editorDoc.clippedPos(position.row, position.column) + end = { + row: start.row + lines.length - 1, + column: (if lines.length == 1 then start.column else 0) + lines[lines.length - 1].length + } + suppress = true - editorDoc.insert offsetToPos(pos), text + editorDoc.applyDelta({ + start: start, + end: end, + action: "insert", + lines: lines, + remote: true + }); suppress = false check() doc.on 'delete', (pos, text) -> - suppress = true range = Range.fromPoints offsetToPos(pos), offsetToPos(pos + text.length) - editorDoc.remove range + start = editorDoc.clippedPos(range.start.row, range.start.column) + end = editorDoc.clippedPos(range.end.row, range.end.column) + suppress = true + editorDoc.applyDelta({ + start: start, + end: end, + action: "remove", + lines: editorDoc.getLinesForRange({start: start, end: end}) + remote: true + }); suppress = false check()