From 898277b4afee2bf973747ccf4239a357665d6bfb Mon Sep 17 00:00:00 2001 From: James Allen Date: Tue, 13 Dec 2016 17:34:29 +0000 Subject: [PATCH] Refactor ops model so it all happens in Document --- .../web/app/views/project/editor/editor.jade | 1 - .../public/coffee/ide/editor/Document.coffee | 23 +++++- .../coffee/ide/editor/ShareJsDoc.coffee | 4 +- .../ide/editor/directives/aceEditor.coffee | 1 - .../track-changes/TrackChangesManager.coffee | 78 ++++++------------- .../editor/sharejs/vendor/client/doc.coffee | 5 +- .../editor/sharejs/vendor/types/text.coffee | 66 ++++++++++++++-- .../controllers/ReviewPanelController.coffee | 11 ++- 8 files changed, 115 insertions(+), 74 deletions(-) diff --git a/services/web/app/views/project/editor/editor.jade b/services/web/app/views/project/editor/editor.jade index 3126d63df2..8d58e35f2e 100644 --- a/services/web/app/views/project/editor/editor.jade +++ b/services/web/app/views/project/editor/editor.jade @@ -53,7 +53,6 @@ div.full-size( events-bridge="reviewPanelEventsBridge" track-changes-enabled="trackChangesFeatureFlag", track-changes= "editor.trackChanges", - ranges-tracker="reviewPanel.rangesTracker", doc-id="editor.open_doc_id" ) diff --git a/services/web/public/coffee/ide/editor/Document.coffee b/services/web/public/coffee/ide/editor/Document.coffee index 213fbe3514..a0c07443c8 100644 --- a/services/web/public/coffee/ide/editor/Document.coffee +++ b/services/web/public/coffee/ide/editor/Document.coffee @@ -1,7 +1,8 @@ define [ "utils/EventEmitter" "ide/editor/ShareJsDoc" -], (EventEmitter, ShareJsDoc) -> + "ide/review-panel/RangesTracker" +], (EventEmitter, ShareJsDoc, RangesTracker) -> class Document extends EventEmitter @getDocument: (ide, doc_id) -> @openDocs ||= {} @@ -247,14 +248,15 @@ define [ @joined = true @doc.catchUp( updates ) # TODO: Worry about whether these ranges are consistent with the doc still - @opening_ranges = ranges + @ranges?.changes = ranges?.changes + @ranges?.comments = ranges?.comments callback() else @ide.socket.emit 'joinDoc', @doc_id, (error, docLines, version, updates, ranges) => return callback(error) if error? @joined = true @doc = new ShareJsDoc @doc_id, docLines, version, @ide.socket - @opening_ranges = ranges + @ranges = new RangesTracker(ranges?.changes, ranges?.comments) @_bindToShareJsDocEvents() callback() @@ -313,6 +315,8 @@ define [ inflightOp: inflightOp, pendingOp: pendingOp v: version + @doc.on "change", (ops, oldSnapshot, msg) => + @_applyOpsToRanges(ops, oldSnapshot, msg) _onError: (error, meta = {}) -> meta.doc_id = @doc_id @@ -325,3 +329,16 @@ define [ # the disconnect event, which means we try to leaveDoc when the connection comes back. # This could intefere with the new connection of a new instance of this document. @_cleanUp() + + _applyOpsToRanges: (ops = [], oldSnapshot, msg) -> + track_changes_as = null + remote_op = msg? + if remote_op and msg.meta?.tc + track_changes_as = msg.meta.user_id + else if !remote_op and @track_changes_as? + track_changes_as = @track_changes_as + console.log "CHANGED", oldSnapshot, ops, track_changes_as + @ranges.track_changes = track_changes_as? + for op in ops + console.log "APPLYING OP", op, @ranges.track_changes + @ranges.applyOp op, { user_id: track_changes_as } diff --git a/services/web/public/coffee/ide/editor/ShareJsDoc.coffee b/services/web/public/coffee/ide/editor/ShareJsDoc.coffee index bb2d6b3e38..feb82c1e3f 100644 --- a/services/web/public/coffee/ide/editor/ShareJsDoc.coffee +++ b/services/web/public/coffee/ide/editor/ShareJsDoc.coffee @@ -34,8 +34,8 @@ define [ @_doc = new ShareJs.Doc @connection, @doc_id, type: @type @_doc.setFlushDelay(SINGLE_USER_FLUSH_DELAY) - @_doc.on "change", () => - @trigger "change" + @_doc.on "change", (args...) => + @trigger "change", args... @_doc.on "acknowledge", () => @lastAcked = new Date() # note time of last ack from server for an op we sent @trigger "acknowledge" diff --git a/services/web/public/coffee/ide/editor/directives/aceEditor.coffee b/services/web/public/coffee/ide/editor/directives/aceEditor.coffee index bbbcff5a85..88ce71fcd2 100644 --- a/services/web/public/coffee/ide/editor/directives/aceEditor.coffee +++ b/services/web/public/coffee/ide/editor/directives/aceEditor.coffee @@ -56,7 +56,6 @@ define [ eventsBridge: "=" trackChanges: "=" trackChangesEnabled: "=" - rangesTracker: "=" docId: "=" } link: (scope, element, attrs) -> 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 54b20f7018..ca8452f5c0 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 @@ -10,22 +10,15 @@ define [ constructor: (@$scope, @editor, @element) -> window.trackChangesManager ?= @ - @$scope.$watch "rangesTracker", (rangesTracker) => - return if !rangesTracker? - @disconnectFromRangesTracker() - @rangesTracker = rangesTracker - @connectToRangesTracker() - @$scope.$watch "trackChanges", (track_changes) => return if !track_changes? - @rangesTracker?.track_changes = track_changes + @setTrackChanges(track_changes) @$scope.$watch "sharejsDoc", (doc) => return if !doc? - if doc.opening_ranges?.changes? - @rangesTracker.changes = doc.opening_ranges.changes - if doc.opening_ranges?.comments? - @rangesTracker.comments = doc.opening_ranges.comments + @disconnectFromRangesTracker() + @rangesTracker = doc.ranges + @connectToRangesTracker() @$scope.$on "comment:add", (e, comment) => @addCommentToSelection(comment) @@ -65,48 +58,15 @@ define [ onResize = () => @recalculateReviewEntriesScreenPositions() - onChange = (e) => - if !@editor.initing - # 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 () => - 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 - - was_tracking = @rangesTracker.track_changes - if @dont_track_next_update - @rangesTracker.track_changes = false - @dont_track_next_update = false - @applyChange(e, { user_id }) - @rangesTracker.track_changes = was_tracking - - # TODO: Just for debugging, remove before going live. - setTimeout () => - @checkMapping() - , 100 - onChangeSession = (e) => - e.oldSession?.getDocument().off "change", onChange - e.session.getDocument().on "change", onChange @redrawAnnotations() bindToAce = () => - @editor.getSession().getDocument().on "change", onChange @editor.on "changeSelection", onChangeSelection @editor.on "changeSession", onChangeSession @editor.renderer.on "resize", onResize unbindFromAce = () => - @editor.getSession().getDocument().off "change", onChange @editor.off "changeSelection", onChangeSelection @editor.off "changeSession", onChangeSession @editor.renderer.off "resize", onResize @@ -132,41 +92,49 @@ define [ @rangesTracker.off "comment:removed" @rangesTracker.off "comment:resolved" @rangesTracker.off "comment:unresolved" + + setTrackChanges: (value) -> + if value + @$scope.sharejsDoc?.track_changes_as = window.user.id + else + @$scope.sharejsDoc?.track_changes_as = null connectToRangesTracker: () -> - @rangesTracker.track_changes = @$scope.trackChanges + @setTrackChanges(@$scope.trackChanges) + # Add a timeout because on remote ops, we get these notifications before + # ace has updated @rangesTracker.on "insert:added", (change) => sl_console.log "[insert:added]", change - @_onInsertAdded(change) + setTimeout () => @_onInsertAdded(change) @rangesTracker.on "insert:removed", (change) => sl_console.log "[insert:removed]", change - @_onInsertRemoved(change) + setTimeout () => @_onInsertRemoved(change) @rangesTracker.on "delete:added", (change) => sl_console.log "[delete:added]", change - @_onDeleteAdded(change) + setTimeout () => @_onDeleteAdded(change) @rangesTracker.on "delete:removed", (change) => sl_console.log "[delete:removed]", change - @_onDeleteRemoved(change) + setTimeout () => @_onDeleteRemoved(change) @rangesTracker.on "changes:moved", (changes) => sl_console.log "[changes:moved]", changes - @_onChangesMoved(changes) + setTimeout () => @_onChangesMoved(changes) @rangesTracker.on "comment:added", (comment) => sl_console.log "[comment:added]", comment - @_onCommentAdded(comment) + setTimeout () => @_onCommentAdded(comment) @rangesTracker.on "comment:moved", (comment) => sl_console.log "[comment:moved]", comment - @_onCommentMoved(comment) + setTimeout () => @_onCommentMoved(comment) @rangesTracker.on "comment:removed", (comment) => sl_console.log "[comment:removed]", comment - @_onCommentRemoved(comment) + setTimeout () => @_onCommentRemoved(comment) @rangesTracker.on "comment:resolved", (comment) => sl_console.log "[comment:resolved]", comment - @_onCommentRemoved(comment) + setTimeout () => @_onCommentRemoved(comment) @rangesTracker.on "comment:unresolved", (comment) => sl_console.log "[comment:unresolved]", comment - @_onCommentAdded(comment) + setTimeout () => @_onCommentAdded(comment) redrawAnnotations: () -> for change in @rangesTracker.changes 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 d25baf89d5..301c4d5e04 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 @@ -71,7 +71,7 @@ class Doc # Its important that these event handlers are called with oldSnapshot. # 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 'change', docOp, oldSnapshot, msg @emit 'remoteop', docOp, oldSnapshot, msg if isRemote _connectionStateChanged: (state, data) -> @@ -274,6 +274,7 @@ class Doc submitOp: (op, callback) -> op = @type.normalize(op) if @type.normalize? + oldSnapshot = @snapshot # If this throws an exception, no changes should have been made to the doc @snapshot = @type.apply @snapshot, op @@ -284,7 +285,7 @@ class Doc @pendingCallbacks.push callback if callback - @emit 'change', op + @emit 'change', op, oldSnapshot @delayedFlush() diff --git a/services/web/public/coffee/ide/editor/sharejs/vendor/types/text.coffee b/services/web/public/coffee/ide/editor/sharejs/vendor/types/text.coffee index c64b4dfa68..2a3b79997d 100644 --- a/services/web/public/coffee/ide/editor/sharejs/vendor/types/text.coffee +++ b/services/web/public/coffee/ide/editor/sharejs/vendor/types/text.coffee @@ -31,7 +31,8 @@ checkValidComponent = (c) -> i_type = typeof c.i d_type = typeof c.d - throw new Error 'component needs an i or d field' unless (i_type == 'string') ^ (d_type == 'string') + c_type = typeof c.c + throw new Error 'component needs an i, d or c field' unless (i_type == 'string') ^ (d_type == 'string') ^ (c_type == 'string') throw new Error 'position cannot be negative' unless c.p >= 0 @@ -44,11 +45,15 @@ text.apply = (snapshot, op) -> for component in op if component.i? snapshot = strInject snapshot, component.p, component.i - else + else if component.d? deleted = snapshot[component.p...(component.p + component.d.length)] throw new Error "Delete component '#{component.d}' does not match deleted text '#{deleted}'" unless component.d == deleted snapshot = snapshot[...component.p] + snapshot[(component.p + component.d.length)..] - + else if component.c? + comment = snapshot[component.p...(component.p + component.c.length)] + throw new Error "Comment component '#{component.c}' does not match commented text '#{comment}'" unless component.c == comment + else + throw new Error "Unknown op type" snapshot @@ -112,7 +117,7 @@ transformPosition = (pos, c, insertAfter) -> pos + c.i.length else pos - else + else if c.d? # I think this could also be written as: Math.min(c.p, Math.min(c.p - otherC.p, otherC.d.length)) # but I think its harder to read that way, and it compiles using ternary operators anyway # so its no slower written like this. @@ -122,6 +127,10 @@ transformPosition = (pos, c, insertAfter) -> c.p else pos - c.d.length + else if c.c? + pos + else + throw new Error("unknown op type") # Helper method to transform a cursor position as a result of an op. # @@ -143,7 +152,7 @@ text._tc = transformComponent = (dest, c, otherC, side) -> if c.i? append dest, {i:c.i, p:transformPosition(c.p, otherC, side == 'right')} - else # Delete + else if c.d? # Delete if otherC.i? # delete vs insert s = c.d if c.p < otherC.p @@ -152,7 +161,7 @@ text._tc = transformComponent = (dest, c, otherC, side) -> if s != '' append dest, {d:s, p:c.p + otherC.i.length} - else # Delete vs delete + else if otherC.d? # Delete vs delete if c.p >= otherC.p + otherC.d.length append dest, {d:c.d, p:c.p - otherC.d.length} else if c.p + c.d.length <= otherC.p @@ -177,6 +186,51 @@ text._tc = transformComponent = (dest, c, otherC, side) -> # This could be rewritten similarly to insert v delete, above. newC.p = transformPosition newC.p, otherC append dest, newC + + else if otherC.c? + append dest, c + + else + throw new Error("unknown op type") + + else if c.c? # Comment + if otherC.i? + if c.p < otherC.p < c.p + c.c.length + offset = otherC.p - c.p + new_c = (c.c[0..(offset-1)] + otherC.i + c.c[offset...]) + append dest, {c:new_c, p:c.p, t: c.t} + else + append dest, {c:c.c, p:transformPosition(c.p, otherC, true), t: c.t} + + else if otherC.d? + if c.p >= otherC.p + otherC.d.length + append dest, {c:c.c, p:c.p - otherC.d.length, t: c.t} + else if c.p + c.c.length <= otherC.p + append dest, c + else # Delete overlaps comment + # They overlap somewhere. + newC = {c:'', p:c.p, t: c.t} + if c.p < otherC.p + newC.c = c.c[...(otherC.p - c.p)] + if c.p + c.c.length > otherC.p + otherC.d.length + newC.c += c.c[(otherC.p + otherC.d.length - c.p)..] + + # This is entirely optional - just for a check that the deleted + # text in the two ops matches + intersectStart = Math.max c.p, otherC.p + intersectEnd = Math.min c.p + c.c.length, otherC.p + otherC.d.length + cIntersect = c.c[intersectStart - c.p...intersectEnd - c.p] + otherIntersect = otherC.d[intersectStart - otherC.p...intersectEnd - otherC.p] + throw new Error 'Delete ops delete different text in the same region of the document' unless cIntersect == otherIntersect + + newC.p = transformPosition newC.p, otherC + append dest, newC + + else if otherC.c? + append dest, c + + else + throw new Error("unknown op type") dest 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 0990baac4d..df4afe306a 100644 --- a/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee +++ b/services/web/public/coffee/ide/review-panel/controllers/ReviewPanelController.coffee @@ -154,10 +154,13 @@ define [ if view == $scope.SubViews.OVERVIEW refreshOverviewPanel() - $scope.$watch "editor.open_doc_id", (open_doc_id) -> - return if !open_doc_id? - rangesTrackers[open_doc_id] ?= new RangesTracker() - $scope.reviewPanel.rangesTracker = rangesTrackers[open_doc_id] + $scope.$watch "editor.sharejs_doc", (doc) -> + return if !doc? + console.log "DOC changed", doc + # The open doc range tracker is kept up to date in real-time so + # replace any outdated info with this + rangesTrackers[doc.doc_id] = doc.ranges + $scope.reviewPanel.rangesTracker = rangesTrackers[doc.doc_id] $scope.$watch (() -> entries = $scope.reviewPanel.entries[$scope.editor.open_doc_id] or {}