From 016710af6a10cf6547668567ed6c029f7f1b4d02 Mon Sep 17 00:00:00 2001 From: Jorik Schellekens Date: Thu, 9 Jul 2020 00:43:49 +0100 Subject: [PATCH 1/5] Noop first breadcrumb --- src/components/views/rooms/RoomBreadcrumbs2.tsx | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/components/views/rooms/RoomBreadcrumbs2.tsx b/src/components/views/rooms/RoomBreadcrumbs2.tsx index 687f4dd73e..f08adebcd2 100644 --- a/src/components/views/rooms/RoomBreadcrumbs2.tsx +++ b/src/components/views/rooms/RoomBreadcrumbs2.tsx @@ -16,7 +16,6 @@ limitations under the License. import React from "react"; import { BreadcrumbsStore } from "../../../stores/BreadcrumbsStore"; -import AccessibleButton from "../elements/AccessibleButton"; import DecoratedRoomAvatar from "../avatars/DecoratedRoomAvatar"; import { _t } from "../../../languageHandler"; import { Room } from "matrix-js-sdk/src/models/room"; @@ -88,16 +87,19 @@ export default class RoomBreadcrumbs2 extends React.PureComponent { Analytics.trackEvent("Breadcrumbs", "click_node", index); - defaultDispatcher.dispatch({action: "view_room", room_id: room.roomId}); + // If we're rendering the first breadcrumb and this is it no-op + if (!this.state.skipFirst && index === 0) { + return; + } else { + defaultDispatcher.dispatch({action: "view_room", room_id: room.roomId}); + } }; public render(): React.ReactElement { - // TODO: Decorate crumbs with icons: https://github.com/vector-im/riot-web/issues/14040 - // TODO: Scrolling: https://github.com/vector-im/riot-web/issues/14040 - // TODO: Tooltips: https://github.com/vector-im/riot-web/issues/14040 const tiles = BreadcrumbsStore.instance.rooms.map((r, i) => { const roomTags = RoomListStore.instance.getTagsForRoom(r); const roomTag = roomTags.includes(DefaultTagID.DM) ? DefaultTagID.DM : roomTags[0]; + const anon = () => this.viewRoom(r, i); return ( Date: Wed, 8 Jul 2020 18:27:50 -0600 Subject: [PATCH 2/5] Move list layout management to its own store This is more general maintenance than performance as the RoomList doesn't need to be generating layouts for the sublists, and it certainly doesn't need to be creating a bunch of extra ones. The sublists are perfectly capable of getting their own layout instance and using it, and we are perfectly able to limit the number of these things we create through the session's lifespan. --- src/@types/global.d.ts | 2 + src/components/views/rooms/RoomList2.tsx | 11 +---- src/components/views/rooms/RoomSublist2.tsx | 44 +++++++++-------- src/stores/room-list/RoomListLayoutStore.ts | 54 +++++++++++++++++++++ src/stores/room-list/RoomListStore2.ts | 13 ++--- 5 files changed, 84 insertions(+), 40 deletions(-) create mode 100644 src/stores/room-list/RoomListLayoutStore.ts diff --git a/src/@types/global.d.ts b/src/@types/global.d.ts index 2c2fec759c..fc52296d8b 100644 --- a/src/@types/global.d.ts +++ b/src/@types/global.d.ts @@ -21,6 +21,7 @@ import ToastStore from "../stores/ToastStore"; import DeviceListener from "../DeviceListener"; import { RoomListStore2 } from "../stores/room-list/RoomListStore2"; import { PlatformPeg } from "../PlatformPeg"; +import RoomListLayoutStore from "../stores/room-list/RoomListLayoutStore"; declare global { interface Window { @@ -34,6 +35,7 @@ declare global { mx_ToastStore: ToastStore; mx_DeviceListener: DeviceListener; mx_RoomListStore2: RoomListStore2; + mx_RoomListLayoutStore: RoomListLayoutStore; mxPlatformPeg: PlatformPeg; } diff --git a/src/components/views/rooms/RoomList2.tsx b/src/components/views/rooms/RoomList2.tsx index fb0136fb29..348c424927 100644 --- a/src/components/views/rooms/RoomList2.tsx +++ b/src/components/views/rooms/RoomList2.tsx @@ -32,7 +32,6 @@ import defaultDispatcher from "../../../dispatcher/dispatcher"; import RoomSublist2 from "./RoomSublist2"; import { ActionPayload } from "../../../dispatcher/payloads"; import { NameFilterCondition } from "../../../stores/room-list/filters/NameFilterCondition"; -import { ListLayout } from "../../../stores/room-list/ListLayout"; import { MatrixClientPeg } from "../../../MatrixClientPeg"; import GroupAvatar from "../avatars/GroupAvatar"; import TemporaryTile from "./TemporaryTile"; @@ -66,7 +65,6 @@ interface IProps { interface IState { sublists: ITagMap; - layouts: Map; } const TAG_ORDER: TagID[] = [ @@ -151,7 +149,6 @@ export default class RoomList2 extends React.Component { this.state = { sublists: {}, - layouts: new Map(), }; this.dispatcherRef = defaultDispatcher.register(this.onAction); @@ -227,12 +224,7 @@ export default class RoomList2 extends React.Component { const newLists = RoomListStore.instance.orderedLists; console.log("new lists", newLists); - const layoutMap = new Map(); - for (const tagId of Object.keys(newLists)) { - layoutMap.set(tagId, new ListLayout(tagId)); - } - - this.setState({sublists: newLists, layouts: layoutMap}, () => { + this.setState({sublists: newLists}, () => { this.props.onResize(); }); }; @@ -302,7 +294,6 @@ export default class RoomList2 extends React.Component { onAddRoom={onAddRoomFn} addRoomLabel={aesthetics.addRoomLabel} isInvite={aesthetics.isInvite} - layout={this.state.layouts.get(orderedTagId)} isMinimized={this.props.isMinimized} onResize={this.props.onResize} extraBadTilesThatShouldntExist={extraTiles} diff --git a/src/components/views/rooms/RoomSublist2.tsx b/src/components/views/rooms/RoomSublist2.tsx index 732585d3ac..3eadc9a313 100644 --- a/src/components/views/rooms/RoomSublist2.tsx +++ b/src/components/views/rooms/RoomSublist2.tsx @@ -45,6 +45,7 @@ import {ActionPayload} from "../../../dispatcher/payloads"; import { Enable, Resizable } from "re-resizable"; import { Direction } from "re-resizable/lib/resizer"; import { polyfillTouchEvent } from "../../../@types/polyfill"; +import RoomListLayoutStore from "../../../stores/room-list/RoomListLayoutStore"; // TODO: Remove banner on launch: https://github.com/vector-im/riot-web/issues/14231 // TODO: Rename on launch: https://github.com/vector-im/riot-web/issues/14231 @@ -74,7 +75,6 @@ interface IProps { onAddRoom?: () => void; addRoomLabel: string; isInvite: boolean; - layout?: ListLayout; isMinimized: boolean; tagId: TagID; onResize: () => void; @@ -98,10 +98,13 @@ export default class RoomSublist2 extends React.Component { private headerButton = createRef(); private sublistRef = createRef(); private dispatcherRef: string; + private layout: ListLayout; constructor(props: IProps) { super(props); + this.layout = RoomListLayoutStore.instance.getLayoutFor(this.props.tagId); + this.state = { notificationState: new ListNotificationState(this.props.isInvite, this.props.tagId), contextMenuPosition: null, @@ -116,8 +119,7 @@ export default class RoomSublist2 extends React.Component { } private get numVisibleTiles(): number { - if (!this.props.layout) return 0; - const nVisible = Math.floor(this.props.layout.visibleTiles); + const nVisible = Math.floor(this.layout.visibleTiles); return Math.min(nVisible, this.numTiles); } @@ -135,7 +137,7 @@ export default class RoomSublist2 extends React.Component { // 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 isCollapsed = this.props.layout.isCollapsed; + const isCollapsed = this.layout.isCollapsed; const roomIndex = this.props.rooms.findIndex((r) => r.roomId === payload.room_id); if (isCollapsed && roomIndex > -1) { @@ -143,7 +145,7 @@ export default class RoomSublist2 extends React.Component { } // extend the visible section to include the room if it is entirely invisible if (roomIndex >= this.numVisibleTiles) { - this.props.layout.visibleTiles = this.props.layout.tilesWithPadding(roomIndex + 1, MAX_PADDING_HEIGHT); + this.layout.visibleTiles = this.layout.tilesWithPadding(roomIndex + 1, MAX_PADDING_HEIGHT); this.forceUpdate(); // because the layout doesn't trigger a re-render } }); @@ -170,10 +172,10 @@ export default class RoomSublist2 extends React.Component { // resizing started*, meaning it is fairly useless for us. This is why we just use // the client height and run with it. - const heightBefore = this.props.layout.visibleTiles; - const heightInTiles = this.props.layout.pixelsToTiles(refToElement.clientHeight); - this.props.layout.setVisibleTilesWithin(heightInTiles, this.numTiles); - if (heightBefore === this.props.layout.visibleTiles) return; // no-op + const heightBefore = this.layout.visibleTiles; + const heightInTiles = this.layout.pixelsToTiles(refToElement.clientHeight); + this.layout.setVisibleTilesWithin(heightInTiles, this.numTiles); + if (heightBefore === this.layout.visibleTiles) return; // no-op this.forceUpdate(); // because the layout doesn't trigger a re-render }; @@ -187,13 +189,13 @@ export default class RoomSublist2 extends React.Component { private onShowAllClick = () => { const numVisibleTiles = this.numVisibleTiles; - this.props.layout.visibleTiles = this.props.layout.tilesWithPadding(this.numTiles, MAX_PADDING_HEIGHT); + this.layout.visibleTiles = this.layout.tilesWithPadding(this.numTiles, MAX_PADDING_HEIGHT); this.forceUpdate(); // because the layout doesn't trigger a re-render setImmediate(this.focusRoomTile, numVisibleTiles); // focus the tile after the current bottom one }; private onShowLessClick = () => { - this.props.layout.visibleTiles = this.props.layout.defaultVisibleTiles; + this.layout.visibleTiles = this.layout.defaultVisibleTiles; this.forceUpdate(); // because the layout doesn't trigger a re-render // focus will flow to the show more button here }; @@ -241,7 +243,7 @@ export default class RoomSublist2 extends React.Component { }; private onMessagePreviewChanged = () => { - this.props.layout.showPreviews = !this.props.layout.showPreviews; + this.layout.showPreviews = !this.layout.showPreviews; this.forceUpdate(); // because the layout doesn't trigger a re-render }; @@ -293,13 +295,13 @@ export default class RoomSublist2 extends React.Component { }; private toggleCollapsed = () => { - this.props.layout.isCollapsed = !this.props.layout.isCollapsed; + this.layout.isCollapsed = !this.layout.isCollapsed; this.forceUpdate(); // because the layout doesn't trigger an update setImmediate(() => this.props.onResize()); // needs to happen when the DOM is updated }; private onHeaderKeyDown = (ev: React.KeyboardEvent) => { - const isCollapsed = this.props.layout && this.props.layout.isCollapsed; + const isCollapsed = this.layout && this.layout.isCollapsed; switch (ev.key) { case Key.ARROW_LEFT: ev.stopPropagation(); @@ -339,7 +341,7 @@ export default class RoomSublist2 extends React.Component { }; private renderVisibleTiles(): React.ReactElement[] { - if (this.props.layout && this.props.layout.isCollapsed) { + if (this.layout && this.layout.isCollapsed) { // don't waste time on rendering return []; } @@ -353,7 +355,7 @@ export default class RoomSublist2 extends React.Component { @@ -404,7 +406,7 @@ export default class RoomSublist2 extends React.Component { {_t("Message preview")} @@ -496,7 +498,7 @@ export default class RoomSublist2 extends React.Component { const collapseClasses = classNames({ 'mx_RoomSublist2_collapseBtn': true, - 'mx_RoomSublist2_collapseBtn_collapsed': this.props.layout && this.props.layout.isCollapsed, + 'mx_RoomSublist2_collapseBtn_collapsed': this.layout && this.layout.isCollapsed, }); const classes = classNames({ @@ -524,7 +526,7 @@ export default class RoomSublist2 extends React.Component { tabIndex={tabIndex} className="mx_RoomSublist2_headerText" role="treeitem" - aria-expanded={!this.props.layout || !this.props.layout.isCollapsed} + aria-expanded={!this.layout.isCollapsed} aria-level={1} onClick={this.onHeaderClick} onContextMenu={this.onContextMenu} @@ -558,7 +560,7 @@ export default class RoomSublist2 extends React.Component { let content = null; if (visibleTiles.length > 0) { - const layout = this.props.layout; // to shorten calls + const layout = this.layout; // to shorten calls const maxTilesFactored = layout.tilesWithResizerBoxFactor(this.numTiles); const showMoreBtnClasses = classNames({ @@ -587,7 +589,7 @@ export default class RoomSublist2 extends React.Component { {showMoreText} ); - } else if (this.numTiles <= visibleTiles.length && this.numTiles > this.props.layout.defaultVisibleTiles) { + } else if (this.numTiles <= visibleTiles.length && this.numTiles > this.layout.defaultVisibleTiles) { // we have all tiles visible - add a button to show less let showLessText = ( diff --git a/src/stores/room-list/RoomListLayoutStore.ts b/src/stores/room-list/RoomListLayoutStore.ts new file mode 100644 index 0000000000..dd4364f5fc --- /dev/null +++ b/src/stores/room-list/RoomListLayoutStore.ts @@ -0,0 +1,54 @@ +/* +Copyright 2020 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import { TagID } from "./models"; +import { ListLayout } from "./ListLayout"; + +export default class RoomListLayoutStore { + private static internalInstance: RoomListLayoutStore; + + private readonly layoutMap = new Map(); + + public ensureLayoutExists(tagId: TagID) { + if (!this.layoutMap.has(tagId)) { + this.layoutMap.set(tagId, new ListLayout(tagId)); + } + } + + public getLayoutFor(tagId: TagID): ListLayout { + if (!this.layoutMap.has(tagId)) { + this.layoutMap.set(tagId, new ListLayout(tagId)); + } + return this.layoutMap.get(tagId); + } + + // Note: this primarily exists for debugging, and isn't really intended to be used by anything. + public async resetLayouts() { + console.warn("Resetting layouts for room list"); + for (const layout of this.layoutMap.values()) { + layout.reset(); + } + } + + public static get instance(): RoomListLayoutStore { + if (!RoomListLayoutStore.internalInstance) { + RoomListLayoutStore.internalInstance = new RoomListLayoutStore(); + } + return RoomListLayoutStore.internalInstance; + } +} + +window.mx_RoomListLayoutStore = RoomListLayoutStore.instance; diff --git a/src/stores/room-list/RoomListStore2.ts b/src/stores/room-list/RoomListStore2.ts index 82c419d79d..6020e46a12 100644 --- a/src/stores/room-list/RoomListStore2.ts +++ b/src/stores/room-list/RoomListStore2.ts @@ -32,6 +32,7 @@ import { Algorithm, LIST_UPDATED_EVENT } from "./algorithms/Algorithm"; import { EffectiveMembership, getEffectiveMembership } from "./membership"; import { ListLayout } from "./ListLayout"; import { isNullOrUndefined } from "matrix-js-sdk/src/utils"; +import RoomListLayoutStore from "./RoomListLayoutStore"; interface IState { tagsEnabled?: boolean; @@ -50,6 +51,7 @@ export class RoomListStore2 extends AsyncStore { private algorithm = new Algorithm(); private filterConditions: IFilterCondition[] = []; private tagWatcher = new TagWatcher(this); + private layoutMap: Map = new Map(); private readonly watchedSettings = [ 'feature_custom_tags', @@ -416,6 +418,8 @@ export class RoomListStore2 extends AsyncStore { for (const tagId of OrderedDefaultTagIDs) { sorts[tagId] = this.calculateTagSorting(tagId); orders[tagId] = this.calculateListOrder(tagId); + + RoomListLayoutStore.instance.ensureLayoutExists(tagId); } if (this.state.tagsEnabled) { @@ -434,15 +438,6 @@ export class RoomListStore2 extends AsyncStore { this.emit(LISTS_UPDATE_EVENT, this); } - // Note: this primarily exists for debugging, and isn't really intended to be used by anything. - public async resetLayouts() { - console.warn("Resetting layouts for room list"); - for (const tagId of Object.keys(this.orderedLists)) { - new ListLayout(tagId).reset(); - } - await this.regenerateAllLists(); - } - public addFilter(filter: IFilterCondition): void { // TODO: Remove debug: https://github.com/vector-im/riot-web/issues/14035 console.log("Adding filter condition:", filter); From 2baa78d26baf80c0208c1ec50022d023ad8c4141 Mon Sep 17 00:00:00 2001 From: Jorik Schellekens Date: Thu, 9 Jul 2020 01:31:44 +0100 Subject: [PATCH 3/5] Move no-op to breadcrumb store --- .../views/rooms/RoomBreadcrumbs2.tsx | 8 +---- src/stores/BreadcrumbsStore.ts | 35 +++++++++++++------ 2 files changed, 25 insertions(+), 18 deletions(-) diff --git a/src/components/views/rooms/RoomBreadcrumbs2.tsx b/src/components/views/rooms/RoomBreadcrumbs2.tsx index f08adebcd2..602c697d8a 100644 --- a/src/components/views/rooms/RoomBreadcrumbs2.tsx +++ b/src/components/views/rooms/RoomBreadcrumbs2.tsx @@ -87,19 +87,13 @@ export default class RoomBreadcrumbs2 extends React.PureComponent { Analytics.trackEvent("Breadcrumbs", "click_node", index); - // If we're rendering the first breadcrumb and this is it no-op - if (!this.state.skipFirst && index === 0) { - return; - } else { - defaultDispatcher.dispatch({action: "view_room", room_id: room.roomId}); - } + defaultDispatcher.dispatch({action: "view_room", room_id: room.roomId}); }; public render(): React.ReactElement { const tiles = BreadcrumbsStore.instance.rooms.map((r, i) => { const roomTags = RoomListStore.instance.getTagsForRoom(r); const roomTag = roomTags.includes(DefaultTagID.DM) ? DefaultTagID.DM : roomTags[0]; - const anon = () => this.viewRoom(r, i); return ( { } private async appendRoom(room: Room) { + let updated = false; const rooms = (this.state.rooms || []).slice(); // cheap clone // If the room is upgraded, use that room instead. We'll also splice out @@ -136,30 +137,42 @@ export class BreadcrumbsStore extends AsyncStoreWithClient { // Take out any room that isn't the most recent room for (let i = 0; i < history.length - 1; i++) { const idx = rooms.findIndex(r => r.roomId === history[i].roomId); - if (idx !== -1) rooms.splice(idx, 1); + if (idx !== -1) { + rooms.splice(idx, 1); + updated = true; + } } } // Remove the existing room, if it is present const existingIdx = rooms.findIndex(r => r.roomId === room.roomId); - if (existingIdx !== -1) { - rooms.splice(existingIdx, 1); - } - // Splice the room to the start of the list - rooms.splice(0, 0, room); + // If we're focusing on the first room no-op + if (existingIdx !== 0) { + if (existingIdx !== -1) { + rooms.splice(existingIdx, 1); + } + + // Splice the room to the start of the list + rooms.splice(0, 0, room); + updated = true; + } if (rooms.length > MAX_ROOMS) { // This looks weird, but it's saying to start at the MAX_ROOMS point in the // list and delete everything after it. rooms.splice(MAX_ROOMS, rooms.length - MAX_ROOMS); + updated = true; } - // Update the breadcrumbs - await this.updateState({rooms}); - const roomIds = rooms.map(r => r.roomId); - if (roomIds.length > 0) { - await SettingsStore.setValue("breadcrumb_rooms", null, SettingLevel.ACCOUNT, roomIds); + + if (updated) { + // Update the breadcrumbs + await this.updateState({rooms}); + const roomIds = rooms.map(r => r.roomId); + if (roomIds.length > 0) { + await SettingsStore.setValue("breadcrumb_rooms", null, SettingLevel.ACCOUNT, roomIds); + } } } From c8f90be81d5a1d5df687066192b2af649bef77d5 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 8 Jul 2020 18:32:12 -0600 Subject: [PATCH 4/5] Ensure the map gets cleared upon logout --- src/stores/room-list/RoomListLayoutStore.ts | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/src/stores/room-list/RoomListLayoutStore.ts b/src/stores/room-list/RoomListLayoutStore.ts index dd4364f5fc..4d8c98a4f8 100644 --- a/src/stores/room-list/RoomListLayoutStore.ts +++ b/src/stores/room-list/RoomListLayoutStore.ts @@ -16,12 +16,21 @@ limitations under the License. import { TagID } from "./models"; import { ListLayout } from "./ListLayout"; +import { AsyncStoreWithClient } from "../AsyncStoreWithClient"; +import defaultDispatcher from "../../dispatcher/dispatcher"; +import { ActionPayload } from "../../dispatcher/payloads"; -export default class RoomListLayoutStore { +interface IState {} + +export default class RoomListLayoutStore extends AsyncStoreWithClient { private static internalInstance: RoomListLayoutStore; private readonly layoutMap = new Map(); + constructor() { + super(defaultDispatcher); + } + public ensureLayoutExists(tagId: TagID) { if (!this.layoutMap.has(tagId)) { this.layoutMap.set(tagId, new ListLayout(tagId)); @@ -49,6 +58,16 @@ export default class RoomListLayoutStore { } return RoomListLayoutStore.internalInstance; } + + protected async onNotReady(): Promise { + // On logout, clear the map. + this.layoutMap.clear(); + } + + // We don't need this function, but our contract says we do + protected async onAction(payload: ActionPayload): Promise { + return Promise.resolve(); + } } window.mx_RoomListLayoutStore = RoomListLayoutStore.instance; From 62b4596c049225ab767f7ef5a91c71cc2749d9fa Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 8 Jul 2020 18:36:56 -0600 Subject: [PATCH 5/5] Be consistent with other stores --- src/stores/room-list/RoomListLayoutStore.ts | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/stores/room-list/RoomListLayoutStore.ts b/src/stores/room-list/RoomListLayoutStore.ts index 4d8c98a4f8..fbc7d7719d 100644 --- a/src/stores/room-list/RoomListLayoutStore.ts +++ b/src/stores/room-list/RoomListLayoutStore.ts @@ -31,6 +31,13 @@ export default class RoomListLayoutStore extends AsyncStoreWithClient { super(defaultDispatcher); } + public static get instance(): RoomListLayoutStore { + if (!RoomListLayoutStore.internalInstance) { + RoomListLayoutStore.internalInstance = new RoomListLayoutStore(); + } + return RoomListLayoutStore.internalInstance; + } + public ensureLayoutExists(tagId: TagID) { if (!this.layoutMap.has(tagId)) { this.layoutMap.set(tagId, new ListLayout(tagId)); @@ -52,13 +59,6 @@ export default class RoomListLayoutStore extends AsyncStoreWithClient { } } - public static get instance(): RoomListLayoutStore { - if (!RoomListLayoutStore.internalInstance) { - RoomListLayoutStore.internalInstance = new RoomListLayoutStore(); - } - return RoomListLayoutStore.internalInstance; - } - protected async onNotReady(): Promise { // On logout, clear the map. this.layoutMap.clear();