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

Commit

Permalink
Merge pull request #5054 from matrix-org/travis/perf6
Browse files Browse the repository at this point in the history 
Minor improvements to filtering performance
  • Loading branch information
@turt2live
turt2live authored Jul 28, 2020
2 parents 0ee30a0 + 1573c88 commit 3561de3
Show file tree
Hide file tree
Showing 12 changed files with 150 additions and 81 deletions.
8 changes: 0 additions & 8 deletions src/components/structures/LeftPanel.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ interface IProps {
}

interface IState {
searchFilter: string;
showBreadcrumbs: boolean;
showTagPanel: boolean;
}
Expand All @@ -69,7 +68,6 @@ export default class LeftPanel extends React.Component<IProps, IState> {
super(props);

this.state = {
searchFilter: "",
showBreadcrumbs: BreadcrumbsStore.instance.visible,
showTagPanel: SettingsStore.getValue('TagPanel.enableTagPanel'),
};
Expand Down Expand Up @@ -97,10 +95,6 @@ export default class LeftPanel extends React.Component<IProps, IState> {
this.props.resizeNotifier.off("middlePanelResizedNoisy", this.onResize);
}

private onSearch = (term: string): void => {
this.setState({searchFilter: term});
};

private onExplore = () => {
dis.fire(Action.ViewRoomDirectory);
};
Expand Down Expand Up @@ -366,7 +360,6 @@ export default class LeftPanel extends React.Component<IProps, IState> {
onKeyDown={this.onKeyDown}
>
<RoomSearch
onQueryUpdate={this.onSearch}
isMinimized={this.props.isMinimized}
onVerticalArrow={this.onKeyDown}
onEnter={this.onEnter}
Expand All @@ -392,7 +385,6 @@ export default class LeftPanel extends React.Component<IProps, IState> {
onKeyDown={this.onKeyDown}
resizeNotifier={null}
collapsed={false}
searchFilter={this.state.searchFilter}
onFocus={this.onFocus}
onBlur={this.onBlur}
isMinimized={this.props.isMinimized}
Expand Down
30 changes: 18 additions & 12 deletions src/components/structures/RoomSearch.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,10 @@ import { throttle } from 'lodash';
import { Key } from "../../Keyboard";
import AccessibleButton from "../views/elements/AccessibleButton";
import { Action } from "../../dispatcher/actions";
import RoomListStore from "../../stores/room-list/RoomListStore";
import { NameFilterCondition } from "../../stores/room-list/filters/NameFilterCondition";

interface IProps {
onQueryUpdate: (newQuery: string) => void;
isMinimized: boolean;
onVerticalArrow(ev: React.KeyboardEvent): void;
onEnter(ev: React.KeyboardEvent): boolean;
Expand All @@ -40,6 +41,7 @@ interface IState {
export default class RoomSearch extends React.PureComponent<IProps, IState> {
private dispatcherRef: string;
private inputRef: React.RefObject<HTMLInputElement> = createRef();
private searchFilter: NameFilterCondition = new NameFilterCondition();

constructor(props: IProps) {
super(props);
Expand All @@ -52,6 +54,21 @@ export default class RoomSearch extends React.PureComponent<IProps, IState> {
this.dispatcherRef = defaultDispatcher.register(this.onAction);
}

public componentDidUpdate(prevProps: Readonly<IProps>, prevState: Readonly<IState>): void {
if (prevState.query !== this.state.query) {
const hadSearch = !!this.searchFilter.search.trim();
const haveSearch = !!this.state.query.trim();
this.searchFilter.search = this.state.query;
if (!hadSearch && haveSearch) {
// started a new filter - add the condition
RoomListStore.instance.addFilter(this.searchFilter);
} else if (hadSearch && !haveSearch) {
// cleared a filter - remove the condition
RoomListStore.instance.removeFilter(this.searchFilter);
} // else the filter hasn't changed enough for us to care here
}
}

public componentWillUnmount() {
defaultDispatcher.unregister(this.dispatcherRef);
}
Expand All @@ -78,19 +95,8 @@ export default class RoomSearch extends React.PureComponent<IProps, IState> {
private onChange = () => {
if (!this.inputRef.current) return;
this.setState({query: this.inputRef.current.value});
this.onSearchUpdated();
};

// it wants this at the top of the file, but we know better
// tslint:disable-next-line
private onSearchUpdated = throttle(
() => {
// We can't use the state variable because it can lag behind the input.
// The lag is most obvious when deleting/clearing text with the keyboard.
this.props.onQueryUpdate(this.inputRef.current.value);
}, 200, {trailing: true, leading: true},
);

private onFocus = (ev: React.FocusEvent<HTMLInputElement>) => {
this.setState({focused: true});
ev.target.select();
Expand Down
26 changes: 3 additions & 23 deletions src/components/views/rooms/RoomList.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ import dis from "../../../dispatcher/dispatcher";
import defaultDispatcher from "../../../dispatcher/dispatcher";
import RoomSublist from "./RoomSublist";
import { ActionPayload } from "../../../dispatcher/payloads";
import { NameFilterCondition } from "../../../stores/room-list/filters/NameFilterCondition";
import { MatrixClientPeg } from "../../../MatrixClientPeg";
import GroupAvatar from "../avatars/GroupAvatar";
import TemporaryTile from "./TemporaryTile";
Expand All @@ -52,7 +51,6 @@ interface IProps {
onResize: () => void;
resizeNotifier: ResizeNotifier;
collapsed: boolean;
searchFilter: string;
isMinimized: boolean;
}

Expand Down Expand Up @@ -150,8 +148,7 @@ function customTagAesthetics(tagId: TagID): ITagAesthetics {
};
}

export default class RoomList extends React.Component<IProps, IState> {
private searchFilter: NameFilterCondition = new NameFilterCondition();
export default class RoomList extends React.PureComponent<IProps, IState> {
private dispatcherRef;
private customTagStoreRef;

Expand All @@ -165,21 +162,6 @@ export default class RoomList extends React.Component<IProps, IState> {
this.dispatcherRef = defaultDispatcher.register(this.onAction);
}

public componentDidUpdate(prevProps: Readonly<IProps>): void {
if (prevProps.searchFilter !== this.props.searchFilter) {
const hadSearch = !!this.searchFilter.search.trim();
const haveSearch = !!this.props.searchFilter.trim();
this.searchFilter.search = this.props.searchFilter;
if (!hadSearch && haveSearch) {
// started a new filter - add the condition
RoomListStore.instance.addFilter(this.searchFilter);
} else if (hadSearch && !haveSearch) {
// cleared a filter - remove the condition
RoomListStore.instance.removeFilter(this.searchFilter);
} // else the filter hasn't changed enough for us to care here
}
}

public componentDidMount(): void {
RoomListStore.instance.on(LISTS_UPDATE_EVENT, this.updateLists);
this.customTagStoreRef = CustomRoomTagStore.addListener(this.updateLists);
Expand Down Expand Up @@ -266,12 +248,11 @@ export default class RoomList extends React.Component<IProps, IState> {
}
};

private renderCommunityInvites(): React.ReactElement[] {
private renderCommunityInvites(): TemporaryTile[] {
// TODO: Put community invites in a more sensible place (not in the room list)
// See https://github.com/vector-im/riot-web/issues/14456
return MatrixClientPeg.get().getGroups().filter(g => {
if (g.myMembership !== 'invite') return false;
return !this.searchFilter || this.searchFilter.matches(g.name || "");
return g.myMembership === 'invite';
}).map(g => {
const avatar = (
<GroupAvatar
Expand Down Expand Up @@ -340,7 +321,6 @@ export default class RoomList extends React.Component<IProps, IState> {
isMinimized={this.props.isMinimized}
onResize={this.props.onResize}
extraBadTilesThatShouldntExist={extraTiles}
isFiltered={!!this.searchFilter.search}
/>
);
}
Expand Down
83 changes: 57 additions & 26 deletions src/components/views/rooms/RoomSublist.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ import { DefaultTagID, TagID } from "../../../stores/room-list/models";
import dis from "../../../dispatcher/dispatcher";
import defaultDispatcher from "../../../dispatcher/dispatcher";
import NotificationBadge from "./NotificationBadge";
import { ListNotificationState } from "../../../stores/notifications/ListNotificationState";
import AccessibleTooltipButton from "../elements/AccessibleTooltipButton";
import { Key } from "../../../Keyboard";
import { ActionPayload } from "../../../dispatcher/payloads";
Expand All @@ -49,6 +48,8 @@ import { RoomNotificationStateStore } from "../../../stores/notifications/RoomNo
import RoomListLayoutStore from "../../../stores/room-list/RoomListLayoutStore";
import { arrayHasOrderChange } from "../../../utils/arrays";
import { objectExcluding, objectHasValueChange } from "../../../utils/objects";
import TemporaryTile from "./TemporaryTile";
import { ListNotificationState } from "../../../stores/notifications/ListNotificationState";

const SHOW_N_BUTTON_HEIGHT = 28; // As defined by CSS
const RESIZE_HANDLE_HEIGHT = 4; // As defined by CSS
Expand All @@ -68,11 +69,10 @@ interface IProps {
isMinimized: boolean;
tagId: TagID;
onResize: () => void;
isFiltered: boolean;

// TODO: Don't use this. It's for community invites, and community invites shouldn't be here.
// You should feel bad if you use this.
extraBadTilesThatShouldntExist?: React.ReactElement[];
extraBadTilesThatShouldntExist?: TemporaryTile[];

// TODO: Account for https://github.com/vector-im/riot-web/issues/14179
}
Expand All @@ -86,12 +86,12 @@ interface ResizeDelta {
type PartialDOMRect = Pick<DOMRect, "left" | "top" | "height">;

interface IState {
notificationState: ListNotificationState;
contextMenuPosition: PartialDOMRect;
isResizing: boolean;
isExpanded: boolean; // used for the for expand of the sublist when the room list is being filtered
height: number;
rooms: Room[];
filteredExtraTiles?: TemporaryTile[];
}

export default class RoomSublist extends React.Component<IProps, IState> {
Expand All @@ -100,23 +100,25 @@ export default class RoomSublist extends React.Component<IProps, IState> {
private dispatcherRef: string;
private layout: ListLayout;
private heightAtStart: number;
private isBeingFiltered: boolean;
private notificationState: ListNotificationState;

constructor(props: IProps) {
super(props);

this.layout = RoomListLayoutStore.instance.getLayoutFor(this.props.tagId);
this.heightAtStart = 0;
this.isBeingFiltered = !!RoomListStore.instance.getFirstNameFilterCondition();
this.notificationState = RoomNotificationStateStore.instance.getListState(this.props.tagId);
this.state = {
notificationState: RoomNotificationStateStore.instance.getListState(this.props.tagId),
contextMenuPosition: null,
isResizing: false,
isExpanded: this.props.isFiltered ? this.props.isFiltered : !this.layout.isCollapsed,
isExpanded: this.isBeingFiltered ? this.isBeingFiltered : !this.layout.isCollapsed,
height: 0, // to be fixed in a moment, we need `rooms` to calculate this.
rooms: RoomListStore.instance.orderedLists[this.props.tagId] || [],
};
// 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);
}
Expand Down Expand Up @@ -146,8 +148,18 @@ export default class RoomSublist extends React.Component<IProps, IState> {
return padding;
}

private get extraTiles(): TemporaryTile[] | null {
if (this.state.filteredExtraTiles) {
return this.state.filteredExtraTiles;
}
if (this.props.extraBadTilesThatShouldntExist) {
return this.props.extraBadTilesThatShouldntExist;
}
return null;
}

private get numTiles(): number {
return RoomSublist.calcNumTiles(this.state.rooms, this.props.extraBadTilesThatShouldntExist);
return RoomSublist.calcNumTiles(this.state.rooms, this.extraTiles);
}

private static calcNumTiles(rooms: Room[], extraTiles: any[]) {
Expand All @@ -160,17 +172,10 @@ export default class RoomSublist extends React.Component<IProps, IState> {
}

public componentDidUpdate(prevProps: Readonly<IProps>, prevState: Readonly<IState>) {
this.state.notificationState.setRooms(this.state.rooms);
if (prevProps.isFiltered !== this.props.isFiltered) {
if (this.props.isFiltered) {
this.setState({isExpanded: true});
} else {
this.setState({isExpanded: !this.layout.isCollapsed});
}
}
const prevExtraTiles = prevState.filteredExtraTiles || prevProps.extraBadTilesThatShouldntExist;
// 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(prevState.rooms, prevProps.extraBadTilesThatShouldntExist) !== this.numTiles) {
if (RoomSublist.calcNumTiles(prevState.rooms, prevExtraTiles) !== this.numTiles) {
this.setState({height: this.calculateInitialHeight()});
}
}
Expand All @@ -191,14 +196,14 @@ export default class RoomSublist extends React.Component<IProps, IState> {
// 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 || [];
const nextExtraTiles = (nextState.filteredExtraTiles || 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) {
if (RoomSublist.calcNumTiles(nextState.rooms, nextExtraTiles) !== this.numTiles) {
return true;
}

Expand Down Expand Up @@ -232,16 +237,41 @@ export default class RoomSublist extends React.Component<IProps, IState> {
}

public componentWillUnmount() {
this.state.notificationState.destroy();
defaultDispatcher.unregister(this.dispatcherRef);
RoomListStore.instance.off(LISTS_UPDATE_EVENT, this.onListsUpdated);
}

private onListsUpdated = () => {
const stateUpdates: IState & any = {}; // &any is to avoid a cast on the initializer

if (this.props.extraBadTilesThatShouldntExist) {
const nameCondition = RoomListStore.instance.getFirstNameFilterCondition();
if (nameCondition) {
stateUpdates.filteredExtraTiles = this.props.extraBadTilesThatShouldntExist
.filter(t => nameCondition.matches(t.props.displayName || ""));
} else if (this.state.filteredExtraTiles) {
stateUpdates.filteredExtraTiles = null;
}
}

const currentRooms = this.state.rooms;
const newRooms = RoomListStore.instance.orderedLists[this.props.tagId] || [];
if (arrayHasOrderChange(currentRooms, newRooms)) {
this.setState({rooms: newRooms});
stateUpdates.rooms = newRooms;
}

const isStillBeingFiltered = !!RoomListStore.instance.getFirstNameFilterCondition();
if (isStillBeingFiltered !== this.isBeingFiltered) {
this.isBeingFiltered = isStillBeingFiltered;
if (isStillBeingFiltered) {
stateUpdates.isExpanded = true;
} else {
stateUpdates.isExpanded = !this.layout.isCollapsed;
}
}

if (Object.keys(stateUpdates).length > 0) {
this.setState(stateUpdates);
}
};

Expand Down Expand Up @@ -376,8 +406,8 @@ export default class RoomSublist extends React.Component<IProps, IState> {
} else {
// find the first room with a count of the same colour as the badge count
room = this.state.rooms.find((r: Room) => {
const notifState = this.state.notificationState.getForRoom(r);
return notifState.count > 0 && notifState.color === this.state.notificationState.color;
const notifState = this.notificationState.getForRoom(r);
return notifState.count > 0 && notifState.color === this.notificationState.color;
});
}

Expand Down Expand Up @@ -484,8 +514,9 @@ export default class RoomSublist extends React.Component<IProps, IState> {
}
}

if (this.props.extraBadTilesThatShouldntExist) {
tiles.push(...this.props.extraBadTilesThatShouldntExist);
if (this.extraTiles) {
// HACK: We break typing here, but this 'extra tiles' property shouldn't exist.
(tiles as any[]).push(...this.extraTiles);
}

// We only have to do this because of the extra tiles. We do it conditionally
Expand Down Expand Up @@ -592,7 +623,7 @@ export default class RoomSublist extends React.Component<IProps, IState> {
const badge = (
<NotificationBadge
forceCount={true}
notification={this.state.notificationState}
notification={this.notificationState}
onClick={this.onBadgeClick}
tabIndex={tabIndex}
aria-label={ariaLabel}
Expand Down
2 changes: 0 additions & 2 deletions src/settings/SettingsStore.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,6 @@ export default class SettingsStore {
callbackFn(originalSettingName, changedInRoomId, atLevel, newValAtLevel, newValue);
};

console.log(`Starting watcher for ${settingName}@${roomId || '<null room>'} as ID ${watcherId}`);
SettingsStore._watchers[watcherId] = localizedCallback;
defaultWatchManager.watchSetting(settingName, roomId, localizedCallback);

Expand All @@ -167,7 +166,6 @@ export default class SettingsStore {
return;
}

console.log(`Ending watcher ID ${watcherReference}`);
defaultWatchManager.unwatchSetting(SettingsStore._watchers[watcherReference]);
delete SettingsStore._watchers[watcherReference];
}
Expand Down
Loading

0 comments on commit 3561de3

Please sign in to comment.