From 9509e87dc19f3c5284d57b6f4b2848dc9c3f21a7 Mon Sep 17 00:00:00 2001 From: James Allen Date: Fri, 11 Nov 2016 16:09:32 +0000 Subject: [PATCH] Update entries in place and position via DOM attributes directly --- .../web/app/views/project/editor/editor.jade | 2 - .../track-changes/ChangesTracker.coffee | 29 +++++++++-- .../track-changes/TrackChangesManager.coffee | 50 +++++++++++++++---- .../directives/reviewPanelSorted.coffee | 17 ++++--- 4 files changed, 74 insertions(+), 24 deletions(-) diff --git a/services/web/app/views/project/editor/editor.jade b/services/web/app/views/project/editor/editor.jade index 539cc0a2ce..50e43bdfd8 100644 --- a/services/web/app/views/project/editor/editor.jade +++ b/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 diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor/track-changes/ChangesTracker.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor/track-changes/ChangesTracker.coffee index bfb03baac7..250f1c6fe1 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor/track-changes/ChangesTracker.coffee +++ b/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 } diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor/track-changes/TrackChangesManager.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor/track-changes/TrackChangesManager.coffee index f5059c6820..15ce190f64 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor/track-changes/TrackChangesManager.coffee +++ b/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() diff --git a/services/web/public/coffee/ide/review-panel/directives/reviewPanelSorted.coffee b/services/web/public/coffee/ide/review-panel/directives/reviewPanelSorted.coffee index 0b492a75c5..669d52df17 100644 --- a/services/web/public/coffee/ide/review-panel/directives/reviewPanelSorted.coffee +++ b/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()