From 47b19818ff40b10fa181e6f11fa31b4dfb4e809f Mon Sep 17 00:00:00 2001 From: James Allen Date: Mon, 12 Dec 2016 17:53:43 +0000 Subject: [PATCH] Add in new comment op type --- .../app/coffee/sharejs/types/text.coffee | 66 +++- .../coffee/ShareJS/TextTransformTests.coffee | 284 ++++++++++++++++++ 2 files changed, 344 insertions(+), 6 deletions(-) create mode 100644 services/document-updater/test/unit/coffee/ShareJS/TextTransformTests.coffee diff --git a/services/document-updater/app/coffee/sharejs/types/text.coffee b/services/document-updater/app/coffee/sharejs/types/text.coffee index c64b4dfa68..dcf2ef4cfe 100644 --- a/services/document-updater/app/coffee/sharejs/types/text.coffee +++ b/services/document-updater/app/coffee/sharejs/types/text.coffee @@ -31,7 +31,8 @@ checkValidComponent = (c) -> i_type = typeof c.i d_type = typeof c.d - throw new Error 'component needs an i or d field' unless (i_type == 'string') ^ (d_type == 'string') + c_type = typeof c.c + throw new Error 'component needs an i, d or c field' unless (i_type == 'string') ^ (d_type == 'string') ^ (c_type == 'string') throw new Error 'position cannot be negative' unless c.p >= 0 @@ -44,11 +45,15 @@ text.apply = (snapshot, op) -> for component in op if component.i? snapshot = strInject snapshot, component.p, component.i - else + else if component.d? deleted = snapshot[component.p...(component.p + component.d.length)] throw new Error "Delete component '#{component.d}' does not match deleted text '#{deleted}'" unless component.d == deleted snapshot = snapshot[...component.p] + snapshot[(component.p + component.d.length)..] - + else if component.c? + comment = snapshot[component.p...(component.p + component.c.length)] + throw new Error "Comment component '#{component.c}' does not match commented text '#{comment}'" unless component.c == comment + else + throw new Error "Unknown op type" snapshot @@ -112,7 +117,7 @@ transformPosition = (pos, c, insertAfter) -> pos + c.i.length else pos - else + else if c.d? # I think this could also be written as: Math.min(c.p, Math.min(c.p - otherC.p, otherC.d.length)) # but I think its harder to read that way, and it compiles using ternary operators anyway # so its no slower written like this. @@ -122,6 +127,10 @@ transformPosition = (pos, c, insertAfter) -> c.p else pos - c.d.length + else if c.c? + pos + else + throw new Error("unknown op type") # Helper method to transform a cursor position as a result of an op. # @@ -143,7 +152,7 @@ text._tc = transformComponent = (dest, c, otherC, side) -> if c.i? append dest, {i:c.i, p:transformPosition(c.p, otherC, side == 'right')} - else # Delete + else if c.d? # Delete if otherC.i? # delete vs insert s = c.d if c.p < otherC.p @@ -152,7 +161,7 @@ text._tc = transformComponent = (dest, c, otherC, side) -> if s != '' append dest, {d:s, p:c.p + otherC.i.length} - else # Delete vs delete + 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} else if c.p + c.d.length <= otherC.p @@ -177,6 +186,51 @@ text._tc = transformComponent = (dest, c, otherC, side) -> # This could be rewritten similarly to insert v delete, above. newC.p = transformPosition newC.p, otherC append dest, newC + + else if otherC.c? + append dest, c + + else + throw new Error("unknown op type") + + else if c.c? # Comment + if otherC.i? + 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} + else + append dest, {c:c.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} + else if c.p + c.c.length <= otherC.p + append dest, c + else # Delete overlaps comment + # They overlap somewhere. + newC = {c:'', p:c.p} + if c.p < otherC.p + newC.c = c.c[...(otherC.p - c.p)] + if c.p + c.c.length > otherC.p + otherC.d.length + newC.c += c.c[(otherC.p + otherC.d.length - c.p)..] + + # This is entirely optional - just for a check that the deleted + # text in the two ops matches + intersectStart = Math.max c.p, otherC.p + intersectEnd = Math.min c.p + c.c.length, otherC.p + otherC.d.length + cIntersect = c.c[intersectStart - c.p...intersectEnd - c.p] + otherIntersect = otherC.d[intersectStart - otherC.p...intersectEnd - otherC.p] + throw new Error 'Delete ops delete different text in the same region of the document' unless cIntersect == otherIntersect + + newC.p = transformPosition newC.p, otherC + append dest, newC + + else if otherC.c? + append dest, c + + else + throw new Error("unknown op type") dest diff --git a/services/document-updater/test/unit/coffee/ShareJS/TextTransformTests.coffee b/services/document-updater/test/unit/coffee/ShareJS/TextTransformTests.coffee new file mode 100644 index 0000000000..e0f75d2756 --- /dev/null +++ b/services/document-updater/test/unit/coffee/ShareJS/TextTransformTests.coffee @@ -0,0 +1,284 @@ +text = require "../../../../app/js/sharejs/types/text" +require("chai").should() +RangesTracker = require "../../../../app/js/RangesTracker" + +describe "ShareJS text type", -> + describe "transform", -> + describe "insert / insert", -> + it "with an insert before", -> + dest = [] + text._tc(dest, { i: "foo", p: 9 }, { i: "bar", p: 3 }) + dest.should.deep.equal [{ i: "foo", p: 12 }] + + it "with an insert after", -> + dest = [] + text._tc(dest, { i: "foo", p: 3 }, { i: "bar", p: 9 }) + dest.should.deep.equal [{ i: "foo", p: 3 }] + + it "with an insert at the same place with side == 'right'", -> + dest = [] + text._tc(dest, { i: "foo", p: 3 }, { i: "bar", p: 3 }, 'right') + dest.should.deep.equal [{ i: "foo", p: 6 }] + + it "with an insert at the same place with side == 'left'", -> + dest = [] + text._tc(dest, { i: "foo", p: 3 }, { i: "bar", p: 3 }, 'left') + dest.should.deep.equal [{ i: "foo", p: 3 }] + + describe "insert / delete", -> + it "with a delete before", -> + dest = [] + text._tc(dest, { i: "foo", p: 9 }, { d: "bar", p: 3 }) + dest.should.deep.equal [{ i: "foo", p: 6 }] + + it "with a delete after", -> + dest = [] + text._tc(dest, { i: "foo", p: 3 }, { d: "bar", p: 9 }) + dest.should.deep.equal [{ i: "foo", p: 3 }] + + it "with a delete at the same place with side == 'right'", -> + dest = [] + text._tc(dest, { i: "foo", p: 3 }, { d: "bar", p: 3 }, 'right') + dest.should.deep.equal [{ i: "foo", p: 3 }] + + 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 }] + + describe "delete / insert", -> + it "with an insert before", -> + dest = [] + text._tc(dest, { d: "foo", p: 9 }, { i: "bar", p: 3 }) + dest.should.deep.equal [{ d: "foo", p: 12 }] + + it "with an insert after", -> + dest = [] + text._tc(dest, { d: "foo", p: 3 }, { i: "bar", p: 9 }) + dest.should.deep.equal [{ d: "foo", p: 3 }] + + it "with an insert at the same place with side == 'right'", -> + dest = [] + text._tc(dest, { d: "foo", p: 3 }, { i: "bar", p: 3 }, 'right') + dest.should.deep.equal [{ d: "foo", p: 6 }] + + it "with an insert at the same place with side == 'left'", -> + dest = [] + text._tc(dest, { d: "foo", p: 3 }, { i: "bar", p: 3 }, 'left') + dest.should.deep.equal [{ d: "foo", p: 6 }] + + it "with a delete that overlaps the insert location", -> + dest = [] + text._tc(dest, { d: "foo", p: 3 }, { i: "bar", p: 4 }) + dest.should.deep.equal [{ d: "f", p: 3 }, { d: "oo", p: 6 }] + + + describe "delete / delete", -> + it "with a delete before", -> + dest = [] + text._tc(dest, { d: "foo", p: 9 }, { d: "bar", p: 3 }) + dest.should.deep.equal [{ d: "foo", p: 6 }] + + it "with a delete after", -> + dest = [] + text._tc(dest, { d: "foo", p: 3 }, { d: "bar", p: 9 }) + dest.should.deep.equal [{ d: "foo", p: 3 }] + + it "with deleting the same content", -> + dest = [] + text._tc(dest, { d: "foo", p: 3 }, { d: "foo", p: 3 }, 'right') + dest.should.deep.equal [] + + it "with the delete overlapping before", -> + dest = [] + text._tc(dest, { d: "foobar", p: 3 }, { d: "abcfoo", p: 0 }, 'right') + dest.should.deep.equal [{ d: "bar", p: 0 }] + + it "with the delete overlapping after", -> + dest = [] + text._tc(dest, { d: "abcfoo", p: 3 }, { d: "foobar", p: 6 }) + dest.should.deep.equal [{ d: "abc", p: 3 }] + + it "with the delete overlapping the whole delete", -> + dest = [] + text._tc(dest, { d: "abcfoo123", p: 3 }, { d: "foo", p: 6 }) + dest.should.deep.equal [{ d: "abc123", p: 3 }] + + it "with the delete inside the whole delete", -> + dest = [] + text._tc(dest, { d: "foo", p: 6 }, { d: "abcfoo123", p: 3 }) + dest.should.deep.equal [] + + describe "comment / insert", -> + it "with an insert before", -> + dest = [] + text._tc(dest, { c: "foo", p: 9 }, { i: "bar", p: 3 }) + dest.should.deep.equal [{ c: "foo", p: 12 }] + + it "with an insert after", -> + dest = [] + text._tc(dest, { c: "foo", p: 3 }, { i: "bar", p: 9 }) + dest.should.deep.equal [{ c: "foo", p: 3 }] + + it "with an insert at the left edge", -> + dest = [] + text._tc(dest, { c: "foo", p: 3 }, { i: "bar", p: 3 }) + # RangesTracker doesn't inject inserts into comments on edges, so neither should we + dest.should.deep.equal [{ c: "foo", p: 6 }] + + it "with an insert at the right edge", -> + dest = [] + text._tc(dest, { c: "foo", p: 3 }, { i: "bar", p: 6 }) + # RangesTracker doesn't inject inserts into comments on edges, so neither should we + dest.should.deep.equal [{ c: "foo", p: 3 }] + + it "with an insert in the middle", -> + dest = [] + text._tc(dest, { c: "foo", p: 3 }, { i: "bar", p: 5 }) + dest.should.deep.equal [{ c: "fobaro", p: 3 }] + + describe "comment / delete", -> + it "with a delete before", -> + dest = [] + text._tc(dest, { c: "foo", p: 9 }, { d: "bar", p: 3 }) + dest.should.deep.equal [{ c: "foo", p: 6 }] + + it "with a delete after", -> + dest = [] + text._tc(dest, { c: "foo", p: 3 }, { i: "bar", p: 9 }) + dest.should.deep.equal [{ c: "foo", p: 3 }] + + it "with a delete overlapping the comment content before", -> + dest = [] + text._tc(dest, { c: "foobar", p: 6 }, { d: "123foo", p: 3 }) + dest.should.deep.equal [{ c: "bar", p: 3 }] + + it "with a delete overlapping the comment content after", -> + dest = [] + text._tc(dest, { c: "foobar", p: 6 }, { d: "bar123", p: 9 }) + dest.should.deep.equal [{ c: "foo", p: 6 }] + + it "with a delete overlapping the comment content in the middle", -> + dest = [] + text._tc(dest, { c: "foo123bar", p: 6 }, { d: "123", p: 9 }) + dest.should.deep.equal [{ c: "foobar", p: 6 }] + + it "with a delete overlapping the whole comment", -> + dest = [] + text._tc(dest, { c: "foo", p: 6 }, { d: "123foo456", p: 3 }) + dest.should.deep.equal [{ c: "", p: 3 }] + + describe "comment / insert", -> + it "should not do anything", -> + dest = [] + text._tc(dest, { i: "foo", p: 6 }, { c: "bar", p: 3 }) + dest.should.deep.equal [{ i: "foo", p: 6 }] + + describe "comment / delete", -> + it "should not do anything", -> + dest = [] + text._tc(dest, { d: "foo", p: 6 }, { c: "bar", p: 3 }) + dest.should.deep.equal [{ d: "foo", p: 6 }] + + describe "comment / comment", -> + it "should not do anything", -> + dest = [] + text._tc(dest, { c: "foo", p: 6 }, { c: "bar", p: 3 }) + dest.should.deep.equal [{ c: "foo", p: 6 }] + + describe "apply", -> + it "should apply an insert", -> + text.apply("foo", [{ i: "bar", p: 2 }]).should.equal "fobaro" + + it "should apply a delete", -> + text.apply("foo123bar", [{ d: "123", p: 3 }]).should.equal "foobar" + + it "should do nothing with a comment", -> + text.apply("foo123bar", [{ c: "123", p: 3 }]).should.equal "foo123bar" + + it "should throw an error when deleted content does not match", -> + (() -> + text.apply("foo123bar", [{ d: "456", p: 3 }]) + ).should.throw(Error) + + it "should throw an error when comment content does not match", -> + (() -> + text.apply("foo123bar", [{ c: "456", p: 3 }]) + ).should.throw(Error) + + describe "applying ops and comments in different orders", -> + it "should not matter which op or comment is applied first", -> + transform = (op1, op2, side) -> + d = [] + text._tc(d, op1, op2, side) + return d + + applySnapshot = (snapshot, op) -> + return text.apply(snapshot, op) + + applyRanges = (rangesTracker, ops) -> + for op in ops + if op.c? + rangesTracker.addComment(op.p, op.c.length, {}) + else + rangesTracker.applyOp(op, {}) + return rangesTracker + + commentsEqual = (comments1, comments2) -> + return false if comments1.length != comments2.length + comments1.sort (a,b) -> + if a.offset - b.offset == 0 + return a.length - b.length + else + return a.offset - b.offset + comments2.sort (a,b) -> + if a.offset - b.offset == 0 + return a.length - b.length + else + return a.offset - b.offset + for comment1, i in comments1 + comment2 = comments2[i] + if comment1.offset != comment2.offset or comment1.length != comment2.length + return false + return true + + SNAPSHOT = "123" + + OPS = [] + # Insert ops + for p in [0..SNAPSHOT.length] + OPS.push {i: "a", p: p} + OPS.push {i: "bc", p: p} + for p in [0..(SNAPSHOT.length-1)] + for length in [1..(SNAPSHOT.length - p)] + OPS.push {d: SNAPSHOT.slice(p, p+length), p} + for p in [0..(SNAPSHOT.length-1)] + for length in [1..(SNAPSHOT.length - p)] + OPS.push {c: SNAPSHOT.slice(p, p+length), p} + + for op1 in OPS + for op2 in OPS + op1_t = transform(op1, op2, "left") + op2_t = transform(op2, op1, "right") + + rt12 = new RangesTracker() + snapshot12 = applySnapshot(applySnapshot(SNAPSHOT, [op1]), op2_t) + applyRanges(rt12, [op1]) + applyRanges(rt12, op2_t) + + rt21 = new RangesTracker() + snapshot21 = applySnapshot(applySnapshot(SNAPSHOT, [op2]), op1_t) + applyRanges(rt21, [op2]) + applyRanges(rt21, op1_t) + + if snapshot12 != snapshot21 + console.error {op1, op2, op1_t, op2_t, snapshot12, snapshot21}, "Ops are not consistent" + throw new Error("OT is inconsistent") + + if !commentsEqual(rt12.comments, rt21.comments) + console.log rt12.comments + console.log rt21.comments + console.error {op1, op2, op1_t, op2_t, rt12_comments: rt12.comments, rt21_comments: rt21.comments}, "Comments are not consistent" + throw new Error("OT is inconsistent") + \ No newline at end of file