diff --git a/spec/unit/utils.spec.js b/spec/unit/utils.spec.js index e922302cf..84921a24c 100644 --- a/spec/unit/utils.spec.js +++ b/spec/unit/utils.spec.js @@ -4,6 +4,7 @@ import { averageBetweenStrings, baseToString, DEFAULT_ALPHABET, + lexicographicCompare, nextString, prevString, stringToBase, @@ -279,15 +280,9 @@ describe("utils", function() { describe('alphabetPad', () => { it('should pad to the alphabet length', () => { - const defaultPrefixFor1char = [""].reduce(() => { - let s = ""; - for (let i = 0; i < DEFAULT_ALPHABET.length - 1; i++) { - s += DEFAULT_ALPHABET[0]; - } - return s; - }, ""); - expect(alphabetPad("a")).toEqual(defaultPrefixFor1char + "a"); - expect(alphabetPad("a", "123")).toEqual("11a"); + const len = 12; + expect(alphabetPad("a", len)).toEqual("a" + ("".padEnd(len - 1, DEFAULT_ALPHABET[0]))); + expect(alphabetPad("a", len, "123")).toEqual("a" + ("".padEnd(len - 1, '1'))); }); }); @@ -313,12 +308,12 @@ describe("utils", function() { describe('averageBetweenStrings', () => { it('should average appropriately', () => { - expect(averageBetweenStrings('A', 'z')).toEqual(']'); - expect(averageBetweenStrings('a', 'z', "abcdefghijklmnopqrstuvwxyz")).toEqual('m'); + expect(averageBetweenStrings('A', 'z')).toEqual('^'); + expect(averageBetweenStrings('a', 'z', "abcdefghijklmnopqrstuvwxyz")).toEqual('n'); expect(averageBetweenStrings('AA', 'zz')).toEqual('^.'); - expect(averageBetweenStrings('aa', 'zz', "abcdefghijklmnopqrstuvwxyz")).toEqual('mz'); - expect(averageBetweenStrings('cat', 'doggo')).toEqual("BH65B"); - expect(averageBetweenStrings('cat', 'doggo', "abcdefghijklmnopqrstuvwxyz")).toEqual("buedq"); + expect(averageBetweenStrings('aa', 'zz', "abcdefghijklmnopqrstuvwxyz")).toEqual('na'); + expect(averageBetweenStrings('cat', 'doggo')).toEqual("d9>Cw"); + expect(averageBetweenStrings('cat', 'doggo', "abcdefghijklmnopqrstuvwxyz")).toEqual("cumqh"); }); }); @@ -339,4 +334,46 @@ describe("utils", function() { expect(prevString('cau', 'abcdefghijklmnopqrstuvwxyz')).toEqual('cat'); }); }); + + // Let's just ensure the ordering is sensible for lexicographic ordering + describe('string averaging unified', () => { + it('should be truly previous and next', () => { + let midpoint = "cat"; + + // We run this test 100 times to ensure we end up with a sane sequence. + for (let i = 0; i < 100; i++) { + const next = nextString(midpoint); + const prev = prevString(midpoint); + console.log({i, midpoint, next, prev}); // for test debugging + + expect(lexicographicCompare(midpoint, next) < 0).toBe(true); + expect(lexicographicCompare(midpoint, prev) > 0).toBe(true); + expect(averageBetweenStrings(prev, next)).toBe(midpoint); + + midpoint = next; + } + }); + }); + + describe('lexicographicCompare', () => { + it('should work', () => { + // Simple tests + expect(lexicographicCompare('a', 'b') < 0).toBe(true); + expect(lexicographicCompare('ab', 'b') < 0).toBe(true); + expect(lexicographicCompare('cat', 'dog') < 0).toBe(true); + + // Simple tests (reversed) + expect(lexicographicCompare('b', 'a') > 0).toBe(true); + expect(lexicographicCompare('b', 'ab') > 0).toBe(true); + expect(lexicographicCompare('dog', 'cat') > 0).toBe(true); + + // Simple equality tests + expect(lexicographicCompare('a', 'a') === 0).toBe(true); + expect(lexicographicCompare('A', 'A') === 0).toBe(true); + + // ASCII rule testing + expect(lexicographicCompare('A', 'a') < 0).toBe(true); + expect(lexicographicCompare('a', 'A') > 0).toBe(true); + }); + }); }); diff --git a/src/client.ts b/src/client.ts index ad0f87510..8015b2ad5 100644 --- a/src/client.ts +++ b/src/client.ts @@ -7776,8 +7776,8 @@ export class MatrixClient extends EventEmitter { UNSTABLE_MSC3088_PURPOSE.name, UNSTABLE_MSC3089_TREE_SUBTYPE.name); - if (!createEvent || Array.isArray(createEvent)) throw new Error("Expected single room create event"); - if (!purposeEvent || Array.isArray(purposeEvent)) return null; + if (!createEvent) throw new Error("Expected single room create event"); + if (!purposeEvent) return null; if (!purposeEvent.getContent()?.[UNSTABLE_MSC3088_ENABLED.name]) return null; if (createEvent.getContent()?.[RoomCreateTypeField] !== RoomType.Space) return null; diff --git a/src/models/MSC3089TreeSpace.ts b/src/models/MSC3089TreeSpace.ts index 74e6a7163..b7849392e 100644 --- a/src/models/MSC3089TreeSpace.ts +++ b/src/models/MSC3089TreeSpace.ts @@ -19,7 +19,7 @@ import { EventType, IEncryptedFile, MsgType, UNSTABLE_MSC3089_BRANCH, UNSTABLE_M import { Room } from "./room"; import { logger } from "../logger"; import { MatrixEvent } from "./event"; -import { averageBetweenStrings, DEFAULT_ALPHABET, nextString, prevString } from "../utils"; +import { averageBetweenStrings, DEFAULT_ALPHABET, lexicographicCompare, nextString, prevString } from "../utils"; import { MSC3089Branch } from "./MSC3089Branch"; /** @@ -233,17 +233,17 @@ export class MSC3089TreeSpace { const roomA = this.client.getRoom(a.roomId); const roomB = this.client.getRoom(b.roomId); if (!roomA || !roomB) { // just don't bother trying to do more partial sorting - return a.roomId.localeCompare(b.roomId); + return lexicographicCompare(a.roomId, b.roomId); } const createTsA = roomA.currentState.getStateEvents(EventType.RoomCreate, "")?.getTs() ?? 0; const createTsB = roomB.currentState.getStateEvents(EventType.RoomCreate, "")?.getTs() ?? 0; if (createTsA === createTsB) { - return a.roomId.localeCompare(b.roomId); + return lexicographicCompare(a.roomId, b.roomId); } return createTsA - createTsB; } else { // both not-null orders - return a.order.localeCompare(b.order); + return lexicographicCompare(a.order, b.order); } }); return ordered; diff --git a/src/utils.ts b/src/utils.ts index dea8511c3..326fe359c 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -475,16 +475,17 @@ export const DEFAULT_ALPHABET = [""].reduce(() => { }, ""); /** - * Pads a string using the given alphabet as a base. The returned string will be the - * same length as the alphabet, and padded with the first character in the alphabet. + * Pads a string using the given alphabet as a base. The returned string will be + * padded at the end with the first character in the alphabet. * * This is intended for use with string averaging. * @param {string} s The string to pad. + * @param {number} n The length to pad to. * @param {string} alphabet The alphabet to use as a single string. * @returns {string} The padded string. */ -export function alphabetPad(s: string, alphabet = DEFAULT_ALPHABET): string { - return s.padStart(alphabet.length, alphabet[0]); +export function alphabetPad(s: string, n: number, alphabet = DEFAULT_ALPHABET): string { + return s.padEnd(n, alphabet[0]); } /** @@ -512,12 +513,12 @@ export function baseToString(n: number, alphabet = DEFAULT_ALPHABET): string { * @returns {number} The baseN number. */ export function stringToBase(s: string, alphabet = DEFAULT_ALPHABET): number { - s = alphabetPad(s, alphabet); const len = alphabet.length; - const reversedStr = Array.from(s).reverse(); + const reversedStr = Array.from(s).reverse().join(""); // keep as string let result = 0; - for (let i = 0; i < len; i++) { - result += alphabet.indexOf(reversedStr[i]) * (len ** i); + for (let i = 0; i < reversedStr.length; i++) { + // Cost effective version of `result += alphabet.indexOf(reversedStr[i]) * (len ** i);` + result += (reversedStr.charCodeAt(i) - alphabet.charCodeAt(0)) * (len ** i); } return result; } @@ -532,7 +533,10 @@ export function stringToBase(s: string, alphabet = DEFAULT_ALPHABET): number { * @returns {string} The midpoint between the strings, as a string. */ export function averageBetweenStrings(a: string, b: string, alphabet = DEFAULT_ALPHABET): string { - return baseToString(Math.floor((stringToBase(a, alphabet) + stringToBase(b, alphabet)) / 2), alphabet); + const padN = Math.max(a.length, b.length); + const baseA = stringToBase(alphabetPad(a, padN, alphabet), alphabet); + const baseB = stringToBase(alphabetPad(b, padN, alphabet), alphabet); + return baseToString(Math.round((baseA + baseB) / 2), alphabet); } /** @@ -558,3 +562,16 @@ export function nextString(s: string, alphabet = DEFAULT_ALPHABET): string { export function prevString(s: string, alphabet = DEFAULT_ALPHABET): string { return baseToString(stringToBase(s, alphabet) - 1, alphabet); } + +/** + * Compares strings lexicographically as a sort-safe function. + * @param {string} a The first (reference) string. + * @param {string} b The second (compare) string. + * @returns {number} Negative if the reference string is before the compare string; + * positive if the reference string is after; and zero if equal. + */ +export function lexicographicCompare(a: string, b: string): number { + // Dev note: this exists because I'm sad that you can use math operators on strings, so I've + // hidden the operation in this function. + return (a < b) ? -1 : ((a === b) ? 0 : 1); +}