From e9c8fc7c2047d42afcb4f56d639231fe8309c2b4 Mon Sep 17 00:00:00 2001 From: James Allen Date: Tue, 11 Oct 2016 14:24:01 +0100 Subject: [PATCH 01/13] Add in review panel layout --- services/web/app/views/project/editor.jade | 2 +- .../web/app/views/project/editor/editor.jade | 66 +++++++++++-------- services/web/public/coffee/ide.coffee | 4 ++ .../web/public/stylesheets/app/editor.less | 8 +-- .../public/stylesheets/app/editor/chat.less | 6 ++ .../stylesheets/app/editor/review-panel.less | 5 ++ 6 files changed, 55 insertions(+), 36 deletions(-) create mode 100644 services/web/public/stylesheets/app/editor/review-panel.less diff --git a/services/web/app/views/project/editor.jade b/services/web/app/views/project/editor.jade index bc8016dd86..86088ded50 100644 --- a/services/web/app/views/project/editor.jade +++ b/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", diff --git a/services/web/app/views/project/editor/editor.jade b/services/web/app/views/project/editor/editor.jade index 07208a6642..4e04bf6753 100644 --- a/services/web/app/views/project/editor/editor.jade +++ b/services/web/app/views/project/editor/editor.jade @@ -9,35 +9,45 @@ div.full-size( minimum-restore-size-east="300" ) .ui-layout-center - .loading-panel(ng-show="!editor.sharejs_doc || editor.opening") - span(ng-show="editor.open_doc_id") - i.fa.fa-spin.fa-refresh - |   #{translate("loading")}... - span(ng-show="!editor.open_doc_id") - i.fa.fa-arrow-left - |   #{translate("open_a_file_on_the_left")} - - #editor( - ace-editor="editor", - ng-show="!!editor.sharejs_doc && !editor.opening" - theme="settings.theme", - keybindings="settings.mode", - font-size="settings.fontSize", - auto-complete="settings.autoComplete", - spell-check="true", - spell-check-language="project.spellCheckLanguage", - highlights="onlineUserCursorHighlights[editor.open_doc_id]" - show-print-margin="false", - sharejs-doc="editor.sharejs_doc", - last-updated="editor.last_updated", - cursor-position="editor.cursorPosition", - goto-line="editor.gotoLine", - resize-on="layout:main:resize,layout:pdf:resize", - annotations="pdf.logEntryAnnotations[editor.open_doc_id]", - read-only="!permissions.write", - on-ctrl-enter="recompileViaKey" - syntax-validation="settings.syntaxValidation" + #review-panel-wrapper.full-size( + layout="review" + spacing-open="12" + spacing-closed="0" + init-closed-east="true" + open-east="ui.reviewPanelOpen" ) + .ui-layout-center + .loading-panel(ng-show="!editor.sharejs_doc || editor.opening") + span(ng-show="editor.open_doc_id") + i.fa.fa-spin.fa-refresh + |   #{translate("loading")}... + span(ng-show="!editor.open_doc_id") + i.fa.fa-arrow-left + |   #{translate("open_a_file_on_the_left")} + + #editor( + ace-editor="editor", + ng-show="!!editor.sharejs_doc && !editor.opening" + theme="settings.theme", + keybindings="settings.mode", + font-size="settings.fontSize", + auto-complete="settings.autoComplete", + spell-check="true", + spell-check-language="project.spellCheckLanguage", + highlights="onlineUserCursorHighlights[editor.open_doc_id]" + show-print-margin="false", + sharejs-doc="editor.sharejs_doc", + last-updated="editor.last_updated", + cursor-position="editor.cursorPosition", + goto-line="editor.gotoLine", + resize-on="layout:main:resize,layout:pdf:resize,layout:review:resize", + annotations="pdf.logEntryAnnotations[editor.open_doc_id]", + read-only="!permissions.write", + on-ctrl-enter="recompileViaKey" + syntax-validation="settings.syntaxValidation" + ) + .ui-layout-east + strong Hello world .ui-layout-east div(ng-if="ui.pdfLayout == 'sideBySide'") diff --git a/services/web/public/coffee/ide.coffee b/services/web/public/coffee/ide.coffee index 01f7363c80..201d324821 100644 --- a/services/web/public/coffee/ide.coffee +++ b/services/web/public/coffee/ide.coffee @@ -64,6 +64,7 @@ define [ view: "editor" chatOpen: false pdfLayout: 'sideBySide' + reviewPanelOpen: false } $scope.user = window.user $scope.settings = window.userSettings @@ -71,6 +72,9 @@ 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)) diff --git a/services/web/public/stylesheets/app/editor.less b/services/web/public/stylesheets/app/editor.less index 2db26059c4..4b38201aff 100644 --- a/services/web/public/stylesheets/app/editor.less +++ b/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"; .full-size { position: absolute; @@ -34,13 +35,6 @@ } } -#chat-wrapper { - .full-size; - > .ui-layout-resizer > .ui-layout-toggler { - display: none !important; - } -} - #ide-body { .full-size; top: 40px; diff --git a/services/web/public/stylesheets/app/editor/chat.less b/services/web/public/stylesheets/app/editor/chat.less index d702a225fe..d0ad76fe9c 100644 --- a/services/web/public/stylesheets/app/editor/chat.less +++ b/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; diff --git a/services/web/public/stylesheets/app/editor/review-panel.less b/services/web/public/stylesheets/app/editor/review-panel.less new file mode 100644 index 0000000000..1d7d807028 --- /dev/null +++ b/services/web/public/stylesheets/app/editor/review-panel.less @@ -0,0 +1,5 @@ +#review-panel-wrapper { + > .ui-layout-resizer > .ui-layout-toggler { + display: none !important; + } +} \ No newline at end of file From fc782581ea973f7d8911986259afe8bae2afe143 Mon Sep 17 00:00:00 2001 From: James Allen Date: Wed, 12 Oct 2016 17:27:20 +0100 Subject: [PATCH 02/13] Change review panel to feel like part of ace --- .../web/app/views/project/editor/editor.jade | 76 +++++++++---------- services/web/public/coffee/ide.coffee | 2 + .../ide/editor/directives/aceEditor.coffee | 18 ++++- .../track-changes/TrackChangesManager.coffee | 38 +++++++++- .../review-panel/ReviewPanelManager.coffee | 3 + .../controllers/ReviewPanelController.coffee | 43 +++++++++++ .../stylesheets/app/editor/review-panel.less | 47 +++++++++++- 7 files changed, 181 insertions(+), 46 deletions(-) create mode 100644 services/web/public/coffee/ide/review-panel/ReviewPanelManager.coffee create mode 100644 services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee diff --git a/services/web/app/views/project/editor/editor.jade b/services/web/app/views/project/editor/editor.jade index 4e04bf6753..2b71ff6735 100644 --- a/services/web/app/views/project/editor/editor.jade +++ b/services/web/app/views/project/editor/editor.jade @@ -8,46 +8,44 @@ div.full-size( initial-size-east="'50%'" minimum-restore-size-east="300" ) - .ui-layout-center - #review-panel-wrapper.full-size( - layout="review" - spacing-open="12" - spacing-closed="0" - init-closed-east="true" - open-east="ui.reviewPanelOpen" - ) - .ui-layout-center - .loading-panel(ng-show="!editor.sharejs_doc || editor.opening") - span(ng-show="editor.open_doc_id") - i.fa.fa-spin.fa-refresh - |   #{translate("loading")}... - span(ng-show="!editor.open_doc_id") - i.fa.fa-arrow-left - |   #{translate("open_a_file_on_the_left")} + .ui-layout-center(ng-controller="ReviewPanelController") + .loading-panel(ng-show="!editor.sharejs_doc || editor.opening") + span(ng-show="editor.open_doc_id") + i.fa.fa-spin.fa-refresh + |   #{translate("loading")}... + span(ng-show="!editor.open_doc_id") + i.fa.fa-arrow-left + |   #{translate("open_a_file_on_the_left")} - #editor( - ace-editor="editor", - ng-show="!!editor.sharejs_doc && !editor.opening" - theme="settings.theme", - keybindings="settings.mode", - font-size="settings.fontSize", - auto-complete="settings.autoComplete", - spell-check="true", - spell-check-language="project.spellCheckLanguage", - highlights="onlineUserCursorHighlights[editor.open_doc_id]" - show-print-margin="false", - sharejs-doc="editor.sharejs_doc", - last-updated="editor.last_updated", - cursor-position="editor.cursorPosition", - goto-line="editor.gotoLine", - resize-on="layout:main:resize,layout:pdf:resize,layout:review:resize", - annotations="pdf.logEntryAnnotations[editor.open_doc_id]", - read-only="!permissions.write", - on-ctrl-enter="recompileViaKey" - syntax-validation="settings.syntaxValidation" - ) - .ui-layout-east - strong Hello world + #editor.has-review-panel( + ace-editor="editor", + ng-show="!!editor.sharejs_doc && !editor.opening" + theme="settings.theme", + keybindings="settings.mode", + font-size="settings.fontSize", + auto-complete="settings.autoComplete", + spell-check="true", + spell-check-language="project.spellCheckLanguage", + highlights="onlineUserCursorHighlights[editor.open_doc_id]" + show-print-margin="false", + sharejs-doc="editor.sharejs_doc", + last-updated="editor.last_updated", + cursor-position="editor.cursorPosition", + goto-line="editor.gotoLine", + resize-on="layout:main:resize,layout:pdf:resize,layout:review:resize", + annotations="pdf.logEntryAnnotations[editor.open_doc_id]", + read-only="!permissions.write", + on-ctrl-enter="recompileViaKey", + syntax-validation="settings.syntaxValidation", + review-panel="reviewPanel", + on-scroll="onScroll", + scroll-events="scrollEvents" + ) + #review-panel + .review-panel-scroller + .review-entry-list + .review-entry(ng-repeat="(entry_id, entry) in reviewPanel.entries", ng-style="{'top': entry.screenPos.y}") + {{ entry.content }} .ui-layout-east div(ng-if="ui.pdfLayout == 'sideBySide'") diff --git a/services/web/public/coffee/ide.coffee b/services/web/public/coffee/ide.coffee index c8c0da5b59..83ef177a92 100644 --- a/services/web/public/coffee/ide.coffee +++ b/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/settings/index" "ide/share/index" @@ -41,6 +42,7 @@ define [ PdfManager BinaryFilesManager ReferencesManager + ReviewPanelManager SafariScrollPatcher ) -> diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor.coffee index 2dc9440ffa..e4cd18c7ec 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor.coffee @@ -42,9 +42,12 @@ define [ text: "=" readOnly: "=" annotations: "=" - navigateHighlights: "=", + navigateHighlights: "=" onCtrlEnter: "=" syntaxValidation: "=" + reviewPanel: "=" + onScroll: "=" + scrollEvents: "=" } link: (scope, element, attrs) -> # Don't freak out if we're already in an apply callback @@ -210,6 +213,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") @@ -222,6 +234,8 @@ define [ doc = session.getDocument() doc.on "change", onChange + + session.on "changeScrollTop", onScroll sharejs_doc.on "remoteop.recordForUndo", () => undoManager.nextUpdateIsRemote = true @@ -240,6 +254,8 @@ define [ sharejs_doc.off "remoteop.recordForUndo" session = editor.getSession() + session.off "changeScrollTop" + doc = session.getDocument() doc.off "change", onChange 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 1c092ad190..ef38ca6dfe 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 @@ -9,6 +9,7 @@ define [ @changesTracker = new ChangesTracker() @changeIdToMarkerIdMap = {} @enabled = false + console.log "Track Changes", @$scope.reviewPanel @changesTracker.on "insert:added", (change) => @_onInsertAdded(change) @@ -27,12 +28,18 @@ define [ setTimeout () => @checkMapping() , 100 + + # onScroll = () => + # @recalculateReviewEntriesScreenPositions() @editor.on "changeSession", (e) => e.oldSession?.getDocument().off "change", onChange e.session.getDocument().on "change", onChange + # e.oldSession?.off "changeScrollTop", onScroll + # e.session.on "changeScrollTop", onScroll @editor.getSession().getDocument().on "change", onChange - + # @editor.getSession().on "changeScrollTop", onScroll + checkMapping: () -> session = @editor.getSession() @@ -65,9 +72,28 @@ define [ applyChange: (delta) -> op = @_aceChangeToShareJs(delta) - console.log "Applying change", delta, op @changesTracker.applyOp(op) + 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 + 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) @@ -76,6 +102,7 @@ define [ ace_range = new Range(start.row, start.column, end.row, end.column) marker_id = session.addMarker(ace_range, "track-changes-added-marker", "text") @changeIdToMarkerIdMap[change.id] = marker_id + @updateReviewEntriesScope() _onDeleteAdded: (change) -> position = @_shareJsOffsetToAcePosition(change.op.p) @@ -97,16 +124,19 @@ define [ marker_id = session.addMarker(ace_range, "track-changes-deleted-marker", "text") @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 +168,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,9 +178,9 @@ 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 + @updateReviewEntriesScope() class ChangesTracker extends EventEmitter # The purpose of this class is to track a set of inserts and deletes to a document, like diff --git a/services/web/public/coffee/ide/review-panel/ReviewPanelManager.coffee b/services/web/public/coffee/ide/review-panel/ReviewPanelManager.coffee new file mode 100644 index 0000000000..2670310f25 --- /dev/null +++ b/services/web/public/coffee/ide/review-panel/ReviewPanelManager.coffee @@ -0,0 +1,3 @@ +define [ + "ide/review-panel/controllers/ReviewPanelController" +], () -> \ No newline at end of file diff --git a/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee b/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee new file mode 100644 index 0000000000..baaf09dbb5 --- /dev/null +++ b/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee @@ -0,0 +1,43 @@ +define [ + "base", + "utils/EventEmitter" +], (App, EventEmitter) -> + App.controller "ReviewPanelController", ($scope, $element) -> + $scope.reviewPanel = + entries: {} + + scroller = $element.find(".review-panel-scroller") + list = $element.find(".review-entry-list") + + ignoreNextPanelEvent = false + ignoreNextAceEvent = false + + $scope.onScroll = (scrollTop, height) -> + if ignoreNextAceEvent + # console.log "Ignoring ace event" + ignoreNextAceEvent = false + else + ignoreNextPanelEvent = true + list.height(height) + scroller.scrollTop(scrollTop) + + $scope.scrollEvents = new EventEmitter() + + scrollAce = (e) -> + now = new Date() + if ignoreNextPanelEvent + # console.log "Ignoring review panel event" + ignoreNextPanelEvent = false + else + # console.log "review panel scrolled", e + ignoreNextAceEvent = true + $scope.scrollEvents.emit "scroll", e.target.scrollTop + lastScroll = now + + previousScroll = new Date() + scroller.on "scroll", scrollAce + ace.require("ace/lib/event").addMouseWheelListener scroller[0], (e) -> + deltaY = e.wheelY + # console.log "mousewheel", deltaY + scroller.scrollTop(scroller.scrollTop() + deltaY * 4) + e.preventDefault() \ No newline at end of file diff --git a/services/web/public/stylesheets/app/editor/review-panel.less b/services/web/public/stylesheets/app/editor/review-panel.less index 1d7d807028..aab21241f1 100644 --- a/services/web/public/stylesheets/app/editor/review-panel.less +++ b/services/web/public/stylesheets/app/editor/review-panel.less @@ -1,5 +1,46 @@ -#review-panel-wrapper { - > .ui-layout-resizer > .ui-layout-toggler { - display: none !important; +#review-panel { + position: absolute; + width: 160px; + top: 0px; + bottom: 0px; + right: 0px; + background-color: #eee; + overflow: hidden; +} + +.review-panel-scroller { + position: absolute; + top: 0; + bottom: 0; + left: 0; + right: -30px; // Hide scroll bar + overflow-y: scroll; +} + +.review-entry-list { + position: relative; + width: 160px; +} + +.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; +} + +#editor.has-review-panel { + right: 160px; + left: 0px; + width: auto; + .ace-editor-body { + overflow: visible; + .ace_scrollbar-v { + right: -160px; + } } } \ No newline at end of file From 8d968f6865dc5bbd89615e09fb687e5f415887bc Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 13 Oct 2016 10:09:59 +0100 Subject: [PATCH 03/13] Tidy up ReviewPanelController --- .../controllers/ReviewPanelController.coffee | 23 +++++++++++-------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee b/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee index baaf09dbb5..58f4c7d91f 100644 --- a/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee +++ b/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee @@ -9,33 +9,38 @@ define [ 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.onScroll = (scrollTop, height) -> + $scope.scrollEvents = new EventEmitter() + + scrollPanel = (scrollTop, height) -> if ignoreNextAceEvent - # console.log "Ignoring ace event" ignoreNextAceEvent = false else ignoreNextPanelEvent = true list.height(height) scroller.scrollTop(scrollTop) - - $scope.scrollEvents = new EventEmitter() scrollAce = (e) -> - now = new Date() if ignoreNextPanelEvent - # console.log "Ignoring review panel event" ignoreNextPanelEvent = false else - # console.log "review panel scrolled", e ignoreNextAceEvent = true $scope.scrollEvents.emit "scroll", e.target.scrollTop - lastScroll = now - previousScroll = new Date() scroller.on "scroll", scrollAce + $scope.onScroll = scrollPanel # Passed into the editor directive for it to call + + # 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 From 77c02042f8dc6db9b4b5c8bd69a94553c7cab4c4 Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 13 Oct 2016 12:09:18 +0100 Subject: [PATCH 04/13] Recalculate change offsets on editor resize --- .../aceEditor/track-changes/TrackChangesManager.coffee | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) 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 ef38ca6dfe..47026eeb04 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 @@ -9,7 +9,6 @@ define [ @changesTracker = new ChangesTracker() @changeIdToMarkerIdMap = {} @enabled = false - console.log "Track Changes", @$scope.reviewPanel @changesTracker.on "insert:added", (change) => @_onInsertAdded(change) @@ -28,17 +27,14 @@ define [ setTimeout () => @checkMapping() , 100 - - # onScroll = () => - # @recalculateReviewEntriesScreenPositions() @editor.on "changeSession", (e) => e.oldSession?.getDocument().off "change", onChange e.session.getDocument().on "change", onChange - # e.oldSession?.off "changeScrollTop", onScroll - # e.session.on "changeScrollTop", onScroll @editor.getSession().getDocument().on "change", onChange - # @editor.getSession().on "changeScrollTop", onScroll + + @editor.renderer.on "resize", () => + @recalculateReviewEntriesScreenPositions() checkMapping: () -> session = @editor.getSession() From 42ab2e816596d6c05d20d9c484e2c567557234f5 Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 13 Oct 2016 12:21:49 +0100 Subject: [PATCH 05/13] Init review panel with ace editor height --- .../public/coffee/ide/editor/directives/aceEditor.coffee | 8 ++++++-- .../review-panel/controllers/ReviewPanelController.coffee | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor.coffee index e4cd18c7ec..4a697f39f9 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor.coffee @@ -234,8 +234,6 @@ define [ doc = session.getDocument() doc.on "change", onChange - - session.on "changeScrollTop", onScroll sharejs_doc.on "remoteop.recordForUndo", () => undoManager.nextUpdateIsRemote = true @@ -243,9 +241,15 @@ define [ 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() diff --git a/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee b/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee index 58f4c7d91f..44704b2daa 100644 --- a/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee +++ b/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee @@ -45,4 +45,4 @@ define [ deltaY = e.wheelY # console.log "mousewheel", deltaY scroller.scrollTop(scroller.scrollTop() + deltaY * 4) - e.preventDefault() \ No newline at end of file + e.preventDefault() From 7a4bebd7850bab55d5a7b3fa7420d8b92db86261 Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 13 Oct 2016 14:22:23 +0100 Subject: [PATCH 06/13] Allow review panel to be toggled --- .../web/app/views/project/editor/editor.jade | 6 ++-- .../controllers/ReviewPanelController.coffee | 16 ++++++++--- .../stylesheets/app/editor/review-panel.less | 28 ++++++++++++------- 3 files changed, 33 insertions(+), 17 deletions(-) diff --git a/services/web/app/views/project/editor/editor.jade b/services/web/app/views/project/editor/editor.jade index 2b71ff6735..c824130cbc 100644 --- a/services/web/app/views/project/editor/editor.jade +++ b/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(ng-controller="ReviewPanelController") + .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 @@ -17,7 +17,7 @@ div.full-size( i.fa.fa-arrow-left |   #{translate("open_a_file_on_the_left")} - #editor.has-review-panel( + #editor( ace-editor="editor", ng-show="!!editor.sharejs_doc && !editor.opening" theme="settings.theme", @@ -32,7 +32,7 @@ div.full-size( last-updated="editor.last_updated", cursor-position="editor.cursorPosition", goto-line="editor.gotoLine", - resize-on="layout:main:resize,layout:pdf:resize,layout:review: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", on-ctrl-enter="recompileViaKey", diff --git a/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee b/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee index 44704b2daa..8f80f1c833 100644 --- a/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee +++ b/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee @@ -2,7 +2,7 @@ define [ "base", "utils/EventEmitter" ], (App, EventEmitter) -> - App.controller "ReviewPanelController", ($scope, $element) -> + App.controller "ReviewPanelController", ($scope, $element, ide) -> $scope.reviewPanel = entries: {} @@ -31,9 +31,17 @@ define [ else ignoreNextAceEvent = true $scope.scrollEvents.emit "scroll", e.target.scrollTop - - scroller.on "scroll", scrollAce - $scope.onScroll = scrollPanel # Passed into the editor directive for it to call + + $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 diff --git a/services/web/public/stylesheets/app/editor/review-panel.less b/services/web/public/stylesheets/app/editor/review-panel.less index aab21241f1..169272721e 100644 --- a/services/web/public/stylesheets/app/editor/review-panel.less +++ b/services/web/public/stylesheets/app/editor/review-panel.less @@ -1,11 +1,14 @@ +@review-panel-width: 230px; + #review-panel { position: absolute; - width: 160px; + width: @review-panel-width; top: 0px; bottom: 0px; right: 0px; background-color: #eee; overflow: hidden; + display: none; } .review-panel-scroller { @@ -19,7 +22,7 @@ .review-entry-list { position: relative; - width: 160px; + width: @review-panel-width;; } .review-entry { @@ -33,14 +36,19 @@ word-wrap: break-word; } -#editor.has-review-panel { - right: 160px; - left: 0px; - width: auto; - .ace-editor-body { - overflow: visible; - .ace_scrollbar-v { - right: -160px; +.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; + } } \ No newline at end of file From 09195882f443a2090e831de564797394406df84e Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 13 Oct 2016 14:25:46 +0100 Subject: [PATCH 07/13] Add scrollbar todo note --- services/web/public/stylesheets/app/editor/review-panel.less | 1 + 1 file changed, 1 insertion(+) diff --git a/services/web/public/stylesheets/app/editor/review-panel.less b/services/web/public/stylesheets/app/editor/review-panel.less index 169272721e..07de04be33 100644 --- a/services/web/public/stylesheets/app/editor/review-panel.less +++ b/services/web/public/stylesheets/app/editor/review-panel.less @@ -16,6 +16,7 @@ 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; } From c88624bf4c18eba6f01d07746c411da607ffb562 Mon Sep 17 00:00:00 2001 From: James Allen Date: Tue, 18 Oct 2016 18:01:52 +0100 Subject: [PATCH 08/13] Pass through the data needed to trackChangesManager to determine if a remote was local or remote --- .../public/coffee/ide/editor/Document.coffee | 4 ++-- .../coffee/ide/editor/ShareJsDoc.coffee | 4 ++-- .../ide/editor/directives/aceEditor.coffee | 5 +++-- .../track-changes/TrackChangesManager.coffee | 21 ++++++++++++++++--- .../editor/sharejs/vendor/client/doc.coffee | 8 +++---- 5 files changed, 29 insertions(+), 13 deletions(-) diff --git a/services/web/public/coffee/ide/editor/Document.coffee b/services/web/public/coffee/ide/editor/Document.coffee index 78431b8b6b..005d722a56 100644 --- a/services/web/public/coffee/ide/editor/Document.coffee +++ b/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 diff --git a/services/web/public/coffee/ide/editor/ShareJsDoc.coffee b/services/web/public/coffee/ide/editor/ShareJsDoc.coffee index 27d325676c..4f9960f9a0 100644 --- a/services/web/public/coffee/ide/editor/ShareJsDoc.coffee +++ b/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) diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor.coffee index 4a697f39f9..bc4e656a1c 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor.coffee @@ -235,8 +235,9 @@ 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) @@ -255,7 +256,7 @@ define [ detachFromAce = (sharejs_doc) -> sharejs_doc.detachFromAce() - sharejs_doc.off "remoteop.recordForUndo" + sharejs_doc.off "remoteop.recordRemote" session = editor.getSession() session.off "changeScrollTop" 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 47026eeb04..abcb8817c1 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 @@ -23,10 +23,24 @@ define [ 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? + # 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 + + @applyChange(e) + + # TODO: Just for debugging, remove before going live. + setTimeout () => + @checkMapping() + , 100 @editor.on "changeSession", (e) => e.oldSession?.getDocument().off "change", onChange @@ -176,6 +190,7 @@ define [ marker = markers[marker_id] marker.range.start = start marker.range.end = end + @editor.renderer.updateBackMarkers() @updateReviewEntriesScope() class ChangesTracker extends EventEmitter diff --git a/services/web/public/coffee/ide/editor/sharejs/vendor/client/doc.coffee b/services/web/public/coffee/ide/editor/sharejs/vendor/client/doc.coffee index dd4f26ceb0..d25baf89d5 100644 --- a/services/web/public/coffee/ide/editor/sharejs/vendor/client/doc.coffee +++ b/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 From 52f3fe73038afd5c916340f7bdd94dfe31da3c08 Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 20 Oct 2016 12:15:22 +0100 Subject: [PATCH 09/13] Show different users changes in different colours --- .../controllers/ChatMessageController.coffee | 5 +- .../coffee/ide/colors/ColorManager.coffee | 44 +++ .../highlights/HighlightsManager.coffee | 31 +- .../track-changes/ChangesTracker.coffee | 322 ++++++++++++++++++ .../track-changes/TrackChangesManager.coffee | 296 ++-------------- .../coffee/ide/history/HistoryManager.coffee | 9 +- .../online-users/OnlineUsersManager.coffee | 23 +- .../web/public/stylesheets/app/editor.less | 1 - 8 files changed, 412 insertions(+), 319 deletions(-) create mode 100644 services/web/public/coffee/ide/colors/ColorManager.coffee create mode 100644 services/web/public/coffee/ide/editor/directives/aceEditor/track-changes/ChangesTracker.coffee diff --git a/services/web/public/coffee/ide/chat/controllers/ChatMessageController.coffee b/services/web/public/coffee/ide/chat/controllers/ChatMessageController.coffee index f8ed988d62..6c79efd0ad 100644 --- a/services/web/public/coffee/ide/chat/controllers/ChatMessageController.coffee +++ b/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) ] \ No newline at end of file diff --git a/services/web/public/coffee/ide/colors/ColorManager.coffee b/services/web/public/coffee/ide/colors/ColorManager.coffee new file mode 100644 index 0000000000..c55539bed0 --- /dev/null +++ b/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 + \ No newline at end of file diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor/highlights/HighlightsManager.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor/highlights/HighlightsManager.coffee index d1f520d8f1..92f3ac599c 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor/highlights/HighlightsManager.coffee +++ b/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 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 new file mode 100644 index 0000000000..b6c953d030 --- /dev/null +++ b/services/web/public/coffee/ide/editor/directives/aceEditor/track-changes/ChangesTracker.coffee @@ -0,0 +1,322 @@ +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 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 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 !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 + # 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 + + for change in remove_changes + @_removeChange change + + op.d = @_applyOpModifications(op.d, op_modifications) + if 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_changed = moved_changes.concat(results.moved_changes) + for change in results.remove_changes + @_removeChange 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 } 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 abcb8817c1..bceb4b7d9a 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 @@ -1,7 +1,9 @@ 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 @@ -9,6 +11,7 @@ define [ @changesTracker = new ChangesTracker() @changeIdToMarkerIdMap = {} @enabled = false + window.changesTracker ?= @changesTracker @changesTracker.on "insert:added", (change) => @_onInsertAdded(change) @@ -28,14 +31,17 @@ define [ # will have fired, before we decide if it was a remote op. setTimeout () => 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) + @applyChange(e, { user_id }) # TODO: Just for debugging, remove before going live. setTimeout () => @@ -80,9 +86,9 @@ define [ if marker.clazz.match("track-changes") console.error "Orphaned ace marker", marker - applyChange: (delta) -> + applyChange: (delta, metadata) -> op = @_aceChangeToShareJs(delta) - @changesTracker.applyOp(op) + @changesTracker.applyOp(op, metadata) updateReviewEntriesScope: () -> # TODO: Update in place so Angular doesn't have to redo EVERYTHING @@ -110,7 +116,19 @@ define [ 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 = "background-color: #{colorScheme.highlightBackgroundColor}" + 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) + + # marker_id = session.addMarker(ace_range, "track-changes-added-marker", "text") @changeIdToMarkerIdMap[change.id] = marker_id @updateReviewEntriesScope() @@ -132,7 +150,14 @@ 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() @@ -192,262 +217,3 @@ define [ marker.range.end = end @editor.renderer.updateBackMarkers() @updateReviewEntriesScope() - - 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 \ No newline at end of file diff --git a/services/web/public/coffee/ide/history/HistoryManager.coffee b/services/web/public/coffee/ide/history/HistoryManager.coffee index 12de2149d8..66c855ebc6 100644 --- a/services/web/public/coffee/ide/history/HistoryManager.coffee +++ b/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 diff --git a/services/web/public/coffee/ide/online-users/OnlineUsersManager.coffee b/services/web/public/coffee/ide/online-users/OnlineUsersManager.coffee index f6633267c9..90fd5ed6b5 100644 --- a/services/web/public/coffee/ide/online-users/OnlineUsersManager.coffee +++ b/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 - diff --git a/services/web/public/stylesheets/app/editor.less b/services/web/public/stylesheets/app/editor.less index c8773b07d8..a7f4a80062 100644 --- a/services/web/public/stylesheets/app/editor.less +++ b/services/web/public/stylesheets/app/editor.less @@ -143,7 +143,6 @@ .track-changes-added-marker { border-radius: 0; position: absolute; - background-color: hsl(100, 70%, 70%); } .track-changes-deleted-marker { border-radius: 0; From 2b94c69795dd7e7e1a1679fecc6c7349e495bc67 Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 20 Oct 2016 12:19:57 +0100 Subject: [PATCH 10/13] Don't use the trackChanges engine in the history editor panel --- services/web/app/views/project/editor/editor.jade | 3 ++- .../web/public/coffee/ide/editor/directives/aceEditor.coffee | 3 ++- .../aceEditor/track-changes/TrackChangesManager.coffee | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/services/web/app/views/project/editor/editor.jade b/services/web/app/views/project/editor/editor.jade index c824130cbc..ecdc435831 100644 --- a/services/web/app/views/project/editor/editor.jade +++ b/services/web/app/views/project/editor/editor.jade @@ -39,7 +39,8 @@ div.full-size( syntax-validation="settings.syntaxValidation", review-panel="reviewPanel", on-scroll="onScroll", - scroll-events="scrollEvents" + scroll-events="scrollEvents", + track-changes-enabled="true" ) #review-panel .review-panel-scroller diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor.coffee index bc4e656a1c..0ba4f269e3 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor.coffee @@ -48,6 +48,7 @@ define [ reviewPanel: "=" onScroll: "=" scrollEvents: "=" + trackChangesEnabled: "=" } link: (scope, element, attrs) -> # Don't freak out if we're already in an apply callback @@ -74,7 +75,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 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 bceb4b7d9a..80260f4888 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 @@ -103,7 +103,7 @@ define [ recalculateReviewEntriesScreenPositions: () -> session = @editor.getSession() renderer = @editor.renderer - for entry_id, entry of @$scope.reviewPanel.entries + 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 From 8f9a4882c8c792498ab2530599cbf59fc3b471a7 Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 20 Oct 2016 15:04:10 +0100 Subject: [PATCH 11/13] Don't let widgets in review panel overlap --- .../web/app/views/project/editor/editor.jade | 4 +-- .../review-panel/ReviewPanelManager.coffee | 1 + .../directives/reviewPanelSorted.coffee | 26 +++++++++++++++++++ 3 files changed, 29 insertions(+), 2 deletions(-) create mode 100644 services/web/public/coffee/ide/review-panel/directives/reviewPanelSorted.coffee diff --git a/services/web/app/views/project/editor/editor.jade b/services/web/app/views/project/editor/editor.jade index ecdc435831..f33e79ee12 100644 --- a/services/web/app/views/project/editor/editor.jade +++ b/services/web/app/views/project/editor/editor.jade @@ -44,8 +44,8 @@ div.full-size( ) #review-panel .review-panel-scroller - .review-entry-list - .review-entry(ng-repeat="(entry_id, entry) in reviewPanel.entries", ng-style="{'top': entry.screenPos.y}") + .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 diff --git a/services/web/public/coffee/ide/review-panel/ReviewPanelManager.coffee b/services/web/public/coffee/ide/review-panel/ReviewPanelManager.coffee index 2670310f25..6a27e13f3e 100644 --- a/services/web/public/coffee/ide/review-panel/ReviewPanelManager.coffee +++ b/services/web/public/coffee/ide/review-panel/ReviewPanelManager.coffee @@ -1,3 +1,4 @@ define [ "ide/review-panel/controllers/ReviewPanelController" + "ide/review-panel/directives/reviewPanelSorted" ], () -> \ No newline at end of file diff --git a/services/web/public/coffee/ide/review-panel/directives/reviewPanelSorted.coffee b/services/web/public/coffee/ide/review-panel/directives/reviewPanelSorted.coffee new file mode 100644 index 0000000000..5d7e14365b --- /dev/null +++ b/services/web/public/coffee/ide/review-panel/directives/reviewPanelSorted.coffee @@ -0,0 +1,26 @@ +define [ + "base" +], (App) -> + console.log "Defingint", "reviePanelSorted" + App.directive "reviewPanelSorted", () -> + return { + link: (scope, element, attrs) -> + scope.$watch "reviewPanel.entries", (value) -> + return if !value? + console.log "reviewPanel.entries updates", entries + 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 + } \ No newline at end of file From 60a81beb1168bb0d2e52ba6f71bc987b0649dcec Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 20 Oct 2016 15:18:45 +0100 Subject: [PATCH 12/13] Try out different styling for highlights --- .../aceEditor/track-changes/TrackChangesManager.coffee | 3 +-- services/web/public/stylesheets/app/editor.less | 2 ++ 2 files changed, 3 insertions(+), 2 deletions(-) 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 80260f4888..7f68670849 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 @@ -121,14 +121,13 @@ define [ colorScheme = ColorManager.getColorScheme(hue, @element) markerLayer = @editor.renderer.$markerBack klass = "track-changes-added-marker" - style = "background-color: #{colorScheme.highlightBackgroundColor}" + 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) - # marker_id = session.addMarker(ace_range, "track-changes-added-marker", "text") @changeIdToMarkerIdMap[change.id] = marker_id @updateReviewEntriesScope() diff --git a/services/web/public/stylesheets/app/editor.less b/services/web/public/stylesheets/app/editor.less index a7f4a80062..8eb670e5a6 100644 --- a/services/web/public/stylesheets/app/editor.less +++ b/services/web/public/stylesheets/app/editor.less @@ -143,6 +143,8 @@ .track-changes-added-marker { border-radius: 0; position: absolute; + border-bottom: 1px dashed green; + background-color: hsl(100, 70%, 85%); } .track-changes-deleted-marker { border-radius: 0; From 2ac405e58c3931e347459efba802f5e9707483ca Mon Sep 17 00:00:00 2001 From: James Allen Date: Thu, 20 Oct 2016 16:59:58 +0100 Subject: [PATCH 13/13] Allow track changes to be toggled on and off --- .../track-changes/ChangesTracker.coffee | 30 ++++++++++++------- .../track-changes/TrackChangesManager.coffee | 6 ++++ .../directives/reviewPanelSorted.coffee | 2 -- 3 files changed, 25 insertions(+), 13 deletions(-) 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 b6c953d030..4c87a2ddd3 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 @@ -101,7 +101,8 @@ define [ # 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 + if @track_changes and + is_change_overlapping and !is_insert_blocked_by_delete and !already_merged and is_same_user @@ -115,7 +116,7 @@ define [ # 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 and change_start < op_start < change_end + 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 @@ -139,7 +140,7 @@ define [ previous_change = change - if !already_merged + if @track_changes and !already_merged @_addOp op, metadata for {op, metadata} in new_changes @_addOp op, metadata @@ -216,21 +217,27 @@ define [ 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 + 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 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 + 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 op.d.length > 0 + 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. @@ -240,9 +247,10 @@ define [ # |-- user_1 insert --||-- user_1 insert --| # We need to merge these together again results = @_scanAndMergeAdjacentUpdates() - moved_changed = moved_changes.concat(results.moved_changes) + 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 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 7f68670849..c5e2cb5669 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 @@ -9,19 +9,25 @@ define [ 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) => 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 5d7e14365b..6a3ff888c2 100644 --- a/services/web/public/coffee/ide/review-panel/directives/reviewPanelSorted.coffee +++ b/services/web/public/coffee/ide/review-panel/directives/reviewPanelSorted.coffee @@ -1,13 +1,11 @@ define [ "base" ], (App) -> - console.log "Defingint", "reviePanelSorted" App.directive "reviewPanelSorted", () -> return { link: (scope, element, attrs) -> scope.$watch "reviewPanel.entries", (value) -> return if !value? - console.log "reviewPanel.entries updates", entries entries = [] for el in element.find(".review-entry") entries.push {