Refactor subroom code

This introduces a new function, `setParent`, to handle the details
of setting up subrooms.

`roomid`, `parent`, and `subRooms` are now read-only, so they can't
be accidentally be set directly rather than through their setters
(`rename`, `setParent`, and `clearSubRooms`).

I don't think setters should be used for this, because I think it's
important to be clear that `rename` and `setParent` will change a lot
of other state and induce network activity.

server/chat-commands/room-settings.ts

@@ -880,16 +880,9 @@ export const commands: ChatCommands = {
 			}
 		}
 
-		if (room.subRooms) {
-			for (const subRoom of room.subRooms.values()) subRoom.parent = null;
-		}
-
 		room.add(`|raw|<div class="broadcast-red"><b>This room has been deleted.</b></div>`);
-		room.update(); // |expire| needs to be its own message
-		room.add(`|expire|This room has been deleted.`);
-		this.sendReply(`The room "${title}" was deleted.`);
 		room.update();
-		if (room.roomid === 'lobby') Rooms.lobby = null;
+		room.send(`|expire|This room has been deleted.`);
 		room.destroy();
 	},
 	deleteroomhelp: [
@@ -1137,27 +1130,17 @@ export const commands: ChatCommands = {
 		if (!parent.persist) return this.errorReply(`Temporary rooms cannot be parent rooms.`);
 		if (room === parent) return this.errorReply(`You cannot set a room to be a subroom of itself.`);
 
-		room.parent = parent;
+		const settingsList = Rooms.global.settingsList;
-		if (!parent.subRooms) parent.subRooms = new Map();
-		parent.subRooms.set(room.roomid, room);
 
-		const mainIdx = Rooms.global.settingsList.findIndex(r => r.title === parent.title);
+		const parentIndex = settingsList.findIndex(r => r.title === parent.title);
-		// can be asserted since we want this to crash if room is null (it should never be)
+		const index = settingsList.findIndex(r => r.title === room!.title);
-		const subIdx = Rooms.global.settingsList.findIndex(r => r.title === room!.title);
 
-		// This is needed to ensure that the main room gets loaded before the subroom.
+		// Ensure that the parent room gets loaded before the subroom.
-		if (mainIdx > subIdx) {
+		if (parentIndex > index) {
-			const tmp = Rooms.global.settingsList[mainIdx];
+			[settingsList[parentIndex], settingsList[index]] = [settingsList[index], settingsList[parentIndex]];
-			Rooms.global.settingsList[mainIdx] = Rooms.global.settingsList[subIdx];
-			Rooms.global.settingsList[subIdx] = tmp;
 		}
 
-		room.settings.parentid = parent.roomid;
+		room.setParent(parent);
-		room.saveSettings();
-
-		for (const userid in room.users) {
-			room.users[userid].updateIdentity(room.roomid);
-		}
 
 		this.modlog('SUBROOM', null, `of ${parent.title}`);
 		return this.addModAction(`This room was set as a subroom of ${parent.title} by ${user.name}.`);
@@ -1172,20 +1155,7 @@ export const commands: ChatCommands = {
 			return this.errorReply(`This room is not currently a subroom of a public room.`);
 		}
 
-		const parent = room.parent;
+		room.setParent(null);
-		if (parent?.subRooms) {
-			parent.subRooms.delete(room.roomid);
-			if (!parent.subRooms.size) parent.subRooms = null;
-		}
-
-		room.parent = null;
-
-		delete room.settings.parentid;
-		room.saveSettings();
-
-		for (const userid in room.users) {
-			room.users[userid].updateIdentity(room.roomid);
-		}
 
 		this.modlog('UNSUBROOM');
 		return this.addModAction(`This room was unset as a subroom by ${user.name}.`);

server/rooms.ts

@@ -137,7 +137,8 @@ import type {RoomEvent, RoomEventAlias, RoomEventCategory} from './chat-plugins/
 import type {Tournament} from './tournaments/index';
 
 export abstract class BasicRoom {
-	roomid: RoomID;
+	/** to rename use room.rename */
+	readonly roomid: RoomID;
 	title: string;
 	readonly type: 'chat' | 'battle';
 	readonly users: UserTable;
@@ -169,8 +170,10 @@ export abstract class BasicRoom {
 	tour: Tournament | null;
 
 	auth: RoomAuth;
-	parent: Room | null;
+	/** use `setParent` to set this */
-	subRooms: Map<string, Room> | null;
+	readonly parent: Room | null;
+	/** use `subroom.setParent` to set this, or `clearSubRooms` to clear it */
+	readonly subRooms: ReadonlyMap<string, Room> | null;
 
 	readonly muteQueue: MuteEntry[];
 	userCount: number;
@@ -265,13 +268,7 @@ export abstract class BasicRoom {
 		this.minorActivity = null;
 		this.minorActivityQueue = null;
 		if (options.parentid) {
-			const parent = Rooms.get(options.parentid);
+			this.setParent(Rooms.get(options.parentid) || null);
-
-			if (parent) {
-				if (!parent.subRooms) parent.subRooms = new Map();
-				parent.subRooms.set(this.roomid, this as ChatRoom);
-				this.parent = parent;
-			}
 		}
 
 		this.subRooms = null;
@@ -703,6 +700,42 @@ export abstract class BasicRoom {
 		if (newID.length > MAX_CHATROOM_ID_LENGTH) throw new Chat.ErrorMessage("The given room title is too long.");
 		if (Rooms.search(newTitle)) throw new Chat.ErrorMessage(`The room '${newTitle}' already exists.`);
 	}
+	setParent(room: Room | null) {
+		if (this.parent === room) return;
+
+		if (this.parent) {
+			(this as any).parent.subRooms.delete(this.roomid);
+			if (!this.parent.subRooms!.size) {
+				(this as any).parent.subRooms = null;
+			}
+		}
+		(this as any).parent = room;
+		if (room) {
+			if (!room.subRooms) {
+				(room as any).subRooms = new Map();
+			}
+			(room as any).subRooms.set(this.roomid, this);
+			this.settings.parentid = room.roomid;
+		} else {
+			delete this.settings.parentid;
+		}
+
+		this.saveSettings();
+
+		for (const userid in this.users) {
+			this.users[userid].updateIdentity(this.roomid);
+		}
+	}
+	clearSubRooms() {
+		if (!this.subRooms) return;
+		for (const room of this.subRooms.values()) {
+			(room as any).parent = null;
+		}
+		(this as any).subRooms = null;
+
+		// this doesn't update parentid or subroom user symbols because it's
+		// intended to be used for cleanup only
+	}
 	setPrivate(privacy: boolean | 'voice' | 'hidden') {
 		this.settings.isPrivate = privacy;
 		this.saveSettings();
@@ -760,7 +793,7 @@ export abstract class BasicRoom {
 			throw new Chat.ErrorMessage(`Please finish your game (${this.game.title}) before renaming ${this.roomid}.`);
 		}
 		const oldID = this.roomid;
-		this.roomid = newID;
+		(this as any).roomid = newID;
 		this.title = newTitle;
 		Rooms.rooms.delete(oldID);
 		Rooms.rooms.set(newID, this as Room);
@@ -803,13 +836,13 @@ export abstract class BasicRoom {
 		}
 
 		if (this.parent && this.parent.subRooms) {
-			this.parent.subRooms.delete(oldID);
+			(this as any).parent.subRooms.delete(oldID);
-			this.parent.subRooms.set(newID, this as ChatRoom);
+			(this as any).parent.subRooms.set(newID, this as ChatRoom);
 		}
-
 		if (this.subRooms) {
 			for (const subRoom of this.subRooms.values()) {
-				subRoom.parent = this as ChatRoom;
+				(subRoom as any).parent = this as ChatRoom;
+				subRoom.settings.parentid = newID;
 			}
 		}
 
@@ -916,10 +949,8 @@ export abstract class BasicRoom {
 			delete this.users[i];
 		}
 
-		if (this.parent && this.parent.subRooms) {
+		this.setParent(null);
-			this.parent.subRooms.delete(this.roomid);
+		this.clearSubRooms();
-			if (!this.parent.subRooms.size) this.parent.subRooms = null;
-		}
 
 		Rooms.global.deregisterChatRoom(this.roomid);
 		Rooms.global.delistChatRoom(this.roomid);
@@ -960,8 +991,8 @@ export abstract class BasicRoom {
 
 		void this.log.destroy(true);
 
-		// get rid of some possibly-circular references
 		Rooms.rooms.delete(this.roomid);
+		if (this.roomid === 'lobby') Rooms.lobby = null;
 	}
 	tr(strings: string | TemplateStringsArray, ...keys: any[]) {
 		return Chat.tr(this.settings.language || 'english' as ID, strings, ...keys);
@@ -1545,17 +1576,9 @@ export class GlobalRoomState {
 export class ChatRoom extends BasicRoom {
 	// This is not actually used, this is just a fake class to keep
 	// TypeScript happy
-	battle: null;
+	battle = null;
-	active: false;
+	active: false = false;
-	type: 'chat';
+	type: 'chat' = 'chat';
-	parent: ChatRoom | null;
-	constructor() {
-		super('');
-		this.battle = null;
-		this.active = false;
-		this.type = 'chat';
-		this.parent = null;
-	}
 }
 
 export class GameRoom extends BasicRoom {
@@ -1574,7 +1597,6 @@ export class GameRoom extends BasicRoom {
 	game: RoomGame;
 	modchatUser: string;
 	active: boolean;
-	parent: ChatRoom | null;
 	constructor(roomid: RoomID, title?: string, options: Partial<RoomSettings> & AnyObject = {}) {
 		options.noLogTimes = true;
 		options.noAutoTruncate = true;
@@ -1590,7 +1612,7 @@ export class GameRoom extends BasicRoom {
 		// console.log("NEW BATTLE");
 
 		this.tour = options.tour || null;
-		this.parent = options.parent || (this.tour && this.tour.room) || null;
+		this.setParent(options.parent || (this.tour && this.tour.room) || null);
 
 		this.p1 = options.p1 || null;
 		this.p2 = options.p2 || null;

server/tournaments/index.ts

@@ -253,11 +253,7 @@ export class Tournament extends Rooms.RoomGame {
 				const match = player.inProgressMatch;
 				if (match) {
 					match.room.tour = null;
-					if (match.room.parent) {
+					match.room.setParent(null);
-						match.room.parent.subRooms?.delete(match.room.roomid);
-						if (!match.room.parent.subRooms?.size) match.room.parent.subRooms = null;
-						match.room.parent = null;
-					}
 					match.room.addRaw(`<div class="broadcast-red"><b>The tournament was forcefully ended.</b><br />You can finish playing, but this battle is no longer considered a tournament battle.</div>`);
 				}
 			}
@@ -504,13 +500,7 @@ export class Tournament extends Rooms.RoomGame {
 				Utils.html`<div class="broadcast-red"><b>${user.name} is no longer in the tournament.<br />` +
 				`You can finish playing, but this battle is no longer considered a tournament battle.</div>`
 			).update();
-			if (matchPlayer.inProgressMatch.room.parent) {
+			matchPlayer.inProgressMatch.room.setParent(null);
-				matchPlayer.inProgressMatch.room.parent.subRooms?.delete(matchPlayer.inProgressMatch.room.roomid);
-				if (!matchPlayer.inProgressMatch.room.parent.subRooms?.size) {
-					matchPlayer.inProgressMatch.room.parent.subRooms = null;
-				}
-				matchPlayer.inProgressMatch.room.parent = null;
-			}
 			this.completedMatches.add(matchPlayer.inProgressMatch.room.roomid);
 			matchPlayer.inProgressMatch = null;
 		}
@@ -730,13 +720,7 @@ export class Tournament extends Rooms.RoomGame {
 		if (matchFrom) {
 			matchFrom.to.isBusy = false;
 			player.inProgressMatch = null;
-			if (matchFrom.room.parent) {
+			matchFrom.room.setParent(null);
-				matchFrom.room.parent.subRooms?.delete(matchFrom.room.roomid);
-				if (!matchFrom.room.parent.subRooms?.size) {
-					matchFrom.room.parent.subRooms = null;
-				}
-				matchFrom.room.parent = null;
-			}
 			this.completedMatches.add(matchFrom.room.roomid);
 			if (matchFrom.room.battle) matchFrom.room.battle.forfeit(player.name);
 		}
@@ -749,13 +733,7 @@ export class Tournament extends Rooms.RoomGame {
 		if (matchTo) {
 			matchTo.isBusy = false;
 			const matchRoom = matchTo.inProgressMatch!.room;
-			if (matchRoom.parent) {
+			matchRoom.setParent(null);
-				matchRoom.parent.subRooms?.delete(matchRoom.roomid);
-				if (!matchRoom.parent.subRooms?.size) {
-					matchRoom.parent.subRooms = null;
-				}
-				matchRoom.parent = null;
-			}
 			this.completedMatches.add(matchRoom.roomid);
 			if (matchRoom.battle) matchRoom.battle.forfeit(player.id);
 			matchTo.inProgressMatch = null;
@@ -1048,11 +1026,7 @@ export class Tournament extends Rooms.RoomGame {
 	onBattleWin(room: GameRoom, winnerid: ID) {
 		if (this.completedMatches.has(room.roomid)) return;
 		this.completedMatches.add(room.roomid);
-		if (room.parent) {
+		room.setParent(null);
-			room.parent.subRooms?.delete(room.roomid);
-			if (!room.parent.subRooms?.size) room.parent.subRooms = null;
-			room.parent = null;
-		}
 		if (!room.battle) throw new Error("onBattleWin called without a battle");
 		if (!room.p1 || !room.p2) throw new Error("onBattleWin called with missing players");
 		const p1 = this.playerTable[room.p1.id];