From e1481df76a42ff5aaff62fdb6efda08aac80e241 Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 23 Feb 2017 11:39:19 +0100 Subject: [PATCH 1/7] Don't consume partial parts of delete when inserting a change --- services/document-updater/app/coffee/RangesTracker.coffee | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/services/document-updater/app/coffee/RangesTracker.coffee b/services/document-updater/app/coffee/RangesTracker.coffee index 865ecf4ef6..6a07e38c0e 100644 --- a/services/document-updater/app/coffee/RangesTracker.coffee +++ b/services/document-updater/app/coffee/RangesTracker.coffee @@ -206,12 +206,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. From 80284e1b0141cf370ebc234c0d51871e9e373e29 Mon Sep 17 00:00:00 2001 From: James Allen Date: Fri, 24 Feb 2017 14:21:06 +0100 Subject: [PATCH 2/7] Only cancel deletes with inserts on undo and reject --- .../app/coffee/RangesTracker.coffee | 82 ++++++++++++------- .../app/coffee/sharejs/types/text-api.coffee | 16 ++-- .../app/coffee/sharejs/types/text.coffee | 33 +++++--- .../coffee/ShareJS/TextTransformTests.coffee | 53 +++++++++++- 4 files changed, 133 insertions(+), 51 deletions(-) diff --git a/services/document-updater/app/coffee/RangesTracker.coffee b/services/document-updater/app/coffee/RangesTracker.coffee index 6a07e38c0e..a9c43e9816 100644 --- a/services/document-updater/app/coffee/RangesTracker.coffee +++ b/services/document-updater/app/coffee/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,12 +166,13 @@ 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 op_length = op.i.length op_end = op.p + op_length + undoing = !!op.u already_merged = false @@ -184,8 +190,9 @@ load = (EventEmitter) -> 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 == "" @@ -203,15 +210,15 @@ load = (EventEmitter) -> # 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. @@ -281,8 +288,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 +413,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 +434,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 +487,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() diff --git a/services/document-updater/app/coffee/sharejs/types/text-api.coffee b/services/document-updater/app/coffee/sharejs/types/text-api.coffee index 96243ceffb..98bb3fd503 100644 --- a/services/document-updater/app/coffee/sharejs/types/text-api.coffee +++ b/services/document-updater/app/coffee/sharejs/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 @@ -28,5 +34,5 @@ text.api = for component in op if component.i != undefined @emit 'insert', component.p, component.i - else + else if component.d != undefined @emit 'delete', component.p, component.d diff --git a/services/document-updater/app/coffee/sharejs/types/text.coffee b/services/document-updater/app/coffee/sharejs/types/text.coffee index 2a3b79997d..ee7bf57043 100644 --- a/services/document-updater/app/coffee/sharejs/types/text.coffee +++ b/services/document-updater/app/coffee/sharejs/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/document-updater/test/unit/coffee/ShareJS/TextTransformTests.coffee b/services/document-updater/test/unit/coffee/ShareJS/TextTransformTests.coffee index 81440bfe5b..5477b47b38 100644 --- a/services/document-updater/test/unit/coffee/ShareJS/TextTransformTests.coffee +++ b/services/document-updater/test/unit/coffee/ShareJS/TextTransformTests.coffee @@ -27,6 +27,11 @@ describe "ShareJS text type", -> dest = [] text._tc(dest, { i: "foo", p: 3 }, { i: "bar", p: 3 }, 'left') dest.should.deep.equal [{ i: "foo", p: 3 }] + + it "should preserve the undo flag", -> + dest = [] + text._tc(dest, { i: "foo", p: 9, u: true }, { i: "bar", p: 3 }) + dest.should.deep.equal [{ i: "foo", p: 12, u: true }] describe "insert / delete", -> it "with a delete before", -> @@ -46,9 +51,13 @@ describe "ShareJS text type", -> it "with a delete at the same place with side == 'left'", -> dest = [] - text._tc(dest, { i: "foo", p: 3 }, { d: "bar", p: 3 }, 'left') dest.should.deep.equal [{ i: "foo", p: 3 }] + + it "should preserve the undo flag", -> + dest = [] + text._tc(dest, { i: "foo", p: 9, u: true }, { d: "bar", p: 3 }) + dest.should.deep.equal [{ i: "foo", p: 6, u: true }] describe "delete / insert", -> it "with an insert before", -> @@ -75,7 +84,11 @@ describe "ShareJS text type", -> dest = [] text._tc(dest, { d: "foo", p: 3 }, { i: "bar", p: 4 }) dest.should.deep.equal [{ d: "f", p: 3 }, { d: "oo", p: 6 }] - + + it "should preserve the undo flag", -> + dest = [] + text._tc(dest, { d: "foo", p: 9, u: true }, { i: "bar", p: 3 }) + dest.should.deep.equal [{ d: "foo", p: 12, u: true }] describe "delete / delete", -> it "with a delete before", -> @@ -112,6 +125,11 @@ describe "ShareJS text type", -> dest = [] text._tc(dest, { d: "foo", p: 6 }, { d: "abcfoo123", p: 3 }) dest.should.deep.equal [] + + it "should preserve the undo flag", -> + dest = [] + text._tc(dest, { d: "foo", p: 9, u: true }, { d: "bar", p: 3 }) + dest.should.deep.equal [{ d: "foo", p: 6, u: true }] describe "comment / insert", -> it "with an insert before", -> @@ -210,6 +228,37 @@ describe "ShareJS text type", -> text.apply("foo123bar", [{ c: "456", p: 3 }]) ).should.throw(Error) + describe "_append", -> + it "should combine adjacent inserts", -> + dest = [{ i: "foo", p: 3 }] + text._append dest, { i: "bar", p: 6 } + dest.should.deep.equal [{ i: "foobar", p: 3 }] + + it "should combine adjacent undo inserts", -> + dest = [{ i: "foo", p: 3, u: true }] + text._append dest, { i: "bar", p: 6, u: true } + dest.should.deep.equal [{ i: "foobar", p: 3, u: true }] + + it "should not combine an undo and a normal insert", -> + dest = [{ i: "foo", p: 3, u: true }] + text._append dest, { i: "bar", p: 6 } + dest.should.deep.equal [{ i: "foo", p: 3, u: true }, { i: "bar", p: 6 }] + + it "should combine adjacent deletes", -> + dest = [{ d: "bar", p: 6 }] + text._append dest, { d: "foobaz", p: 3 } + dest.should.deep.equal [{ d: "foobarbaz", p: 3 }] + + it "should combine adjacent undo deletes", -> + dest = [{ d: "foo", p: 3, u: true }] + text._append dest, { d: "bar", p: 3, u: true } + dest.should.deep.equal [{ d: "foobar", p: 3, u: true }] + + it "should not combine an undo and a normal insert", -> + dest = [{ d: "foo", p: 3, u: true }] + text._append dest, { d: "bar", p: 3 } + dest.should.deep.equal [{ d: "foo", p: 3, u: true }, { d: "bar", p: 3 }] + describe "applying ops and comments in different orders", -> it "should not matter which op or comment is applied first", -> transform = (op1, op2, side) -> From d56bb5595320cbc2a0273ed98421ea85b0f6c417 Mon Sep 17 00:00:00 2001 From: James Allen Date: Wed, 1 Mar 2017 16:49:46 +0000 Subject: [PATCH 3/7] Revert PR #19 --- .../app/coffee/RangesTracker.coffee | 78 +++++++------------ .../app/coffee/sharejs/types/text-api.coffee | 16 ++-- .../app/coffee/sharejs/types/text.coffee | 33 ++++---- .../coffee/ShareJS/TextTransformTests.coffee | 53 +------------ 4 files changed, 49 insertions(+), 131 deletions(-) diff --git a/services/document-updater/app/coffee/RangesTracker.coffee b/services/document-updater/app/coffee/RangesTracker.coffee index a9c43e9816..865ecf4ef6 100644 --- a/services/document-updater/app/coffee/RangesTracker.coffee +++ b/services/document-updater/app/coffee/RangesTracker.coffee @@ -1,5 +1,5 @@ -load = () -> - class RangesTracker +load = (EventEmitter) -> + class RangesTracker extends EventEmitter # 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,7 +36,6 @@ load = () -> # 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 @@ -76,7 +75,7 @@ load = () -> comment = @getComment(comment_id) return if !comment? @comments = @comments.filter (c) -> c.id != comment_id - @_markAsDirty comment, "comment", "removed" + @emit "comment:removed", comment getChange: (change_id) -> change = null @@ -104,11 +103,7 @@ load = () -> @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 = { @@ -119,18 +114,18 @@ load = () -> t: op.t metadata } - @_markAsDirty comment, "comment", "added" + @emit "comment:added", comment return comment applyInsertToComments: (op) -> for comment in @comments if op.p <= comment.op.p comment.op.p += op.i.length - @_markAsDirty comment, "comment", "moved" + @emit "comment:moved", comment 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...] - @_markAsDirty comment, "comment", "moved" + @emit "comment:moved", comment applyDeleteToComments: (op) -> op_start = op.p @@ -143,7 +138,7 @@ load = () -> if op_end <= comment_start # delete is fully before comment comment.op.p -= op_length - @_markAsDirty comment, "comment", "moved" + @emit "comment:moved", comment else if op_start >= comment_end # delete is fully after comment, nothing to do else @@ -166,13 +161,12 @@ load = () -> comment.op.p = Math.min(comment_start, op_start) comment.op.c = remaining_before + remaining_after - @_markAsDirty comment, "comment", "moved" + @emit "comment:moved", comment applyInsertToChanges: (op, metadata) -> op_start = op.p op_length = op.i.length op_end = op.p + op_length - undoing = !!op.u already_merged = false @@ -190,9 +184,8 @@ load = () -> change.op.p += op_length moved_changes.push change else if op_start == change_start - # 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 + # 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 change.op.d = change.op.d.slice(op.i.length) change.op.p += op.i.length if change.op.d == "" @@ -210,15 +203,15 @@ load = () -> # Only merge inserts if they are from the same user is_same_user = metadata.user_id == change.metadata.user_id - # 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. + # 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 # inserted 'foo' --^ ^-- deleted 'bar' # 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 = undoing and 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.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. @@ -288,8 +281,8 @@ load = () -> for change in remove_changes @_removeChange change - for change in moved_changes - @_markAsDirty change, "change", "moved" + if moved_changes.length > 0 + @emit "changes:moved", moved_changes applyDeleteToChanges: (op, metadata) -> op_start = op.p @@ -413,8 +406,8 @@ load = () -> @_removeChange change moved_changes = moved_changes.filter (c) -> c != change - for change in moved_changes - @_markAsDirty change, "change", "moved" + if moved_changes.length > 0 + @emit "changes:moved", moved_changes _addOp: (op, metadata) -> change = { @@ -434,11 +427,17 @@ load = () -> else return -1 - @_markAsDirty(change, "change", "added") + if op.d? + @emit "delete:added", change + else if op.i? + @emit "insert:added", change _removeChange: (change) -> @changes = @changes.filter (c) -> c.id != change.id - @_markAsDirty change, "change", "removed" + if change.op.d? + @emit "delete:removed", change + else if change.op.i? + @emit "insert:removed", change _applyOpModifications: (content, op_modifications) -> # Put in descending position order, with deleting first if at the same offset @@ -487,32 +486,13 @@ load = () -> 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 [], load + define ["utils/EventEmitter"], load else - module.exports = load() + EventEmitter = require("events").EventEmitter + module.exports = load(EventEmitter) \ No newline at end of file diff --git a/services/document-updater/app/coffee/sharejs/types/text-api.coffee b/services/document-updater/app/coffee/sharejs/types/text-api.coffee index 98bb3fd503..96243ceffb 100644 --- a/services/document-updater/app/coffee/sharejs/types/text-api.coffee +++ b/services/document-updater/app/coffee/sharejs/types/text-api.coffee @@ -11,20 +11,14 @@ text.api = # Get the text contents of a document getText: -> @snapshot - insert: (pos, text, fromUndo, callback) -> - op = {p:pos, i:text} - if fromUndo - op.u = true - op = [op] + insert: (pos, text, callback) -> + op = [{p:pos, i:text}] @submitOp op, callback op - del: (pos, length, fromUndo, callback) -> - op = {p:pos, d:@snapshot[pos...(pos + length)]} - if fromUndo - op.u = true - op = [op] + del: (pos, length, callback) -> + op = [{p:pos, d:@snapshot[pos...(pos + length)]}] @submitOp op, callback op @@ -34,5 +28,5 @@ text.api = for component in op if component.i != undefined @emit 'insert', component.p, component.i - else if component.d != undefined + else @emit 'delete', component.p, component.d diff --git a/services/document-updater/app/coffee/sharejs/types/text.coffee b/services/document-updater/app/coffee/sharejs/types/text.coffee index ee7bf57043..2a3b79997d 100644 --- a/services/document-updater/app/coffee/sharejs/types/text.coffee +++ b/services/document-updater/app/coffee/sharejs/types/text.coffee @@ -56,13 +56,6 @@ 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. # @@ -76,10 +69,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) 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}) + 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} else newOp.push c @@ -157,25 +150,25 @@ text._tc = transformComponent = (dest, c, otherC, side) -> checkValidOp [otherC] if c.i? - append dest, cloneAndModify(c, {p:transformPosition(c.p, otherC, side == 'right')}) + append dest, {i:c.i, 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, cloneAndModify(c, {d:s[...otherC.p - c.p]}) + append dest, {d:s[...otherC.p - c.p], p:c.p} s = s[(otherC.p - c.p)..] if s != '' - append dest, cloneAndModify(c, {d:s, p:c.p + otherC.i.length}) + append dest, {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, cloneAndModify(c, {p:c.p - otherC.d.length}) + append dest, {d:c.d, p:c.p - otherC.d.length} else if c.p + c.d.length <= otherC.p append dest, c else # They overlap somewhere. - newC = cloneAndModify(c, {d:''}) + newC = {d:'', p:c.p} if c.p < otherC.p newC.d = c.d[...(otherC.p - c.p)] if c.p + c.d.length > otherC.p + otherC.d.length @@ -205,18 +198,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, cloneAndModify(c, {c:new_c}) + append dest, {c:new_c, p:c.p, t: c.t} else - append dest, cloneAndModify(c, {p:transformPosition(c.p, otherC, true)}) + append dest, {c:c.c, p:transformPosition(c.p, otherC, true), t: c.t} else if otherC.d? if c.p >= otherC.p + otherC.d.length - append dest, cloneAndModify(c, {p:c.p - otherC.d.length}) + append dest, {c:c.c, p:c.p - otherC.d.length, t: c.t} else if c.p + c.c.length <= otherC.p append dest, c else # Delete overlaps comment # They overlap somewhere. - newC = cloneAndModify(c, {c:''}) + newC = {c:'', p:c.p, t: c.t} 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/document-updater/test/unit/coffee/ShareJS/TextTransformTests.coffee b/services/document-updater/test/unit/coffee/ShareJS/TextTransformTests.coffee index 5477b47b38..81440bfe5b 100644 --- a/services/document-updater/test/unit/coffee/ShareJS/TextTransformTests.coffee +++ b/services/document-updater/test/unit/coffee/ShareJS/TextTransformTests.coffee @@ -27,11 +27,6 @@ describe "ShareJS text type", -> dest = [] text._tc(dest, { i: "foo", p: 3 }, { i: "bar", p: 3 }, 'left') dest.should.deep.equal [{ i: "foo", p: 3 }] - - it "should preserve the undo flag", -> - dest = [] - text._tc(dest, { i: "foo", p: 9, u: true }, { i: "bar", p: 3 }) - dest.should.deep.equal [{ i: "foo", p: 12, u: true }] describe "insert / delete", -> it "with a delete before", -> @@ -51,13 +46,9 @@ describe "ShareJS text type", -> it "with a delete at the same place with side == 'left'", -> dest = [] + text._tc(dest, { i: "foo", p: 3 }, { d: "bar", p: 3 }, 'left') dest.should.deep.equal [{ i: "foo", p: 3 }] - - it "should preserve the undo flag", -> - dest = [] - text._tc(dest, { i: "foo", p: 9, u: true }, { d: "bar", p: 3 }) - dest.should.deep.equal [{ i: "foo", p: 6, u: true }] describe "delete / insert", -> it "with an insert before", -> @@ -84,11 +75,7 @@ describe "ShareJS text type", -> dest = [] text._tc(dest, { d: "foo", p: 3 }, { i: "bar", p: 4 }) dest.should.deep.equal [{ d: "f", p: 3 }, { d: "oo", p: 6 }] - - it "should preserve the undo flag", -> - dest = [] - text._tc(dest, { d: "foo", p: 9, u: true }, { i: "bar", p: 3 }) - dest.should.deep.equal [{ d: "foo", p: 12, u: true }] + describe "delete / delete", -> it "with a delete before", -> @@ -125,11 +112,6 @@ describe "ShareJS text type", -> dest = [] text._tc(dest, { d: "foo", p: 6 }, { d: "abcfoo123", p: 3 }) dest.should.deep.equal [] - - it "should preserve the undo flag", -> - dest = [] - text._tc(dest, { d: "foo", p: 9, u: true }, { d: "bar", p: 3 }) - dest.should.deep.equal [{ d: "foo", p: 6, u: true }] describe "comment / insert", -> it "with an insert before", -> @@ -228,37 +210,6 @@ describe "ShareJS text type", -> text.apply("foo123bar", [{ c: "456", p: 3 }]) ).should.throw(Error) - describe "_append", -> - it "should combine adjacent inserts", -> - dest = [{ i: "foo", p: 3 }] - text._append dest, { i: "bar", p: 6 } - dest.should.deep.equal [{ i: "foobar", p: 3 }] - - it "should combine adjacent undo inserts", -> - dest = [{ i: "foo", p: 3, u: true }] - text._append dest, { i: "bar", p: 6, u: true } - dest.should.deep.equal [{ i: "foobar", p: 3, u: true }] - - it "should not combine an undo and a normal insert", -> - dest = [{ i: "foo", p: 3, u: true }] - text._append dest, { i: "bar", p: 6 } - dest.should.deep.equal [{ i: "foo", p: 3, u: true }, { i: "bar", p: 6 }] - - it "should combine adjacent deletes", -> - dest = [{ d: "bar", p: 6 }] - text._append dest, { d: "foobaz", p: 3 } - dest.should.deep.equal [{ d: "foobarbaz", p: 3 }] - - it "should combine adjacent undo deletes", -> - dest = [{ d: "foo", p: 3, u: true }] - text._append dest, { d: "bar", p: 3, u: true } - dest.should.deep.equal [{ d: "foobar", p: 3, u: true }] - - it "should not combine an undo and a normal insert", -> - dest = [{ d: "foo", p: 3, u: true }] - text._append dest, { d: "bar", p: 3 } - dest.should.deep.equal [{ d: "foo", p: 3, u: true }, { d: "bar", p: 3 }] - describe "applying ops and comments in different orders", -> it "should not matter which op or comment is applied first", -> transform = (op1, op2, side) -> From b186a01c04596c38d4cf9f652d23fd8dc8225b50 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Fri, 3 Mar 2017 15:27:42 +0000 Subject: [PATCH 4/7] don't log errors from redis backend this also picks up errors from RedisManager like "doc ops range is not loaded in redis" --- services/document-updater/app/coffee/RedisBackend.coffee | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/services/document-updater/app/coffee/RedisBackend.coffee b/services/document-updater/app/coffee/RedisBackend.coffee index 9ec479c01f..d69cd21a6e 100644 --- a/services/document-updater/app/coffee/RedisBackend.coffee +++ b/services/document-updater/app/coffee/RedisBackend.coffee @@ -100,7 +100,8 @@ class MultiClient cb(error, result) async.parallel jobs, (error, results) -> if error? - logger.error {err: error}, "error in redis backend" + # suppress logging of errors + # logger.error {err: error}, "error in redis backend" else compareResults(results, "exec") callback(primaryError, primaryResult) From 3f13263ecfebbc6dd4da584031fce96df85a3f4e Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Fri, 3 Mar 2017 15:32:11 +0000 Subject: [PATCH 5/7] upgrade to logger-sharelatex 1.5.3 --- services/document-updater/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/document-updater/package.json b/services/document-updater/package.json index 72a2e28121..76bb426cf4 100644 --- a/services/document-updater/package.json +++ b/services/document-updater/package.json @@ -11,7 +11,7 @@ "coffee-script": "1.4.0", "express": "3.3.4", "ioredis": "^2.2.0", - "logger-sharelatex": "git+https://github.com/sharelatex/logger-sharelatex.git#v1.5.2", + "logger-sharelatex": "git+https://github.com/sharelatex/logger-sharelatex.git#v1.5.3", "lynx": "0.0.11", "metrics-sharelatex": "git+https://github.com/sharelatex/metrics-sharelatex.git#v1.5.0", "redis-sharelatex": "0.0.9", From d086e0b61b032a34ecec503596c14524e96af013 Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Fri, 3 Mar 2017 15:57:44 +0000 Subject: [PATCH 6/7] log doclines on hash mismatch --- services/document-updater/app/coffee/RedisManager.coffee | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/services/document-updater/app/coffee/RedisManager.coffee b/services/document-updater/app/coffee/RedisManager.coffee index d822f4ef74..1ce0062fe6 100644 --- a/services/document-updater/app/coffee/RedisManager.coffee +++ b/services/document-updater/app/coffee/RedisManager.coffee @@ -52,7 +52,7 @@ module.exports = RedisManager = # check the hash computed on the redis server writeHash = result?[0] if logHashWriteErrors and writeHash? and writeHash isnt docHash - logger.error project_id: project_id, doc_id: doc_id, writeHash: writeHash, origHash: docHash, "hash mismatch on putDocInMemory" + logger.error project_id: project_id, doc_id: doc_id, writeHash: writeHash, origHash: docHash, docLines:docLines, "hash mismatch on putDocInMemory" # update docsInProject set rclient.sadd keys.docsInProject(project_id:project_id), doc_id, callback @@ -92,7 +92,7 @@ module.exports = RedisManager = if docLines? and storedHash? computedHash = RedisManager._computeHash(docLines) if logHashReadErrors and computedHash isnt storedHash - logger.error project_id: project_id, doc_id: doc_id, doc_project_id: doc_project_id, computedHash: computedHash, storedHash: storedHash, "hash mismatch on retrieved document" + logger.error project_id: project_id, doc_id: doc_id, doc_project_id: doc_project_id, computedHash: computedHash, storedHash: storedHash, docLines:docLines, "hash mismatch on retrieved document" try docLines = JSON.parse docLines @@ -181,7 +181,7 @@ module.exports = RedisManager = # check the hash computed on the redis server writeHash = result?[0] if logHashWriteErrors and writeHash? and writeHash isnt newHash - logger.error doc_id: doc_id, writeHash: writeHash, origHash: newHash, "hash mismatch on updateDocument" + logger.error doc_id: doc_id, writeHash: writeHash, origHash: newHash, docLines:newDocLines, "hash mismatch on updateDocument" return callback() getDocIdsInProject: (project_id, callback = (error, doc_ids) ->) -> From 501d907299cf8265d7d3b32e10f10d6de7fb355e Mon Sep 17 00:00:00 2001 From: Brian Gough Date: Fri, 3 Mar 2017 16:08:14 +0000 Subject: [PATCH 7/7] upgrade to logger-sharelatex 1.5.4 --- services/document-updater/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/document-updater/package.json b/services/document-updater/package.json index 76bb426cf4..9f8439ea04 100644 --- a/services/document-updater/package.json +++ b/services/document-updater/package.json @@ -11,7 +11,7 @@ "coffee-script": "1.4.0", "express": "3.3.4", "ioredis": "^2.2.0", - "logger-sharelatex": "git+https://github.com/sharelatex/logger-sharelatex.git#v1.5.3", + "logger-sharelatex": "git+https://github.com/sharelatex/logger-sharelatex.git#v1.5.4", "lynx": "0.0.11", "metrics-sharelatex": "git+https://github.com/sharelatex/metrics-sharelatex.git#v1.5.0", "redis-sharelatex": "0.0.9",