From 55c342134c657948f8d2696825c94487fe3566bf Mon Sep 17 00:00:00 2001 From: Eric Mc Sween <5454374+emcsween@users.noreply.github.com> Date: Tue, 4 Jun 2024 07:36:44 -0400 Subject: [PATCH] Merge pull request #18659 from overleaf/em-crop-comments-tracked-deletes Crop comments when processing tracked deletes GitOrigin-RevId: 662c9ed86a8ed4959d1671ce466548487f334f45 --- .../lib/operation/add_comment_operation.js | 6 + .../document-updater/app/js/RangesManager.js | 84 ++++++++++++- .../js/RangesManager/RangesManagerTests.js | 117 ++++++++++++++++++ .../app/js/UpdateTranslator.js | 8 +- .../UpdateTranslator/UpdateTranslatorTests.js | 6 + 5 files changed, 214 insertions(+), 7 deletions(-) diff --git a/libraries/overleaf-editor-core/lib/operation/add_comment_operation.js b/libraries/overleaf-editor-core/lib/operation/add_comment_operation.js index 6e50d4d51e..2b08d02572 100644 --- a/libraries/overleaf-editor-core/lib/operation/add_comment_operation.js +++ b/libraries/overleaf-editor-core/lib/operation/add_comment_operation.js @@ -23,6 +23,12 @@ class AddCommentOperation extends EditOperation { constructor(commentId, ranges, resolved = false) { super() + for (const range of ranges) { + if (range.isEmpty()) { + throw new Error("AddCommentOperation can't be built with empty ranges") + } + } + /** @readonly */ this.commentId = commentId diff --git a/services/document-updater/app/js/RangesManager.js b/services/document-updater/app/js/RangesManager.js index 4846850782..0970de5fb4 100644 --- a/services/document-updater/app/js/RangesManager.js +++ b/services/document-updater/app/js/RangesManager.js @@ -56,20 +56,36 @@ const RangesManager = { RangesManager._emptyRangesCount(rangesTracker) const historyUpdates = [] for (const update of updates) { - rangesTracker.track_changes = Boolean(update.meta?.tc) + const trackingChanges = Boolean(update.meta?.tc) + rangesTracker.track_changes = trackingChanges if (update.meta?.tc) { rangesTracker.setIdSeed(update.meta.tc) } const historyOps = [] for (const op of update.op) { + let croppedCommentOps = [] if (opts.historyRangesSupport) { historyOps.push( getHistoryOp(op, rangesTracker.comments, rangesTracker.changes) ) + if (isDelete(op) && trackingChanges) { + // If a tracked delete overlaps a comment, the comment must be + // cropped. The extent of the cropping is calculated before the + // delete is applied, but the cropping operations are applied + // later, after the delete is applied. + croppedCommentOps = getCroppedCommentOps(op, rangesTracker.comments) + } } else if (isInsert(op) || isDelete(op)) { historyOps.push(op) } rangesTracker.applyOp(op, { user_id: update.meta?.user_id }) + if (croppedCommentOps.length > 0) { + historyOps.push( + ...croppedCommentOps.map(op => + getHistoryOpForComment(op, rangesTracker.changes) + ) + ) + } } if (historyOps.length > 0) { historyUpdates.push({ ...update, op: historyOps }) @@ -486,4 +502,70 @@ function getHistoryOpForComment(op, changes) { return historyOp } +/** + * Return the ops necessary to properly crop comments when a tracked delete is + * received + * + * The editor treats a tracked delete as a proper delete and updates the + * comment range accordingly. The history doesn't do that and remembers the + * extent of the comment in the tracked delete. In order to keep the history + * consistent with the editor, we'll send ops that will crop the comment in + * the history. + * + * @param {DeleteOp} op + * @param {Comment[]} comments + * @returns {CommentOp[]} + */ +function getCroppedCommentOps(op, comments) { + const deleteStart = op.p + const deleteLength = op.d.length + const deleteEnd = deleteStart + deleteLength + + /** @type {HistoryCommentOp[]} */ + const historyCommentOps = [] + for (const comment of comments) { + const commentStart = comment.op.p + const commentLength = comment.op.c.length + const commentEnd = commentStart + commentLength + + if (deleteStart <= commentStart && deleteEnd > commentStart) { + // The comment overlaps the start of the comment or all of it. + const overlapLength = Math.min(deleteEnd, commentEnd) - commentStart + + /** @type {CommentOp} */ + const commentOp = { + p: deleteStart, + c: comment.op.c.slice(overlapLength), + t: comment.op.t, + } + if (comment.op.resolved) { + commentOp.resolved = true + } + + historyCommentOps.push(commentOp) + } else if ( + deleteStart > commentStart && + deleteStart < commentEnd && + deleteEnd >= commentEnd + ) { + // The comment overlaps the end of the comment. + const overlapLength = commentEnd - deleteStart + + /** @type {CommentOp} */ + const commentOp = { + p: commentStart, + c: comment.op.c.slice(0, -overlapLength), + t: comment.op.t, + } + if (comment.op.resolved) { + commentOp.resolved = true + } + + historyCommentOps.push(commentOp) + } + } + + return historyCommentOps +} + module.exports = RangesManager diff --git a/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js b/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js index 564c8cb679..7c4bd51229 100644 --- a/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js +++ b/services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js @@ -518,6 +518,123 @@ describe('RangesManager', function () { ]) }) }) + + describe('tracked delete that overlaps the start of a comment', function () { + beforeEach(function () { + // original text is "one three four five" + this.ranges = { + comments: makeRanges([{ c: 'three', p: 4, t: 'comment-id-1' }]), + } + this.updates = makeUpdates([{ d: 'ne thr', p: 1 }], { + tc: 'tracking-id', + }) + this.newDocLines = ['oee four five'] + this.result = this.RangesManager.applyUpdate( + this.project_id, + this.doc_id, + this.ranges, + this.updates, + this.newDocLines, + { historyRangesSupport: true } + ) + }) + + it('should crop the beginning of the comment', function () { + expect(this.result.historyUpdates.map(x => x.op)).to.deep.equal([ + [ + { d: 'ne thr', p: 1 }, + { c: 'ee', p: 1, hpos: 7, t: 'comment-id-1' }, + ], + ]) + }) + }) + + describe('tracked delete that overlaps a whole comment', function () { + beforeEach(function () { + // original text is "one three four five" + this.ranges = { + comments: makeRanges([{ c: 'three', p: 4, t: 'comment-id-1' }]), + } + this.updates = makeUpdates([{ d: 'ne three f', p: 1 }], { + tc: 'tracking-id', + }) + this.newDocLines = ['oour five'] + this.result = this.RangesManager.applyUpdate( + this.project_id, + this.doc_id, + this.ranges, + this.updates, + this.newDocLines, + { historyRangesSupport: true } + ) + }) + + it('should crop the beginning of the comment', function () { + expect(this.result.historyUpdates.map(x => x.op)).to.deep.equal([ + [ + { d: 'ne three f', p: 1 }, + { c: '', p: 1, hpos: 11, t: 'comment-id-1' }, + ], + ]) + }) + }) + + describe('tracked delete that overlaps the end of a comment', function () { + beforeEach(function () { + // original text is "one three four five" + this.ranges = { + comments: makeRanges([{ c: 'three', p: 4, t: 'comment-id-1' }]), + } + this.updates = makeUpdates([{ d: 'ee f', p: 7 }], { + tc: 'tracking-id', + }) + this.newDocLines = ['one throur five'] + this.result = this.RangesManager.applyUpdate( + this.project_id, + this.doc_id, + this.ranges, + this.updates, + this.newDocLines, + { historyRangesSupport: true } + ) + }) + + it('should crop the end of the comment', function () { + expect(this.result.historyUpdates.map(x => x.op)).to.deep.equal([ + [ + { d: 'ee f', p: 7 }, + { c: 'thr', p: 4, t: 'comment-id-1' }, + ], + ]) + }) + }) + + describe('tracked delete that overlaps the inside of a comment', function () { + beforeEach(function () { + // original text is "one three four five" + this.ranges = { + comments: makeRanges([{ c: 'three', p: 4, t: 'comment-id-1' }]), + } + this.updates = makeUpdates([{ d: 'hre', p: 5 }], { + tc: 'tracking-id', + }) + this.newDocLines = ['one te four five'] + this.result = this.RangesManager.applyUpdate( + this.project_id, + this.doc_id, + this.ranges, + this.updates, + this.newDocLines, + { historyRangesSupport: true } + ) + }) + + it('should not crop the comment', function () { + expect(this.result.historyUpdates.map(x => x.op)).to.deep.equal([ + [{ d: 'hre', p: 5 }], + ]) + }) + }) }) }) diff --git a/services/project-history/app/js/UpdateTranslator.js b/services/project-history/app/js/UpdateTranslator.js index 3a9ead2056..ade0fce53f 100644 --- a/services/project-history/app/js/UpdateTranslator.js +++ b/services/project-history/app/js/UpdateTranslator.js @@ -280,15 +280,11 @@ class OperationsBuilder { this.pushTextOperation() // Add a comment operation + const commentLength = op.hlen ?? op.c.length const commentOp = { pathname: this.pathname, commentId: op.t, - ranges: [ - { - pos, - length: op.hlen ?? op.c.length, - }, - ], + ranges: commentLength > 0 ? [{ pos, length: commentLength }] : [], } if ('resolved' in op) { commentOp.resolved = op.resolved diff --git a/services/project-history/test/unit/js/UpdateTranslator/UpdateTranslatorTests.js b/services/project-history/test/unit/js/UpdateTranslator/UpdateTranslatorTests.js index a925a15ef8..cbab154db2 100644 --- a/services/project-history/test/unit/js/UpdateTranslator/UpdateTranslatorTests.js +++ b/services/project-history/test/unit/js/UpdateTranslator/UpdateTranslatorTests.js @@ -776,6 +776,7 @@ describe('UpdateTranslator', function () { { p: 3, d: 'bar' }, { p: 5, c: 'comment this', t: 'comment-id-1' }, { p: 7, c: 'another comment', t: 'comment-id-2' }, + { p: 9, c: '', t: 'comment-id-3' }, { p: 10, i: 'baz' }, ], v: this.version, @@ -812,6 +813,11 @@ describe('UpdateTranslator', function () { commentId: 'comment-id-2', ranges: [{ pos: 7, length: 15 }], }, + { + pathname: 'main.tex', + commentId: 'comment-id-3', + ranges: [], + }, { pathname: 'main.tex', textOperation: [10, 'baz', 10],