Revert "Support a {dr:...} op for deleting ranges"

This reverts commit 24c58e5ad430e0240533cc1e5c21122859fe8dc9.

services/document-updater/app/coffee/RangesTracker.coffee

@@ -35,25 +35,18 @@ load = (EventEmitter) ->
 		#   * Inserts by another user will not combine with inserts by the first user. If they are in the
 		#     middle of a previous insert by the first user, the original insert will be split into two.
 		constructor: (@changes = [], @comments = []) ->
-
-		@_increment: 0
-		@newId: () ->
-			# Generate a Mongo ObjectId
-			# Reference: https://github.com/dreampulse/ObjectId.js/blob/master/src/main/javascript/Objectid.js
-			@_pid ?= Math.floor(Math.random() * (32767))
-			@_machine ?= Math.floor(Math.random() * (16777216))
-			timestamp = Math.floor(new Date().valueOf() / 1000)
-			@_increment++
-			
-			timestamp = timestamp.toString(16)
-			machine = @_machine.toString(16)
-			pid = @_pid.toString(16)
-			increment = @_increment.toString(16)
-			
-			return '00000000'.substr(0, 8 - timestamp.length) + timestamp +
-				'000000'.substr(0, 6 - machine.length) + machine +
-				'0000'.substr(0, 4 - pid.length) + pid +
-				'000000'.substr(0, 6 - increment.length) + increment;
+			# Change objects have the following structure:
+			# {
+			#   id: ... # Uniquely generated by us
+			#   op: { # ShareJs style op tracking the offset (p) and content inserted (i) or deleted (d)
+			#     i: "..."
+			#     p: 42
+			#   }
+			# }
+			#
+			# Ids are used to uniquely identify a change, e.g. for updating it in the database, or keeping in
+			# sync with Ace ranges.
+			@id = 0
 		
 		getComment: (comment_id) ->
 			comment = null
@@ -63,6 +56,19 @@ load = (EventEmitter) ->
 					break
 			return comment
 		
+		resolveCommentId: (comment_id, resolved_data) ->
+			comment = @getComment(comment_id)
+			return if !comment?
+			comment.metadata.resolved = true
+			comment.metadata.resolved_data = resolved_data
+			@emit "comment:resolved", comment
+		
+		unresolveCommentId: (comment_id) ->
+			comment = @getComment(comment_id)
+			return if !comment?
+			comment.metadata.resolved = false
+			@emit "comment:unresolved", comment
+		
 		removeCommentId: (comment_id) ->
 			comment = @getComment(comment_id)
 			return if !comment?
@@ -82,7 +88,7 @@ load = (EventEmitter) ->
 			return if !change?
 			@_removeChange(change)
 
-		applyOp: (op, metadata = {}) ->
+		applyOp: (op, metadata) ->
 			metadata.ts ?= new Date()
 			# Apply an op that has been applied to the document to our changes to keep them up to date
 			if op.i?
@@ -91,8 +97,6 @@ load = (EventEmitter) ->
 			else if op.d?
 				@applyDeleteToChanges(op, metadata)
 				@applyDeleteToComments(op)
-			else if op.dr?
-				@applyDeleteRangeToChanges(op)
 			else if op.c?
 				@addComment(op, metadata)
 			else
@@ -101,7 +105,7 @@ load = (EventEmitter) ->
 		addComment: (op, metadata) ->
 			# TODO: Don't allow overlapping comments?
 			@comments.push comment = {
-				id: RangesTracker.newId()
+				id: @_newId()
 				op: # Copy because we'll modify in place
 					c: op.c
 					p: op.p
@@ -390,20 +394,28 @@ load = (EventEmitter) ->
 			if moved_changes.length > 0
 				@emit "changes:moved", moved_changes
 
-		applyDeleteRangeToChanges: (op) ->
-			remove_changes = []
-			for change in @changes
-				change_text = change.op.i or change.op.d
-				if op.p == change.op.p and op.dr == change_text
-					remove_changes.push change
-			if remove_changes.length == 0
-				throw new Error("no range to remove")
-			for change in remove_changes
-				@_removeChange(change)
+		_newId: () ->
+			# Generate a Mongo ObjectId
+			# Reference: https://github.com/dreampulse/ObjectId.js/blob/master/src/main/javascript/Objectid.js
+			@_pid ?= Math.floor(Math.random() * (32767))
+			@_machine ?= Math.floor(Math.random() * (16777216))
+			timestamp = Math.floor(new Date().valueOf() / 1000)
+			@_increment ?= 0
+			@_increment++
+			
+			timestamp = timestamp.toString(16)
+			machine = @_machine.toString(16)
+			pid = @_pid.toString(16)
+			increment = @_increment.toString(16)
+			
+			return '00000000'.substr(0, 8 - timestamp.length) + timestamp +
+				'000000'.substr(0, 6 - machine.length) + machine +
+				'0000'.substr(0, 4 - pid.length) + pid +
+				'000000'.substr(0, 6 - increment.length) + increment;
 
 		_addOp: (op, metadata) ->
 			change = {
-				id: RangesTracker.newId()
+				id: @_newId()
 				op: op
 				metadata: metadata
 			}

services/document-updater/app/coffee/sharejs/types/text.coffee

@@ -32,8 +32,7 @@ checkValidComponent = (c) ->
   i_type = typeof c.i
   d_type = typeof c.d
   c_type = typeof c.c
-  dr_type = typeof c.dr
-  throw new Error 'component needs an i, d, c or dr field' unless (i_type == 'string') ^ (d_type == 'string') ^ (c_type == 'string') ^ (dr_type == 'string')
+  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
 
@@ -41,26 +40,6 @@ checkValidOp = (op) ->
   checkValidComponent(c) for c in op
   true
 
-componentText = (c) ->
-  if c.c?
-    text = c.c
-  if c.dr?
-    text = c.dr
-  throw new Error("invalid component") if !text?
-  return text
-
-duplicateComponent = (c) ->
-  newC = {}
-  for key, value of c
-    newC[key] = value
-  return newC
-
-setComponentText = (c, text) ->
-  if c.c?
-    c.c = text
-  if c.dr?
-    c.dr = text
-
 text.apply = (snapshot, op) ->
   checkValidOp op
   for component in op
@@ -70,10 +49,9 @@ text.apply = (snapshot, op) ->
       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? or component.dr?
-      c_text = componentText(component)
-      range = snapshot[component.p...(component.p + c_text.length)]
-      throw new Error "Range component '#{c_text}' does not match range text '#{range}'" unless c_text == range
+    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
@@ -149,7 +127,7 @@ transformPosition = (pos, c, insertAfter) ->
       c.p
     else
       pos - c.d.length
-  else if c.c? or c.dr?
+  else if c.c?
     pos
   else
     throw new Error("unknown op type")
@@ -209,54 +187,46 @@ text._tc = transformComponent = (dest, c, otherC, side) ->
           newC.p = transformPosition newC.p, otherC
           append dest, newC
     
-    else if otherC.c? or otherC.dr?
+    else if otherC.c?
       append dest, c
     
     else
       throw new Error("unknown op type")
 
-  else if c.c? or c.dr? # Comment or delete range
-    c_text = componentText(c)
+  else if c.c? # Comment
     if otherC.i?
-      if c.p < otherC.p < c.p + c_text.length
+      if c.p < otherC.p < c.p + c.c.length
         offset = otherC.p - c.p
-        newText = (c_text[0..(offset-1)] + otherC.i + c_text[offset...])
-        newC = duplicateComponent(c)
-        setComponentText(newC, newText)
-        append dest, newC
+        new_c = (c.c[0..(offset-1)] + otherC.i + c.c[offset...])
+        append dest, {c:new_c, p:c.p, t: c.t}
       else
-        newC = duplicateComponent(c)
-        newC.p = transformPosition(c.p, otherC, true)
-        append dest, newC
+        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
-        newC = duplicateComponent(c)
-        newC.p = c.p - otherC.d.length
-        append dest, newC
-      else if c.p + c_text.length <= otherC.p
+        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 = duplicateComponent(c)
-        setComponentText(newC, '')
+        newC = {c:'', p:c.p, t: c.t}
         if c.p < otherC.p
-          setComponentText(newC, c_text[...(otherC.p - c.p)])
-        if c.p + c_text.length > otherC.p + otherC.d.length
-          setComponentText(newC, componentText(newC) + c_text[(otherC.p + otherC.d.length - c.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_text.length, otherC.p + otherC.d.length
-        cIntersect = c_text[intersectStart - c.p...intersectEnd - c.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 op text does not match range being modified' unless cIntersect == otherIntersect
+        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? or otherC.dr?
+    else if otherC.c?
       append dest, c
     
     else

services/document-updater/test/acceptance/coffee/RangesTests.coffee

@@ -1,7 +1,6 @@
 sinon = require "sinon"
 chai = require("chai")
 chai.should()
-expect = chai.expect
 async = require "async"
 rclient = require("redis").createClient()
 
@@ -55,105 +54,82 @@ describe "Ranges", ->
 				change.op.should.deep.equal { i: "456", p: 3 }
 				change.metadata.user_id.should.equal @user_id
 				done()
-		
-		describe "removing ranges", ->
-			it "should delete the range (and perform OT)", (done) ->
-				@conflicting_update = {
-					doc: @doc.id
-					op: [{ i: "X", p: 1 }]
-					v: 3
-					meta: { user_id: @user_id }
-				}
-				@delete_range = {
-					doc: @doc.id
-					op: [{ dr: "456", p: 3 }]
-					v: 3
-					meta: { user_id: @user_id }
-				}
-				DocUpdaterClient.sendUpdate @project_id, @doc.id, @conflicting_update, (error) =>
-					throw error if error?
-					DocUpdaterClient.sendUpdate @project_id, @doc.id, @delete_range, (error) =>
+	
+		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?
-						DocUpdaterClient.getDoc @project_id, @doc.id, (error, res, data) =>
+						async.series jobs, (error) ->
 							throw error if error?
-							expect(data.ranges.changes).to.be.undefined
-							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) ->
+							setTimeout done, 200
+				
+				it "should update the ranges", (done) ->
+					DocUpdaterClient.getDoc @project_id, @doc.id, (error, res, data) =>
 						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()
+						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) ->
+			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?
-						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()
+						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

@@ -189,95 +189,6 @@ describe "ShareJS text type", ->
 				dest = []
 				text._tc(dest, { c: "foo", p: 6 }, { c: "bar", p: 3 })
 				dest.should.deep.equal [{ c: "foo", p: 6 }]
-	
-		describe "comment / delete_range", ->
-			it "should not do anything", ->
-				dest = []
-				text._tc(dest, { c: "foo", p: 6 }, { dr: "bar", p: 3 })
-				dest.should.deep.equal [{ c: "foo", p: 6 }]
-		
-		describe "delete_range / insert", ->
-			it "with an insert before", ->
-				dest = []
-				text._tc(dest, { dr: "foo", p: 9 }, { i: "bar", p: 3 })
-				dest.should.deep.equal [{ dr: "foo", p: 12 }]
-
-			it "with an insert after", ->
-				dest = []
-				text._tc(dest, { dr: "foo", p: 3 }, { i: "bar", p: 9 })
-				dest.should.deep.equal [{ dr: "foo", p: 3 }]
-
-			it "with an insert at the left edge", ->
-				dest = []
-				text._tc(dest, { dr: "foo", p: 3 }, { i: "bar", p: 3 })
-				# RangesTracker doesn't inject inserts into comments on edges, so neither should we
-				dest.should.deep.equal [{ dr: "foo", p: 6 }]
-
-			it "with an insert at the right edge", ->
-				dest = []
-				text._tc(dest, { dr: "foo", p: 3 }, { i: "bar", p: 6 })
-				# RangesTracker doesn't inject inserts into comments on edges, so neither should we
-				dest.should.deep.equal [{ dr: "foo", p: 3 }]
-
-			it "with an insert in the middle", ->
-				dest = []
-				text._tc(dest, { dr: "foo", p: 3 }, { i: "bar", p: 5 })
-				dest.should.deep.equal [{ dr: "fobaro", p: 3 }]
-	
-		describe "delete_range / delete", ->
-			it "with a delete before", ->
-				dest = []
-				text._tc(dest, { dr: "foo", p: 9 }, { d: "bar", p: 3 })
-				dest.should.deep.equal [{ dr: "foo", p: 6 }]
-
-			it "with a delete after", ->
-				dest = []
-				text._tc(dest, { dr: "foo", p: 3 }, { i: "bar", p: 9 })
-				dest.should.deep.equal [{ dr: "foo", p: 3 }]
-
-			it "with a delete overlapping the comment content before", ->
-				dest = []
-				text._tc(dest, { dr: "foobar", p: 6 }, { d: "123foo", p: 3 })
-				dest.should.deep.equal [{ dr: "bar", p: 3 }]
-
-			it "with a delete overlapping the comment content after", ->
-				dest = []
-				text._tc(dest, { dr: "foobar", p: 6 }, { d: "bar123", p: 9 })
-				dest.should.deep.equal [{ dr: "foo", p: 6 }]
-
-			it "with a delete overlapping the comment content in the middle", ->
-				dest = []
-				text._tc(dest, { dr: "foo123bar", p: 6 }, { d: "123", p: 9 })
-				dest.should.deep.equal [{ dr: "foobar", p: 6 }]
-
-			it "with a delete overlapping the whole comment", ->
-				dest = []
-				text._tc(dest, { dr: "foo", p: 6 }, { d: "123foo456", p: 3 })
-				dest.should.deep.equal [{ dr: "", p: 3 }]
-	
-		describe "delete_range / insert", ->
-			it "should not do anything", ->
-				dest = []
-				text._tc(dest, { i: "foo", p: 6 }, { dr: "bar", p: 3 })
-				dest.should.deep.equal [{ i: "foo", p: 6 }]
-	
-		describe "delete_range / delete", ->
-			it "should not do anything", ->
-				dest = []
-				text._tc(dest, { d: "foo", p: 6 }, { dr: "bar", p: 3 })
-				dest.should.deep.equal [{ d: "foo", p: 6 }]
-	
-		describe "delete_range / comment", ->
-			it "should not do anything", ->
-				dest = []
-				text._tc(dest, { c: "foo", p: 6 }, { dr: "bar", p: 3 })
-				dest.should.deep.equal [{ c: "foo", p: 6 }]
-	
-		describe "delete_range / delete_range", ->
-			it "should not do anything", ->
-				dest = []
-				text._tc(dest, { dr: "foo", p: 6 }, { dr: "bar", p: 3 })
-				dest.should.deep.equal [{ dr: "foo", p: 6 }]
 
 	describe "apply", ->
 		it "should apply an insert", ->
@@ -288,9 +199,6 @@ describe "ShareJS text type", ->
 
 		it "should do nothing with a comment", ->
 			text.apply("foo123bar", [{ c: "123", p: 3 }]).should.equal "foo123bar"
-
-		it "should do nothing with a delete_range", ->
-			text.apply("foo123bar", [{ dr: "123", p: 3 }]).should.equal "foo123bar"
 		
 		it "should throw an error when deleted content does not match", ->
 			(() ->