Changeset: Avoid unnecessary StringAssembler class

Author: rhansen

src/node/utils/ExportHtml.js

@@ -124,7 +124,7 @@ const getHTMLFromAtext = async (pad, atext, authorColors) => {
     // becomes
     // <b>Just bold <i>Bold and italics</i></b> <i>Just italics</i>
     const taker = Changeset.stringIterator(text);
-    const assem = Changeset.stringAssembler();
+    let assem = '';
     const openTags = [];
 
     const getSpanClassFor = (i) => {
@@ -160,31 +160,15 @@ const getHTMLFromAtext = async (pad, atext, authorColors) => {
     const emitOpenTag = (i) => {
       openTags.unshift(i);
       const spanClass = getSpanClassFor(i);
-
-      if (spanClass) {
-        assem.append('<span class="');
-        assem.append(spanClass);
-        assem.append('">');
-      } else {
-        assem.append('<');
-        assem.append(tags[i]);
-        assem.append('>');
-      }
+      assem += spanClass ? `<span class="${spanClass}">` : `<${tags[i]}>`;
     };
 
     // this closes an open tag and removes its reference from openTags
     const emitCloseTag = (i) => {
       openTags.shift();
       const spanClass = getSpanClassFor(i);
       const spanWithData = isSpanWithData(i);
-
-      if (spanClass || spanWithData) {
-        assem.append('</span>');
-      } else {
-        assem.append('</');
-        assem.append(tags[i]);
-        assem.append('>');
-      }
+      assem += spanClass || spanWithData ? '</span>' : `</${tags[i]}>`;
     };
 
     const urls = padutils.findURLs(text);
@@ -245,7 +229,7 @@ const getHTMLFromAtext = async (pad, atext, authorColors) => {
         // from but they break the abiword parser and are completly useless
         s = s.replace(String.fromCharCode(12), '');
 
-        assem.append(_encodeWhitespace(Security.escapeHTML(s)));
+        assem += _encodeWhitespace(Security.escapeHTML(s));
       } // end iteration over spans in line
 
       // close all the tags that are open after the last op
@@ -268,14 +252,14 @@ const getHTMLFromAtext = async (pad, atext, authorColors) => {
         // https://html.spec.whatwg.org/multipage/links.html#link-type-noopener
         // https://mathiasbynens.github.io/rel-noopener/
         // https://github.com/ether/etherpad-lite/pull/3636
-        assem.append(`<a href="${Security.escapeHTMLAttribute(url)}" rel="noreferrer noopener">`);
+        assem += `<a href="${Security.escapeHTMLAttribute(url)}" rel="noreferrer noopener">`;
         processNextChars(urlLength);
-        assem.append('</a>');
+        assem += '</a>';
       });
     }
     processNextChars(text.length - idx);
 
-    return _processSpaces(assem.toString());
+    return _processSpaces(assem);
   };
   // end getLineHTML
   const pieces = [css];

src/node/utils/ExportTxt.js

@@ -66,7 +66,7 @@ const getTXTFromAtext = (pad, atext, authorColors) => {
     // becomes
     // <b>Just bold <i>Bold and italics</i></b> <i>Just italics</i>
     const taker = Changeset.stringIterator(text);
-    const assem = Changeset.stringAssembler();
+    let assem = '';
 
     let idx = 0;
 
@@ -160,7 +160,7 @@ const getTXTFromAtext = (pad, atext, authorColors) => {
         // plugins from being able to display * at the beginning of a line
         // s = s.replace("*", ""); // Then remove it
 
-        assem.append(s);
+        assem += s;
       } // end iteration over spans in line
 
       const tags2close = [];
@@ -174,7 +174,7 @@ const getTXTFromAtext = (pad, atext, authorColors) => {
     // end processNextChars
 
     processNextChars(text.length - idx);
-    return (assem.toString());
+    return assem;
   };
   // end getLineHTML
 

src/node/utils/padDiff.js

@@ -326,21 +326,21 @@ PadDiff.prototype._createDeletionChangeset = function (cs, startAText, apool) {
 
   const nextText = (numChars) => {
     let len = 0;
-    const assem = Changeset.stringAssembler();
+    let assem = '';
     const firstString = linesGet(curLine).substring(curChar);
     len += firstString.length;
-    assem.append(firstString);
+    assem += firstString;
 
     let lineNum = curLine + 1;
 
     while (len < numChars) {
       const nextString = linesGet(lineNum);
       len += nextString.length;
-      assem.append(nextString);
+      assem += nextString;
       lineNum++;
     }
 
-    return assem.toString().substring(0, numChars);
+    return assem.substring(0, numChars);
   };
 
   const cachedStrFunc = (func) => {

src/static/js/Changeset.js

@@ -730,7 +730,10 @@ class StringAssembler {
 /**
  * @returns {StringAssembler}
  */
-exports.stringAssembler = () => new StringAssembler();
+exports.stringAssembler = () => {
+  warnDeprecated('Changeset.stringAssembler() is deprecated; build a string manually instead');
+  return new StringAssembler();
+};
 
 /**
  * Class to iterate and modify texts which have several lines. It is used for applying Changesets on
@@ -1142,7 +1145,7 @@ exports.applyToText = (cs, str) => {
   assert(str.length === unpacked.oldLen, 'mismatched apply: ', str.length, ' / ', unpacked.oldLen);
   const bankIter = exports.stringIterator(unpacked.charBank);
   const strIter = exports.stringIterator(str);
-  const assem = new StringAssembler();
+  let assem = '';
   for (const op of new exports.OpIter(unpacked.ops)) {
     switch (op.opcode) {
       case '+':
@@ -1151,7 +1154,7 @@ exports.applyToText = (cs, str) => {
         if (op.lines !== bankIter.peek(op.chars).split('\n').length - 1) {
           throw new Error(`newline count is wrong in op +; cs:${cs} and text:${str}`);
         }
-        assem.append(bankIter.take(op.chars));
+        assem += bankIter.take(op.chars);
         break;
       case '-':
       // op is - and op.lines 0: no newlines must be in the deleted string
@@ -1167,12 +1170,12 @@ exports.applyToText = (cs, str) => {
         if (op.lines !== strIter.peek(op.chars).split('\n').length - 1) {
           throw new Error(`newline count is wrong in op =; cs:${cs} and text:${str}`);
         }
-        assem.append(strIter.take(op.chars));
+        assem += strIter.take(op.chars);
         break;
     }
   }
-  assem.append(strIter.take(strIter.remaining()));
-  return assem.toString();
+  assem += strIter.take(strIter.remaining());
+  return assem;
 };
 
 /**
@@ -1247,12 +1250,11 @@ exports.composeAttributes = (att1, att2, resultIsMutation, pool) => {
     }
     return '';
   });
-  const buf = new StringAssembler();
+  let buf = '';
   for (const att of [...atts].sort((a, b) => (a[0] > b[0]) - (a[0] < b[0]))) {
-    buf.append('*');
-    buf.append(exports.numToString(pool.putAttrib(att)));
+    buf += `*${exports.numToString(pool.putAttrib(att))}`;
   }
-  return buf.toString();
+  return buf;
 };
 
 /**
@@ -1468,7 +1470,7 @@ exports.compose = (cs1, cs2, pool) => {
   const len3 = unpacked2.newLen;
   const bankIter1 = exports.stringIterator(unpacked1.charBank);
   const bankIter2 = exports.stringIterator(unpacked2.charBank);
-  const bankAssem = new StringAssembler();
+  let bankAssem = '';
 
   const newOps = applyZip(unpacked1.ops, unpacked2.ops, (op1, op2) => {
     const op1code = op1.opcode;
@@ -1478,16 +1480,12 @@ exports.compose = (cs1, cs2, pool) => {
     }
     const opOut = slicerZipperFunc(op1, op2, pool);
     if (opOut.opcode === '+') {
-      if (op2code === '+') {
-        bankAssem.append(bankIter2.take(opOut.chars));
-      } else {
-        bankAssem.append(bankIter1.take(opOut.chars));
-      }
+      bankAssem += (op2code === '+' ? bankIter2 : bankIter1).take(opOut.chars);
     }
     return opOut;
   });
 
-  return exports.pack(len1, len3, newOps, bankAssem.toString());
+  return exports.pack(len1, len3, newOps, bankAssem);
 };
 
 /**
@@ -1902,7 +1900,7 @@ exports.Builder = class {
   constructor(oldLen) {
     this._oldLen = oldLen;
     this._ops = [];
-    this._charBank = new StringAssembler();
+    this._charBank = '';
   }
 
   /**
@@ -1925,7 +1923,7 @@ exports.Builder = class {
 
   insert(text, attribs, pool) {
     this._ops.push(...opsFromText('+', text, attribs, pool));
-    this._charBank.append(text);
+    this._charBank += text;
     return this;
   }
 
@@ -1944,7 +1942,7 @@ exports.Builder = class {
       lengthChange = yield* exports.canonicalizeOps(this._ops, true);
     }).call(this));
     const newLen = this._oldLen + lengthChange;
-    return exports.pack(this._oldLen, newLen, serializedOps, this._charBank.toString());
+    return exports.pack(this._oldLen, newLen, serializedOps, this._charBank);
   }
 };
 
@@ -2103,20 +2101,20 @@ exports.inverse = (cs, lines, alines, pool) => {
 
   const nextText = (numChars) => {
     let len = 0;
-    const assem = new StringAssembler();
+    let assem = '';
     const firstString = linesGet(curLine).substring(curChar);
     len += firstString.length;
-    assem.append(firstString);
+    assem += firstString;
 
     let lineNum = curLine + 1;
     while (len < numChars) {
       const nextString = linesGet(lineNum);
       len += nextString.length;
-      assem.append(nextString);
+      assem += nextString;
       lineNum++;
     }
 
-    return assem.toString().substring(0, numChars);
+    return assem.substring(0, numChars);
   };
 
   const cachedStrFunc = (func) => {
@@ -2329,12 +2327,9 @@ const followAttributes = (att1, att2, pool) => {
     return '';
   });
   // we've only removed attributes, so they're already sorted
-  const buf = new StringAssembler();
-  for (const att of atts) {
-    buf.append('*');
-    buf.append(exports.numToString(pool.putAttrib(att)));
-  }
-  return buf.toString();
+  let buf = '';
+  for (const att of atts) buf += `*${exports.numToString(pool.putAttrib(att))}`;
+  return buf;
 };
 
 exports.exportedForTestingOnly = {

src/tests/frontend/specs/easysync.js

@@ -50,7 +50,7 @@ describe('easysync', function () {
   };
 
   const mutationsToChangeset = (oldLen, arrayOfArrays) => {
-    const bank = Changeset.stringAssembler();
+    let bank = '';
     let oldPos = 0;
     let newLen = 0;
     const ops = (function* () {
@@ -69,7 +69,7 @@ describe('easysync', function () {
           oldPos += op.chars;
         } else if (a[0] === 'insert') {
           op.opcode = '+';
-          bank.append(a[1]);
+          bank += a[1];
           op.chars = a[1].length;
           op.lines = (a[2] || 0);
           newLen += op.chars;
@@ -79,7 +79,7 @@ describe('easysync', function () {
     })();
     const serializedOps = Changeset.serializeOps(Changeset.canonicalizeOps(ops, true));
     newLen += oldLen - oldPos;
-    return Changeset.pack(oldLen, newLen, serializedOps, bank.toString());
+    return Changeset.pack(oldLen, newLen, serializedOps, bank);
   };
 
   const runMutationTest = (testId, origLines, muts, correct) => {
@@ -350,29 +350,19 @@ describe('easysync', function () {
       ]);
 
   const randomInlineString = (len) => {
-    const assem = Changeset.stringAssembler();
-    for (let i = 0; i < len; i++) {
-      assem.append(String.fromCharCode(randInt(26) + 97));
-    }
-    return assem.toString();
+    let assem = '';
+    for (let i = 0; i < len; i++) assem += String.fromCharCode(randInt(26) + 97);
+    return assem;
   };
 
   const randomMultiline = (approxMaxLines, approxMaxCols) => {
     const numParts = randInt(approxMaxLines * 2) + 1;
-    const txt = Changeset.stringAssembler();
-    txt.append(randInt(2) ? '\n' : '');
+    let txt = '';
+    txt += randInt(2) ? '\n' : '';
     for (let i = 0; i < numParts; i++) {
-      if ((i % 2) === 0) {
-        if (randInt(10)) {
-          txt.append(randomInlineString(randInt(approxMaxCols) + 1));
-        } else {
-          txt.append('\n');
-        }
-      } else {
-        txt.append('\n');
-      }
+      txt += i % 2 === 0 && randInt(10) ? randomInlineString(randInt(approxMaxCols) + 1) : '\n';
     }
-    return txt.toString();
+    return txt;
   };
 
   const randomStringOperation = (numCharsLeft) => {
@@ -494,9 +484,9 @@ describe('easysync', function () {
   };
 
   const randomTestChangeset = (origText, withAttribs) => {
-    const charBank = Changeset.stringAssembler();
+    let charBank = '';
     let textLeft = origText; // always keep final newline
-    const outTextAssem = Changeset.stringAssembler();
+    let outTextAssem = '';
     const ops = [];
     const oldLen = origText.length;
 
@@ -521,13 +511,13 @@ describe('easysync', function () {
       const o = randomStringOperation(textLeft.length);
       if (o.insert) {
         const txt = o.insert;
-        charBank.append(txt);
-        outTextAssem.append(txt);
+        charBank += txt;
+        outTextAssem += txt;
         appendMultilineOp('+', txt);
       } else if (o.skip) {
         const txt = textLeft.substring(0, o.skip);
         textLeft = textLeft.substring(o.skip);
-        outTextAssem.append(txt);
+        outTextAssem += txt;
         appendMultilineOp('=', txt);
       } else if (o.remove) {
         const txt = textLeft.substring(0, o.remove);
@@ -538,9 +528,9 @@ describe('easysync', function () {
 
     while (textLeft.length > 1) doOp();
     for (let i = 0; i < 5; i++) doOp(); // do some more (only insertions will happen)
-    const outText = `${outTextAssem.toString()}\n`;
+    const outText = `${outTextAssem}\n`;
     const serializedOps = Changeset.serializeOps(Changeset.canonicalizeOps(ops, true));
-    const cs = Changeset.pack(oldLen, outText.length, serializedOps, charBank.toString());
+    const cs = Changeset.pack(oldLen, outText.length, serializedOps, charBank);
     Changeset.checkRep(cs);
     return [cs, outText];
   };