diff --git a/src/components/views/rooms/RoomList.tsx b/src/components/views/rooms/RoomList.tsx index aff6099ab0..dd8b567c26 100644 --- a/src/components/views/rooms/RoomList.tsx +++ b/src/components/views/rooms/RoomList.tsx @@ -42,6 +42,8 @@ import { ViewRoomDeltaPayload } from "../../../dispatcher/payloads/ViewRoomDelta import { RoomNotificationStateStore } from "../../../stores/notifications/RoomNotificationStateStore"; import SettingsStore from "../../../settings/SettingsStore"; import CustomRoomTagStore from "../../../stores/CustomRoomTagStore"; +import { arrayFastClone, arrayHasDiff } from "../../../utils/arrays"; +import { objectShallowClone } from "../../../utils/objects"; interface IProps { onKeyDown: (ev: React.KeyboardEvent) => void; @@ -80,7 +82,7 @@ interface ITagAesthetics { sectionLabel: string; sectionLabelRaw?: string; addRoomLabel?: string; - onAddRoom?: (dispatcher: Dispatcher) => void; + onAddRoom?: (dispatcher?: Dispatcher) => void; isInvite: boolean; defaultHidden: boolean; } @@ -104,14 +106,18 @@ const TAG_AESTHETICS: { isInvite: false, defaultHidden: false, addRoomLabel: _td("Start chat"), - onAddRoom: (dispatcher: Dispatcher) => dispatcher.dispatch({action: 'view_create_chat'}), + onAddRoom: (dispatcher?: Dispatcher) => { + (dispatcher || defaultDispatcher).dispatch({action: 'view_create_chat'}); + }, }, [DefaultTagID.Untagged]: { sectionLabel: _td("Rooms"), isInvite: false, defaultHidden: false, addRoomLabel: _td("Create room"), - onAddRoom: (dispatcher: Dispatcher) => dispatcher.dispatch({action: 'view_create_room'}), + onAddRoom: (dispatcher?: Dispatcher) => { + (dispatcher || defaultDispatcher).dispatch({action: 'view_create_room'}) + }, }, [DefaultTagID.LowPriority]: { sectionLabel: _td("Low priority"), @@ -231,9 +237,33 @@ export default class RoomList extends React.Component { console.log("new lists", newLists); } - this.setState({sublists: newLists}, () => { - this.props.onResize(); - }); + const previousListIds = Object.keys(this.state.sublists); + const newListIds = Object.keys(newLists); + + let doUpdate = arrayHasDiff(previousListIds, newListIds); + if (!doUpdate) { + // so we didn't have the visible sublists change, but did the contents of those + // sublists change significantly enough to break the sticky headers? Probably, so + // let's check the length of each. + for (const tagId of newListIds) { + const oldRooms = this.state.sublists[tagId]; + const newRooms = newLists[tagId]; + if (oldRooms.length !== newRooms.length) { + doUpdate = true; + break; + } + } + } + + if (doUpdate) { + // We have to break our reference to the room list store if we want to be able to + // diff the object for changes, so do that. + const sublists = objectShallowClone(newLists, (k, v) => arrayFastClone(v)); + + this.setState({sublists}, () => { + this.props.onResize(); + }); + } }; private renderCommunityInvites(): React.ReactElement[] { @@ -298,16 +328,14 @@ export default class RoomList extends React.Component { : TAG_AESTHETICS[orderedTagId]; if (!aesthetics) throw new Error(`Tag ${orderedTagId} does not have aesthetics`); - const onAddRoomFn = aesthetics.onAddRoom ? () => aesthetics.onAddRoom(dis) : null; components.push( void; @@ -90,6 +91,7 @@ interface IState { isResizing: boolean; isExpanded: boolean; // used for the for expand of the sublist when the room list is being filtered height: number; + rooms: Room[]; } export default class RoomSublist extends React.Component { @@ -104,16 +106,19 @@ export default class RoomSublist extends React.Component { this.layout = RoomListLayoutStore.instance.getLayoutFor(this.props.tagId); this.heightAtStart = 0; - const height = this.calculateInitialHeight(); this.state = { notificationState: RoomNotificationStateStore.instance.getListState(this.props.tagId), contextMenuPosition: null, isResizing: false, isExpanded: this.props.isFiltered ? this.props.isFiltered : !this.layout.isCollapsed, - height, + height: 0, // to be fixed in a moment, we need `rooms` to calculate this. + rooms: RoomListStore.instance.orderedLists[this.props.tagId] || [], }; - this.state.notificationState.setRooms(this.props.rooms); + // Why Object.assign() and not this.state.height? Because TypeScript says no. + this.state = Object.assign(this.state, {height: this.calculateInitialHeight()}); + this.state.notificationState.setRooms(this.state.rooms); this.dispatcherRef = defaultDispatcher.register(this.onAction); + RoomListStore.instance.on(LISTS_UPDATE_EVENT, this.onListsUpdated); } private calculateInitialHeight() { @@ -142,11 +147,11 @@ export default class RoomSublist extends React.Component { } private get numTiles(): number { - return RoomSublist.calcNumTiles(this.props); + return RoomSublist.calcNumTiles(this.state.rooms, this.props.extraBadTilesThatShouldntExist); } - private static calcNumTiles(props) { - return (props.rooms || []).length + (props.extraBadTilesThatShouldntExist || []).length; + private static calcNumTiles(rooms: Room[], extraTiles: any[]) { + return (rooms || []).length + (extraTiles || []).length; } private get numVisibleTiles(): number { @@ -154,8 +159,8 @@ export default class RoomSublist extends React.Component { return Math.min(nVisible, this.numTiles); } - public componentDidUpdate(prevProps: Readonly) { - this.state.notificationState.setRooms(this.props.rooms); + public componentDidUpdate(prevProps: Readonly, prevState: Readonly) { + this.state.notificationState.setRooms(this.state.rooms); if (prevProps.isFiltered !== this.props.isFiltered) { if (this.props.isFiltered) { this.setState({isExpanded: true}); @@ -165,22 +170,87 @@ export default class RoomSublist extends React.Component { } // as the rooms can come in one by one we need to reevaluate // the amount of available rooms to cap the amount of requested visible rooms by the layout - if (RoomSublist.calcNumTiles(prevProps) !== this.numTiles) { + if (RoomSublist.calcNumTiles(prevState.rooms, prevProps.extraBadTilesThatShouldntExist) !== this.numTiles) { this.setState({height: this.calculateInitialHeight()}); } } + public shouldComponentUpdate(nextProps: Readonly, nextState: Readonly): boolean { + if (objectHasValueChange(this.props, nextProps)) { + // Something we don't care to optimize has updated, so update. + return true; + } + + // Do the same check used on props for state, without the rooms we're going to no-op + const prevStateNoRooms = objectExcluding(this.state, ['rooms']); + const nextStateNoRooms = objectExcluding(nextState, ['rooms']); + if (objectHasValueChange(prevStateNoRooms, nextStateNoRooms)) { + return true; + } + + // If we're supposed to handle extra tiles, take the performance hit and re-render all the + // time so we don't have to consider them as part of the visible room optimization. + const prevExtraTiles = this.props.extraBadTilesThatShouldntExist || []; + const nextExtraTiles = nextProps.extraBadTilesThatShouldntExist || []; + if (prevExtraTiles.length > 0 || nextExtraTiles.length > 0) { + return true; + } + + // If we're about to update the height of the list, we don't really care about which rooms + // are visible or not for no-op purposes, so ensure that the height calculation runs through. + if (RoomSublist.calcNumTiles(nextState.rooms, nextProps.extraBadTilesThatShouldntExist) !== this.numTiles) { + return true; + } + + // Before we go analyzing the rooms, we can see if we're collapsed. If we're collapsed, we don't need + // to render anything. We do this after the height check though to ensure that the height gets appropriately + // calculated for when/if we become uncollapsed. + if (!nextState.isExpanded) { + return false; + } + + // Quickly double check we're not about to break something due to the number of rooms changing. + if (this.state.rooms.length !== nextState.rooms.length) { + return true; + } + + // Finally, determine if the room update (as presumably that's all that's left) is within + // our visible range. If it is, then do a render. If the update is outside our visible range + // then we can skip the update. + // + // We also optimize for order changing here: if the update did happen in our visible range + // but doesn't result in the list re-sorting itself then there's no reason for us to update + // on our own. + const prevSlicedRooms = this.state.rooms.slice(0, this.numVisibleTiles); + const nextSlicedRooms = nextState.rooms.slice(0, this.numVisibleTiles); + if (arrayHasOrderChange(prevSlicedRooms, nextSlicedRooms)) { + return true; + } + + // Finally, nothing happened so no-op the update + return false; + } + public componentWillUnmount() { this.state.notificationState.destroy(); defaultDispatcher.unregister(this.dispatcherRef); + RoomListStore.instance.off(LISTS_UPDATE_EVENT, this.onListsUpdated); } + private onListsUpdated = () => { + const currentRooms = this.state.rooms; + const newRooms = RoomListStore.instance.orderedLists[this.props.tagId] || []; + if (arrayHasOrderChange(currentRooms, newRooms)) { + this.setState({rooms: newRooms}); + } + }; + private onAction = (payload: ActionPayload) => { - if (payload.action === "view_room" && payload.show_room_tile && this.props.rooms) { + if (payload.action === "view_room" && payload.show_room_tile && this.state.rooms) { // XXX: we have to do this a tick later because we have incorrect intermediate props during a room change // where we lose the room we are changing from temporarily and then it comes back in an update right after. setImmediate(() => { - const roomIndex = this.props.rooms.findIndex((r) => r.roomId === payload.room_id); + const roomIndex = this.state.rooms.findIndex((r) => r.roomId === payload.room_id); if (!this.state.isExpanded && roomIndex > -1) { this.toggleCollapsed(); @@ -302,10 +372,10 @@ export default class RoomSublist extends React.Component { let room; if (this.props.tagId === DefaultTagID.Invite) { // switch to first room as that'll be the top of the list for the user - room = this.props.rooms && this.props.rooms[0]; + room = this.state.rooms && this.state.rooms[0]; } else { // find the first room with a count of the same colour as the badge count - room = this.props.rooms.find((r: Room) => { + room = this.state.rooms.find((r: Room) => { const notifState = this.state.notificationState.getForRoom(r); return notifState.count > 0 && notifState.color === this.state.notificationState.color; }); @@ -399,8 +469,8 @@ export default class RoomSublist extends React.Component { const tiles: React.ReactElement[] = []; - if (this.props.rooms) { - const visibleRooms = this.props.rooms.slice(0, this.numVisibleTiles); + if (this.state.rooms) { + const visibleRooms = this.state.rooms.slice(0, this.numVisibleTiles); for (const room of visibleRooms) { tiles.push( { ActiveRoomObserver.addListener(this.props.room.roomId, this.onActiveRoomUpdate); this.dispatcherRef = defaultDispatcher.register(this.onAction); + MessagePreviewStore.instance.on(ROOM_PREVIEW_CHANGED, this.onRoomPreviewChanged); } private get showContextMenu(): boolean { @@ -150,6 +151,7 @@ export default class RoomTile extends React.Component { ActiveRoomObserver.removeListener(this.props.room.roomId, this.onActiveRoomUpdate); } defaultDispatcher.unregister(this.dispatcherRef); + MessagePreviewStore.instance.off(ROOM_PREVIEW_CHANGED, this.onRoomPreviewChanged); } private onAction = (payload: ActionPayload) => { @@ -160,6 +162,12 @@ export default class RoomTile extends React.Component { } }; + private onRoomPreviewChanged = (room: Room) => { + if (this.props.room && room.roomId === this.props.room.roomId) { + this.forceUpdate(); // we don't have any state to set, so just complain that we need an update + } + }; + private scrollIntoView = () => { if (!this.roomTileRef.current) return; this.roomTileRef.current.scrollIntoView({ diff --git a/src/stores/room-list/MessagePreviewStore.ts b/src/stores/room-list/MessagePreviewStore.ts index f61488c3bb..2803f0a23e 100644 --- a/src/stores/room-list/MessagePreviewStore.ts +++ b/src/stores/room-list/MessagePreviewStore.ts @@ -28,6 +28,10 @@ import { StickerEventPreview } from "./previews/StickerEventPreview"; import { ReactionEventPreview } from "./previews/ReactionEventPreview"; import { UPDATE_EVENT } from "../AsyncStore"; +// Emitted event for when a room's preview has changed. First argument will the room for which +// the change happened. +export const ROOM_PREVIEW_CHANGED = "room_preview_changed"; + const PREVIEWS = { 'm.room.message': { isState: false, @@ -146,6 +150,7 @@ export class MessagePreviewStore extends AsyncStoreWithClient { // We've muted the underlying Map, so just emit that we've changed. this.previews.set(room.roomId, map); this.emit(UPDATE_EVENT, this); + this.emit(ROOM_PREVIEW_CHANGED, room); } return; // we're done } @@ -153,6 +158,7 @@ export class MessagePreviewStore extends AsyncStoreWithClient { // At this point, we didn't generate a preview so clear it this.previews.set(room.roomId, new Map()); this.emit(UPDATE_EVENT, this); + this.emit(ROOM_PREVIEW_CHANGED, room); } protected async onAction(payload: ActionPayload) { diff --git a/src/utils/arrays.ts b/src/utils/arrays.ts index 8175d89464..2a5b1b5f16 100644 --- a/src/utils/arrays.ts +++ b/src/utils/arrays.ts @@ -14,11 +14,38 @@ See the License for the specific language governing permissions and limitations under the License. */ +/** + * Clones an array as fast as possible, retaining references of the array's values. + * @param a The array to clone. Must be defined. + * @returns A copy of the array. + */ +export function arrayFastClone(a: any[]): any[] { + return a.slice(0, a.length); +} + +/** + * Determines if the two arrays are different either in length, contents, + * or order of those contents. + * @param a The first array. Must be defined. + * @param b The second array. Must be defined. + * @returns True if they are different, false otherwise. + */ +export function arrayHasOrderChange(a: any[], b: any[]): boolean { + if (a.length === b.length) { + for (let i = 0; i < a.length; i++) { + if (a[i] !== b[i]) return true; + } + return false; + } else { + return true; // like arrayHasDiff, a difference in length is a natural change + } +} + /** * Determines if two arrays are different through a shallow comparison. * @param a The first array. Must be defined. * @param b The second array. Must be defined. - * @returns True if they are the same, false otherwise. + * @returns True if they are different, false otherwise. */ export function arrayHasDiff(a: any[], b: any[]): boolean { if (a.length === b.length) { diff --git a/src/utils/objects.ts b/src/utils/objects.ts index 14fa928ce2..db8248759d 100644 --- a/src/utils/objects.ts +++ b/src/utils/objects.ts @@ -14,7 +14,63 @@ See the License for the specific language governing permissions and limitations under the License. */ -import { arrayDiff, arrayMerge, arrayUnion } from "./arrays"; +import { arrayDiff, arrayHasDiff, arrayMerge, arrayUnion } from "./arrays"; + +/** + * Gets a new object which represents the provided object, excluding some properties. + * @param a The object to strip properties of. Must be defined. + * @param props The property names to remove. + * @returns The new object without the provided properties. + */ +export function objectExcluding(a: any, props: string[]): any { + // We use a Map to avoid hammering the `delete` keyword, which is slow and painful. + const tempMap = new Map(Object.entries(a)); + for (const prop of props) { + tempMap.delete(prop); + } + + // Convert the map to an object again + return Array.from(tempMap.entries()).reduce((c, [k, v]) => { + c[k] = v; + return c; + }, {}); +} + +/** + * Clones an object to a caller-controlled depth. When a propertyCloner is supplied, the + * object's properties will be passed through it with the return value used as the new + * object's type. This is intended to be used to deep clone a reference, but without + * having to deep clone the entire object. This function is safe to call recursively within + * the propertyCloner. + * @param a The object to clone. Must be defined. + * @param propertyCloner The function to clone the properties of the object with, optionally. + * First argument is the property key with the second being the current value. + * @returns A cloned object. + */ +export function objectShallowClone(a: any, propertyCloner?: (k: string, v: any) => any): any { + const newObj = {}; + for (const [k, v] of Object.entries(a)) { + newObj[k] = v; + if (propertyCloner) { + newObj[k] = propertyCloner(k, v); + } + } + return newObj; +} + +/** + * Determines if the two objects, which are assumed to be of the same + * key shape, have a difference in their values. If a difference is + * determined, true is returned. + * @param a The first object. Must be defined. + * @param b The second object. Must be defined. + * @returns True if there's a perceptual difference in the object's values. + */ +export function objectHasValueChange(a: any, b: any): boolean { + const aValues = Object.values(a); + const bValues = Object.values(b); + return arrayHasDiff(aValues, bValues); +} /** * Determines the keys added, changed, and removed between two objects.