Update entries in place and position via DOM attributes directly

services/web/app/views/project/editor/editor.jade

@@ -55,11 +55,9 @@ div.full-size(
 					)
 						.rp-entry-callout(
 							ng-class="'rp-entry-callout-' + entry.type"
-							style="top: {{ entry.screenPos.y + 15 }}px; height: {{ top - entry.screenPos.y }}px"
 						)
 						.rp-entry(
 							ng-class="'rp-entry-' + entry.type"
-							ng-style="{'top': top}"
 						)
 							div(ng-if="entry.type == 'insert' || entry.type == 'delete'")
 								.rp-entry-header

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

@@ -283,7 +283,10 @@ define [
 						moved_changes.push change
 					else if op_start <= change_start <= op_end
 						if @track_changes
-							# If we overlap a delete, add it in our content, and delete the existing change
+							# If we overlap a delete, add it in our content, and delete the existing change.
+							# It's easier to do it this way, rather than modifying the existing delete in case
+							# we overlap many deletes and we'd need to track that. We have a workaround to
+							# update the delete in place if possible below.
 							offset = change_start - op_start
 							op_modifications.push { i: change.op.d, p: offset }
 							remove_changes.push change
@@ -291,14 +294,24 @@ define [
 							change.op.p = op_start
 							moved_changes.push change
 
-			for change in remove_changes
-				@_removeChange change
-			
 			# Copy rather than modify because we still need to apply it to comments
 			op = {
 				p: op.p
 				d: @_applyOpModifications(op.d, op_modifications)
 			}
+
+			for change in remove_changes
+				# This is a bit of hack to avoid removing one delete and replacing it with another.
+				# If we don't do this, it causes the UI to flicker
+				if op.d.length > 0 and change.op.d? and op.p <= change.op.p <= op.p + op.d.length
+					change.op.p = op.p
+					change.op.d = op.d
+					change.metadata = metadata
+					moved_changes.push change
+					op.d = "" # stop it being added
+				else
+					@_removeChange change
+
 			if @track_changes and op.d.length > 0
 				@_addOp op, metadata
 			else
@@ -388,5 +401,11 @@ define [
 						remove_changes.push change
 						previous_change.op.i += change.op.i
 						moved_changes.push previous_change
-				previous_change = change
+				else if previous_change?.op.d? and change.op.d? and previous_change.op.p == change.op.p
+					# Merge adjacent deletes
+					previous_change.op.d += change.op.d
+					remove_changes.push change
+					moved_changes.push previous_change
+				else # Only update to the current change if we haven't removed it.
+					previous_change = change
 			return { moved_changes, remove_changes }

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

@@ -35,9 +35,16 @@ define [
 				sl_console.log "[comment:moved]", comment
 				@_onCommentMoved(comment)
 			
-			@editor.on "changeSelection", (e) =>
-				# TODO: Make this only update focus when we get a more optimised updating system
-				@updateReviewEntriesScope()
+			changingSelection = false
+			@editor.on "changeSelection", (args...) =>
+				# Deletes can send about 5 changeSelection events, so
+				# just act on the last one.
+				if !changingSelection
+					changingSelection = true
+					@$scope.$evalAsync () =>
+						changingSelection = false
+						@updateFocus()
+						@recalculateReviewEntriesScreenPositions()
 			
 			@$scope.$on "comment:add", (e, comment) =>
 				@addCommentToSelection(comment)
@@ -189,30 +196,54 @@ define [
 			@changesTracker.applyOp(op, metadata)
 		
 		updateReviewEntriesScope: () ->
-			# TODO: Update in place so Angular doesn't have to redo EVERYTHING
-			@$scope.reviewPanel.entries = {}
+			# Assume we'll delete everything until we see it, then we'll remove it from this object
+			delete_changes = {}
+			delete_changes[change_id] = true for change_id, change of @$scope.reviewPanel.entries
+
 			for change in @changesTracker.changes
-				@$scope.reviewPanel.entries[change.id] = {
+				delete delete_changes[change.id]
+				@$scope.reviewPanel.entries[change.id] ?= {}
+					
+				# Update in place to avoid a full DOM redraw via angular
+				metadata = {}
+				metadata[key] = value for key, value of change.metadata
+				new_entry = {
 					type: if change.op.i then "insert" else "delete"
 					content: change.op.i or change.op.d
 					offset: change.op.p
 					metadata: change.metadata
 				}
+				for key, value of new_entry
+					@$scope.reviewPanel.entries[change.id][key] = value
+
 			for comment in @changesTracker.comments
-				@$scope.reviewPanel.entries[comment.id] = {
+				delete delete_changes[comment.id]
+				@$scope.reviewPanel.entries[comment.id] ?= {}
+				new_entry = {
 					type: "comment"
 					thread: comment.metadata.thread
 					offset: comment.offset
 				}
+				for key, value of new_entry
+					@$scope.reviewPanel.entries[comment.id][key] = value
+
+			for change_id, _ of delete_changes
+				delete @$scope.reviewPanel.entries[change_id]	
+			
 			@updateFocus()
 			@recalculateReviewEntriesScreenPositions()
 		
 		updateFocus: () ->
+			@updateEntryGeneration()
 			@$scope.reviewPanel.entries["focus-position"] = {
 				type: "focus-position"
 				offset: @_aceRangeToShareJs(@editor.getSelectionRange().start)
 			}
-			@recalculateReviewEntriesScreenPositions()
+		
+		updateEntryGeneration: () ->
+			# Rather than making angular deep watch the whole entries array
+			@$scope.reviewPanel.entryGeneration ?= 0
+			@$scope.reviewPanel.entryGeneration++
 		
 		recalculateReviewEntriesScreenPositions: () ->
 			session = @editor.getSession()
@@ -221,7 +252,8 @@ define [
 				doc_position = @_shareJsOffsetToAcePosition(entry.offset)
 				screen_position = session.documentToScreenPosition(doc_position.row, doc_position.column)
 				y = screen_position.row * renderer.lineHeight
-				entry.screenPos = { y }
+				entry.screenPos ?= {}
+				entry.screenPos.y = y
 
 			@$scope.$apply()
 

services/web/public/coffee/ide/review-panel/directives/reviewPanelSorted.coffee

@@ -6,23 +6,24 @@ define [
 			link: (scope, element, attrs) ->
 				layout = () ->
 					entries = []
-					for el in element.find(".rp-entry")
+					for el in element.find(".rp-entry-wrapper")
 						entries.push {
-							el: el
+							$box_el: $(el).find(".rp-entry")
+							$callout_el: $(el).find(".rp-entry-callout")
 							scope: angular.element(el).scope()
 						}
 					entries.sort (a,b) -> a.scope.entry.offset - b.scope.entry.offset
 					
 					previousBottom = 28 # This should start at the height of the toolbar
 					for entry in entries
-						height = $(entry.el).height()
-						top = entry.scope.entry.screenPos.y
-						top = Math.max(top, previousBottom + 12)
+						height = entry.$box_el.height()
+						original_top = entry.scope.entry.screenPos.y
+						top = Math.max(original_top, previousBottom + 12)
 						previousBottom = top + height
-						entry.scope.top = top
+						entry.$box_el.css(top: top)
+						entry.$callout_el.css(top: original_top + 15, height: top - original_top)
 				
-				scope.$watch "reviewPanel.entries", (value) ->
-					return if !value?
+				scope.$watch "reviewPanel.entryGeneration", (value) ->
 					scope.$evalAsync () ->
 						layout()