diff --git a/services/document-updater/app/coffee/RangesTracker.coffee b/services/document-updater/app/coffee/RangesTracker.coffee index 6a3625fd09..1b865a600d 100644 --- a/services/document-updater/app/coffee/RangesTracker.coffee +++ b/services/document-updater/app/coffee/RangesTracker.coffee @@ -48,15 +48,6 @@ load = (EventEmitter) -> # sync with Ace ranges. @id = 0 - addComment: (offset, length, metadata) -> - # TODO: Don't allow overlapping comments? - @comments.push comment = { - id: @_newId() - offset, length, metadata - } - @emit "comment:added", comment - return comment - getComment: (comment_id) -> comment = null for c in @comments @@ -106,14 +97,32 @@ load = (EventEmitter) -> else if op.d? @applyDeleteToChanges(op, metadata) @applyDeleteToComments(op) + else if op.c? + @addComment(op, metadata) + else + throw new Error("unknown op type") + + addComment: (op, metadata) -> + # TODO: Don't allow overlapping comments? + @comments.push comment = { + id: @_newId() + op: # Copy because we'll modify in place + c: op.c + p: op.p + t: op.t + metadata + } + @emit "comment:added", comment + return comment applyInsertToComments: (op) -> for comment in @comments - if op.p <= comment.offset - comment.offset += op.i.length + if op.p <= comment.op.p + comment.op.p += op.i.length @emit "comment:moved", comment - else if op.p < comment.offset + comment.length - comment.length += op.i.length + 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 applyDeleteToComments: (op) -> @@ -121,20 +130,35 @@ load = (EventEmitter) -> op_length = op.d.length op_end = op.p + op_length for comment in @comments - comment_end = comment.offset + comment.length - if op_end <= comment.offset + comment_start = comment.op.p + comment_end = comment.op.p + comment.op.c.length + comment_length = comment_end - comment_start + if op_end <= comment_start # delete is fully before comment - comment.offset -= op_length + comment.op.p -= op_length @emit "comment:moved", comment else if op_start >= comment_end # delete is fully after comment, nothing to do else # delete and comment overlap - delete_length_before = Math.max(0, comment.offset - op_start) - delete_length_after = Math.max(0, op_end - comment_end) - delete_length_overlapping = op_length - delete_length_before - delete_length_after - comment.offset = Math.min(comment.offset, op_start) - comment.length -= delete_length_overlapping + if op_start <= comment_start + remaining_before = "" + else + remaining_before = comment.op.c.slice(0, op_start - comment_start) + if op_end >= comment_end + remaining_after = "" + else + remaining_after = comment.op.c.slice(op_end - comment_start) + + # Check deleted content matches delete op + deleted_comment = comment.op.c.slice(remaining_before.length, comment_length - remaining_after.length) + offset = Math.max(0, comment_start - op_start) + deleted_op_content = op.d.slice(offset).slice(0, deleted_comment.length) + if deleted_comment != deleted_op_content + throw new Error("deleted content does not match comment content") + + comment.op.p = Math.min(comment_start, op_start) + comment.op.c = remaining_before + remaining_after @emit "comment:moved", comment applyInsertToChanges: (op, metadata) -> diff --git a/services/document-updater/app/coffee/sharejs/types/text.coffee b/services/document-updater/app/coffee/sharejs/types/text.coffee index dcf2ef4cfe..2a3b79997d 100644 --- a/services/document-updater/app/coffee/sharejs/types/text.coffee +++ b/services/document-updater/app/coffee/sharejs/types/text.coffee @@ -198,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, {c:new_c, p:c.p} + append dest, {c:new_c, p:c.p, t: c.t} else - append dest, {c:c.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, {c:c.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 = {c:'', p:c.p} + 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/acceptance/coffee/RangesTests.coffee b/services/document-updater/test/acceptance/coffee/RangesTests.coffee index 95a7fae9d4..0cee1598aa 100644 --- a/services/document-updater/test/acceptance/coffee/RangesTests.coffee +++ b/services/document-updater/test/acceptance/coffee/RangesTests.coffee @@ -54,6 +54,82 @@ describe "Ranges", -> change.op.should.deep.equal { i: "456", p: 3 } change.metadata.user_id.should.equal @user_id done() + + describe "Adding comments", -> + describe "standalone", -> + before (done) -> + @project_id = DocUpdaterClient.randomId() + @user_id = DocUpdaterClient.randomId() + @doc = { + id: DocUpdaterClient.randomId() + lines: ["foo bar baz"] + } + @updates = [{ + doc: @doc.id + op: [{ c: "bar", p: 4, t: @tid = DocUpdaterClient.randomId() }] + v: 0 + }] + MockWebApi.insertDoc @project_id, @doc.id, { + lines: @doc.lines + version: 0 + } + jobs = [] + for update in @updates + do (update) => + jobs.push (callback) => DocUpdaterClient.sendUpdate @project_id, @doc.id, update, callback + DocUpdaterClient.preloadDoc @project_id, @doc.id, (error) => + throw error if error? + async.series jobs, (error) -> + throw error if error? + setTimeout done, 200 + + it "should update the ranges", (done) -> + DocUpdaterClient.getDoc @project_id, @doc.id, (error, res, data) => + throw error if error? + ranges = data.ranges + comment = ranges.comments[0] + comment.op.should.deep.equal { c: "bar", p: 4, t: @tid } + done() + + describe "with conflicting ops needing OT", -> + before (done) -> + @project_id = DocUpdaterClient.randomId() + @user_id = DocUpdaterClient.randomId() + @doc = { + id: DocUpdaterClient.randomId() + lines: ["foo bar baz"] + } + @updates = [{ + doc: @doc.id + op: [{ i: "ABC", p: 3 }] + v: 0 + meta: { user_id: @user_id } + }, { + doc: @doc.id + op: [{ c: "bar", p: 4, t: @tid = DocUpdaterClient.randomId() }] + v: 0 + }] + MockWebApi.insertDoc @project_id, @doc.id, { + lines: @doc.lines + version: 0 + } + jobs = [] + for update in @updates + do (update) => + jobs.push (callback) => DocUpdaterClient.sendUpdate @project_id, @doc.id, update, callback + DocUpdaterClient.preloadDoc @project_id, @doc.id, (error) => + throw error if error? + async.series jobs, (error) -> + throw error if error? + setTimeout done, 200 + + it "should update the comments with the OT shifted comment", (done) -> + DocUpdaterClient.getDoc @project_id, @doc.id, (error, res, data) => + throw error if error? + ranges = data.ranges + comment = ranges.comments[0] + comment.op.should.deep.equal { c: "bar", p: 7, t: @tid } + done() describe "Loading ranges from persistence layer", -> before (done) -> diff --git a/services/document-updater/test/unit/coffee/ShareJS/TextTransformTests.coffee b/services/document-updater/test/unit/coffee/ShareJS/TextTransformTests.coffee index e0f75d2756..81440bfe5b 100644 --- a/services/document-updater/test/unit/coffee/ShareJS/TextTransformTests.coffee +++ b/services/document-updater/test/unit/coffee/ShareJS/TextTransformTests.coffee @@ -3,6 +3,9 @@ require("chai").should() RangesTracker = require "../../../../app/js/RangesTracker" describe "ShareJS text type", -> + beforeEach -> + @t = "mock-thread-id" + describe "transform", -> describe "insert / insert", -> it "with an insert before", -> @@ -113,61 +116,61 @@ describe "ShareJS text type", -> 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 }] + text._tc(dest, { c: "foo", p: 9, @t }, { i: "bar", p: 3 }) + dest.should.deep.equal [{ c: "foo", p: 12, @t }] 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 }] + text._tc(dest, { c: "foo", p: 3, @t }, { i: "bar", p: 9 }) + dest.should.deep.equal [{ c: "foo", p: 3, @t }] it "with an insert at the left edge", -> dest = [] - text._tc(dest, { c: "foo", p: 3 }, { i: "bar", p: 3 }) + text._tc(dest, { c: "foo", p: 3, @t }, { 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 }] + dest.should.deep.equal [{ c: "foo", p: 6, @t }] it "with an insert at the right edge", -> dest = [] - text._tc(dest, { c: "foo", p: 3 }, { i: "bar", p: 6 }) + text._tc(dest, { c: "foo", p: 3, @t }, { 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 }] + dest.should.deep.equal [{ c: "foo", p: 3, @t }] 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 }] + text._tc(dest, { c: "foo", p: 3, @t }, { i: "bar", p: 5 }) + dest.should.deep.equal [{ c: "fobaro", p: 3, @t }] 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 }] + text._tc(dest, { c: "foo", p: 9, @t }, { d: "bar", p: 3 }) + dest.should.deep.equal [{ c: "foo", p: 6, @t }] 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 }] + text._tc(dest, { c: "foo", p: 3, @t }, { i: "bar", p: 9 }) + dest.should.deep.equal [{ c: "foo", p: 3, @t }] 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 }] + text._tc(dest, { c: "foobar", p: 6, @t }, { d: "123foo", p: 3 }) + dest.should.deep.equal [{ c: "bar", p: 3, @t }] 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 }] + text._tc(dest, { c: "foobar", p: 6, @t }, { d: "bar123", p: 9 }) + dest.should.deep.equal [{ c: "foo", p: 6, @t }] 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 }] + text._tc(dest, { c: "foo123bar", p: 6, @t }, { d: "123", p: 9 }) + dest.should.deep.equal [{ c: "foobar", p: 6, @t }] 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 }] + text._tc(dest, { c: "foo", p: 6, @t }, { d: "123foo456", p: 3 }) + dest.should.deep.equal [{ c: "", p: 3, @t }] describe "comment / insert", -> it "should not do anything", -> @@ -219,10 +222,7 @@ describe "ShareJS text type", -> applyRanges = (rangesTracker, ops) -> for op in ops - if op.c? - rangesTracker.addComment(op.p, op.c.length, {}) - else - rangesTracker.applyOp(op, {}) + rangesTracker.applyOp(op, {}) return rangesTracker commentsEqual = (comments1, comments2) -> @@ -255,8 +255,8 @@ describe "ShareJS text type", -> 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} - + OPS.push {c: SNAPSHOT.slice(p, p+length), p, @t} + for op1 in OPS for op2 in OPS op1_t = transform(op1, op2, "left") @@ -281,4 +281,3 @@ describe "ShareJS text type", -> 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