From dd0271e7994a856b1505382ce7966dedbd3f4f9b Mon Sep 17 00:00:00 2001 From: James Allen Date: Fri, 24 Feb 2017 14:20:26 +0100 Subject: [PATCH] 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()