From c0a3a3b49b766f9b7006120e13aac3f6a3c9040b Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Sun, 21 Mar 2021 14:34:02 -0400 Subject: [PATCH 1/4] Changeset: Turn `textLinesMutator()` into a real class --- src/static/js/Changeset.js | 298 +++++++++++++-------------- src/tests/frontend/specs/easysync.js | 8 +- 2 files changed, 143 insertions(+), 163 deletions(-) diff --git a/src/static/js/Changeset.js b/src/static/js/Changeset.js index 949002c7de8..fd34f39e4b3 100644 --- a/src/static/js/Changeset.js +++ b/src/static/js/Changeset.js @@ -639,43 +639,34 @@ exports.stringAssembler = () => ({ * actually a newline, but for the purposes of N and L values, the caller should pretend it is, and * for things to work right in that case, the input to the `insert` method should be a single line * with no newlines. - * - * @typedef {object} TextLinesMutator - * @property {Function} close - - * @property {Function} hasMore - - * @property {Function} insert - - * @property {Function} remove - - * @property {Function} removeLines - - * @property {Function} skip - - * @property {Function} skipLines - - */ - -/** - * @param {(string[]|StringArrayLike)} lines - Lines to mutate (in place). - * @returns {TextLinesMutator} */ -const textLinesMutator = (lines) => { +class TextLinesMutator { /** - * curSplice holds values that will be passed as arguments to lines.splice() to insert, delete, or - * change lines: - * - curSplice[0] is an index into the lines array. - * - curSplice[1] is the number of lines that will be removed from the lines array starting at - * the index. - * - The other elements represent mutated (changed by ops) lines or new lines (added by ops) to - * insert at the index. - * - * @type {[number, number?, ...string[]?]} + * @param {(string[]|StringArrayLike)} lines - Lines to mutate (in place). */ - const curSplice = [0, 0]; - let inSplice = false; - - // position in lines after curSplice is applied: - let curLine = 0; - let curCol = 0; - // invariant: if (inSplice) then (curLine is in curSplice[0] + curSplice.length - {2,3}) && - // curLine >= curSplice[0] - // invariant: if (inSplice && (curLine >= curSplice[0] + curSplice.length - 2)) then - // curCol == 0 + constructor(lines) { + this._lines = lines; + /** + * this._curSplice holds values that will be passed as arguments to this._lines.splice() to + * insert, delete, or change lines: + * - this._curSplice[0] is an index into the this._lines array. + * - this._curSplice[1] is the number of lines that will be removed from the this._lines array + * starting at the index. + * - The other elements represent mutated (changed by ops) lines or new lines (added by ops) + * to insert at the index. + * + * @type {[number, number?, ...string[]?]} + */ + this._curSplice = [0, 0]; + this._inSplice = false; + // position in lines after curSplice is applied: + this._curLine = 0; + this._curCol = 0; + // invariant: if (inSplice) then (curLine is in curSplice[0] + curSplice.length - {2,3}) && + // curLine >= curSplice[0] + // invariant: if (inSplice && (curLine >= curSplice[0] + curSplice.length - 2)) then + // curCol == 0 + } /** * Get a line from `lines` at given index. @@ -683,13 +674,13 @@ const textLinesMutator = (lines) => { * @param {number} idx - an index * @returns {string} */ - const linesGet = (idx) => { - if ('get' in lines) { - return lines.get(idx); + _linesGet(idx) { + if ('get' in this._lines) { + return this._lines.get(idx); } else { - return lines[idx]; + return this._lines[idx]; } - }; + } /** * Return a slice from `lines`. @@ -698,51 +689,50 @@ const textLinesMutator = (lines) => { * @param {number} end - the end index * @returns {string[]} */ - const linesSlice = (start, end) => { - if (lines.slice) { - return lines.slice(start, end); + _linesSlice(start, end) { + // can be unimplemented if removeLines's return value not needed + if (this._lines.slice) { + return this._lines.slice(start, end); } else { return []; } - }; + } /** * Return the length of `lines`. * * @returns {number} */ - const linesLength = () => { - if ((typeof lines.length) === 'number') { - return lines.length; + _linesLength() { + if (typeof this._lines.length === 'number') { + return this._lines.length; } else { - return lines.length(); + return this._lines.length(); } - }; + } /** * Starts a new splice. */ - const enterSplice = () => { - curSplice[0] = curLine; - curSplice[1] = 0; + _enterSplice() { + this._curSplice[0] = this._curLine; + this._curSplice[1] = 0; // TODO(doc) when is this the case? // check all enterSplice calls and changes to curCol - if (curCol > 0) { - putCurLineInSplice(); - } - inSplice = true; - }; + if (this._curCol > 0) this._putCurLineInSplice(); + this._inSplice = true; + } /** * Changes the lines array according to the values in curSplice and resets curSplice. Called via * close or TODO(doc). */ - const leaveSplice = () => { - lines.splice(...curSplice); - curSplice.length = 2; - curSplice[0] = curSplice[1] = 0; - inSplice = false; - }; + _leaveSplice() { + this._lines.splice(...this._curSplice); + this._curSplice.length = 2; + this._curSplice[0] = this._curSplice[1] = 0; + this._inSplice = false; + } /** * Indicates if curLine is already in the splice. This is necessary because the last element in @@ -752,20 +742,23 @@ const textLinesMutator = (lines) => { * * @returns {boolean} true if curLine is in splice */ - const isCurLineInSplice = () => (curLine - curSplice[0] < (curSplice.length - 2)); + _isCurLineInSplice() { + return this._curLine - this._curSplice[0] < this._curSplice.length - 2; + } /** * Incorporates current line into the splice and marks its old position to be deleted. * * @returns {number} the index of the added line in curSplice */ - const putCurLineInSplice = () => { - if (!isCurLineInSplice()) { - curSplice.push(linesGet(curSplice[0] + curSplice[1])); - curSplice[1]++; + _putCurLineInSplice() { + if (!this._isCurLineInSplice()) { + this._curSplice.push(this._linesGet(this._curSplice[0] + this._curSplice[1])); + this._curSplice[1]++; } - return 2 + curLine - curSplice[0]; // TODO should be the same as curSplice.length - 1 - }; + // TODO should be the same as this._curSplice.length - 1 + return 2 + this._curLine - this._curSplice[0]; + } /** * It will skip some newlines by putting them into the splice. @@ -773,30 +766,30 @@ const textLinesMutator = (lines) => { * @param {number} L - * @param {boolean} includeInSplice - indicates if attributes are present */ - const skipLines = (L, includeInSplice) => { + skipLines(L, includeInSplice) { if (!L) return; if (includeInSplice) { - if (!inSplice) enterSplice(); + if (!this._inSplice) this._enterSplice(); // 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++; + this._curCol = 0; + this._putCurLineInSplice(); + this._curLine++; } } else { - if (inSplice) { + if (this._inSplice) { if (L > 1) { // TODO(doc) figure out why single lines are incorporated into splice instead of ignored - leaveSplice(); + this._leaveSplice(); } else { - putCurLineInSplice(); + this._putCurLineInSplice(); } } - curLine += L; - curCol = 0; + this._curLine += L; + this._curCol = 0; } // tests case foo in remove(), which isn't otherwise covered in current impl - }; + } /** * Skip some characters. Can contain newlines. @@ -805,20 +798,20 @@ const textLinesMutator = (lines) => { * @param {number} L - number of newlines to skip * @param {boolean} includeInSplice - indicates if attributes are present */ - const skip = (N, L, includeInSplice) => { + skip(N, L, includeInSplice) { if (!N) return; if (L) { - skipLines(L, includeInSplice); + this.skipLines(L, includeInSplice); } else { - if (includeInSplice && !inSplice) enterSplice(); - if (inSplice) { + if (includeInSplice && !this._inSplice) this._enterSplice(); + if (this._inSplice) { // although the line is put into splice curLine is not increased, because // only some chars are skipped, not the whole line - putCurLineInSplice(); + this._putCurLineInSplice(); } - curCol += N; + this._curCol += N; } - }; + } /** * Remove whole lines from lines array. @@ -826,9 +819,9 @@ const textLinesMutator = (lines) => { * @param {number} L - number of lines to remove * @returns {string} */ - const removeLines = (L) => { + removeLines(L) { if (!L) return ''; - if (!inSplice) enterSplice(); + if (!this._inSplice) this._enterSplice(); /** * Gets a string of joined lines after the end of the splice. @@ -837,32 +830,32 @@ const textLinesMutator = (lines) => { * @returns {string} joined lines */ const nextKLinesText = (k) => { - const m = curSplice[0] + curSplice[1]; - return linesSlice(m, m + k).join(''); + const m = this._curSplice[0] + this._curSplice[1]; + return this._linesSlice(m, m + k).join(''); }; let removed = ''; - if (isCurLineInSplice()) { - if (curCol === 0) { - removed = curSplice[curSplice.length - 1]; - curSplice.length--; + if (this._isCurLineInSplice()) { + if (this._curCol === 0) { + removed = this._curSplice[this._curSplice.length - 1]; + this._curSplice.length--; removed += nextKLinesText(L - 1); - curSplice[1] += L - 1; + this._curSplice[1] += L - 1; } else { removed = nextKLinesText(L - 1); - curSplice[1] += L - 1; - const sline = curSplice.length - 1; - removed = curSplice[sline].substring(curCol) + removed; - curSplice[sline] = curSplice[sline].substring(0, curCol) + - linesGet(curSplice[0] + curSplice[1]); - curSplice[1] += 1; + this._curSplice[1] += L - 1; + const sline = this._curSplice.length - 1; + removed = this._curSplice[sline].substring(this._curCol) + removed; + this._curSplice[sline] = this._curSplice[sline].substring(0, this._curCol) + + this._linesGet(this._curSplice[0] + this._curSplice[1]); + this._curSplice[1] += 1; } } else { removed = nextKLinesText(L); - curSplice[1] += L; + this._curSplice[1] += L; } return removed; - }; + } /** * Remove text from lines array. @@ -871,18 +864,18 @@ const textLinesMutator = (lines) => { * @param {number} L - lines to delete * @returns {string} */ - const remove = (N, L) => { + remove(N, L) { if (!N) return ''; - if (L) return removeLines(L); - if (!inSplice) enterSplice(); + if (L) return this.removeLines(L); + if (!this._inSplice) this._enterSplice(); // although the line is put into splice, curLine is not increased, because // only some chars are removed not the whole line - const sline = putCurLineInSplice(); - const removed = curSplice[sline].substring(curCol, curCol + N); - curSplice[sline] = curSplice[sline].substring(0, curCol) + - curSplice[sline].substring(curCol + N); + const sline = this._putCurLineInSplice(); + const removed = this._curSplice[sline].substring(this._curCol, this._curCol + N); + this._curSplice[sline] = this._curSplice[sline].substring(0, this._curCol) + + this._curSplice[sline].substring(this._curCol + N); return removed; - }; + } /** * Inserts text into lines array. @@ -890,82 +883,69 @@ const textLinesMutator = (lines) => { * @param {string} text - the text to insert * @param {number} L - number of newlines in text */ - const insert = (text, L) => { + insert(text, L) { if (!text) return; - if (!inSplice) enterSplice(); + if (!this._inSplice) this._enterSplice(); if (L) { const newLines = exports.splitTextLines(text); - if (isCurLineInSplice()) { - const sline = curSplice.length - 1; + if (this._isCurLineInSplice()) { + const sline = this._curSplice.length - 1; /** @type {string} */ - const theLine = curSplice[sline]; - const lineCol = curCol; + const theLine = this._curSplice[sline]; + const lineCol = this._curCol; // insert the first new line - curSplice[sline] = theLine.substring(0, lineCol) + newLines[0]; - curLine++; + this._curSplice[sline] = theLine.substring(0, lineCol) + newLines[0]; + this._curLine++; newLines.splice(0, 1); // insert the remaining new lines - curSplice.push(...newLines); - curLine += newLines.length; + this._curSplice.push(...newLines); + this._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)); - curCol = 0; // TODO(doc) why is this not set to the length of last line? + this._curSplice.push(theLine.substring(lineCol)); + this._curCol = 0; // TODO(doc) why is this not set to the length of last line? } else { - curSplice.push(...newLines); - curLine += newLines.length; + this._curSplice.push(...newLines); + this._curLine += newLines.length; } } else { // 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]) { + const sline = this._putCurLineInSplice(); + if (!this._curSplice[sline]) { const err = new Error( 'curSplice[sline] not populated, actual curSplice contents is ' + - `${JSON.stringify(curSplice)}. Possibly related to ` + + `${JSON.stringify(this._curSplice)}. Possibly related to ` + 'https://github.com/ether/etherpad-lite/issues/2802'); console.error(err.stack || err.toString()); } - curSplice[sline] = curSplice[sline].substring(0, curCol) + text + - curSplice[sline].substring(curCol); - curCol += text.length; + this._curSplice[sline] = this._curSplice[sline].substring(0, this._curCol) + text + + this._curSplice[sline].substring(this._curCol); + this._curCol += text.length; } - }; + } /** * Checks if curLine (the line we are in when curSplice is applied) is the last line in `lines`. * * @returns {boolean} indicates if there are lines left */ - const hasMore = () => { - let docLines = linesLength(); - if (inSplice) { - docLines += curSplice.length - 2 - curSplice[1]; + hasMore() { + let docLines = this._linesLength(); + if (this._inSplice) { + docLines += this._curSplice.length - 2 - this._curSplice[1]; } - return curLine < docLines; - }; + return this._curLine < docLines; + } /** * Closes the splice */ - const close = () => { - if (inSplice) { - leaveSplice(); - } - }; - - const self = { - skip, - remove, - insert, - close, - hasMore, - removeLines, - skipLines, - }; - return self; -}; + close() { + if (this._inSplice) this._leaveSplice(); + } +} /** * Apply operations to other operations. @@ -1106,7 +1086,7 @@ exports.mutateTextLines = (cs, lines) => { const unpacked = exports.unpack(cs); const csIter = exports.opIterator(unpacked.ops); const bankIter = exports.stringIterator(unpacked.charBank); - const mut = textLinesMutator(lines); + const mut = new TextLinesMutator(lines); while (csIter.hasNext()) { const op = csIter.next(); switch (op.opcode) { @@ -1239,7 +1219,7 @@ exports.mutateAttributionLines = (cs, lines, pool) => { const csBank = unpacked.charBank; let csBankIndex = 0; // treat the attribution lines as text lines, mutating a line at a time - const mut = textLinesMutator(lines); + const mut = new TextLinesMutator(lines); /** @type {?OpIter} */ let lineIter = null; @@ -2310,7 +2290,7 @@ const followAttributes = (att1, att2, pool) => { }; exports.exportedForTestingOnly = { + TextLinesMutator, followAttributes, - textLinesMutator, toSplices, }; diff --git a/src/tests/frontend/specs/easysync.js b/src/tests/frontend/specs/easysync.js index 79e81f41fc5..3ffd2a2110b 100644 --- a/src/tests/frontend/specs/easysync.js +++ b/src/tests/frontend/specs/easysync.js @@ -92,7 +92,7 @@ describe('easysync', function () { const runMutationTest = (testId, origLines, muts, correct) => { it(`runMutationTest#${testId}`, async function () { let lines = origLines.slice(); - const mu = Changeset.exportedForTestingOnly.textLinesMutator(lines); + const mu = new Changeset.exportedForTestingOnly.TextLinesMutator(lines); applyMutations(mu, muts); mu.close(); expect(lines).to.eql(correct); @@ -211,7 +211,7 @@ describe('easysync', function () { const lines = ['1\n', '2\n', '3\n', '4\n']; let mu; - mu = Changeset.exportedForTestingOnly.textLinesMutator(lines); + mu = new Changeset.exportedForTestingOnly.TextLinesMutator(lines); expect(mu.hasMore()).to.be(true); mu.skip(8, 4); expect(mu.hasMore()).to.be(false); @@ -219,7 +219,7 @@ describe('easysync', function () { expect(mu.hasMore()).to.be(false); // still 1,2,3,4 - mu = Changeset.exportedForTestingOnly.textLinesMutator(lines); + mu = new Changeset.exportedForTestingOnly.TextLinesMutator(lines); expect(mu.hasMore()).to.be(true); mu.remove(2, 1); expect(mu.hasMore()).to.be(true); @@ -235,7 +235,7 @@ describe('easysync', function () { expect(mu.hasMore()).to.be(false); // 2,3,4,5 now - mu = Changeset.exportedForTestingOnly.textLinesMutator(lines); + mu = new Changeset.exportedForTestingOnly.TextLinesMutator(lines); expect(mu.hasMore()).to.be(true); mu.remove(6, 3); expect(mu.hasMore()).to.be(true); From 15e3e48743163c7723ff7ca8a9794a7a043d2584 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Mon, 25 Oct 2021 01:21:19 -0400 Subject: [PATCH 2/4] Changeset: Turn `newOp()` into a real class --- CHANGELOG.md | 1 + src/node/utils/padDiff.js | 4 +- src/static/js/Changeset.js | 143 +++++++++++++++++---------- src/static/js/ace2_inner.js | 2 +- src/static/js/contentcollector.js | 2 +- src/static/js/linestylefilter.js | 2 +- src/tests/frontend/specs/easysync.js | 6 +- 7 files changed, 99 insertions(+), 61 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e1f3f565bc..e609aed66b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ * `opAttributeValue()` * `appendATextToAssembler()`: Deprecated in favor of the new `opsFromAText()` generator function. + * `newOp()`: Deprecated in favor of the new `Op` class. # 1.8.15 diff --git a/src/node/utils/padDiff.js b/src/node/utils/padDiff.js index 05cba308fa6..d0c61633f7b 100644 --- a/src/node/utils/padDiff.js +++ b/src/node/utils/padDiff.js @@ -270,7 +270,7 @@ PadDiff.prototype._createDeletionChangeset = function (cs, startAText, apool) { let curChar = 0; let curLineOpIter = null; let curLineOpIterLine; - let curLineNextOp = Changeset.newOp('+'); + let curLineNextOp = new Changeset.Op('+'); const unpacked = Changeset.unpack(cs); const csIter = Changeset.opIterator(unpacked.ops); @@ -302,7 +302,7 @@ PadDiff.prototype._createDeletionChangeset = function (cs, startAText, apool) { } if (!curLineNextOp.chars) { - curLineNextOp = curLineOpIter.hasNext() ? curLineOpIter.next() : Changeset.newOp(); + curLineNextOp = curLineOpIter.hasNext() ? curLineOpIter.next() : new Changeset.Op(); } const charsToUse = Math.min(numChars, curLineNextOp.chars); diff --git a/src/static/js/Changeset.js b/src/static/js/Changeset.js index fd34f39e4b3..0b3b01a7cb0 100644 --- a/src/static/js/Changeset.js +++ b/src/static/js/Changeset.js @@ -83,31 +83,74 @@ exports.numToString = (num) => num.toString(36).toLowerCase(); /** * An operation to apply to a shared document. - * - * @typedef {object} Op - * @property {('+'|'-'|'='|'')} opcode - The operation's operator: - * - '=': Keep the next `chars` characters (containing `lines` newlines) from the base - * document. - * - '-': Remove the next `chars` characters (containing `lines` newlines) from the base - * document. - * - '+': Insert `chars` characters (containing `lines` newlines) at the current position in - * the document. The inserted characters come from the changeset's character bank. - * - '' (empty string): Invalid operator used in some contexts to signifiy the lack of an - * operation. - * @property {number} chars - The number of characters to keep, insert, or delete. - * @property {number} lines - The number of characters among the `chars` characters that are - * newlines. If non-zero, the last character must be a newline. - * @property {string} attribs - Identifiers of attributes to apply to the text, represented as a - * repeated (zero or more) sequence of asterisk followed by a non-negative base-36 (lower-case) - * integer. For example, '*2*1o' indicates that attributes 2 and 60 apply to the text affected - * by the operation. The identifiers come from the document's attribute pool. This is the empty - * string for remove ('-') operations. For keep ('=') operations, the attributes are merged with - * the base text's existing attributes: - * - A keep op attribute with a non-empty value replaces an existing base text attribute that - * has the same key. - * - A keep op attribute with an empty value is interpreted as an instruction to remove an - * existing base text attribute that has the same key, if one exists. */ +class Op { + /** + * @param {(''|'='|'+'|'-')} [opcode=''] - Initial value of the `opcode` property. + */ + constructor(opcode = '') { + /** + * The operation's operator: + * - '=': Keep the next `chars` characters (containing `lines` newlines) from the base + * document. + * - '-': Remove the next `chars` characters (containing `lines` newlines) from the base + * document. + * - '+': Insert `chars` characters (containing `lines` newlines) at the current position in + * the document. The inserted characters come from the changeset's character bank. + * - '' (empty string): Invalid operator used in some contexts to signifiy the lack of an + * operation. + * + * @type {(''|'='|'+'|'-')} + * @public + */ + this.opcode = opcode; + + /** + * The number of characters to keep, insert, or delete. + * + * @type {number} + * @public + */ + this.chars = 0; + + /** + * The number of characters among the `chars` characters that are newlines. If non-zero, the + * last character must be a newline. + * + * @type {number} + * @public + */ + this.lines = 0; + + /** + * Identifiers of attributes to apply to the text, represented as a repeated (zero or more) + * sequence of asterisk followed by a non-negative base-36 (lower-case) integer. For example, + * '*2*1o' indicates that attributes 2 and 60 apply to the text affected by the operation. The + * identifiers come from the document's attribute pool. + * + * For keep ('=') operations, the attributes are merged with the base text's existing + * attributes: + * - A keep op attribute with a non-empty value replaces an existing base text attribute that + * has the same key. + * - A keep op attribute with an empty value is interpreted as an instruction to remove an + * existing base text attribute that has the same key, if one exists. + * + * This is the empty string for remove ('-') operations. + * + * @type {string} + * @public + */ + this.attribs = ''; + } + + toString() { + if (!this.opcode) throw new TypeError('null op'); + if (typeof this.attribs !== 'string') throw new TypeError('attribs must be a string'); + const l = this.lines ? `|${exports.numToString(this.lines)}` : ''; + return this.attribs + l + this.opcode + exports.numToString(this.chars); + } +} +exports.Op = Op; /** * Describes changes to apply to a document. Does not include the attribute pool or the original @@ -166,8 +209,7 @@ exports.opIterator = (opsStr) => { }; let regexResult = nextRegexMatch(); - const next = (optOp) => { - const op = optOp || exports.newOp(); + const next = (op = new Op()) => { if (regexResult[0]) { op.attribs = regexResult[1]; op.lines = exports.parseNum(regexResult[2] || '0'); @@ -203,15 +245,14 @@ const clearOp = (op) => { /** * Creates a new Op object * + * @deprecated Use the `Op` class instead. * @param {('+'|'-'|'='|'')} [optOpcode=''] - The operation's operator. * @returns {Op} */ -exports.newOp = (optOpcode) => ({ - opcode: (optOpcode || ''), - chars: 0, - lines: 0, - attribs: '', -}); +exports.newOp = (optOpcode) => { + padutils.warnWithStack('Changeset.newOp() is deprecated; use the Changeset.Op class instead'); + return new Op(optOpcode); +}; /** * Copies op1 to op2 @@ -220,7 +261,7 @@ exports.newOp = (optOpcode) => ({ * @param {Op} [op2] - dest Op. If not given, a new Op is used. * @returns {Op} `op2` */ -const copyOp = (op1, op2 = exports.newOp()) => Object.assign(op2, op1); +const copyOp = (op1, op2 = new Op()) => Object.assign(op2, op1); /** * Serializes a sequence of Ops. @@ -257,7 +298,7 @@ const copyOp = (op1, op2 = exports.newOp()) => Object.assign(op2, op1); * @returns {Generator} */ const opsFromText = function* (opcode, text, attribs = '', pool = null) { - const op = exports.newOp(opcode); + const op = new Op(opcode); op.attribs = typeof attribs === 'string' ? attribs : new AttributeMap(pool).update(attribs || [], opcode === '+').toString(); const lastNewlinePos = text.lastIndexOf('\n'); @@ -447,7 +488,7 @@ exports.smartOpAssembler = () => { */ exports.mergingOpAssembler = () => { const assem = exports.opAssembler(); - const bufOp = exports.newOp(); + const bufOp = new Op(); // If we get, for example, insertions [xxx\n,yyy], those don't merge, // but if we get [xxx\n,yyy,zzz\n], that merges to [xxx\nyyyzzz\n]. @@ -523,12 +564,8 @@ exports.opAssembler = () => { * @param {Op} op - Operation to add. Ownership remains with the caller. */ const append = (op) => { - if (!op.opcode) throw new TypeError('null op'); - if (typeof op.attribs !== 'string') throw new TypeError('attribs must be a string'); - serialized += op.attribs; - if (op.lines) serialized += `|${exports.numToString(op.lines)}`; - serialized += op.opcode; - serialized += exports.numToString(op.chars); + assert(op instanceof Op, 'argument must be an instance of Op'); + serialized += op.toString(); }; const toString = () => serialized; @@ -972,8 +1009,8 @@ const applyZip = (in1, in2, func) => { const iter1 = exports.opIterator(in1); const iter2 = exports.opIterator(in2); const assem = exports.smartOpAssembler(); - const op1 = exports.newOp(); - const op2 = exports.newOp(); + const op1 = new Op(); + const op2 = new Op(); while (op1.opcode || iter1.hasNext() || op2.opcode || iter2.hasNext()) { if ((!op1.opcode) && iter1.hasNext()) iter1.next(op1); if ((!op2.opcode) && iter2.hasNext()) iter2.next(op2); @@ -1148,7 +1185,7 @@ exports.composeAttributes = (att1, att2, resultIsMutation, pool) => { * @returns {Op} The result of applying `csOp` to `attOp`. */ const slicerZipperFunc = (attOp, csOp, pool) => { - const opOut = exports.newOp(); + const opOut = new Op(); if (!attOp.opcode) { copyOp(csOp, opOut); csOp.opcode = ''; @@ -1231,7 +1268,7 @@ exports.mutateAttributionLines = (cs, lines, pool) => { const line = mut.removeLines(1); lineIter = exports.opIterator(line); } - if (!lineIter || !lineIter.hasNext()) return exports.newOp(); + if (!lineIter || !lineIter.hasNext()) return new Op(); return lineIter.next(); }; let lineAssem = null; @@ -1248,8 +1285,8 @@ exports.mutateAttributionLines = (cs, lines, pool) => { lineAssem = null; }; - let csOp = exports.newOp(); - let attOp = exports.newOp(); + let csOp = new Op(); + let attOp = new Op(); while (csOp.opcode || csIter.hasNext() || attOp.opcode || isNextMutOp()) { if (!csOp.opcode && csIter.hasNext()) csOp = csIter.next(); if ((!csOp.opcode) && (!attOp.opcode) && (!lineAssem) && (!(lineIter && lineIter.hasNext()))) { @@ -1826,7 +1863,7 @@ exports.attribsAttributeValue = (attribs, key, pool) => { */ exports.builder = (oldLen) => { const assem = exports.smartOpAssembler(); - const o = exports.newOp(); + const o = new Op(); const charBank = exports.stringAssembler(); const self = { @@ -1930,8 +1967,8 @@ exports.makeAttribsString = (opcode, attribs, pool) => { exports.subattribution = (astr, start, optEnd) => { const iter = exports.opIterator(astr); const assem = exports.smartOpAssembler(); - let attOp = exports.newOp(); - const csOp = exports.newOp(); + let attOp = new Op(); + const csOp = new Op(); const doCsOp = () => { if (!csOp.chars) return; @@ -1994,7 +2031,7 @@ exports.inverse = (cs, lines, alines, pool) => { let curChar = 0; let curLineOpIter = null; let curLineOpIterLine; - let curLineNextOp = exports.newOp('+'); + let curLineNextOp = new Op('+'); const unpacked = exports.unpack(cs); const csIter = exports.opIterator(unpacked.ops); @@ -2025,7 +2062,7 @@ exports.inverse = (cs, lines, alines, pool) => { curLineOpIter = exports.opIterator(alinesGet(curLine)); } if (!curLineNextOp.chars) { - curLineNextOp = curLineOpIter.hasNext() ? curLineOpIter.next() : exports.newOp(); + curLineNextOp = curLineOpIter.hasNext() ? curLineOpIter.next() : new Op(); } const charsToUse = Math.min(numChars, curLineNextOp.chars); func(charsToUse, curLineNextOp.attribs, charsToUse === curLineNextOp.chars && @@ -2135,7 +2172,7 @@ exports.follow = (cs1, cs2, reverseInsertOrder, pool) => { const hasInsertFirst = exports.attributeTester(['insertorder', 'first'], pool); const newOps = applyZip(unpacked1.ops, unpacked2.ops, (op1, op2) => { - const opOut = exports.newOp(); + const opOut = new Op(); if (op1.opcode === '+' || op2.opcode === '+') { let whichToDo; if (op2.opcode !== '+') { diff --git a/src/static/js/ace2_inner.js b/src/static/js/ace2_inner.js index 3597959acce..484205c0646 100644 --- a/src/static/js/ace2_inner.js +++ b/src/static/js/ace2_inner.js @@ -523,7 +523,7 @@ function Ace2Inner(editorInfo, cssManagers) { const upToLastLine = rep.lines.offsetOfIndex(numLines - 1); const lastLineLength = rep.lines.atIndex(numLines - 1).text.length; const assem = Changeset.smartOpAssembler(); - const o = Changeset.newOp('-'); + const o = new Changeset.Op('-'); o.chars = upToLastLine; o.lines = numLines - 1; assem.append(o); diff --git a/src/static/js/contentcollector.js b/src/static/js/contentcollector.js index 2e6005bd881..7dd70e51287 100644 --- a/src/static/js/contentcollector.js +++ b/src/static/js/contentcollector.js @@ -83,7 +83,7 @@ const makeContentCollector = (collectStyles, abrowser, apool, className2Author) const textArray = []; const attribsArray = []; let attribsBuilder = null; - const op = Changeset.newOp('+'); + const op = new Changeset.Op('+'); const self = { length: () => textArray.length, atColumnZero: () => textArray[textArray.length - 1] === '', diff --git a/src/static/js/linestylefilter.js b/src/static/js/linestylefilter.js index f6bcbb54e62..19751999c56 100644 --- a/src/static/js/linestylefilter.js +++ b/src/static/js/linestylefilter.js @@ -102,7 +102,7 @@ linestylefilter.getLineStyleFilter = (lineLength, aline, textAndClassFunc, apool let nextOp, nextOpClasses; const goNextOp = () => { - nextOp = attributionIter.hasNext() ? attributionIter.next() : Changeset.newOp(); + nextOp = attributionIter.hasNext() ? attributionIter.next() : new Changeset.Op(); nextOpClasses = (nextOp.opcode && attribsToClasses(nextOp.attribs)); }; goNextOp(); diff --git a/src/tests/frontend/specs/easysync.js b/src/tests/frontend/specs/easysync.js index 3ffd2a2110b..fc56c62a181 100644 --- a/src/tests/frontend/specs/easysync.js +++ b/src/tests/frontend/specs/easysync.js @@ -57,7 +57,7 @@ describe('easysync', function () { const mutationsToChangeset = (oldLen, arrayOfArrays) => { const assem = Changeset.smartOpAssembler(); - const op = Changeset.newOp(); + const op = new Changeset.Op(); const bank = Changeset.stringAssembler(); let oldPos = 0; let newLen = 0; @@ -507,7 +507,7 @@ describe('easysync', function () { const opAssem = Changeset.smartOpAssembler(); const oldLen = origText.length; - const nextOp = Changeset.newOp(); + const nextOp = new Changeset.Op(); const appendMultilineOp = (opcode, txt) => { nextOp.opcode = opcode; @@ -651,7 +651,7 @@ describe('easysync', function () { const testSplitJoinAttributionLines = (randomSeed) => { const stringToOps = (str) => { const assem = Changeset.mergingOpAssembler(); - const o = Changeset.newOp('+'); + const o = new Changeset.Op('+'); o.chars = 1; for (let i = 0; i < str.length; i++) { const c = str.charAt(i); From 3e0e428b23832367b3e88e410eec520108cfeff3 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Thu, 21 Oct 2021 03:17:28 -0400 Subject: [PATCH 3/4] Changeset: Throw on unexpected chars while iterating ops --- src/static/js/Changeset.js | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/static/js/Changeset.js b/src/static/js/Changeset.js index 0b3b01a7cb0..5d4871c5e23 100644 --- a/src/static/js/Changeset.js +++ b/src/static/js/Changeset.js @@ -197,20 +197,21 @@ exports.newLen = (cs) => exports.unpack(cs).newLen; * @returns {OpIter} Operator iterator object. */ exports.opIterator = (opsStr) => { - const regex = /((?:\*[0-9a-z]+)*)(?:\|([0-9a-z]+))?([-+=])([0-9a-z]+)|\?|/g; + const regex = /((?:\*[0-9a-z]+)*)(?:\|([0-9a-z]+))?([-+=])([0-9a-z]+)|(.)/g; const nextRegexMatch = () => { const result = regex.exec(opsStr); - if (result[0] === '?') { - error('Hit error opcode in op stream'); - } - + if (!result) return null; + if (result[5] === '$') return null; // Start of the insert operation character bank. + if (result[5] != null) error(`invalid operation: ${opsStr.slice(regex.lastIndex - 1)}`); return result; }; let regexResult = nextRegexMatch(); + const hasNext = () => regexResult && !!regexResult[0]; + const next = (op = new Op()) => { - if (regexResult[0]) { + if (hasNext()) { op.attribs = regexResult[1]; op.lines = exports.parseNum(regexResult[2] || '0'); op.opcode = regexResult[3]; @@ -222,8 +223,6 @@ exports.opIterator = (opsStr) => { return op; }; - const hasNext = () => !!(regexResult[0]); - return { next, hasNext, From d36f79f97aaaae581106568beec5cc7a83e95ae3 Mon Sep 17 00:00:00 2001 From: Richard Hansen Date: Wed, 13 Oct 2021 15:42:03 -0400 Subject: [PATCH 4/4] Changeset: Turn `opIterator()` into a real class --- src/static/js/Changeset.js | 114 ++++++++++++++++++++----------------- 1 file changed, 63 insertions(+), 51 deletions(-) diff --git a/src/static/js/Changeset.js b/src/static/js/Changeset.js index 5d4871c5e23..2b0e82482a1 100644 --- a/src/static/js/Changeset.js +++ b/src/static/js/Changeset.js @@ -184,11 +184,54 @@ exports.newLen = (cs) => exports.unpack(cs).newLen; * Iterator over a changeset's operations. * * Note: This class does NOT implement the ECMAScript iterable or iterator protocols. - * - * @typedef {object} OpIter - * @property {Function} hasNext - - * @property {Function} next - */ +class OpIter { + /** + * @param {string} ops - String encoding the change operations to iterate over. + */ + constructor(ops) { + this._ops = ops; + this._regex = /((?:\*[0-9a-z]+)*)(?:\|([0-9a-z]+))?([-+=])([0-9a-z]+)|(.)/g; + this._nextMatch = this._nextRegexMatch(); + } + + _nextRegexMatch() { + const match = this._regex.exec(this._ops); + if (!match) return null; + if (match[5] === '$') return null; // Start of the insert operation character bank. + if (match[5] != null) error(`invalid operation: ${this._ops.slice(this._regex.lastIndex - 1)}`); + return match; + } + + /** + * @returns {boolean} Whether there are any remaining operations. + */ + hasNext() { + return this._nextMatch && !!this._nextMatch[0]; + } + + /** + * Returns the next operation object and advances the iterator. + * + * Note: This does NOT implement the ECMAScript iterator protocol. + * + * @param {Op} [opOut] - Deprecated. Operation object to recycle for the return value. + * @returns {Op} The next operation, or an operation with a falsy `opcode` property if there are + * no more operations. + */ + next(opOut = new Op()) { + if (this.hasNext()) { + opOut.attribs = this._nextMatch[1]; + opOut.lines = exports.parseNum(this._nextMatch[2] || '0'); + opOut.opcode = this._nextMatch[3]; + opOut.chars = exports.parseNum(this._nextMatch[4]); + this._nextMatch = this._nextRegexMatch(); + } else { + clearOp(opOut); + } + return opOut; + } +} /** * Creates an iterator which decodes string changeset operations. @@ -196,38 +239,7 @@ exports.newLen = (cs) => exports.unpack(cs).newLen; * @param {string} opsStr - String encoding of the change operations to perform. * @returns {OpIter} Operator iterator object. */ -exports.opIterator = (opsStr) => { - const regex = /((?:\*[0-9a-z]+)*)(?:\|([0-9a-z]+))?([-+=])([0-9a-z]+)|(.)/g; - - const nextRegexMatch = () => { - const result = regex.exec(opsStr); - if (!result) return null; - if (result[5] === '$') return null; // Start of the insert operation character bank. - if (result[5] != null) error(`invalid operation: ${opsStr.slice(regex.lastIndex - 1)}`); - return result; - }; - let regexResult = nextRegexMatch(); - - const hasNext = () => regexResult && !!regexResult[0]; - - const next = (op = new Op()) => { - if (hasNext()) { - op.attribs = regexResult[1]; - op.lines = exports.parseNum(regexResult[2] || '0'); - op.opcode = regexResult[3]; - op.chars = exports.parseNum(regexResult[4]); - regexResult = nextRegexMatch(); - } else { - clearOp(op); - } - return op; - }; - - return { - next, - hasNext, - }; -}; +exports.opIterator = (opsStr) => new OpIter(opsStr); /** * Cleans an Op object. @@ -352,7 +364,7 @@ exports.checkRep = (cs) => { let oldPos = 0; let calcNewLen = 0; let numInserted = 0; - const iter = exports.opIterator(ops); + const iter = new OpIter(ops); while (iter.hasNext()) { const o = iter.next(); switch (o.opcode) { @@ -1005,8 +1017,8 @@ class TextLinesMutator { * @returns {string} the integrated changeset */ const applyZip = (in1, in2, func) => { - const iter1 = exports.opIterator(in1); - const iter2 = exports.opIterator(in2); + const iter1 = new OpIter(in1); + const iter2 = new OpIter(in2); const assem = exports.smartOpAssembler(); const op1 = new Op(); const op2 = new Op(); @@ -1075,7 +1087,7 @@ exports.pack = (oldLen, newLen, opsStr, bank) => { exports.applyToText = (cs, str) => { const unpacked = exports.unpack(cs); assert(str.length === unpacked.oldLen, `mismatched apply: ${str.length} / ${unpacked.oldLen}`); - const csIter = exports.opIterator(unpacked.ops); + const csIter = new OpIter(unpacked.ops); const bankIter = exports.stringIterator(unpacked.charBank); const strIter = exports.stringIterator(str); const assem = exports.stringAssembler(); @@ -1120,7 +1132,7 @@ exports.applyToText = (cs, str) => { */ exports.mutateTextLines = (cs, lines) => { const unpacked = exports.unpack(cs); - const csIter = exports.opIterator(unpacked.ops); + const csIter = new OpIter(unpacked.ops); const bankIter = exports.stringIterator(unpacked.charBank); const mut = new TextLinesMutator(lines); while (csIter.hasNext()) { @@ -1251,7 +1263,7 @@ exports.applyToAttribution = (cs, astr, pool) => { exports.mutateAttributionLines = (cs, lines, pool) => { const unpacked = exports.unpack(cs); - const csIter = exports.opIterator(unpacked.ops); + const csIter = new OpIter(unpacked.ops); const csBank = unpacked.charBank; let csBankIndex = 0; // treat the attribution lines as text lines, mutating a line at a time @@ -1265,7 +1277,7 @@ exports.mutateAttributionLines = (cs, lines, pool) => { const nextMutOp = () => { if ((!(lineIter && lineIter.hasNext())) && mut.hasMore()) { const line = mut.removeLines(1); - lineIter = exports.opIterator(line); + lineIter = new OpIter(line); } if (!lineIter || !lineIter.hasNext()) return new Op(); return lineIter.next(); @@ -1328,7 +1340,7 @@ exports.mutateAttributionLines = (cs, lines, pool) => { exports.joinAttributionLines = (theAlines) => { const assem = exports.mergingOpAssembler(); for (const aline of theAlines) { - const iter = exports.opIterator(aline); + const iter = new OpIter(aline); while (iter.hasNext()) { assem.append(iter.next()); } @@ -1337,7 +1349,7 @@ exports.joinAttributionLines = (theAlines) => { }; exports.splitAttributionLines = (attrOps, text) => { - const iter = exports.opIterator(attrOps); + const iter = new OpIter(attrOps); const assem = exports.mergingOpAssembler(); const lines = []; let pos = 0; @@ -1495,7 +1507,7 @@ const toSplices = (cs) => { const splices = []; let oldPos = 0; - const iter = exports.opIterator(unpacked.ops); + const iter = new OpIter(unpacked.ops); const charIter = exports.stringIterator(unpacked.charBank); let inSplice = false; while (iter.hasNext()) { @@ -1742,7 +1754,7 @@ exports.copyAText = (atext1, atext2) => { */ exports.opsFromAText = function* (atext) { // intentionally skips last newline char of atext - const iter = exports.opIterator(atext.attribs); + const iter = new OpIter(atext.attribs); let lastOp = null; while (iter.hasNext()) { if (lastOp != null) yield lastOp; @@ -1964,7 +1976,7 @@ exports.makeAttribsString = (opcode, attribs, pool) => { * Like "substring" but on a single-line attribution string. */ exports.subattribution = (astr, start, optEnd) => { - const iter = exports.opIterator(astr); + const iter = new OpIter(astr); const assem = exports.smartOpAssembler(); let attOp = new Op(); const csOp = new Op(); @@ -2033,13 +2045,13 @@ exports.inverse = (cs, lines, alines, pool) => { let curLineNextOp = new Op('+'); const unpacked = exports.unpack(cs); - const csIter = exports.opIterator(unpacked.ops); + const csIter = new OpIter(unpacked.ops); const builder = exports.builder(unpacked.newLen); const consumeAttribRuns = (numChars, func /* (len, attribs, endsLine)*/) => { if ((!curLineOpIter) || (curLineOpIterLine !== curLine)) { // create curLineOpIter and advance it to curChar - curLineOpIter = exports.opIterator(alinesGet(curLine)); + curLineOpIter = new OpIter(alinesGet(curLine)); curLineOpIterLine = curLine; let indexIntoLine = 0; while (curLineOpIter.hasNext()) { @@ -2058,7 +2070,7 @@ exports.inverse = (cs, lines, alines, pool) => { curChar = 0; curLineOpIterLine = curLine; curLineNextOp.chars = 0; - curLineOpIter = exports.opIterator(alinesGet(curLine)); + curLineOpIter = new OpIter(alinesGet(curLine)); } if (!curLineNextOp.chars) { curLineNextOp = curLineOpIter.hasNext() ? curLineOpIter.next() : new Op();