From 1ab13e8d46b12e13996ac383ad319a0088bf6771 Mon Sep 17 00:00:00 2001 From: Bach Bui Date: Wed, 13 Nov 2024 14:16:15 -0500 Subject: [PATCH 1/5] fix: extract nested slices Fix for the util method `extractSlices`. When extracting slices, we need to adjust the ranges of marks that appear after each slice in the document. In order to do this, we keep track of the ranges that are to be deleted and then loop over them, adjusting the mark ranges as needed. However, when we have nested slices, each slice may be split by another slice, causing a slice to have multiple ranges. In this case, the list of ranges to be deleted would be overlapping, and thus eventually cause the mark ranges to be adjusted incorrectly. This fix normalizes the list of ranges to be deleted before we start updating mark ranges by first sorting those ranges and then merging overlapping ranges into each other. --- .../renderer-hir/test/renderer-hir.test.ts | 62 +++++++++++++++++++ packages/@atjson/util/src/index.ts | 28 +++++++++ 2 files changed, 90 insertions(+) diff --git a/packages/@atjson/renderer-hir/test/renderer-hir.test.ts b/packages/@atjson/renderer-hir/test/renderer-hir.test.ts index c73256816..d04ff658b 100644 --- a/packages/@atjson/renderer-hir/test/renderer-hir.test.ts +++ b/packages/@atjson/renderer-hir/test/renderer-hir.test.ts @@ -468,5 +468,67 @@ describe("@atjson/renderer-hir", () => { "This document has a spoiler:\nXXXXXXXXXX" ); }); + + test("nested slices and subsequent marks", () => { + let atjson = new TestSource({ + content: "Pre-text\n\nParent Slice\n\nChild Slice\n\nPost-text", + annotations: [ + new Italic({ start: 0, end: 3 }), + new SliceAnnotation({ id: "slice1", start: 10, end: 37 }), + new SliceAnnotation({ id: "slide2", start: 24, end: 30 }), + new Bold({ start: 37, end: 41 }), + ], + }); + + class ConcreteRenderer extends Renderer { + *Bold() { + let text = (yield).join(""); + return `__${text}__`; + } + *Italic() { + let text = (yield).join(""); + return `*${text}*`; + } + *root() { + let results = (yield).join(""); + return results; + } + } + + expect(ConcreteRenderer.render(atjson)).toEqual( + "*Pre*-text\n\n__Post__-text" + ); + }); + + test("overlapping slices and subsequent marks", () => { + let atjson = new TestSource({ + content: "Pre-text\n\nParent Slice\n\nChild Slice\n\nPost-text", + annotations: [ + new Italic({ start: 0, end: 3 }), + new SliceAnnotation({ id: "slice1", start: 10, end: 30 }), + new SliceAnnotation({ id: "slide2", start: 24, end: 37 }), + new Bold({ start: 37, end: 41 }), + ], + }); + + class ConcreteRenderer extends Renderer { + *Bold() { + let text = (yield).join(""); + return `__${text}__`; + } + *Italic() { + let text = (yield).join(""); + return `*${text}*`; + } + *root() { + let results = (yield).join(""); + return results; + } + } + + expect(ConcreteRenderer.render(atjson)).toEqual( + "*Pre*-text\n\n__Post__-text" + ); + }); }); }); diff --git a/packages/@atjson/util/src/index.ts b/packages/@atjson/util/src/index.ts index 86c8814af..dbaf39b15 100644 --- a/packages/@atjson/util/src/index.ts +++ b/packages/@atjson/util/src/index.ts @@ -145,6 +145,16 @@ export function compareSliceTokens(a: SortableSlice, b: SortableSlice) { return 0; } +function compareRanges( + [start1, end1]: [number, number], + [start2, end2]: [number, number] +) { + if (start1 === start2) { + return end1 - end2; + } + return start1 - start2; +} + /** * Extracts slices from a document for the special `slice` * type provided by atjson. This function removes slices @@ -390,6 +400,24 @@ export function extractSlices(value: { }); } + // Normalize the ranges again, as we may have extended ranges into each other + // First sort the ranges + rangesToDelete = rangesToDelete.sort(compareRanges).reduce((acc, range) => { + // Since they are sorted, check if the current range can be merged into + // the previous one + const last = acc[acc.length - 1]; + if (last) { + const [, lastEnd] = last; + if (range[0] <= lastEnd && lastEnd <= range[1]) { + last[1] = range[1]; + return acc; + } + } + // Otherwise add the range to the list + acc.push(range); + return acc; + }, [] as [number, number][]); + let firstRange = rangesToDelete[0]; let text = firstRange ? value.text.slice(0, firstRange[0]) : ""; let lastEnd = firstRange ? firstRange[1] : 0; From 0f0c6a3e9351e0fe9a5eaa60b09c98712dda4bc3 Mon Sep 17 00:00:00 2001 From: Bach Bui Date: Wed, 13 Nov 2024 14:35:35 -0500 Subject: [PATCH 2/5] chore: update unit tests --- .../@atjson/util/test/extract-slices.test.ts | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/packages/@atjson/util/test/extract-slices.test.ts b/packages/@atjson/util/test/extract-slices.test.ts index 0cd6c7710..2259c28b7 100644 --- a/packages/@atjson/util/test/extract-slices.test.ts +++ b/packages/@atjson/util/test/extract-slices.test.ts @@ -422,6 +422,11 @@ describe("extractSlices", () => { refs: ["B00000000"], }, }, + { + id: "M00000003", + type: "italic", + range: "(6..7]", + }, ], }; let [doc, slices] = extractSlices(original); @@ -429,6 +434,17 @@ describe("extractSlices", () => { expect(slices.get("M00000001")?.text).toEqual("B"); expect(slices.get("M00000002")?.text).toEqual("C"); expect(doc.text).toEqual("D"); + expect(doc.marks).toMatchInlineSnapshot(` + [ + { + "end": 2, + "id": "M00000003", + "range": "(1..2]", + "start": 1, + "type": "italic", + }, + ] + `); }); test("start slice position matches", () => { @@ -542,6 +558,11 @@ describe("extractSlices", () => { refs: ["B00000000"], }, }, + { + id: "M00000003", + type: "italic", + range: "(5..6]", + }, ], }; let [doc, slices] = extractSlices(original); @@ -549,6 +570,17 @@ describe("extractSlices", () => { expect(slices.get("M00000001")?.text).toEqual("BB"); expect(slices.get("M00000002")?.text).toEqual("C"); expect(doc.text).toEqual("D"); + expect(doc.marks).toMatchInlineSnapshot(` + [ + { + "end": 2, + "id": "M00000003", + "range": "(1..2]", + "start": 1, + "type": "italic", + }, + ] + `); }); test("hanging overlapping slices", () => { @@ -580,12 +612,28 @@ describe("extractSlices", () => { refs: ["B00000000"], }, }, + { + id: "M00000002", + type: "italic", + range: "(4..5]", + }, ], }; let [doc, slices] = extractSlices(original); expect(slices.get("M00000000")?.text).toEqual("AB"); expect(slices.get("M00000001")?.text).toEqual("BC"); expect(doc.text).toEqual("D"); + expect(doc.marks).toMatchInlineSnapshot(` + [ + { + "end": 2, + "id": "M00000002", + "range": "(1..2]", + "start": 1, + "type": "italic", + }, + ] + `); }); describe("retain", () => { From ebd358da889d5058f3b72077355718d65d025304 Mon Sep 17 00:00:00 2001 From: Bach Bui Date: Wed, 13 Nov 2024 15:16:24 -0500 Subject: [PATCH 3/5] chore: more unit tests --- .../@atjson/util/test/extract-slices.test.ts | 217 ++++++++++++++++++ 1 file changed, 217 insertions(+) diff --git a/packages/@atjson/util/test/extract-slices.test.ts b/packages/@atjson/util/test/extract-slices.test.ts index 2259c28b7..19af8997e 100644 --- a/packages/@atjson/util/test/extract-slices.test.ts +++ b/packages/@atjson/util/test/extract-slices.test.ts @@ -426,6 +426,7 @@ describe("extractSlices", () => { id: "M00000003", type: "italic", range: "(6..7]", + attributes: {}, }, ], }; @@ -437,6 +438,7 @@ describe("extractSlices", () => { expect(doc.marks).toMatchInlineSnapshot(` [ { + "attributes": {}, "end": 2, "id": "M00000003", "range": "(1..2]", @@ -562,6 +564,7 @@ describe("extractSlices", () => { id: "M00000003", type: "italic", range: "(5..6]", + attributes: {}, }, ], }; @@ -573,6 +576,7 @@ describe("extractSlices", () => { expect(doc.marks).toMatchInlineSnapshot(` [ { + "attributes": {}, "end": 2, "id": "M00000003", "range": "(1..2]", @@ -583,6 +587,217 @@ describe("extractSlices", () => { `); }); + test("marks overlapping multiple slices", () => { + const original = { + text: "ABCBD", + blocks: [ + { + id: "B00000000", + type: "paragraph", + parents: [], + selfClosing: false, + attributes: {}, + }, + ], + marks: [ + { + id: "M00000000", + type: "slice", + range: "(1..5]", + attributes: { + refs: ["B00000000"], + }, + }, + { + id: "M00000001", + type: "slice", + range: "(2..5]", + attributes: { + refs: ["B00000000"], + }, + }, + { + id: "M00000002", + type: "slice", + range: "(3..4]", + attributes: { + refs: ["B00000000"], + }, + }, + { + id: "M00000003", + type: "italic", + range: "(5..6]", + attributes: {}, + }, + { + id: "M00000004", + type: "bold", + range: "(1..6]", + attributes: {}, + }, + ], + }; + + let [doc, slices] = extractSlices(original); + expect(slices.get("M00000000")?.text).toEqual("A"); + expect(slices.get("M00000000")?.marks).toHaveLength(0); + expect(slices.get("M00000001")?.text).toEqual("BB"); + expect(slices.get("M00000001")?.marks).toHaveLength(0); + expect(slices.get("M00000002")?.text).toEqual("C"); + expect(slices.get("M00000002")?.marks).toHaveLength(0); + expect(doc.text).toEqual("D"); + expect(doc.marks).toMatchInlineSnapshot(` + [ + { + "attributes": {}, + "end": 2, + "id": "M00000003", + "range": "(1..2]", + "start": 1, + "type": "italic", + }, + { + "attributes": {}, + "end": 2, + "id": "M00000004", + "range": "(1..2]", + "start": 1, + "type": "bold", + }, + ] + `); + }); + + test("marks contained in multiple slices", () => { + const original = { + text: "ABCBD", + blocks: [ + { + id: "B00000000", + type: "paragraph", + parents: [], + selfClosing: false, + attributes: {}, + }, + ], + marks: [ + { + id: "M00000000", + type: "slice", + range: "(1..5]", + attributes: { + refs: ["B00000000"], + }, + }, + { + id: "M00000001", + type: "slice", + range: "(2..5]", + attributes: { + refs: ["B00000000"], + }, + }, + { + id: "M00000002", + type: "slice", + range: "(3..5]", + attributes: { + refs: ["B00000000"], + }, + }, + { + id: "M00000003", + type: "italic", + range: "(3..5]", + attributes: {}, + }, + ], + }; + + let [doc, slices] = extractSlices(original); + expect(slices.get("M00000000")?.text).toEqual("A"); + expect(slices.get("M00000000")?.marks).toHaveLength(0); + expect(slices.get("M00000001")?.text).toEqual("B"); + expect(slices.get("M00000001")?.marks).toHaveLength(0); + expect(slices.get("M00000002")?.text).toEqual("CB"); + expect(slices.get("M00000002")?.marks).toMatchInlineSnapshot(` + [ + { + "attributes": {}, + "end": 3, + "id": "M00000002-M00000003", + "range": "(1..3]", + "start": 1, + "type": "italic", + }, + ] + `); + expect(doc.text).toEqual("D"); + expect(doc.marks).toHaveLength(0); + }); + + test("marks crossing multiple slices but contained in none are dropped", () => { + const original = { + text: "ABCBD", + blocks: [ + { + id: "B00000000", + type: "paragraph", + parents: [], + selfClosing: false, + attributes: {}, + }, + ], + marks: [ + { + id: "M00000000", + type: "slice", + range: "(1..5]", + attributes: { + refs: ["B00000000"], + }, + }, + { + id: "M00000001", + type: "slice", + range: "(2..5]", + attributes: { + refs: ["B00000000"], + }, + }, + { + id: "M00000002", + type: "slice", + range: "(3..5]", + attributes: { + refs: ["B00000000"], + }, + }, + { + id: "M00000003", + type: "italic", + // this mark is split by multiple slices so it is not + // fully contained in any of them. Currently this means + // it is not a part of any slice. In the future we may split + // this mark like we do blocks + range: "(1..5]", + attributes: {}, + }, + ], + }; + + let [doc, slices] = extractSlices(original); + expect(slices.get("M00000000")?.text).toEqual("A"); + expect(slices.get("M00000000")?.marks).toHaveLength(0); + expect(slices.get("M00000001")?.text).toEqual("B"); + expect(slices.get("M00000001")?.marks).toHaveLength(0); + expect(slices.get("M00000002")?.text).toEqual("CB"); + expect(slices.get("M00000002")?.marks).toHaveLength(0); + expect(doc.text).toEqual("D"); + expect(doc.marks).toHaveLength(0); + }); + test("hanging overlapping slices", () => { const original = { text: "ABCD", @@ -616,6 +831,7 @@ describe("extractSlices", () => { id: "M00000002", type: "italic", range: "(4..5]", + attributes: {}, }, ], }; @@ -626,6 +842,7 @@ describe("extractSlices", () => { expect(doc.marks).toMatchInlineSnapshot(` [ { + "attributes": {}, "end": 2, "id": "M00000002", "range": "(1..2]", From 70afbe7e1c2fc19f85c6249635bde1fa3a013f05 Mon Sep 17 00:00:00 2001 From: Bach Bui Date: Wed, 13 Nov 2024 16:38:34 -0500 Subject: [PATCH 4/5] chore: remove tests in renderer-hir --- .../renderer-hir/test/renderer-hir.test.ts | 62 ------------------- 1 file changed, 62 deletions(-) diff --git a/packages/@atjson/renderer-hir/test/renderer-hir.test.ts b/packages/@atjson/renderer-hir/test/renderer-hir.test.ts index d04ff658b..c73256816 100644 --- a/packages/@atjson/renderer-hir/test/renderer-hir.test.ts +++ b/packages/@atjson/renderer-hir/test/renderer-hir.test.ts @@ -468,67 +468,5 @@ describe("@atjson/renderer-hir", () => { "This document has a spoiler:\nXXXXXXXXXX" ); }); - - test("nested slices and subsequent marks", () => { - let atjson = new TestSource({ - content: "Pre-text\n\nParent Slice\n\nChild Slice\n\nPost-text", - annotations: [ - new Italic({ start: 0, end: 3 }), - new SliceAnnotation({ id: "slice1", start: 10, end: 37 }), - new SliceAnnotation({ id: "slide2", start: 24, end: 30 }), - new Bold({ start: 37, end: 41 }), - ], - }); - - class ConcreteRenderer extends Renderer { - *Bold() { - let text = (yield).join(""); - return `__${text}__`; - } - *Italic() { - let text = (yield).join(""); - return `*${text}*`; - } - *root() { - let results = (yield).join(""); - return results; - } - } - - expect(ConcreteRenderer.render(atjson)).toEqual( - "*Pre*-text\n\n__Post__-text" - ); - }); - - test("overlapping slices and subsequent marks", () => { - let atjson = new TestSource({ - content: "Pre-text\n\nParent Slice\n\nChild Slice\n\nPost-text", - annotations: [ - new Italic({ start: 0, end: 3 }), - new SliceAnnotation({ id: "slice1", start: 10, end: 30 }), - new SliceAnnotation({ id: "slide2", start: 24, end: 37 }), - new Bold({ start: 37, end: 41 }), - ], - }); - - class ConcreteRenderer extends Renderer { - *Bold() { - let text = (yield).join(""); - return `__${text}__`; - } - *Italic() { - let text = (yield).join(""); - return `*${text}*`; - } - *root() { - let results = (yield).join(""); - return results; - } - } - - expect(ConcreteRenderer.render(atjson)).toEqual( - "*Pre*-text\n\n__Post__-text" - ); - }); }); }); From 95b081e9fb6121c3c8e3e048a94208a9dced0eaf Mon Sep 17 00:00:00 2001 From: Bach Bui Date: Wed, 13 Nov 2024 16:55:32 -0500 Subject: [PATCH 5/5] chore: replace array.reduce with for loop --- packages/@atjson/util/src/index.ts | 34 ++++++++++++++++++------------ 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/packages/@atjson/util/src/index.ts b/packages/@atjson/util/src/index.ts index dbaf39b15..81f1c1891 100644 --- a/packages/@atjson/util/src/index.ts +++ b/packages/@atjson/util/src/index.ts @@ -401,22 +401,28 @@ export function extractSlices(value: { } // Normalize the ranges again, as we may have extended ranges into each other - // First sort the ranges - rangesToDelete = rangesToDelete.sort(compareRanges).reduce((acc, range) => { - // Since they are sorted, check if the current range can be merged into - // the previous one - const last = acc[acc.length - 1]; - if (last) { - const [, lastEnd] = last; - if (range[0] <= lastEnd && lastEnd <= range[1]) { - last[1] = range[1]; - return acc; + if (rangesToDelete.length > 1) { + const normalizedRanges = [rangesToDelete[0]]; + // First sort the ranges + rangesToDelete.sort(compareRanges); + for ( + let j = 1, lastRange = rangesToDelete[0]; + j < rangesToDelete.length; + j++ + ) { + const currentRange = rangesToDelete[j]; + // Since they are sorted, check if the current range can be merged into the + // previous one + if (currentRange[0] <= lastRange[1] && lastRange[1] <= currentRange[1]) { + lastRange[1] = currentRange[1]; + } else { + // Otherwise we can add the range to the list + normalizedRanges.push(currentRange); + lastRange = currentRange; } } - // Otherwise add the range to the list - acc.push(range); - return acc; - }, [] as [number, number][]); + rangesToDelete = normalizedRanges; + } let firstRange = rangesToDelete[0]; let text = firstRange ? value.text.slice(0, firstRange[0]) : "";