Create comments via comment ops

services/web/public/coffee/ide/editor/Document.coffee

@@ -42,6 +42,8 @@ define [
 			editorDoc?.off "change", @_checkConsistency
 			@ide.$scope.$emit 'document:closed', @doc
 		
+		submitOp: (args...) -> @doc?.submitOp(args...)
+
 		_checkConsistency: () ->
 			# We've been seeing a lot of errors when I think there shouldn't be
 			# any, which may be related to this check happening before the change is

services/web/public/coffee/ide/editor/ShareJsDoc.coffee

@@ -61,6 +61,7 @@ define [
 				@_doc._onMessage message
 			catch error
 				# Version mismatches are thrown as errors
+				console.log error
 				@_handleError(error)
 
 			if message?.meta?.type == "external"

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

@@ -20,8 +20,8 @@ define [
 				@rangesTracker = doc.ranges
 				@connectToRangesTracker()
 			
-			@$scope.$on "comment:add", (e, comment) =>
+			@$scope.$on "comment:add", (e) =>
-				@addCommentToSelection(comment)
+				@addCommentToSelection()
 
 			@$scope.$on "comment:select_line", (e) =>
 				@selectLineIfNoSelection()
@@ -146,21 +146,16 @@ define [
 			for comment in @rangesTracker.comments
 				@_onCommentAdded(comment)
 
-		addComment: (offset, length, content) ->
+		addComment: (offset, content) ->
-			@rangesTracker.addComment offset, length, {
+			op = { c: content, p: offset }
-				thread: [{
+			# @rangesTracker.applyOp op # Will apply via sharejs
-					content: content
+			@$scope.sharejsDoc.submitOp op
-					user_id: window.user_id
-					ts: new Date()
-				}]
-			}
 		
-		addCommentToSelection: (content) ->
+		addCommentToSelection: () ->
 			range = @editor.getSelectionRange()
+			content = @editor.getSelectedText()
 			offset = @_aceRangeToShareJs(range.start)
-			end = @_aceRangeToShareJs(range.end)
+			@addComment(offset, content)
-			length = end - offset
-			@addComment(offset, length, content)
 		
 		selectLineIfNoSelection: () ->
 			if @editor.selection.isEmpty()
@@ -201,6 +196,7 @@ define [
 			@rangesTracker.unresolveCommentId(comment_id)
 
 		checkMapping: () ->
+			# TODO: reintroduce this check
 			session = @editor.getSession()
 
 			# Make a copy of session.getMarkers() so we can modify it
@@ -224,8 +220,8 @@ define [
 			for comment in @rangesTracker.comments
 				if @changeIdToMarkerIdMap[comment.id]?
 					{background_marker_id, callout_marker_id} = @changeIdToMarkerIdMap[comment.id]
-					start = @_shareJsOffsetToAcePosition(comment.offset)
+					start = @_shareJsOffsetToAcePosition(comment.op.p)
-					end = @_shareJsOffsetToAcePosition(comment.offset + comment.length)
+					end = @_shareJsOffsetToAcePosition(comment.op.p + comment.op.c.length)
 					expected_markers.push { marker_id: background_marker_id, start, end }
 					expected_markers.push { marker_id: callout_marker_id, start, end: start }
 			
@@ -341,8 +337,8 @@ define [
 		_onCommentAdded: (comment) ->
 			if !@changeIdToMarkerIdMap[comment.id]?
 				# Only create new markers if they don't already exist
-				start = @_shareJsOffsetToAcePosition(comment.offset)
+				start = @_shareJsOffsetToAcePosition(comment.op.p)
-				end = @_shareJsOffsetToAcePosition(comment.offset + comment.length)
+				end = @_shareJsOffsetToAcePosition(comment.op.p + comment.op.c.length)
 				session = @editor.getSession()
 				doc = session.getDocument()
 				background_range = new Range(start.row, start.column, end.row, end.column)
@@ -387,8 +383,8 @@ define [
 			@broadcastChange()
 		
 		_onCommentMoved: (comment) ->
-			start = @_shareJsOffsetToAcePosition(comment.offset)
+			start = @_shareJsOffsetToAcePosition(comment.op.p)
-			end = @_shareJsOffsetToAcePosition(comment.offset + comment.length)
+			end = @_shareJsOffsetToAcePosition(comment.op.p + comment.op.c.length)
 			@_updateMarker(comment.id, start, end)
 			@editor.renderer.updateBackMarkers()
 			@broadcastChange()

services/web/public/coffee/ide/editor/sharejs/vendor/types/text-api.coffee

@@ -28,5 +28,5 @@ text.api =
       for component in op
         if component.i != undefined
           @emit 'insert', component.p, component.i
-        else
+        else if component.d != undefined
           @emit 'delete', component.p, component.d

services/web/public/coffee/ide/review-panel/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
@@ -97,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?
@@ -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
+				if op.p <= comment.op.p
-					comment.offset += op.i.length
+					comment.op.p += op.i.length
 					@emit "comment:moved", comment
-				else if op.p < comment.offset + comment.length
+				else if op.p < comment.op.p + comment.op.c.length
-					comment.length += op.i.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
+				comment_start = comment.op.p
-				if op_end <= comment.offset
+				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)
+					if op_start <= comment_start
-					delete_length_after = Math.max(0, op_end - comment_end)
+						remaining_before = ""
-					delete_length_overlapping = op_length - delete_length_before - delete_length_after
+					else
-					comment.offset = Math.min(comment.offset, op_start)
+						remaining_before = comment.op.c.slice(0, op_start - comment_start)
-					comment.length -= delete_length_overlapping
+					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/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee

@@ -218,11 +218,11 @@ define [
 				entries[comment.id] ?= {}
 				new_entry = {
 					type: "comment"
-					thread: comment.metadata.thread
+					thread: comment.metadata.thread or []
-					resolved: comment.metadata.resolved
+					resolved: comment.metadata?.resolved
-					resolved_data: comment.metadata.resolved_data
+					resolved_data: comment.metadata?.resolved_data
-					offset: comment.offset
+					content: comment.op.c
-					length: comment.length
+					offset: comment.op.p
 				}
 				for key, value of new_entry
 					entries[comment.id][key] = value