Changeset.js: fix handling of line mutations when dealing with the la…

…st characters of text
ether · Oct 29, 2021 · 3220691 · 3220691
1 parent c828c8e
commit 3220691
Show file tree

Hide file tree

Showing 2 changed files with 80 additions and 13 deletions.
diff --git a/src/static/js/Changeset.js b/src/static/js/Changeset.js
@@ -766,7 +766,6 @@ exports.textLinesMutator = (lines) => {
         // TODO(doc) should this count the number of characters that are skipped to check?
         for (let i = 0; i < L; i++) {
           curCol = 0;
-          putCurLineInSplice();
           curLine++;
         }
       } else {
@@ -844,9 +843,10 @@ exports.textLinesMutator = (lines) => {
           curSplice[1] += L - 1;
           const sline = curSplice.length - 1;
           removed = curSplice[sline].substring(curCol) + removed;
+          const remaining = linesGet(curSplice[0] + curSplice[1]) || '';
           curSplice[sline] = curSplice[sline].substring(0, curCol) +
-              linesGet(curSplice[0] + curSplice[1]);
-          curSplice[1] += 1;
+              remaining;
+          curSplice[1] += remaining ? 1 : 0;
         }
       } else {
         removed = nextKLinesText(L);
@@ -908,9 +908,12 @@ exports.textLinesMutator = (lines) => {
           // insert the remaining new lines
           Array.prototype.push.apply(curSplice, newLines);
           curLine += newLines.length;
-          // insert the remaining chars from the "old" line (e.g. the line we were in
-          // when we started to insert new lines)
-          curSplice.push(theLine.substring(lineCol));
+          const remaining = theLine.substring(lineCol);
+          if (remaining !== '') {
+            // insert the remaining chars from the "old" line (e.g. the line we were in
+            // when we started to insert new lines)
+            curSplice.push(theLine.substring(lineCol));
+          }
           curCol = 0; // TODO(doc) why is this not set to the length of last line?
         } else {
           Array.prototype.push.apply(curSplice, newLines);
@@ -920,13 +923,20 @@ exports.textLinesMutator = (lines) => {
         // there are no additional lines
         // although the line is put into splice, curLine is not increased, because
         // there may be more chars in the line (newline is not reached)
-        const sline = putCurLineInSplice();
-        if (!curSplice[sline]) {
-          console.error('curSplice[sline] not populated, actual curSplice contents is ', curSplice, '. Possibly related to https://github.com/ether/etherpad-lite/issues/2802');
+        if (!hasMore()) {
+          // we are inserting at the end of lines. curCol is 0 or else curLine would be in splice
+          curSplice.push(text);
+          curCol += text.length;
+        } else {
+          const sline = putCurLineInSplice();
+          if (!curSplice[sline]) {
+            // TODO should never happen now
+            console.error('curSplice[sline] not populated, actual curSplice contents is ', curSplice, '. Possibly related to https://github.com/ether/etherpad-lite/issues/2802');
+          }
+          curSplice[sline] = curSplice[sline].substring(0, curCol) + text +
+              curSplice[sline].substring(curCol);
+          curCol += text.length;
         }
-        curSplice[sline] = curSplice[sline].substring(0, curCol) + text +
-            curSplice[sline].substring(curCol);
-        curCol += text.length;
       }
     }
   };

diff --git a/src/tests/frontend/specs/easysync-mutations.js b/src/tests/frontend/specs/easysync-mutations.js
@@ -138,6 +138,54 @@ describe('easysync', function () {
     ['skip', 1, 1, true],
   ], ['banana\n', 'cabbage\n', 'duffle\n']);
 
+  // #2836 regressions
+  runMutationTest(8, ['\n'], [
+    ['remove', 1, 1, '\n'],
+    ['insert', 'c', 0],
+  ], ['c']);
+
+  runMutationTest(9, ['\n'], [
+    ['remove', 1, 1, '\n'],
+    ['insert', 'a'],
+    ['insert', 'c\n', 1],
+  ], ['ac\n']);
+
+  runMutationTest(10, ['\n'], [
+    ['remove', 1, 1, '\n'],
+    ['insert', 'a\n', 1],
+    ['insert', 'c'],
+  ], ['a\n', 'c']);
+
+  runMutationTest(11, ['\n', 'fun\n', '\n'], [
+    ['remove', 1, 1, '\n'],
+    ['skip', 4, 1, false],
+    ['remove', 1, 1, '\n'],
+    ['insert', 'c'],
+  ], ['fun\n', 'c']);
+
+  runMutationTest(12, ['\n', 'fun\n', '\n'], [
+    ['remove', 1, 1, '\n'],
+    ['skip', 3, 0, false],
+    ['remove', 2, 2, '\n\n'],
+    ['insert', 'c'],
+  ], ['func']);
+
+  runMutationTest(13, ['\n', 'fun\n', '\n'], [
+    ['remove', 1, 1, '\n'],
+    ['skip', 3, 0, false],
+    ['remove', 2, 2, '\n\n'],
+    ['insert', 'c'],
+    ['insert', 'a\n', 1],
+    ['insert', 'c'],
+  ], ['funca\n', 'c']);
+
+  runMutationTest(14, ['\n', 'fun\n', '\n'], [
+    ['remove', 1, 1, '\n'],
+    ['skip', 2, 0, false],
+    ['remove', 3, 2, 'n\n\n'],
+    ['insert', 'z'],
+  ], ['fuz']);
+
   it('mutatorHasMore', async function () {
     const lines = ['1\n', '2\n', '3\n', '4\n'];
     let mu;
@@ -178,6 +226,7 @@ describe('easysync', function () {
     expect(mu.hasMore()).to.be(false);
   });
 
+
   describe('mutateTextLines', function () {
     const testMutateTextLines = (testId, cs, lines, correctLines) => {
       it(`testMutateTextLines#${testId}`, async function () {
@@ -189,8 +238,16 @@ describe('easysync', function () {
 
     testMutateTextLines(1, 'Z:4<1|1-2-1|1+1+1$\nc', ['a\n', 'b\n'], ['\n', 'c\n']);
     testMutateTextLines(2, 'Z:4>0|1-2-1|2+3$\nc\n', ['a\n', 'b\n'], ['\n', 'c\n', '\n']);
-  });
 
+    it('mutate keep only lines', async function () {
+      const lines = ['1\n', '2\n', '3\n', '4\n'];
+      const result = lines.slice();
+      const cs = 'Z:8>0*0|1=2|2=2';
+
+      Changeset.mutateTextLines(cs, lines);
+      expect(result).to.eql(lines);
+    });
+  });
 
   describe('mutate attributions', function () {
     const testPoolWithChars = (() => {