From 0491bfe10f160a8c7f22aee2fec7f6bd0344f016 Mon Sep 17 00:00:00 2001 From: Guangcong Luo Date: Mon, 23 Nov 2020 10:46:26 +0000 Subject: [PATCH] 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 | 48 +++------------ server/rooms.ts | 88 +++++++++++++++++---------- server/tournaments/index.ts | 36 ++--------- 3 files changed, 69 insertions(+), 103 deletions(-) diff --git a/server/chat-commands/room-settings.ts b/server/chat-commands/room-settings.ts index 9ae3f6938a..45928a5b51 100644 --- a/server/chat-commands/room-settings.ts +++ b/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|
This room has been deleted.
`); - 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; - if (!parent.subRooms) parent.subRooms = new Map(); - parent.subRooms.set(room.roomid, room); + const settingsList = Rooms.global.settingsList; - const mainIdx = Rooms.global.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 subIdx = Rooms.global.settingsList.findIndex(r => r.title === room!.title); + const parentIndex = settingsList.findIndex(r => r.title === parent.title); + const index = settingsList.findIndex(r => r.title === room!.title); - // This is needed to ensure that the main room gets loaded before the subroom. - if (mainIdx > subIdx) { - const tmp = Rooms.global.settingsList[mainIdx]; - Rooms.global.settingsList[mainIdx] = Rooms.global.settingsList[subIdx]; - Rooms.global.settingsList[subIdx] = tmp; + // Ensure that the parent room gets loaded before the subroom. + if (parentIndex > index) { + [settingsList[parentIndex], settingsList[index]] = [settingsList[index], settingsList[parentIndex]]; } - room.settings.parentid = parent.roomid; - room.saveSettings(); - - for (const userid in room.users) { - room.users[userid].updateIdentity(room.roomid); - } + room.setParent(parent); 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; - 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); - } + room.setParent(null); this.modlog('UNSUBROOM'); return this.addModAction(`This room was unset as a subroom by ${user.name}.`); diff --git a/server/rooms.ts b/server/rooms.ts index 374e3d7347..8dae3f1f36 100644 --- a/server/rooms.ts +++ b/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; - subRooms: Map | null; + /** use `setParent` to set this */ + readonly parent: Room | null; + /** use `subroom.setParent` to set this, or `clearSubRooms` to clear it */ + readonly subRooms: ReadonlyMap | 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); - - if (parent) { - if (!parent.subRooms) parent.subRooms = new Map(); - parent.subRooms.set(this.roomid, this as ChatRoom); - this.parent = parent; - } + this.setParent(Rooms.get(options.parentid) || null); } 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.parent.subRooms.set(newID, this as ChatRoom); + (this as any).parent.subRooms.delete(oldID); + (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.parent.subRooms.delete(this.roomid); - if (!this.parent.subRooms.size) this.parent.subRooms = null; - } + this.setParent(null); + this.clearSubRooms(); 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; - active: false; - type: 'chat'; - parent: ChatRoom | null; - constructor() { - super(''); - this.battle = null; - this.active = false; - this.type = 'chat'; - this.parent = null; - } + battle = null; + active: false = false; + type: 'chat' = 'chat'; } 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 & 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; diff --git a/server/tournaments/index.ts b/server/tournaments/index.ts index caca3714d5..fd75f2ec1b 100644 --- a/server/tournaments/index.ts +++ b/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.parent.subRooms?.delete(match.room.roomid); - if (!match.room.parent.subRooms?.size) match.room.parent.subRooms = null; - match.room.parent = null; - } + match.room.setParent(null); match.room.addRaw(`
The tournament was forcefully ended.
You can finish playing, but this battle is no longer considered a tournament battle.
`); } } @@ -504,13 +500,7 @@ export class Tournament extends Rooms.RoomGame { Utils.html`
${user.name} is no longer in the tournament.
` + `You can finish playing, but this battle is no longer considered a tournament battle.
` ).update(); - if (matchPlayer.inProgressMatch.room.parent) { - 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; - } + matchPlayer.inProgressMatch.room.setParent(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.parent.subRooms?.delete(matchFrom.room.roomid); - if (!matchFrom.room.parent.subRooms?.size) { - matchFrom.room.parent.subRooms = null; - } - matchFrom.room.parent = null; - } + matchFrom.room.setParent(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.parent.subRooms?.delete(matchRoom.roomid); - if (!matchRoom.parent.subRooms?.size) { - matchRoom.parent.subRooms = null; - } - matchRoom.parent = null; - } + matchRoom.setParent(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.parent.subRooms?.delete(room.roomid); - if (!room.parent.subRooms?.size) room.parent.subRooms = null; - room.parent = null; - } + room.setParent(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];