Update string averaging utils

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);
+        });
+    });
 });

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;

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;

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);
+}