From c9b9ae4180e888ebdf668af01118ae8be2b83358 Mon Sep 17 00:00:00 2001 From: Domagoj Kriskovic Date: Tue, 20 Feb 2024 12:28:51 +0100 Subject: [PATCH] [overleaf-core-editor] Make comment properties immutable (#17022) * make range class immutable * rename variable * use newRanges * range readonly props * skrinkBy test * Fix range shrinking bug * [overleaf-core-editor] Make comment properties immutable * remove added line * format fix * make readonly array * [overleaf-editor-core] AddCommentOperation and DeleteCommentOperation (#16871) * [overleaf-editor-core] AddCommentOperation and DeleteCommentOperation * added add comment op test * delete comment op test * import core to escape circle deps * desctructure in tests * require directly in builder * invert of add comment is always delete comment * no merging on compose * NoOp if comment is not found * use comment.clone() * update test * change CommentRawData type * jsdoc assert type * fix formating * EditNoOperation * return other in compose * use ReturnType * Revert "use ReturnType" This reverts commit 2c7e04f1541310e9fc08963170a783a437ed1992. * transorm add comment operation * transform delete comment operation * moved comment.js * format fix * fix transform addComment and textoperation * fix merge * test more complex test operations * change to else if * move range.js * fix types * fix AddComment and TextOperation transform * fixed AddComment-TextOperation trasform, added test * deletecommentoperation should win * should not delete comment * remove unused function, fix type * fix format * add resolved for existing comment * transform EditNoOperation * fix test description * change the order of EditNoOperation * fix DeleteCommentOperation-DeleteCommentOperation transform * fix types after merging main * refactor operation types * fix errors after merging * fix bad merge * format fix * removed comment.clone() * return old comment * remove unused var GitOrigin-RevId: e31d723075cb04b0b7177e7cae0014c295f92a68 --- libraries/overleaf-editor-core/lib/comment.js | 57 +++++------- .../lib/file_data/comment_list.js | 15 +-- .../lib/operation/delete_comment_operation.js | 2 +- .../operation/edit_operation_transformer.js | 6 +- .../overleaf-editor-core/test/comment.test.js | 93 +++++++++---------- 5 files changed, 78 insertions(+), 95 deletions(-) diff --git a/libraries/overleaf-editor-core/lib/comment.js b/libraries/overleaf-editor-core/lib/comment.js index b24a7cd781..59bbf1c8a3 100644 --- a/libraries/overleaf-editor-core/lib/comment.js +++ b/libraries/overleaf-editor-core/lib/comment.js @@ -9,34 +9,24 @@ const Range = require('./range') class Comment { /** - * @type {Range[]} + * @readonly + * @type {ReadonlyArray} */ ranges = [] /** + * @readonly * @type {boolean} */ resolved = false /** - * @param {Range[]} ranges + * @param {ReadonlyArray} ranges * @param {boolean} [resolved] */ constructor(ranges, resolved = false) { this.resolved = resolved - for (const range of ranges) { - this.addRange(range) - } - } - - /** - * - * @param {Range} range - */ - addRange(range) { - this.ranges.push(range) - this.ranges.sort((a, b) => a.start - b.start) - this.mergeRanges() + this.ranges = this.mergeRanges(ranges) } /** @@ -44,6 +34,7 @@ class Comment { * @param {number} cursor * @param {number} length * @param {boolean} [extendComment] + * @returns {Comment} */ applyInsert(cursor, length, extendComment = false) { let existingRangeExtended = false @@ -91,17 +82,18 @@ class Comment { } } - this.ranges = newRanges - // if the insert is not inside any range, add a new range if (extendComment && !existingRangeExtended) { - this.addRange(new Range(cursor, length)) + newRanges.push(new Range(cursor, length)) } + + return new Comment(newRanges, this.resolved) } /** * * @param {Range} deletedRange + * @returns {Comment} */ applyDelete(deletedRange) { const newRanges = [] @@ -116,26 +108,29 @@ class Comment { } } - this.ranges = newRanges - this.mergeRanges() + return new Comment(newRanges, this.resolved) } /** * * @param {TextOperation} operation + * @returns {Comment} */ applyTextOperation(operation) { + /** @type {Comment} */ + let comment = this let cursor = 0 for (const op of operation.ops) { if (op instanceof RetainOp) { cursor += op.length } else if (op instanceof InsertOp) { - this.applyInsert(cursor, op.insertion.length) + comment = comment.applyInsert(cursor, op.insertion.length) cursor += op.insertion.length } else if (op instanceof RemoveOp) { - this.applyDelete(new Range(cursor, op.length)) + comment = comment.applyDelete(new Range(cursor, op.length)) } } + return comment } isEmpty() { @@ -153,11 +148,16 @@ class Comment { } } - mergeRanges() { + /** + * @param {ReadonlyArray} ranges + * @returns {ReadonlyArray} + */ + mergeRanges(ranges) { /** @type {Range[]} */ const mergedRanges = [] - for (const range of this.ranges) { + const sortedRanges = [...ranges].sort((a, b) => a.start - b.start) + for (const range of sortedRanges) { if (range.isEmpty()) { continue } @@ -170,14 +170,7 @@ class Comment { } } - this.ranges = mergedRanges - } - - /** - * @returns {Comment} - */ - clone() { - return Comment.fromRaw(this.toRaw()) + return mergedRanges } /** diff --git a/libraries/overleaf-editor-core/lib/file_data/comment_list.js b/libraries/overleaf-editor-core/lib/file_data/comment_list.js index 4c22d7b596..a925d0ece9 100644 --- a/libraries/overleaf-editor-core/lib/file_data/comment_list.js +++ b/libraries/overleaf-editor-core/lib/file_data/comment_list.js @@ -41,10 +41,9 @@ class CommentList { add(id, newComment) { const existingComment = this.getComment(id) if (existingComment) { - existingComment.resolved = existingComment.resolved && newComment.resolved - for (const range of newComment.ranges) { - existingComment.addRange(range) - } + const resolved = existingComment.resolved && newComment.resolved + const mergedRanges = [...existingComment.ranges, ...newComment.ranges] + this.comments.set(id, new Comment(mergedRanges, resolved)) } else { this.comments.set(id, newComment) } @@ -77,11 +76,12 @@ class CommentList { opts.commentIds = [] } for (const [commentId, comment] of this.comments) { - comment.applyInsert( + const commentAfterInsert = comment.applyInsert( range.pos, range.length, opts.commentIds.includes(commentId) ) + this.comments.set(commentId, commentAfterInsert) } } @@ -89,8 +89,9 @@ class CommentList { * @param {Range} range */ applyDelete(range) { - for (const [, comment] of this.comments) { - comment.applyDelete(range) + for (const [commentId, comment] of this.comments) { + const commentAfterDelete = comment.applyDelete(range) + this.comments.set(commentId, commentAfterDelete) } } diff --git a/libraries/overleaf-editor-core/lib/operation/delete_comment_operation.js b/libraries/overleaf-editor-core/lib/operation/delete_comment_operation.js index 1a33df5441..d18c264d26 100644 --- a/libraries/overleaf-editor-core/lib/operation/delete_comment_operation.js +++ b/libraries/overleaf-editor-core/lib/operation/delete_comment_operation.js @@ -50,7 +50,7 @@ class DeleteCommentOperation extends EditOperation { return new EditNoOperation() } - return new core.AddCommentOperation(this.commentId, comment.clone()) + return new core.AddCommentOperation(this.commentId, comment) } /** diff --git a/libraries/overleaf-editor-core/lib/operation/edit_operation_transformer.js b/libraries/overleaf-editor-core/lib/operation/edit_operation_transformer.js index b3fa32ac37..8d75402bc8 100644 --- a/libraries/overleaf-editor-core/lib/operation/edit_operation_transformer.js +++ b/libraries/overleaf-editor-core/lib/operation/edit_operation_transformer.js @@ -31,14 +31,12 @@ class EditOperationTransformer { } if (a instanceof AddCommentOperation && b instanceof TextOperation) { - const comment = a.comment.clone() - comment.applyTextOperation(b) + const comment = a.comment.applyTextOperation(b) return [new AddCommentOperation(a.commentId, comment), b] } if (a instanceof TextOperation && b instanceof AddCommentOperation) { - const comment = b.comment.clone() - comment.applyTextOperation(a) + const comment = b.comment.applyTextOperation(a) return [a, new AddCommentOperation(b.commentId, comment)] } diff --git a/libraries/overleaf-editor-core/test/comment.test.js b/libraries/overleaf-editor-core/test/comment.test.js index b2686bf4a6..9e066700ed 100644 --- a/libraries/overleaf-editor-core/test/comment.test.js +++ b/libraries/overleaf-editor-core/test/comment.test.js @@ -8,77 +8,72 @@ const Range = require('../lib/range') describe('Comment', function () { it('should move ranges to the right of insert', function () { const comment = new Comment([new Range(5, 10)]) - comment.applyInsert(3, 5, false) - expect(comment.ranges).to.eql([new Range(10, 10)]) + const resComment = comment.applyInsert(3, 5, false) + expect(resComment.ranges).to.eql([new Range(10, 10)]) }) - it('should expand the range after insert inside it', function () { - const comment = new Comment([new Range(5, 10)]) - comment.applyInsert(4, 1) // inserting 1 char before the range - expect(comment.ranges).to.eql([new Range(6, 10)]) - comment.applyInsert(6, 1) // inserting 1 char at the edge (without expandCommand = false) - expect(comment.ranges).to.eql([new Range(7, 10)]) - comment.applyInsert(7, 1, true) // inserting 1 char at the edge (with expandCommand = true) - expect(comment.ranges).to.eql([new Range(7, 11)]) - comment.applyInsert(8, 1, true) // inserting 1 char inside the range - expect(comment.ranges).to.eql([new Range(7, 12)]) + describe('applyInsert', function () { + it('should insert 1 char before the range', function () { + const comment = new Comment([new Range(5, 10)]) + expect(comment.applyInsert(4, 1).ranges).to.eql([new Range(6, 10)]) + }) + + it('should insert 1 char at the edge, without expandCommand', function () { + const comment = new Comment([new Range(5, 10)]) + expect(comment.applyInsert(5, 1).ranges).to.eql([new Range(6, 10)]) + }) + + it('should insert 1 char at the edge, with expandCommand', function () { + const comment = new Comment([new Range(5, 10)]) + expect(comment.applyInsert(5, 1, true).ranges).to.eql([new Range(5, 11)]) + }) + + it('should expand the range after insert inside it', function () { + const comment = new Comment([new Range(5, 10)]) + expect(comment.applyInsert(6, 1, true).ranges).to.eql([new Range(5, 11)]) + }) }) it('should split the range if inside another and expandComment is false', function () { - const comment1 = new Comment([new Range(5, 10)]) - comment1.applyInsert(6, 10, false) - expect(comment1.ranges).to.eql([new Range(5, 1), new Range(16, 9)]) + const comment = new Comment([new Range(5, 10)]) + const commentRes = comment.applyInsert(6, 10, false) + expect(commentRes.ranges).to.eql([new Range(5, 1), new Range(16, 9)]) + }) - // insert at the end of the range - const comment2 = new Comment([new Range(5, 10)]) - comment2.applyInsert(14, 10, false) - expect(comment2.ranges).to.eql([new Range(5, 9), new Range(24, 1)]) + it('should insert the range if expandComment is false', function () { + const comment = new Comment([new Range(5, 10)]) + const commentRes = comment.applyInsert(14, 10, false) + expect(commentRes.ranges).to.eql([new Range(5, 9), new Range(24, 1)]) }) it('should move the range if insert is at range start and expandComment is false', function () { const comment = new Comment([new Range(5, 10)]) - comment.applyInsert(5, 10, false) - expect(comment.ranges).to.eql([new Range(15, 10)]) + const commentRes = comment.applyInsert(5, 10, false) + expect(commentRes.ranges).to.eql([new Range(15, 10)]) }) it('should ignore the range if insert is at range end and expandComment is false', function () { const comment = new Comment([new Range(5, 10)]) - comment.applyInsert(15, 10, false) - expect(comment.ranges).to.eql([new Range(5, 10)]) + const commentRes = comment.applyInsert(15, 10, false) + expect(commentRes.ranges).to.eql([new Range(5, 10)]) }) it('should expand the range after inserting on the edge of it if expandComment is true', function () { const comment = new Comment([new Range(5, 10)]) - comment.applyInsert(15, 10, true) - expect(comment.ranges).to.eql([new Range(5, 20)]) + const commentRes = comment.applyInsert(15, 10, true) + expect(commentRes.ranges).to.eql([new Range(5, 20)]) }) - it('should add a new range if expandComment is true and not inside any range', function () { - const commentNoRanges = new Comment([]) - commentNoRanges.applyInsert(5, 10, true) - expect(commentNoRanges.ranges).to.eql([new Range(5, 10)]) - - const commentWithRanges = new Comment([new Range(5, 10)]) - commentWithRanges.applyInsert(50, 10, true) - expect(commentWithRanges.ranges).to.eql([ - new Range(5, 10), - new Range(50, 10), - ]) - }) - - it('should move ranges if delete is before it', function () { - const from5to14 = new Comment([new Range(5, 10)]) - const from3to7 = new Range(3, 5) - - from5to14.applyDelete(from3to7) - const from3to9 = new Range(3, 7) - expect(from5to14.ranges).to.eql([from3to9]) + it('should move comment ranges if delete is before it', function () { + const comment = new Comment([new Range(5, 10)]) + const commentRes = comment.applyDelete(new Range(3, 5)) + expect(commentRes.ranges).to.eql([new Range(3, 7)]) }) it('should merge ranges after delete', function () { const comment = new Comment([new Range(5, 10), new Range(20, 10)]) - comment.applyDelete(new Range(7, 18)) - expect(comment.ranges).to.eql([new Range(5, 7)]) + const commentRes = comment.applyDelete(new Range(7, 18)) + expect(commentRes.ranges).to.eql([new Range(5, 7)]) }) it('should merge overlapping ranges', function () { @@ -87,7 +82,6 @@ describe('Comment', function () { new Range(15, 20), new Range(50, 10), ]) - comment.mergeRanges() expect(comment.ranges).to.eql([new Range(5, 30), new Range(50, 10)]) }) @@ -97,7 +91,6 @@ describe('Comment', function () { new Range(50, 10), new Range(5, 10), ]) - comment.mergeRanges() expect(comment.ranges).to.eql([new Range(5, 30), new Range(50, 10)]) }) @@ -107,7 +100,6 @@ describe('Comment', function () { new Range(10, 5), new Range(50, 10), ]) - comment.mergeRanges() expect(comment.ranges).to.eql([new Range(5, 10), new Range(50, 10)]) }) @@ -117,7 +109,6 @@ describe('Comment', function () { new Range(15, 5), new Range(50, 10), ]) - comment.mergeRanges() expect(comment.ranges).to.eql([new Range(5, 15), new Range(50, 10)]) }) })