Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Improve RoomList render time when filtering #5874

Merged
merged 9 commits into from
Apr 21, 2021
Merged

Improve RoomList render time when filtering #5874

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a59873d
Set rooms event listeners during the correct life cycle hook
germain-gg Apr 15, 2021
02debd1
Hide RoomSublist when empty rather than unmounting them
germain-gg Apr 15, 2021
b9f80b4
Set correct initial state for notification on room
germain-gg Apr 15, 2021
f1453e8
extract always visible logic to RoomList
germain-gg Apr 16, 2021
3279836
refactor renderSublists for better readability
germain-gg Apr 16, 2021
daaaa3d
add context for roomsublist rendering
germain-gg Apr 16, 2021
c1c3ac2
correct always visible logic
germain-gg Apr 16, 2021
db646d5
Fix end to end tests for DM creation
germain-gg Apr 19, 2021
de5ca92
add e2e session.delay explainer
germain-gg Apr 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions res/css/views/rooms/_RoomSublist.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ limitations under the License.
margin-left: 8px;
margin-bottom: 4px;

&.mx_RoomSublist_hidden {
display: none;
}

.mx_RoomSublist_headerContainer {
// Create a flexbox to make alignment easy
display: flex;
Expand Down
90 changes: 40 additions & 50 deletions src/components/views/rooms/RoomList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,12 +289,11 @@ export default class RoomList extends React.PureComponent<IProps, IState> {
// shallow-copy from the template as we need to make modifications to it
this.tagAesthetics = objectShallowClone(TAG_AESTHETICS);
this.updateDmAddRoomAction();

this.dispatcherRef = defaultDispatcher.register(this.onAction);
this.roomStoreToken = RoomViewStore.addListener(this.onRoomViewStoreUpdate);
}

public componentDidMount(): void {
this.dispatcherRef = defaultDispatcher.register(this.onAction);
this.roomStoreToken = RoomViewStore.addListener(this.onRoomViewStoreUpdate);
Comment on lines +295 to +296
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it actually more performant to move things to componentDidMount? Over the last couple years we've started using the constructor more and more, slowly getting rid of componentDidMount - putting aside arguments about whether that is correct behaviour, I'm curious if there's a performance impact.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends, generally speaking there can be small performance improvements when the component is rendered but not mounted

But in my opinion the real power of this is that you colocate all your event listeners, if you have some event listeners that you need to place on DOM elements, you will have to perform that in componentDidMount. And on top of that if it is immediatly followed by componentWillUnmount you're one step closer to remembering to clean up your listeners

It also helps in tests where you shallow render your components

Overall moving away from lifecycles makes sense, this is pretty much what the React team has been advocating for, but their solution seems to be using hooks instead

SpaceStore.instance.on(SUGGESTED_ROOMS, this.updateSuggestedRooms);
RoomListStore.instance.on(LISTS_UPDATE_EVENT, this.updateLists);
this.customTagStoreRef = CustomRoomTagStore.addListener(this.updateLists);
Expand Down Expand Up @@ -502,59 +501,50 @@ export default class RoomList extends React.PureComponent<IProps, IState> {
}

private renderSublists(): React.ReactElement[] {
const components: React.ReactElement[] = [];

const tagOrder = TAG_ORDER.reduce((p, c) => {
if (c === CUSTOM_TAGS_BEFORE_TAG) {
const customTags = Object.keys(this.state.sublists)
.filter(t => isCustomTag(t));
p.push(...customTags);
}
p.push(c);
return p;
}, [] as TagID[]);

// show a skeleton UI if the user is in no rooms and they are not filtering
const showSkeleton = !this.state.isNameFiltering &&
Object.values(RoomListStore.instance.unfilteredLists).every(list => !list?.length);

for (const orderedTagId of tagOrder) {
const orderedRooms = this.state.sublists[orderedTagId] || [];

let extraTiles = null;
if (orderedTagId === DefaultTagID.Invite) {
extraTiles = this.renderCommunityInvites();
} else if (orderedTagId === DefaultTagID.Suggested) {
extraTiles = this.renderSuggestedRooms();
}

const totalTiles = orderedRooms.length + (extraTiles ? extraTiles.length : 0);
if (totalTiles === 0 && !ALWAYS_VISIBLE_TAGS.includes(orderedTagId)) {
continue; // skip tag - not needed
return TAG_ORDER.reduce((tags, tagId) => {
if (tagId === CUSTOM_TAGS_BEFORE_TAG) {
const customTags = Object.keys(this.state.sublists)
.filter(tagId => isCustomTag(tagId));
tags.push(...customTags);
}
tags.push(tagId);
return tags;
}, [] as TagID[])
.map(orderedTagId => {
let extraTiles = null;
if (orderedTagId === DefaultTagID.Invite) {
extraTiles = this.renderCommunityInvites();
} else if (orderedTagId === DefaultTagID.Suggested) {
extraTiles = this.renderSuggestedRooms();
}

const aesthetics: ITagAesthetics = isCustomTag(orderedTagId)
? customTagAesthetics(orderedTagId)
: this.tagAesthetics[orderedTagId];
if (!aesthetics) throw new Error(`Tag ${orderedTagId} does not have aesthetics`);

components.push(<RoomSublist
key={`sublist-${orderedTagId}`}
tagId={orderedTagId}
forRooms={true}
startAsHidden={aesthetics.defaultHidden}
label={aesthetics.sectionLabelRaw ? aesthetics.sectionLabelRaw : _t(aesthetics.sectionLabel)}
onAddRoom={aesthetics.onAddRoom}
addRoomLabel={aesthetics.addRoomLabel ? _t(aesthetics.addRoomLabel) : aesthetics.addRoomLabel}
addRoomContextMenu={aesthetics.addRoomContextMenu}
isMinimized={this.props.isMinimized}
onResize={this.props.onResize}
showSkeleton={showSkeleton}
extraTiles={extraTiles}
/>);
}

return components;
const aesthetics: ITagAesthetics = isCustomTag(orderedTagId)
? customTagAesthetics(orderedTagId)
: this.tagAesthetics[orderedTagId];
if (!aesthetics) throw new Error(`Tag ${orderedTagId} does not have aesthetics`);

// The cost of mounting/unmounting this component offsets the cost
// of keeping it in the DOM and hiding it when it is not required
return <RoomSublist
key={`sublist-${orderedTagId}`}
tagId={orderedTagId}
forRooms={true}
startAsHidden={aesthetics.defaultHidden}
label={aesthetics.sectionLabelRaw ? aesthetics.sectionLabelRaw : _t(aesthetics.sectionLabel)}
onAddRoom={aesthetics.onAddRoom}
addRoomLabel={aesthetics.addRoomLabel ? _t(aesthetics.addRoomLabel) : aesthetics.addRoomLabel}
addRoomContextMenu={aesthetics.addRoomContextMenu}
isMinimized={this.props.isMinimized}
onResize={this.props.onResize}
showSkeleton={showSkeleton}
extraTiles={extraTiles}
alwaysVisible={ALWAYS_VISIBLE_TAGS.includes(orderedTagId)}
/>
});
}

public render() {
Expand Down
9 changes: 7 additions & 2 deletions src/components/views/rooms/RoomSublist.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ interface IProps {
tagId: TagID;
onResize: () => void;
showSkeleton?: boolean;
alwaysVisible?: boolean;

extraTiles?: ReactComponentElement<typeof ExtraTile>[];

Expand Down Expand Up @@ -125,8 +126,6 @@ export default class RoomSublist extends React.Component<IProps, IState> {
};
// Why Object.assign() and not this.state.height? Because TypeScript says no.
this.state = Object.assign(this.state, {height: this.calculateInitialHeight()});
this.dispatcherRef = defaultDispatcher.register(this.onAction);
RoomListStore.instance.on(LISTS_UPDATE_EVENT, this.onListsUpdated);
}

private calculateInitialHeight() {
Expand Down Expand Up @@ -242,6 +241,11 @@ export default class RoomSublist extends React.Component<IProps, IState> {
return false;
}

public componentDidMount() {
this.dispatcherRef = defaultDispatcher.register(this.onAction);
RoomListStore.instance.on(LISTS_UPDATE_EVENT, this.onListsUpdated);
}

public componentWillUnmount() {
defaultDispatcher.unregister(this.dispatcherRef);
RoomListStore.instance.off(LISTS_UPDATE_EVENT, this.onListsUpdated);
Expand Down Expand Up @@ -759,6 +763,7 @@ export default class RoomSublist extends React.Component<IProps, IState> {
'mx_RoomSublist': true,
'mx_RoomSublist_hasMenuOpen': !!this.state.contextMenuPosition,
'mx_RoomSublist_minimized': this.props.isMinimized,
'mx_RoomSublist_hidden': !this.state.rooms.length && this.props.alwaysVisible !== true,
});

let content = null;
Expand Down
37 changes: 22 additions & 15 deletions src/components/views/rooms/RoomTile.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,22 +97,8 @@ export default class RoomTile extends React.PureComponent<IProps, IState> {
// generatePreview() will return nothing if the user has previews disabled
messagePreview: this.generatePreview(),
};

ActiveRoomObserver.addListener(this.props.room.roomId, this.onActiveRoomUpdate);
this.dispatcherRef = defaultDispatcher.register(this.onAction);
MessagePreviewStore.instance.on(
MessagePreviewStore.getPreviewChangedEventName(this.props.room),
this.onRoomPreviewChanged,
);
this.notificationState = RoomNotificationStateStore.instance.getRoomState(this.props.room);
this.notificationState.on(NOTIFICATION_STATE_UPDATE, this.onNotificationUpdate);
this.roomProps = EchoChamber.forRoom(this.props.room);
this.roomProps.on(PROPERTY_UPDATED, this.onRoomPropertyUpdate);
CommunityPrototypeStore.instance.on(
CommunityPrototypeStore.getUpdateEventName(this.props.room.roomId),
this.onCommunityUpdate,
);
this.props.room.on("Room.name", this.onRoomNameUpdate);
}

private onRoomNameUpdate = (room) => {
Expand Down Expand Up @@ -167,6 +153,20 @@ export default class RoomTile extends React.PureComponent<IProps, IState> {
if (this.state.selected) {
this.scrollIntoView();
}

ActiveRoomObserver.addListener(this.props.room.roomId, this.onActiveRoomUpdate);
this.dispatcherRef = defaultDispatcher.register(this.onAction);
MessagePreviewStore.instance.on(
MessagePreviewStore.getPreviewChangedEventName(this.props.room),
this.onRoomPreviewChanged,
);
this.notificationState.on(NOTIFICATION_STATE_UPDATE, this.onNotificationUpdate);
this.roomProps.on(PROPERTY_UPDATED, this.onRoomPropertyUpdate);
this.roomProps.on("Room.name", this.onRoomNameUpdate);
CommunityPrototypeStore.instance.on(
CommunityPrototypeStore.getUpdateEventName(this.props.room.roomId),
this.onCommunityUpdate,
);
}

public componentWillUnmount() {
Expand All @@ -182,8 +182,15 @@ export default class RoomTile extends React.PureComponent<IProps, IState> {
);
this.props.room.off("Room.name", this.onRoomNameUpdate);
}
ActiveRoomObserver.removeListener(this.props.room.roomId, this.onActiveRoomUpdate);
defaultDispatcher.unregister(this.dispatcherRef);
this.notificationState.off(NOTIFICATION_STATE_UPDATE, this.onNotificationUpdate);
this.roomProps.off(PROPERTY_UPDATED, this.onRoomPropertyUpdate);
this.roomProps.off("Room.name", this.onRoomNameUpdate);
CommunityPrototypeStore.instance.off(
CommunityPrototypeStore.getUpdateEventName(this.props.room.roomId),
this.onCommunityUpdate,
);
}

private onAction = (payload: ActionPayload) => {
Expand Down Expand Up @@ -547,7 +554,7 @@ export default class RoomTile extends React.PureComponent<IProps, IState> {
/>;

let badge: React.ReactNode;
if (!this.props.isMinimized) {
if (!this.props.isMinimized && this.notificationState) {
// aria-hidden because we summarise the unread count/highlight status in a manual aria-label below
badge = (
<div className="mx_RoomTile_badgeContainer" aria-hidden="true">
Expand Down
10 changes: 1 addition & 9 deletions test/end-to-end-tests/src/usecases/create-room.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,7 @@ async function openRoomDirectory(session) {
}

async function findSublist(session, name) {
const sublists = await session.queryAll('.mx_RoomSublist');
for (const sublist of sublists) {
const header = await sublist.$('.mx_RoomSublist_headerText');
const headerText = await session.innerText(header);
if (headerText.toLowerCase().includes(name.toLowerCase())) {
return sublist;
}
}
throw new Error(`could not find room list section that contains '${name}' in header`);
return await session.query(`.mx_RoomSublist[aria-label="${name}" i]`);
}

async function createRoom(session, roomName, encrypted=false) {
Expand Down
3 changes: 3 additions & 0 deletions test/end-to-end-tests/src/usecases/signup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ module.exports = async function signup(session, username, password, homeserver)
// accept homeserver
await nextButton.click();
}
// Delay required because of local race condition on macOs
// Where the form is not query-able despite being present in the DOM
await session.delay(100);
//fill out form
const usernameField = await session.query("#mx_RegistrationForm_username");
const passwordField = await session.query("#mx_RegistrationForm_password");
Expand Down
Loading