Accept comments with thread id as an op type

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) ->

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

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) ->

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")
-