Only cancel deletes with inserts on undo and reject

services/web/public/coffee/ide/editor/directives/aceEditor/track-changes/TrackChangesManager.coffee

@@ -210,14 +210,18 @@ define [
 			if change.op.d?
 				content = change.op.d
 				position = @_shareJsOffsetToAcePosition(change.op.p)
+				session.$fromReject = true # Tell track changes to cancel out delete
 				session.insert(position, content)
+				session.$fromReject = false
 			else if change.op.i?
 				start = @_shareJsOffsetToAcePosition(change.op.p)
 				end = @_shareJsOffsetToAcePosition(change.op.p + change.op.i.length)
 				editor_text = session.getDocument().getTextRange({start, end})
 				if editor_text != change.op.i
 					throw new Error("Op to be removed (#{JSON.stringify(change.op)}), does not match editor text, '#{editor_text}'")
+				session.$fromReject = true
 				session.remove({start, end})
+				session.$fromReject = false
 			else
 				throw new Error("unknown change: #{JSON.stringify(change)}")
 

services/web/public/coffee/ide/editor/sharejs/vendor/client/ace.coffee

@@ -3,7 +3,7 @@
 Range = ace.require("ace/range").Range
 
 # Convert an ace delta into an op understood by share.js
-applyToShareJS = (editorDoc, delta, doc) ->
+applyToShareJS = (editorDoc, delta, doc, fromUndo) ->
   # Get the start position of the range, in no. of characters
   getStartOffsetPosition = (start) ->
     # This is quite inefficient - getLines makes a copy of the entire
@@ -27,11 +27,11 @@ applyToShareJS = (editorDoc, delta, doc) ->
   switch delta.action
     when 'insert'
       text = delta.lines.join('\n')
-      doc.insert pos, text
+      doc.insert pos, text, fromUndo
       
     when 'remove'
       text = delta.lines.join('\n')
-      doc.del pos, text.length
+      doc.del pos, text.length, fromUndo
 
     else throw new Error "unknown action: #{delta.action}"
   
@@ -78,8 +78,10 @@ window.sharejs.extendDoc 'attach_ace', (editor, keepEditorContents, maxDocLength
     if maxDocLength? and editorDoc.getValue().length > maxDocLength
         doc.emit "error", new Error("document length is greater than maxDocLength")
         return
+
+    fromUndo = !!(editor.getSession().$fromUndo or editor.getSession().$fromReject)
     
-    applyToShareJS editorDoc, change, doc
+    applyToShareJS editorDoc, change, doc, fromUndo
 
     check()
 

services/web/public/coffee/ide/editor/sharejs/vendor/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

services/web/public/coffee/ide/editor/sharejs/vendor/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

services/web/public/coffee/ide/review-panel/RangesTracker.coffee

@@ -172,6 +172,7 @@ load = () ->
 			op_start = op.p
 			op_length = op.i.length
 			op_end = op.p + op_length
+			undoing = !!op.u
 
 
 			already_merged = false
@@ -189,8 +190,9 @@ load = () ->
 						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 == ""
@@ -208,15 +210,15 @@ load = () ->
 					# 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.
@@ -513,4 +515,4 @@ load = () ->
 if define?
 	define [], load
 else
-	module.exports = load()
+	module.exports = load()