Changeset: Migrate from smartOpAssembler() to canonicalizeOps()

rhansen · rhansen · commit d19a34edf9f2 · 2021-10-19T03:53:22.000-04:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -41,6 +41,8 @@
       `squashOps()` generator function with `serializeOps()` instead.
     * The `appendATextToAssembler()` function is deprecated; use the new
       `opsFromAText()` generator function instead.
+    * The `smartOpAssembler()` function is deprecated; use the new
+      `canonicalizeOps()` generator function with `serializeOps()` instead.
 
 ### Notable enhancements
 
diff --git a/src/node/db/Pad.js b/src/node/db/Pad.js
@@ -488,21 +488,20 @@ Pad.prototype.copyPadWithoutHistory = async function (destinationID, force) {
   const newPool = newPad.pool;
   newPool.fromJsonable(sourcePad.pool.toJsonable()); // copy that sourceId pool to the new pad
 
-  // based on Changeset.makeSplice
-  const assem = Changeset.smartOpAssembler();
-  for (const op of Changeset.opsFromAText(oldAText)) assem.append(op);
-  assem.endDocument();
+  let newLength;
+  const serializedOps = Changeset.serializeOps((function* () {
+    newLength = yield* Changeset.canonicalizeOps(Changeset.opsFromAText(oldAText), true);
+  })());
 
   // although we have instantiated the newPad with '\n', an additional '\n' is
   // added internally, so the pad text on the revision 0 is "\n\n"
   const oldLength = 2;
 
-  const newLength = assem.getLengthChange();
   const newText = oldAText.text;
 
   // create a changeset that removes the previous text and add the newText with
   // all atributes present on the source pad
-  const changeset = Changeset.pack(oldLength, newLength, assem.toString(), newText);
+  const changeset = Changeset.pack(oldLength, newLength, serializedOps, newText);
   newPad.appendRevision(changeset);
 
   await hooks.aCallAll('padCopy', {originalPad: this, destinationID});
diff --git a/src/static/js/Changeset.js b/src/static/js/Changeset.js
@@ -451,7 +451,7 @@ class MergingOpAssembler {
  * @yields {Op} The canonicalized operations.
  * @returns {number} How much the sequence of operations changes the length (in characters).
  */
-const canonicalizeOps = function* (ops, finalize) {
+exports.canonicalizeOps = function* (ops, finalize) {
   let minusOps = [];
   let plusOps = [];
   let keepOps = [];
@@ -533,6 +533,8 @@ const opsFromText = function* (opcode, text, attribs, pool) {
  *   - strips final "="
  *   - ignores 0-length changes
  *   - reorders consecutive + and - (which MergingOpAssembler doesn't do)
+ *
+ * @deprecated Use `canonicalizeOps` with `serializeOps` instead.
  */
 class SmartOpAssembler {
   constructor() {
@@ -541,7 +543,7 @@ class SmartOpAssembler {
 
   _serialize(finalize) {
     this._serialized = exports.serializeOps((function* () {
-      this._lengthChange = yield* canonicalizeOps(this._ops, finalize);
+      this._lengthChange = yield* exports.canonicalizeOps(this._ops, finalize);
     }).call(this));
   }
 
@@ -592,51 +594,54 @@ exports.checkRep = (cs) => {
   const unpacked = exports.unpack(cs);
   const oldLen = unpacked.oldLen;
   const newLen = unpacked.newLen;
-  const ops = unpacked.ops;
   let charBank = unpacked.charBank;
 
-  const assem = new SmartOpAssembler();
   let oldPos = 0;
   let calcNewLen = 0;
   let numInserted = 0;
-  for (const o of new exports.OpIter(ops)) {
-    switch (o.opcode) {
-      case '=':
-        oldPos += o.chars;
-        calcNewLen += o.chars;
-        break;
-      case '-':
-        oldPos += o.chars;
-        assert(oldPos <= oldLen, oldPos, ' > ', oldLen, ' in ', cs);
-        break;
-      case '+':
-      {
-        calcNewLen += o.chars;
-        numInserted += o.chars;
-        assert(calcNewLen <= newLen, calcNewLen, ' > ', newLen, ' in ', cs);
-        break;
+  const ops = (function* () {
+    for (const o of new exports.OpIter(unpacked.ops)) {
+      switch (o.opcode) {
+        case '=':
+          oldPos += o.chars;
+          calcNewLen += o.chars;
+          break;
+        case '-':
+          oldPos += o.chars;
+          assert(oldPos <= oldLen, oldPos, ' > ', oldLen, ' in ', cs);
+          break;
+        case '+':
+          calcNewLen += o.chars;
+          numInserted += o.chars;
+          assert(calcNewLen <= newLen, calcNewLen, ' > ', newLen, ' in ', cs);
+          break;
       }
+      yield o;
     }
-    assem.append(o);
-  }
+  })();
+  const serializedOps = exports.serializeOps(exports.canonicalizeOps(ops, true));
 
   calcNewLen += oldLen - oldPos;
   charBank = charBank.substring(0, numInserted);
   while (charBank.length < numInserted) {
     charBank += '?';
   }
 
-  assem.endDocument();
-  const normalized = exports.pack(oldLen, calcNewLen, assem.toString(), charBank);
+  const normalized = exports.pack(oldLen, calcNewLen, serializedOps, charBank);
   assert(normalized === cs, 'Invalid changeset (checkRep failed)');
 
   return cs;
 };
 
 /**
+ * @deprecated Use `canonicalizeOps` with `serializeOps` instead.
  * @returns {SmartOpAssembler}
  */
-exports.smartOpAssembler = () => new SmartOpAssembler();
+exports.smartOpAssembler = () => {
+  warnDeprecated(
+      'Changeset.smartOpAssembler() is deprecated; use Changeset.canonicalizeOps() instead');
+  return new SmartOpAssembler();
+};
 
 /**
  * @deprecated Use `squashOps` with `serializeOps` instead.
@@ -1076,19 +1081,19 @@ class TextLinesMutator {
  * @returns {string} the integrated changeset
  */
 const applyZip = (in1, in2, func) => {
-  const iter1 = new exports.OpIter(in1);
-  const iter2 = new exports.OpIter(in2);
-  const assem = new SmartOpAssembler();
-  let op1 = new exports.Op();
-  let op2 = new exports.Op();
-  while (op1.opcode || iter1.hasNext() || op2.opcode || iter2.hasNext()) {
-    if (!op1.opcode && iter1.hasNext()) op1 = iter1.next();
-    if (!op2.opcode && iter2.hasNext()) op2 = iter2.next();
-    const opOut = func(op1, op2);
-    if (opOut && opOut.opcode) assem.append(opOut);
-  }
-  assem.endDocument();
-  return assem.toString();
+  const ops = (function* () {
+    const iter1 = new exports.OpIter(in1);
+    const iter2 = new exports.OpIter(in2);
+    let op1 = new exports.Op();
+    let op2 = new exports.Op();
+    while (op1.opcode || iter1.hasNext() || op2.opcode || iter2.hasNext()) {
+      if (!op1.opcode && iter1.hasNext()) op1 = iter1.next();
+      if (!op2.opcode && iter2.hasNext()) op2 = iter2.next();
+      const opOut = func(op1, op2);
+      if (opOut && opOut.opcode) yield opOut;
+    }
+  })();
+  return exports.serializeOps(exports.canonicalizeOps(ops, true));
 };
 
 /**
@@ -1546,15 +1551,13 @@ exports.makeSplice = (oldFullText, spliceStart, numRemoved, newText, optNewTextA
   const oldText = oldFullText.substring(spliceStart, spliceStart + numRemoved);
   const newLen = oldLen + newText.length - oldText.length;
 
-  const assem = new SmartOpAssembler();
   const ops = (function* () {
     yield* opsFromText('=', oldFullText.substring(0, spliceStart));
     yield* opsFromText('-', oldText);
     yield* opsFromText('+', newText, optNewTextAPairs, pool);
   })();
-  for (const op of ops) assem.append(op);
-  assem.endDocument();
-  return exports.pack(oldLen, newLen, assem.toString(), newText);
+  const serializedOps = exports.serializeOps(exports.canonicalizeOps(ops, true));
+  return exports.pack(oldLen, newLen, serializedOps, newText);
 };
 
 /**
@@ -1683,11 +1686,8 @@ exports.moveOpsToNewPool = (cs, oldPool, newPool) => {
  * @param {string} text - text to insert
  * @returns {string}
  */
-exports.makeAttribution = (text) => {
-  const assem = new SmartOpAssembler();
-  for (const op of opsFromText('+', text)) assem.append(op);
-  return assem.toString();
-};
+exports.makeAttribution =
+    (text) => exports.serializeOps(exports.canonicalizeOps(opsFromText('+', text), false));
 
 /**
  * Iterates over attributes in exports, attribution string, or attribs property of an op and runs
@@ -1919,8 +1919,8 @@ exports.attribsAttributeValue = (attribs, key, pool) => {
  * @returns {Builder}
  */
 exports.builder = (oldLen) => {
-  const assem = new SmartOpAssembler();
-  const o = new exports.Op();
+  const ops = [];
+  let packed = null;
   const charBank = exports.stringAssembler();
 
   const self = {
@@ -1929,34 +1929,44 @@ exports.builder = (oldLen) => {
      *     latter case).
      */
     keep: (N, L, attribs, pool) => {
-      o.opcode = '=';
+      const o = new exports.Op('=');
       o.attribs = (attribs && exports.makeAttribsString('=', attribs, pool)) || '';
       o.chars = N;
       o.lines = (L || 0);
-      assem.append(o);
+      packed = null;
+      ops.push(o);
       return self;
     },
     keepText: (text, attribs, pool) => {
-      for (const op of opsFromText('=', text, attribs, pool)) assem.append(op);
+      packed = null;
+      ops.push(...opsFromText('=', text, attribs, pool));
       return self;
     },
     insert: (text, attribs, pool) => {
-      for (const op of opsFromText('+', text, attribs, pool)) assem.append(op);
+      packed = null;
+      ops.push(...opsFromText('+', text, attribs, pool));
       charBank.append(text);
       return self;
     },
     remove: (N, L) => {
-      o.opcode = '-';
+      const o = new exports.Op('-');
       o.attribs = '';
       o.chars = N;
       o.lines = (L || 0);
-      assem.append(o);
+      packed = null;
+      ops.push(o);
       return self;
     },
     toString: () => {
-      assem.endDocument();
-      const newLen = oldLen + assem.getLengthChange();
-      return exports.pack(oldLen, newLen, assem.toString(), charBank.toString());
+      if (packed == null) {
+        let lengthChange;
+        const serializedOps = exports.serializeOps((function* () {
+          lengthChange = yield* exports.canonicalizeOps(ops, true);
+        })());
+        const newLen = oldLen + lengthChange;
+        packed = exports.pack(oldLen, newLen, serializedOps, charBank.toString());
+      }
+      return packed;
     },
   };
 
@@ -1989,11 +1999,11 @@ exports.makeAttribsString = (opcode, attribs, pool) => {
  */
 exports.subattribution = (astr, start, optEnd) => {
   const iter = new exports.OpIter(astr);
-  const assem = new SmartOpAssembler();
   let attOp = new exports.Op();
-  const csOp = new exports.Op();
+  const csOp = new exports.Op('-');
+  csOp.chars = start;
 
-  const doCsOp = () => {
+  const doCsOp = function* () {
     if (!csOp.chars) return;
     while (csOp.opcode && (attOp.opcode || iter.hasNext())) {
       if (!attOp.opcode) attOp = iter.next();
@@ -2002,27 +2012,22 @@ exports.subattribution = (astr, start, optEnd) => {
         csOp.lines++;
       }
       const opOut = slicerZipperFunc(attOp, csOp, null);
-      if (opOut.opcode) assem.append(opOut);
+      if (opOut.opcode) yield opOut;
     }
   };
 
-  csOp.opcode = '-';
-  csOp.chars = start;
-
-  doCsOp();
-
-  if (optEnd === undefined) {
-    if (attOp.opcode) {
-      assem.append(attOp);
+  const ops = (function* () {
+    yield* doCsOp();
+    if (optEnd === undefined) {
+      if (attOp.opcode) yield attOp;
+      yield* iter;
+    } else {
+      csOp.opcode = '=';
+      csOp.chars = optEnd - start;
+      yield* doCsOp();
     }
-    while (iter.hasNext()) assem.append(iter.next());
-  } else {
-    csOp.opcode = '=';
-    csOp.chars = optEnd - start;
-    doCsOp();
-  }
-
-  return assem.toString();
+  })();
+  return exports.serializeOps(exports.canonicalizeOps(ops, false));
 };
 
 exports.inverse = (cs, lines, alines, pool) => {
diff --git a/src/static/js/ace2_inner.js b/src/static/js/ace2_inner.js
@@ -530,18 +530,24 @@ function Ace2Inner(editorInfo, cssManagers) {
     const numLines = rep.lines.length();
     const upToLastLine = rep.lines.offsetOfIndex(numLines - 1);
     const lastLineLength = rep.lines.atIndex(numLines - 1).text.length;
-    const assem = Changeset.smartOpAssembler();
-    const o = new Changeset.Op('-');
-    o.chars = upToLastLine;
-    o.lines = numLines - 1;
-    assem.append(o);
-    o.chars = lastLineLength;
-    o.lines = 0;
-    assem.append(o);
-    for (const op of Changeset.opsFromAText(atext)) assem.append(op);
-    const newLen = oldLen + assem.getLengthChange();
-    const changeset = Changeset.checkRep(
-        Changeset.pack(oldLen, newLen, assem.toString(), atext.text.slice(0, -1)));
+    const ops = (function* () {
+      const op1 = new Changeset.Op('-');
+      op1.chars = upToLastLine;
+      op1.lines = numLines - 1;
+      yield op1;
+      const op2 = new Changeset.Op('-');
+      op2.chars = lastLineLength;
+      op2.lines = 0;
+      yield op2;
+      yield* Changeset.opsFromAText(atext);
+    })();
+    let lengthChange;
+    const serializedOps = Changeset.serializeOps((function* () {
+      lengthChange = yield* Changeset.canonicalizeOps(ops, false);
+    })());
+    const newLen = oldLen + lengthChange;
+    const changeset =
+        Changeset.checkRep(Changeset.pack(oldLen, newLen, serializedOps, atext.text.slice(0, -1)));
     performDocumentApplyChangeset(changeset);
 
     performSelectionChange(
diff --git a/src/static/js/contentcollector.js b/src/static/js/contentcollector.js
diff --git a/src/tests/frontend/specs/easysync.js b/src/tests/frontend/specs/easysync.js
