Просмотр исходного кода

Merge pull request #343 from sharelatex/ja-review-panel

Ja review panel
James Allen 10 лет назад
Родитель
Сommit
67aabdd200
20 измененных файлов с 675 добавлено и 347 удалено
  1. 2 2
      services/web/app/views/project/editor.jade
  2. 12 3
      services/web/app/views/project/editor/editor.jade
  3. 7 0
      services/web/public/coffee/ide.coffee
  4. 3 2
      services/web/public/coffee/ide/chat/controllers/ChatMessageController.coffee
  5. 44 0
      services/web/public/coffee/ide/colors/ColorManager.coffee
  6. 2 2
      services/web/public/coffee/ide/editor/Document.coffee
  7. 2 2
      services/web/public/coffee/ide/editor/ShareJsDoc.coffee
  8. 25 3
      services/web/public/coffee/ide/editor/directives/aceEditor.coffee
  9. 3 28
      services/web/public/coffee/ide/editor/directives/aceEditor/highlights/HighlightsManager.coffee
  10. 330 0
      services/web/public/coffee/ide/editor/directives/aceEditor/track-changes/ChangesTracker.coffee
  11. 84 270
      services/web/public/coffee/ide/editor/directives/aceEditor/track-changes/TrackChangesManager.coffee
  12. 4 4
      services/web/public/coffee/ide/editor/sharejs/vendor/client/doc.coffee
  13. 5 4
      services/web/public/coffee/ide/history/HistoryManager.coffee
  14. 4 19
      services/web/public/coffee/ide/online-users/OnlineUsersManager.coffee
  15. 4 0
      services/web/public/coffee/ide/review-panel/ReviewPanelManager.coffee
  16. 56 0
      services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee
  17. 24 0
      services/web/public/coffee/ide/review-panel/directives/reviewPanelSorted.coffee
  18. 3 8
      services/web/public/stylesheets/app/editor.less
  19. 6 0
      services/web/public/stylesheets/app/editor/chat.less
  20. 55 0
      services/web/public/stylesheets/app/editor/review-panel.less

+ 2 - 2
services/web/app/views/project/editor.jade

@@ -38,7 +38,7 @@ block content
 
 		include ./editor/left-menu
 
-		#chat-wrapper(
+		#chat-wrapper.full-size(
 			layout="chat",
 			spacing-open="12",
 			spacing-closed="0",
@@ -143,4 +143,4 @@ block requirejs
 		src=buildJsPath('libs/require.js')
 	)
 
-	
+	

+ 12 - 3
services/web/app/views/project/editor/editor.jade

@@ -8,7 +8,7 @@ div.full-size(
 	initial-size-east="'50%'"
 	minimum-restore-size-east="300"
 )
-	.ui-layout-center
+	.ui-layout-center(ng-controller="ReviewPanelController", ng-class="{'has-review-panel': ui.reviewPanelOpen}")
 		.loading-panel(ng-show="!editor.sharejs_doc || editor.opening")
 			span(ng-show="editor.open_doc_id")
 				i.fa.fa-spin.fa-refresh
@@ -32,13 +32,22 @@ div.full-size(
 			last-updated="editor.last_updated",
 			cursor-position="editor.cursorPosition",
 			goto-line="editor.gotoLine",
-			resize-on="layout:main:resize,layout:pdf:resize",
+			resize-on="layout:main:resize,layout:pdf:resize,layout:review:resize,reviewPanel:toggle",
 			annotations="pdf.logEntryAnnotations[editor.open_doc_id]",
 			read-only="!permissions.write",
 			file-name="editor.open_doc_name",
 			on-ctrl-enter="recompileViaKey",
-			syntax-validation="settings.syntaxValidation"
+			syntax-validation="settings.syntaxValidation",
+			review-panel="reviewPanel",
+			on-scroll="onScroll",
+			scroll-events="scrollEvents",
+			track-changes-enabled="true"
 		)
+		#review-panel
+			.review-panel-scroller
+				.review-entry-list(review-panel-sorted)
+					.review-entry(ng-repeat="(entry_id, entry) in reviewPanel.entries", ng-style="{'top': top}")
+						{{ entry.content }}
 
 	.ui-layout-east
 		div(ng-if="ui.pdfLayout == 'sideBySide'")

+ 7 - 0
services/web/public/coffee/ide.coffee

@@ -9,6 +9,7 @@ define [
 	"ide/pdf/PdfManager"
 	"ide/binary-files/BinaryFilesManager"
 	"ide/references/ReferencesManager"
+	"ide/review-panel/ReviewPanelManager"
 	"ide/SafariScrollPatcher"
 	"ide/FeatureOnboardingController"
 	"ide/settings/index"
@@ -42,6 +43,7 @@ define [
 	PdfManager
 	BinaryFilesManager
 	ReferencesManager
+	ReviewPanelManager
 	SafariScrollPatcher
 ) ->
 
@@ -65,6 +67,7 @@ define [
 			view: "editor"
 			chatOpen: false
 			pdfLayout: 'sideBySide'
+			reviewPanelOpen: false
 			showCodeCheckerOnboarding: !window.userSettings.syntaxValidation?
 		}
 		$scope.user = window.user
@@ -79,6 +82,10 @@ define [
 
 		$scope.chat = {}
 
+		ide.toggleReviewPanel = () ->
+			$scope.ui.reviewPanelOpen = !$scope.ui.reviewPanelOpen
+			$scope.$digest()
+
 		# Only run the header AB test for newly registered users.
 		_abTestStartDate = new Date(Date.UTC(2016, 8, 28))
 		_userSignUpDate = new Date(window.user.signUpDate)

+ 3 - 2
services/web/public/coffee/ide/chat/controllers/ChatMessageController.coffee

@@ -1,7 +1,8 @@
 define [
 	"base"
-], (App) ->
+	"ide/colors/ColorManager"
+], (App, ColorManager) ->
 	App.controller "ChatMessageController", ["$scope", "ide", ($scope, ide) ->
 		$scope.hue = (user) ->
-			ide.onlineUsersManager.getHueForUserId(user.id)
+			ColorManager.getHueForUserId(user.id)
 	]

+ 44 - 0
services/web/public/coffee/ide/colors/ColorManager.coffee

@@ -0,0 +1,44 @@
+define [], () ->
+	ColorManager =
+		getColorScheme: (hue, element) ->
+			if @isDarkTheme(element)
+				return {
+					cursor: "hsl(#{hue}, 70%, 50%)"
+					labelBackgroundColor: "hsl(#{hue}, 70%, 50%)"
+					highlightBackgroundColor: "hsl(#{hue}, 100%, 28%);"
+					strikeThroughBackgroundColor: "hsl(#{hue}, 100%, 20%);"
+					strikeThroughForegroundColor: "hsl(#{hue}, 100%, 60%);"
+				}
+			else
+				return {
+					cursor: "hsl(#{hue}, 70%, 50%)"
+					labelBackgroundColor: "hsl(#{hue}, 70%, 50%)"
+					highlightBackgroundColor: "hsl(#{hue}, 70%, 85%);"
+					strikeThroughBackgroundColor: "hsl(#{hue}, 70%, 95%);"
+					strikeThroughForegroundColor: "hsl(#{hue}, 70%, 40%);"
+				}
+
+		isDarkTheme: (element) ->
+			rgb = element.find(".ace_editor").css("background-color");
+			[m, r, g, b] = rgb.match(/rgb\(([0-9]+), ([0-9]+), ([0-9]+)\)/)
+			r = parseInt(r, 10)
+			g = parseInt(g, 10)
+			b = parseInt(b, 10)
+			return r + g + b < 3 * 128
+		
+		OWN_HUE: 200 # We will always appear as this color to ourselves
+		ANONYMOUS_HUE: 100
+		getHueForUserId: (user_id) ->
+			if !user_id? or user_id == "anonymous-user"
+				return @ANONYMOUS_HUE
+
+			if window.user.id == user_id
+				return @OWN_HUE
+
+			hash = CryptoJS.MD5(user_id)
+			hue = parseInt(hash.toString().slice(0,8), 16) % 320
+			# Avoid 20 degrees either side of the personal hue
+			if hue > @OWNER_HUE - 20
+				hue = hue + 40
+			return hue
+		

+ 2 - 2
services/web/public/coffee/ide/editor/Document.coffee

@@ -265,10 +265,10 @@ define [
 				@ide.pushEvent "externalUpdate",
 					doc_id: @doc_id
 				@trigger "externalUpdate", update
-			@doc.on "remoteop", () => 
+			@doc.on "remoteop", (args...) => 
 				@ide.pushEvent "remoteop",
 					doc_id: @doc_id
-				@trigger "remoteop"
+				@trigger "remoteop", args...
 			@doc.on "op:sent", (op) =>
 				@ide.pushEvent "op:sent",
 					doc_id: @doc_id

+ 2 - 2
services/web/public/coffee/ide/editor/ShareJsDoc.coffee

@@ -47,11 +47,11 @@ define [
 				@trigger "change"
 			@_doc.on "acknowledge", () =>
 				@trigger "acknowledge"
-			@_doc.on "remoteop", () =>
+			@_doc.on "remoteop", (args...) =>
 				# As soon as we're working with a collaborator, start sending
 				# ops as quickly as possible for low latency.
 				@_doc.setFlushDelay(0)
-				@trigger "remoteop"
+				@trigger "remoteop", args...
 			@_doc.on "error", (e) =>
 				@_handleError(e)
 

+ 25 - 3
services/web/public/coffee/ide/editor/directives/aceEditor.coffee

@@ -52,6 +52,10 @@ define [
 				fileName: "="
 				onCtrlEnter: "="
 				syntaxValidation: "="
+				reviewPanel: "="
+				onScroll: "="
+				scrollEvents: "="
+				trackChangesEnabled: "="
 			}
 			link: (scope, element, attrs) ->
 				# Don't freak out if we're already in an apply callback
@@ -83,7 +87,7 @@ define [
 				highlightsManager     = new HighlightsManager(scope, editor, element)
 				cursorPositionManager = new CursorPositionManager(scope, editor, element, localStorage)
 				trackChangesManager   = new TrackChangesManager(scope, editor, element)
-				if window.location.search.match /tcon=true/ # track changes on
+				if scope.trackChangesEnabled and window.location.search.match /tcon=true/ # track changes on
 					trackChangesManager.enabled = true
 
 				# Prevert Ctrl|Cmd-S from triggering save dialog
@@ -221,6 +225,15 @@ define [
 					if updateCount == 100
 						event_tracking.send 'editor-interaction', 'multi-doc-update'
 					scope.$emit "#{scope.name}:change"
+				
+				onScroll = (scrollTop) ->
+					return if !scope.onScroll?
+					height = editor.renderer.layerConfig.maxHeight
+					scope.onScroll(scrollTop, height)
+					
+				if scope.scrollEvents?
+					scope.scrollEvents.on "scroll", (position) ->
+						editor.getSession().setScrollTop(position)
 
 				attachToAce = (sharejs_doc) ->
 					lines = sharejs_doc.getSnapshot().split("\n")
@@ -261,23 +274,32 @@ define [
 					doc = session.getDocument()
 					doc.on "change", onChange
 
-					sharejs_doc.on "remoteop.recordForUndo", () =>
+					sharejs_doc.on "remoteop.recordRemote", (op, oldSnapshot, msg) ->
 						undoManager.nextUpdateIsRemote = true
+						trackChangesManager.nextUpdateMetaData = msg?.meta
 
 					editor.initing = true
 					sharejs_doc.attachToAce(editor)
 					editor.initing = false
+
 					# need to set annotations after attaching because attaching
 					# deletes and then inserts document content
 					session.setAnnotations scope.annotations
+					
+					session.on "changeScrollTop", onScroll
+					setTimeout () ->
+						# Let any listeners init themselves
+						onScroll(editor.renderer.getScrollTop())
 
 					editor.focus()
 
 				detachFromAce = (sharejs_doc) ->
 					sharejs_doc.detachFromAce()
-					sharejs_doc.off "remoteop.recordForUndo"
+					sharejs_doc.off "remoteop.recordRemote"
 
 					session = editor.getSession()
+					session.off "changeScrollTop"
+					
 					doc = session.getDocument()
 					doc.off "change", onChange
 

+ 3 - 28
services/web/public/coffee/ide/editor/directives/aceEditor/highlights/HighlightsManager.coffee

@@ -1,6 +1,7 @@
 define [
 	"ace/ace"
-], () ->
+	"ide/colors/ColorManager"
+], (_, ColorManager) ->
 	Range = ace.require("ace/range").Range
 
 	class HighlightsManager
@@ -64,7 +65,7 @@ define [
 
 			for annotation in @$scope.highlights or []
 				do (annotation) =>
-					colorScheme = @_getColorScheme(annotation.hue)
+					colorScheme = ColorManager.getColorScheme(annotation.hue, @element)
 					if annotation.cursor?
 						@labels.push {
 							text: annotation.label
@@ -262,29 +263,3 @@ define [
 				else
 					markerLayer.drawSingleLineMarker(html, range, "#{klass} ace_start", config, 0, style)
 			, foreground
-
-		_getColorScheme: (hue) ->
-			if @_isDarkTheme()
-				return {
-					cursor: "hsl(#{hue}, 70%, 50%)"
-					labelBackgroundColor: "hsl(#{hue}, 70%, 50%)"
-					highlightBackgroundColor: "hsl(#{hue}, 100%, 28%);"
-					strikeThroughBackgroundColor: "hsl(#{hue}, 100%, 20%);"
-					strikeThroughForegroundColor: "hsl(#{hue}, 100%, 60%);"
-				}
-			else
-				return {
-					cursor: "hsl(#{hue}, 70%, 50%)"
-					labelBackgroundColor: "hsl(#{hue}, 70%, 50%)"
-					highlightBackgroundColor: "hsl(#{hue}, 70%, 85%);"
-					strikeThroughBackgroundColor: "hsl(#{hue}, 70%, 95%);"
-					strikeThroughForegroundColor: "hsl(#{hue}, 70%, 40%);"
-				}
-
-		_isDarkTheme: () ->
-			rgb = @element.find(".ace_editor").css("background-color");
-			[m, r, g, b] = rgb.match(/rgb\(([0-9]+), ([0-9]+), ([0-9]+)\)/)
-			r = parseInt(r, 10)
-			g = parseInt(g, 10)
-			b = parseInt(b, 10)
-			return r + g + b < 3 * 128

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

@@ -0,0 +1,330 @@
+define [
+	"utils/EventEmitter"
+], (EventEmitter) ->
+	class ChangesTracker extends EventEmitter
+		# The purpose of this class is to track a set of inserts and deletes to a document, like
+		# track changes in Word. We store these as a set of ShareJs style ranges:
+		#   {i: "foo", p: 42} # Insert 'foo' at offset 42
+		#   {d: "bar", p: 37} # Delete 'bar' at offset 37
+		# We only track the inserts and deletes, not the whole document, but by being given all
+		# updates that are applied to a document, we can update these appropriately.
+		# 
+		# Note that the set of inserts and deletes we store applies to the document as-is at the moment.
+		# So inserts correspond to text which is in the document, while deletes correspond to text which
+		# is no longer there, so their lengths do not affect the position of later offsets.
+		# E.g.
+		#             this is the current text of the document
+		#                         |-----|            |
+		#  {i: "current ", p:12} -^                   ^- {d: "old ", p: 31}
+		#
+		# Track changes rules (should be consistent with Word):
+		#   * When text is inserted at a delete, the text goes to the left of the delete
+		#       I.e. "foo|bar" -> "foobaz|bar", where | is the delete, and 'baz' is inserted
+		#   * Deleting content flagged as 'inserted' does not create a new delete marker, it only
+		#     removes the insert marker. E.g.
+		#       * "abdefghijkl"        -> "abfghijkl"        when 'de' is deleted. No delete marker added
+		#           |---| <- inserted       |-| <- inserted
+		#       * Deletes overlapping regular text and inserted text will insert a delete marker for the
+		#         regular text:
+		#         "abcdefghijkl"    ->    "abcdejkl"   when 'fghi' is deleted
+		#           |----|                  |--||
+		#           ^- inserted 'bcdefg'      \ ^- deleted 'hi'
+		#                                      \--inserted 'bcde'
+		#   * Deletes overlapping other deletes are merged. E.g.
+		#      "abcghijkl"        ->   "ahijkl"     when 'bcg is deleted'
+		#          | <- delete 'def'     | <- delete 'bcdefg'
+		#   * Deletes by another user will consume deletes by the first user
+		#   * 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: () ->
+			# 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.
+			@changes = []
+			@id = 0
+		
+		applyOp: (op, metadata) ->
+			# Apply an op that has been applied to the document to our changes to keep them up to date
+			if op.i?
+				@applyInsert(op, metadata)
+			else if op.d?
+				@applyDelete(op, metadata)
+			
+		applyInsert: (op, metadata) ->
+			op_start = op.p
+			op_length = op.i.length
+			op_end = op.p + op_length
+
+			already_merged = false
+			previous_change = null
+			moved_changes = []
+			new_changes = []
+			for change in @changes
+				change_start = change.op.p
+				
+				if change.op.d?
+					# Shift any deletes after this along by the length of this insert
+					if op_start <= change_start
+						change.op.p += op_length
+						moved_changes.push change
+				else if change.op.i?
+					change_end = change_start + change.op.i.length
+					is_change_overlapping = (op_start >= change_start and op_start <= change_end)
+					
+					# Only merge inserts if they are from the same user
+					is_same_user = metadata.user_id == change.metadata.user_id
+					
+					# 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.
+					# The previous op will be the delete, but it's already been shifted by this insert
+					#
+					# I.e.
+					# Originally: |-- existing insert --|
+					#             | <- existing delete at same offset
+					#
+					# Now:        |-- existing insert --| <- not shifted yet
+					#             |-- this insert --|| <- existing delete shifted along to end of this op
+					# 
+					# After:                         |-- existing insert --|  
+					#             |-- this insert --|| <- existing delete
+					# 
+					# Without the delete, the inserts would be merged.
+					is_insert_blocked_by_delete = (previous_change? and previous_change.op.d? and previous_change.op.p == op_end)
+
+					# If the insert is overlapping another insert, either at the beginning in the middle or touching the end,
+					# then we merge them into one.
+					if @track_changes and
+							is_change_overlapping and
+							!is_insert_blocked_by_delete and
+							!already_merged and 
+							is_same_user
+						offset = op_start - change_start
+						change.op.i = change.op.i.slice(0, offset) + op.i + change.op.i.slice(offset)
+						already_merged = true
+						moved_changes.push change
+					else if op_start <= change_start
+						# If we're fully before the other insert we can just shift the other insert by our length.
+						# If they are touching, and should have been merged, they will have been above.
+						# If not merged above, then it must be blocked by a delete, and will be after this insert, so we shift it along as well
+						change.op.p += op_length
+						moved_changes.push change
+					else if (!is_same_user or !@track_changes) and change_start < op_start < change_end
+						# This user is inserting inside a change by another user, so we need to split the
+						# other user's change into one before and after this one.
+						offset = op_start - change_start
+						before_content = change.op.i.slice(0, offset)
+						after_content = change.op.i.slice(offset)
+						
+						# The existing change can become the 'before' change
+						change.op.i = before_content
+						moved_changes.push change
+						
+						# Create a new op afterwards
+						after_change = {
+							op: {
+								i: after_content
+								p: change_start + offset + op_length
+							}
+							metadata: {}
+						}
+						after_change.metadata[key] = value for key, value of change.metadata
+						new_changes.push after_change
+						
+				previous_change = change
+
+			if @track_changes and !already_merged
+				@_addOp op, metadata
+			for {op, metadata} in new_changes
+				@_addOp op, metadata
+			
+			if moved_changes.length > 0
+				@emit "changes:moved", moved_changes
+		
+		applyDelete: (op, metadata) ->
+			op_start = op.p
+			op_length = op.d.length
+			op_end = op.p + op_length
+			remove_changes = []
+			moved_changes = []
+			
+			# We might end up modifying our delete op if it merges with existing deletes, or cancels out
+			# with an existing insert. Since we might do multiple modifications, we record them and do
+			# all the modifications after looping through the existing changes, so as not to mess up the
+			# offset indexes as we go.
+			op_modifications = []
+			for change in @changes
+				if change.op.i?
+					change_start = change.op.p
+					change_end = change_start + change.op.i.length
+					if op_end <= change_start
+						# Shift ops after us back by our length
+						change.op.p -= op_length
+						moved_changes.push change
+					else if op_start >= change_end
+						# Delete is after insert, nothing to do
+					else
+						# When the new delete overlaps an insert, we should remove the part of the insert that
+						# is now deleted, and also remove the part of the new delete that overlapped. I.e.
+						# the two cancel out where they overlap.
+						if op_start >= change_start
+							#                            |-- existing insert --|
+							# insert_remaining_before -> |.....||--   new delete   --|
+							delete_remaining_before = ""
+							insert_remaining_before = change.op.i.slice(0, op_start - change_start)
+						else
+							# delete_remaining_before -> |.....||-- existing insert --|
+							#                            |-- new delete   --|
+							delete_remaining_before = op.d.slice(0, change_start - op_start)
+							insert_remaining_before = ""
+
+						if op_end <= change_end
+							#    |--  existing insert  --|
+							# |--  new delete   --||.....| <- insert_remaining_after
+							delete_remaining_after = ""
+							insert_remaining_after = change.op.i.slice(op_end - change_start)
+						else
+							# |--  existing insert  --||.....| <- delete_remaining_after
+							#            |--  new delete   --|
+							delete_remaining_after = op.d.slice(change_end - op_start)
+							insert_remaining_after = ""
+
+						insert_remaining = insert_remaining_before + insert_remaining_after
+						if insert_remaining.length > 0
+							change.op.i = insert_remaining
+							change.op.p = Math.min(change_start, op_start)
+							moved_changes.push change
+						else
+							remove_changes.push change
+
+						# We know what we want to preserve of our delete op before (delete_remaining_before) and what we want to preserve
+						# afterwards (delete_remaining_before). Now we need to turn that into a modification which deletes the 
+						# chunk in the middle not covered by these.
+						delete_removed_length = op.d.length - delete_remaining_before.length - delete_remaining_after.length
+						delete_removed_start = delete_remaining_before.length
+						modification = {
+							d: op.d.slice(delete_removed_start, delete_removed_start + delete_removed_length)
+							p: delete_removed_start
+						}
+						if modification.d.length > 0
+							op_modifications.push modification
+				else if change.op.d?
+					change_start = change.op.p
+					if op_end < change_start or (!@track_changes and op_end == change_start)
+						# Shift ops after us back by our length.
+						# If we're tracking changes, it must be strictly before, since we'll merge 
+						# below if they are touching. Otherwise, touching is fine.
+						change.op.p -= op_length
+						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
+							offset = change_start - op_start
+							op_modifications.push { i: change.op.d, p: offset }
+							remove_changes.push change
+						else
+							change.op.p = op_start
+							moved_changes.push change
+
+			for change in remove_changes
+				@_removeChange change
+			
+			op.d = @_applyOpModifications(op.d, op_modifications)
+			if @track_changes and op.d.length > 0
+				@_addOp op, metadata
+			else
+				# It's possible that we deleted an insert between two other inserts. I.e.
+				# If we delete 'user_2 insert' in:
+				#   |-- user_1 insert --||-- user_2 insert --||-- user_1 insert --|
+				# it becomes:
+				#   |-- user_1 insert --||-- user_1 insert --|
+				# We need to merge these together again
+				results = @_scanAndMergeAdjacentUpdates()
+				moved_changes = moved_changes.concat(results.moved_changes)
+				for change in results.remove_changes
+					@_removeChange change
+					moved_changes = moved_changes.filter (c) -> c != change
+			
+			if moved_changes.length > 0
+				@emit "changes:moved", moved_changes
+
+		_newId: () ->
+			@id++
+
+		_addOp: (op, metadata) ->
+			change = {
+				id: @_newId()
+				op: op
+				metadata: metadata
+			}
+			@changes.push change
+
+			# Keep ops in order of offset, with deletes before inserts
+			@changes.sort (c1, c2) ->
+				result = c1.op.p - c2.op.p
+				if result != 0
+					return result
+				else if c1.op.i? and c2.op.d?
+					return 1
+				else
+					return -1
+
+			if op.d?
+				@emit "delete:added", change
+			else if op.i?
+				@emit "insert:added", change
+		
+		_removeChange: (change) ->
+			@changes = @changes.filter (c) -> c.id != change.id
+			if change.op.d?
+				@emit "delete:removed", change
+			else if change.op.i?
+				@emit "insert:removed", change
+			
+		_applyOpModifications: (content, op_modifications) ->
+			# Put in descending position order, with deleting first if at the same offset 
+			# (Inserting first would modify the content that the delete will delete)
+			op_modifications.sort (a, b) ->
+				result = b.p - a.p 
+				if result != 0
+					return result
+				else if a.i? and b.d?
+					return 1
+				else
+					return -1
+
+			for modification in op_modifications
+				if modification.i?
+					content = content.slice(0, modification.p) + modification.i + content.slice(modification.p)
+				else if modification.d?
+					if content.slice(modification.p, modification.p + modification.d.length) != modification.d
+						throw new Error("deleted content does not match. content: #{JSON.stringify(content)}; modification: #{JSON.stringify(modification)}")
+					content = content.slice(0, modification.p) + content.slice(modification.p + modification.d.length)
+			return content
+		
+		_scanAndMergeAdjacentUpdates: () ->
+			# This should only need calling when deleting an update between two
+			# other updates. There's no other way to get two adjacent updates from the
+			# same user, since they would be merged on insert.
+			previous_change = null
+			remove_changes = []
+			moved_changes = []
+			for change in @changes
+				if previous_change?.op.i? and change.op.i?
+					previous_change_end = previous_change.op.p + previous_change.op.i.length
+					previous_change_user_id = previous_change.metadata.user_id
+					change_start = change.op.p
+					change_user_id = change.metadata.user_id
+					if previous_change_end == change_start and previous_change_user_id == change_user_id
+						remove_changes.push change
+						previous_change.op.i += change.op.i
+						moved_changes.push previous_change
+				previous_change = change
+			return { moved_changes, remove_changes }

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

@@ -1,38 +1,67 @@
 define [
 	"ace/ace"
 	"utils/EventEmitter"
-], (_, EventEmitter) ->
+	"ide/editor/directives/aceEditor/track-changes/ChangesTracker"
+	"ide/colors/ColorManager"
+], (_, EventEmitter, ChangesTracker, ColorManager) ->
 	class TrackChangesManager
 		Range = ace.require("ace/range").Range
 		
 		constructor: (@$scope, @editor, @element) ->
 			@changesTracker = new ChangesTracker()
+			@changesTracker.track_changes = true
 			@changeIdToMarkerIdMap = {}
 			@enabled = false
+			window.changesTracker ?= @changesTracker
 
 			@changesTracker.on "insert:added", (change) =>
+				sl_console.log "[insert:added]", change
 				@_onInsertAdded(change)
 			@changesTracker.on "insert:removed", (change) =>
+				sl_console.log "[insert:removed]", change
 				@_onInsertRemoved(change)
 			@changesTracker.on "delete:added", (change) =>
+				sl_console.log "[delete:added]", change
 				@_onDeleteAdded(change)
 			@changesTracker.on "delete:removed", (change) =>
+				sl_console.log "[delete:removed]", change
 				@_onDeleteRemoved(change)
 			@changesTracker.on "changes:moved", (changes) =>
+				sl_console.log "[changes:moved]", changes
 				@_onChangesMoved(changes)
 
 			onChange = (e) =>
 				if !@editor.initing and @enabled
-					@applyChange(e)
+					# This change is trigger by a sharejs 'change' event, which is before the
+					# sharejs 'remoteop' event. So wait until the next event loop when the 'remoteop'
+					# will have fired, before we decide if it was a remote op.
 					setTimeout () =>
-						@checkMapping()
-					, 100
+						if @nextUpdateMetaData?
+							user_id = @nextUpdateMetaData.user_id
+							# The remote op may have contained multiple atomic ops, each of which is an Ace
+							# 'change' event (i.e. bulk commenting out of lines is a single remote op
+							# but gives us one event for each % inserted). These all come in a single event loop
+							# though, so wait until the next one before clearing the metadata.
+							setTimeout () =>
+								@nextUpdateMetaData = null
+						else
+							user_id = window.user.id
+
+						@applyChange(e, { user_id })
+						
+						# TODO: Just for debugging, remove before going live.
+						setTimeout () =>
+							@checkMapping()
+						, 100
 
 			@editor.on "changeSession", (e) =>
 				e.oldSession?.getDocument().off "change", onChange
 				e.session.getDocument().on "change", onChange
 			@editor.getSession().getDocument().on "change", onChange
-		
+			
+			@editor.renderer.on "resize", () =>
+				@recalculateReviewEntriesScreenPositions()
+
 		checkMapping: () ->
 			session = @editor.getSession()
 
@@ -63,19 +92,50 @@ define [
 				if marker.clazz.match("track-changes")
 					console.error "Orphaned ace marker", marker
 		
-		applyChange: (delta) ->
+		applyChange: (delta, metadata) ->
 			op = @_aceChangeToShareJs(delta)
-			console.log "Applying change", delta, op
-			@changesTracker.applyOp(op)
+			@changesTracker.applyOp(op, metadata)
 		
+		updateReviewEntriesScope: () ->
+			# TODO: Update in place so Angular doesn't have to redo EVERYTHING
+			@$scope.reviewPanel.entries = {}
+			for change in @changesTracker.changes
+				@$scope.reviewPanel.entries[change.id] = {
+					content: change.op.i or change.op.d
+					offset: change.op.p
+				}
+			@recalculateReviewEntriesScreenPositions()
+		
+		recalculateReviewEntriesScreenPositions: () ->
+			session = @editor.getSession()
+			renderer = @editor.renderer
+			for entry_id, entry of (@$scope.reviewPanel?.entries or {})
+				doc_position = @_shareJsOffsetToAcePosition(entry.offset)
+				screen_position = session.documentToScreenPosition(doc_position.row, doc_position.column)
+				y = screen_position.row * renderer.lineHeight
+				entry.screenPos = { y }
+			@$scope.$apply()
+				
 		_onInsertAdded: (change) ->
 			start = @_shareJsOffsetToAcePosition(change.op.p)
 			end = @_shareJsOffsetToAcePosition(change.op.p + change.op.i.length)
 			session = @editor.getSession()
 			doc = session.getDocument()
 			ace_range = new Range(start.row, start.column, end.row, end.column)
-			marker_id = session.addMarker(ace_range, "track-changes-added-marker", "text")
+
+			hue = ColorManager.getHueForUserId(change.metadata.user_id)
+			colorScheme = ColorManager.getColorScheme(hue, @element)
+			markerLayer = @editor.renderer.$markerBack
+			klass = "track-changes-added-marker"
+			style = "border-color: #{colorScheme.cursor}"
+			marker_id = session.addMarker ace_range, klass, (html, range, left, top, config) ->
+				if range.isMultiLine()
+					markerLayer.drawTextMarker(html, range, klass, config, style)
+				else
+					markerLayer.drawSingleLineMarker(html, range, "#{klass} ace_start", config, 0, style)
+			
 			@changeIdToMarkerIdMap[change.id] = marker_id
+			@updateReviewEntriesScope()
 		
 		_onDeleteAdded: (change) ->
 			position = @_shareJsOffsetToAcePosition(change.op.p)
@@ -95,18 +155,28 @@ define [
 					false
 				return range
 
-			marker_id = session.addMarker(ace_range, "track-changes-deleted-marker", "text")
+			hue = ColorManager.getHueForUserId(change.metadata.user_id)
+			colorScheme = ColorManager.getColorScheme(hue, @element)
+			markerLayer = @editor.renderer.$markerBack
+			klass = "track-changes-deleted-marker"
+			style = "border-color: #{colorScheme.cursor}"
+			marker_id = session.addMarker ace_range, klass, (html, range, left, top, config) ->
+				markerLayer.drawSingleLineMarker(html, range, "#{klass} ace_start", config, 0, style)
+
 			@changeIdToMarkerIdMap[change.id] = marker_id
+			@updateReviewEntriesScope()
 		
 		_onInsertRemoved: (change) ->
 			marker_id = @changeIdToMarkerIdMap[change.id]
 			session = @editor.getSession()
 			session.removeMarker marker_id
+			@updateReviewEntriesScope()
 		
 		_onDeleteRemoved: (change) ->
 			marker_id = @changeIdToMarkerIdMap[change.id]
 			session = @editor.getSession()
 			session.removeMarker marker_id
+			@updateReviewEntriesScope()
 		
 		_aceChangeToShareJs: (delta) ->
 			start = delta.start
@@ -138,6 +208,8 @@ define [
 		_onChangesMoved: (changes) ->
 			session = @editor.getSession()
 			markers = session.getMarkers()
+			# TODO: PERFORMANCE: Only run through the Ace lines once, and calculate all
+			# change positions as we go.
 			for change in changes
 				start = @_shareJsOffsetToAcePosition(change.op.p)
 				if change.op.i?
@@ -146,265 +218,7 @@ define [
 					end = start
 				marker_id = @changeIdToMarkerIdMap[change.id]
 				marker = markers[marker_id]
-				console.log "moving marker", {marker, start, end, change}
 				marker.range.start = start
 				marker.range.end = end
-	
-	class ChangesTracker extends EventEmitter
-		# The purpose of this class is to track a set of inserts and deletes to a document, like
-		# track changes in Word. We store these as a set of ShareJs style ranges:
-		#   {i: "foo", p: 42} # Insert 'foo' at offset 42
-		#   {d: "bar", p: 37} # Delete 'bar' at offset 37
-		# We only track the inserts and deletes, not the whole document, but by being given all
-		# updates that are applied to a document, we can update these appropriately.
-		# 
-		# Note that the set of inserts and deletes we store applies to the document as-is at the moment.
-		# So inserts correspond to text which is in the document, while deletes correspond to text which
-		# is no longer there, so their lengths do not affect the position of later offsets.
-		# E.g.
-		#             this is the current text of the document
-		#                         |-----|            |
-		#  {i: "current ", p:12} -^                   ^- {d: "old ", p: 31}
-		#
-		# Track changes rules (should be consistent with Word):
-		#   * When text is inserted at a delete, the text goes to the left of the delete
-		#       I.e. "foo|bar" -> "foobaz|bar", where | is the delete, and 'baz' is inserted
-		#   * Deleting content flagged as 'inserted' does not create a new delete marker, it only
-		#     removes the insert marker. E.g.
-		#       * "abdefghijkl"        -> "abfghijkl"        when 'de' is deleted. No delete marker added
-		#           |---| <- inserted       |-| <- inserted
-		#       * Deletes overlapping regular text and inserted text will insert a delete marker for the
-		#         regular text:
-		#         "abcdefghijkl"    ->    "abcdejkl"   when 'fghi' is deleted
-		#           |----|                  |--||
-		#           ^- inserted 'bcdefg'      \ ^- deleted 'hi'
-		#                                      \--inserted 'bcde'
-		#   * Deletes overlapping other deletes are merged. E.g.
-		#      "abcghijkl"        ->   "ahijkl"     when 'bcg is deleted'
-		#          | <- delete 'def'     | <- delete 'bcdefg'
-		constructor: () ->
-			# 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.
-			@changes = []
-			@id = 0
-		
-		applyOp: (op) ->
-			# Apply an op that has been applied to the document to our changes to keep them up to date
-			if op.i?
-				@applyInsert(op)
-			else if op.d?
-				@applyDelete(op)
-			
-		applyInsert: (op) ->
-			op_start = op.p
-			op_length = op.i.length
-			op_end = op.p + op_length
-
-			already_merged = false
-			previous_change = null
-			moved_changes = []
-			for change in @changes
-				change_start = change.op.p
-				
-				if change.op.d?
-					# Shift any deletes after this along by the length of this insert
-					if op_start <= change_start
-						change.op.p += op_length
-						moved_changes.push change
-				else if change.op.i?
-					change_end = change_start + change.op.i.length
-					is_change_overlapping = (op_start >= change_start and op_start <= change_end)
-					
-					# 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.
-					# The previous op will be the delete, but it's already been shifted by this insert
-					#
-					# I.e.
-					# Originally: |-- existing insert --|
-					#             | <- existing delete at same offset
-					#
-					# Now:        |-- existing insert --| <- not shifted yet
-					#             |-- this insert --|| <- existing delete shifted along to end of this op
-					# 
-					# After:                         |-- existing insert --|  
-					#             |-- this insert --|| <- existing delete
-					# 
-					# Without the delete, the inserts would be merged.
-					is_insert_blocked_by_delete = (previous_change? and previous_change.op.d? and previous_change.op.p == op_end)
-
-					# If the insert is overlapping another insert, either at the beginning in the middle or touching the end,
-					# then we merge them into one.
-					if is_change_overlapping and
-							!is_insert_blocked_by_delete and
-							!already_merged # With the way we order our changes, there should only ever be one candidate to merge
-							                # with since changes don't overlap. However, this flag just adds a little bit of protection
-						offset = op_start - change_start
-						change.op.i = change.op.i.slice(0, offset) + op.i + change.op.i.slice(offset)
-						already_merged = true
-						moved_changes.push change
-					else if op_start <= change_start
-						# If we're fully before the other insert we can just shift the other insert by our length.
-						# If they are touching, and should have been merged, they will have been above.
-						# If not merged above, then it must be blocked by a delete, and will be after this insert, so we shift it along as well
-						change.op.p += op_length
-						moved_changes.push change
-				previous_change = change
-
-			if !already_merged
-				@_addOp op
-			
-			if moved_changes.length > 0
-				@emit "changes:moved", moved_changes
-		
-		applyDelete: (op) ->
-			op_start = op.p
-			op_length = op.d.length
-			op_end = op.p + op_length
-			remove_changes = []
-			moved_changes = []
-			
-			# We might end up modifying our delete op if it merges with existing deletes, or cancels out
-			# with an existing insert. Since we might do multiple modifications, we record them and do
-			# all the modifications after looping through the existing changes, so as not to mess up the
-			# offset indexes as we go.
-			op_modifications = []
-			for change in @changes
-				if change.op.i?
-					change_start = change.op.p
-					change_end = change_start + change.op.i.length
-					if op_end <= change_start
-						# Shift ops after us back by our length
-						change.op.p -= op_length
-						moved_changes.push change
-					else if op_start >= change_end
-						# Delete is after insert, nothing to do
-					else
-						# When the new delete overlaps an insert, we should remove the part of the insert that
-						# is now deleted, and also remove the part of the new delete that overlapped. I.e.
-						# the two cancel out where they overlap.
-						if op_start >= change_start
-							#                            |-- existing insert --|
-							# insert_remaining_before -> |.....||--   new delete   --|
-							delete_remaining_before = ""
-							insert_remaining_before = change.op.i.slice(0, op_start - change_start)
-						else
-							# delete_remaining_before -> |.....||-- existing insert --|
-							#                            |-- new delete   --|
-							delete_remaining_before = op.d.slice(0, change_start - op_start)
-							insert_remaining_before = ""
-
-						if op_end <= change_end
-							#    |--  existing insert  --|
-							# |--  new delete   --||.....| <- insert_remaining_after
-							delete_remaining_after = ""
-							insert_remaining_after = change.op.i.slice(op_end - change_start)
-						else
-							# |--  existing insert  --||.....| <- delete_remaining_after
-							#            |--  new delete   --|
-							delete_remaining_after = op.d.slice(change_end - op_start)
-							insert_remaining_after = ""
-
-						insert_remaining = insert_remaining_before + insert_remaining_after
-						if insert_remaining.length > 0
-							change.op.i = insert_remaining
-							change.op.p = Math.min(change_start, op_start)
-							moved_changes.push change
-						else
-							remove_changes.push change
-
-						# We know what we want to preserve of our delete op before (delete_remaining_before) and what we want to preserve
-						# afterwards (delete_remaining_before). Now we need to turn that into a modification which deletes the 
-						# chunk in the middle not covered by these.
-						delete_removed_length = op.d.length - delete_remaining_before.length - delete_remaining_after.length
-						delete_removed_start = delete_remaining_before.length
-						modification = {
-							d: op.d.slice(delete_removed_start, delete_removed_start + delete_removed_length)
-							p: delete_removed_start
-						}
-						if modification.d.length > 0
-							op_modifications.push modification
-				else if change.op.d?
-					change_start = change.op.p
-					if op_end < change_start
-						# Shift ops after us (but not touching) back by our length
-						change.op.p -= op_length
-						moved_changes.push change
-					else if op_start <= change_start <= op_end
-						# If we overlap a delete, add it in our content, and delete the existing change
-						offset = change_start - op_start
-						op_modifications.push { i: change.op.d, p: offset }
-						remove_changes.push change
-			
-			op.d = @_applyOpModifications(op.d, op_modifications)
-			if op.d.length > 0
-				@_addOp op
-
-			for change in remove_changes
-				@_removeChange change
-			
-			if moved_changes.length > 0
-				@emit "changes:moved", moved_changes
-
-		_newId: () ->
-			@id++
-
-		_addOp: (op) ->
-			change = {
-				id: @_newId()
-				op: op
-			}
-			@changes.push change
-
-			# Keep ops in order of offset, with deletes before inserts
-			@changes.sort (c1, c2) ->
-				result = c1.op.p - c2.op.p
-				if result != 0
-					return result
-				else if c1.op.i? and c2.op.d?
-					return 1
-				else
-					return -1
-
-			if op.d?
-				@emit "delete:added", change
-			else if op.i?
-				@emit "insert:added", change
-		
-		_removeChange: (change) ->
-			@changes = @changes.filter (c) -> c.id != change.id
-			if change.op.d?
-				@emit "delete:removed", change
-			else if change.op.i?
-				@emit "insert:removed", change
-			
-		_applyOpModifications: (content, op_modifications) ->
-			# Put in descending position order, with deleting first if at the same offset 
-			# (Inserting first would modify the content that the delete will delete)
-			op_modifications.sort (a, b) ->
-				result = b.p - a.p 
-				if result != 0
-					return result
-				else if a.i? and b.d?
-					return 1
-				else
-					return -1
-
-			for modification in op_modifications
-				if modification.i?
-					content = content.slice(0, modification.p) + modification.i + content.slice(modification.p)
-				else if modification.d?
-					if content.slice(modification.p, modification.p + modification.d.length) != modification.d
-						throw new Error("deleted content does not match. content: #{JSON.stringify(content)}; modification: #{JSON.stringify(modification)}")
-					content = content.slice(0, modification.p) + content.slice(modification.p + modification.d.length)
-			return content
-
-	return TrackChangesManager
+			@editor.renderer.updateBackMarkers()
+			@updateReviewEntriesScope()

+ 4 - 4
services/web/public/coffee/ide/editor/sharejs/vendor/client/doc.coffee

@@ -64,7 +64,7 @@ class Doc
       server_ = @type.transform server, client, 'right'
       return [client_, server_]
   
-  _otApply: (docOp, isRemote) ->
+  _otApply: (docOp, isRemote, msg) ->
     oldSnapshot = @snapshot
     @snapshot = @type.apply(@snapshot, docOp)
 
@@ -72,7 +72,7 @@ class Doc
     # The reason is that the OT type APIs might need to access the snapshots to
     # determine information about the received op.
     @emit 'change', docOp, oldSnapshot
-    @emit 'remoteop', docOp, oldSnapshot if isRemote
+    @emit 'remoteop', docOp, oldSnapshot, msg if isRemote
   
   _connectionStateChanged: (state, data) ->
     switch state
@@ -185,7 +185,7 @@ class Doc
           # functionality, because its really a local op. Basically, the problem is that
           # if the client's op is rejected by the server, the editor window should update
           # to reflect the undo.
-          @_otApply undo, true
+          @_otApply undo, true, msg
         else
           @emit 'error', "Op apply failed (#{error}) and the op could not be reverted"
 
@@ -234,7 +234,7 @@ class Doc
         
       @version++
       # Finally, apply the op to @snapshot and trigger any event listeners
-      @_otApply docOp, true
+      @_otApply docOp, true, msg
 
     else if msg.meta
       {path, value} = msg.meta

+ 5 - 4
services/web/public/coffee/ide/history/HistoryManager.coffee

@@ -1,9 +1,10 @@
 define [
 	"moment"
+	"ide/colors/ColorManager"
 	"ide/history/controllers/HistoryListController"
 	"ide/history/controllers/HistoryDiffController"
 	"ide/history/directives/infiniteScroll"
-], (moment) ->
+], (moment, ColorManager) ->
 	class HistoryManager
 		constructor: (@ide, @$scope) ->
 			@reset()
@@ -172,13 +173,13 @@ define [
 						highlights.push {
 							label: "Added by #{name} on #{date}"
 							highlight: range
-							hue: @ide.onlineUsersManager.getHueForUserId(entry.meta.user?.id)
+							hue: ColorManager.getHueForUserId(entry.meta.user?.id)
 						}
 					else if entry.d?
 						highlights.push {
 							label: "Deleted by #{name} on #{date}"
 							strikeThrough: range
-							hue: @ide.onlineUsersManager.getHueForUserId(entry.meta.user?.id)
+							hue: ColorManager.getHueForUserId(entry.meta.user?.id)
 						}
 
 			return {text, highlights}
@@ -192,7 +193,7 @@ define [
 
 				for user in update.meta.users or []
 					if user?
-						user.hue = @ide.onlineUsersManager.getHueForUserId(user.id)
+						user.hue = ColorManager.getHueForUserId(user.id)
 
 				if !previousUpdate? or !moment(previousUpdate.meta.end_ts).isSame(update.meta.end_ts, "day")
 					update.meta.first_in_day = true

+ 4 - 19
services/web/public/coffee/ide/online-users/OnlineUsersManager.coffee

@@ -1,7 +1,8 @@
 define [
+	"ide/colors/ColorManager"
 	"libs/md5"
 	"ide/online-users/controllers/OnlineUsersController"
-], () ->
+], (ColorManager) ->
 	class OnlineUsersManager
 
 		cursorUpdateInterval:500
@@ -46,7 +47,7 @@ define [
 					@refreshOnlineUsers()
 					
 			@$scope.getHueForUserId = (user_id) =>
-				@getHueForUserId(user_id)
+				ColorManager.getHueForUserId(user_id)
 
 		refreshOnlineUsers: () ->
 			@$scope.onlineUsersArray = []
@@ -74,7 +75,7 @@ define [
 					cursor:
 						row: client.row
 						column: client.column
-					hue: @getHueForUserId(client.user_id)
+					hue: ColorManager.getHueForUserId(client.user_id)
 				}
 
 			if @$scope.onlineUsersArray.length > 0
@@ -101,19 +102,3 @@ define [
 					delete @cursorUpdateTimeout
 				, @cursorUpdateInterval
 
-		OWN_HUE: 200 # We will always appear as this color to ourselves
-		ANONYMOUS_HUE: 100
-		getHueForUserId: (user_id) ->
-			if !user_id? or user_id == "anonymous-user"
-				return @ANONYMOUS_HUE
-
-			if window.user.id == user_id
-				return @OWN_HUE
-
-			hash = CryptoJS.MD5(user_id)
-			hue = parseInt(hash.toString().slice(0,8), 16) % 320
-			# Avoid 20 degrees either side of the personal hue
-			if hue > @OWNER_HUE - 20
-				hue = hue + 40
-			return hue
-

+ 4 - 0
services/web/public/coffee/ide/review-panel/ReviewPanelManager.coffee

@@ -0,0 +1,4 @@
+define [
+	"ide/review-panel/controllers/ReviewPanelController"
+	"ide/review-panel/directives/reviewPanelSorted"
+], () ->

+ 56 - 0
services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee

@@ -0,0 +1,56 @@
+define [
+	"base",
+	"utils/EventEmitter"
+], (App, EventEmitter) ->
+	App.controller "ReviewPanelController", ($scope, $element, ide) ->
+		$scope.reviewPanel =
+			entries: {}
+			
+		scroller = $element.find(".review-panel-scroller")
+		list = $element.find(".review-entry-list")
+
+		# Use these to avoid unnecessary updates. Scrolling one
+		# panel causes us to scroll the other panel, but there's no
+		# need to trigger the event back to the original panel.
+		ignoreNextPanelEvent = false
+		ignoreNextAceEvent = false
+
+		$scope.scrollEvents = new EventEmitter()
+	
+		scrollPanel = (scrollTop, height) ->
+			if ignoreNextAceEvent
+				ignoreNextAceEvent = false
+			else
+				ignoreNextPanelEvent = true
+				list.height(height)
+				scroller.scrollTop(scrollTop)
+	
+		scrollAce = (e) ->
+			if ignoreNextPanelEvent
+				ignoreNextPanelEvent = false
+			else
+				ignoreNextAceEvent = true
+				$scope.scrollEvents.emit "scroll", e.target.scrollTop
+				
+		$scope.$watch "ui.reviewPanelOpen", (reviewPanelOpen) ->
+			return if !reviewPanelOpen?
+			setTimeout () ->
+				$scope.$broadcast "reviewPanel:toggle"
+			if reviewPanelOpen
+				scroller.on "scroll", scrollAce
+				$scope.onScroll = scrollPanel # Passed into the editor directive for it to call
+			else
+				scroller.off "scroll"
+				$scope.onScroll = null
+
+		# If we listen for scroll events in the review panel natively, then with a Mac trackpad
+		# the scroll is very smooth (natively done I'd guess), but we don't get polled regularly
+		# enough to keep Ace in step, and it noticeably lags. If instead, we borrow the manual
+		# mousewheel/trackpad scrolling behaviour from Ace, and turn mousewheel events into
+		# scroll events ourselves, then it makes the review panel slightly less smooth (barely)
+		# noticeable, but keeps it perfectly in step with Ace.
+		ace.require("ace/lib/event").addMouseWheelListener scroller[0], (e) ->
+			deltaY = e.wheelY
+			# console.log "mousewheel", deltaY
+			scroller.scrollTop(scroller.scrollTop() + deltaY * 4)
+			e.preventDefault()

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

@@ -0,0 +1,24 @@
+define [
+	"base"
+], (App) ->
+	App.directive "reviewPanelSorted", () ->
+		return  {
+			link: (scope, element, attrs) ->
+				scope.$watch "reviewPanel.entries", (value) ->
+					return if !value?
+					entries = []
+					for el in element.find(".review-entry")
+						entries.push {
+							el: el
+							scope: angular.element(el).scope()
+						}
+					entries.sort (a,b) -> a.scope.entry.offset - b.scope.entry.offset
+					
+					previousBottom = 0
+					for entry in entries
+						height = $(entry.el).height()
+						top = entry.scope.entry.screenPos.y
+						top = Math.max(top, previousBottom + 12)
+						previousBottom = top + height
+						entry.scope.top = top
+		}

+ 3 - 8
services/web/public/stylesheets/app/editor.less

@@ -11,6 +11,7 @@
 @import "./editor/publish-template.less";
 @import "./editor/online-users.less";
 @import "./editor/hotkeys.less";
+@import "./editor/review-panel.less";
 @import "./editor/feature-onboarding.less";
 
 .full-size {
@@ -35,13 +36,6 @@
 	}
 }
 
-#chat-wrapper {
-	.full-size;
-	> .ui-layout-resizer > .ui-layout-toggler {
-		display: none !important;
-	}
-}
-
 #ide-body {
 	.full-size;
 	top: 40px;
@@ -150,7 +144,8 @@
 	.track-changes-added-marker {
 		border-radius: 0;
 		position: absolute;
-		background-color: hsl(100, 70%, 70%);
+		border-bottom: 1px dashed green;
+		background-color: hsl(100, 70%, 85%);
 	}
 	.track-changes-deleted-marker {
 		border-radius: 0;

+ 6 - 0
services/web/public/stylesheets/app/editor/chat.less

@@ -1,5 +1,11 @@
 @new-message-height: 80px;
 
+#chat-wrapper {
+	> .ui-layout-resizer > .ui-layout-toggler {
+		display: none !important;
+	}
+}
+
 .chat {
 	.loading {
 		font-family: @font-family-serif;

+ 55 - 0
services/web/public/stylesheets/app/editor/review-panel.less

@@ -0,0 +1,55 @@
+@review-panel-width: 230px;
+
+#review-panel {
+	position: absolute;
+	width: @review-panel-width;
+	top: 0px;
+	bottom: 0px;
+	right: 0px;
+	background-color: #eee;
+	overflow: hidden;
+	display: none;
+}
+
+.review-panel-scroller {
+	position: absolute;
+	top: 0;
+	bottom: 0;
+	left: 0;
+	// TODO: Use a more cross-browser method of hiding the scroll bar
+	right: -30px; // Hide scroll bar
+	overflow-y: scroll;
+}
+
+.review-entry-list {
+	position: relative;
+	width: @review-panel-width;;
+}
+
+.review-entry {
+	position: absolute;
+	font-size: 12px;
+	padding: 2px 6px;
+	border: 1px solid #999;
+	margin: 0 6px;
+	background-color: white;
+	max-width: 148px;
+	word-wrap: break-word;
+}
+
+.has-review-panel {
+	#editor {
+		right: @review-panel-width;;
+		left: 0px;
+		width: auto;
+		.ace-editor-body {
+			overflow: visible;
+			.ace_scrollbar-v {
+				right: -@review-panel-width;;
+			}
+		}
+	}
+	#review-panel {
+		display: block;
+	}
+}