From 8672b97f9a57ad879492f05024578ce944991cfd Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Wed, 14 Mar 2018 14:29:55 +0000 Subject: [PATCH 1/2] Improve room list performance when receiving messages In summary this makes RoomTiles (and RoomAvatars) do more for themselves in terms of reacting individually to state changes in the js-sdk. Instead of force updating the entire room list for avatar changes and room name changes, do this in the RoomTile and RoomAvatar instead. This increases the number of listeners listening to the matrix client, but allows us to properly implement a shouldComponentUpdate for RoomTile (because the avatar, name and notification count are now in component state) --- src/components/structures/TimelinePanel.js | 1 + src/components/views/avatars/RoomAvatar.js | 18 +++++++ src/components/views/rooms/RoomList.js | 18 ------- src/components/views/rooms/RoomTile.js | 60 ++++++++++++++++++++-- 4 files changed, 75 insertions(+), 22 deletions(-) diff --git a/src/components/structures/TimelinePanel.js b/src/components/structures/TimelinePanel.js index 12f745146e2..1a03b5d9944 100644 --- a/src/components/structures/TimelinePanel.js +++ b/src/components/structures/TimelinePanel.js @@ -624,6 +624,7 @@ var TimelinePanel = React.createClass({ this.props.timelineSet.room.setUnreadNotificationCount('highlight', 0); dis.dispatch({ action: 'on_room_read', + roomId: this.props.timelineSet.room.roomId, }); } } diff --git a/src/components/views/avatars/RoomAvatar.js b/src/components/views/avatars/RoomAvatar.js index cae02ac4080..0ccf9c66fe0 100644 --- a/src/components/views/avatars/RoomAvatar.js +++ b/src/components/views/avatars/RoomAvatar.js @@ -48,12 +48,30 @@ module.exports = React.createClass({ }; }, + componentWillMount: function() { + MatrixClientPeg.get().on("RoomState.events", this.onRoomStateEvents); + }, + + componentWillUnmount: function() { + MatrixClientPeg.get().removeListener("RoomState.events", this.onRoomStateEvents); + }, + componentWillReceiveProps: function(newProps) { this.setState({ urls: this.getImageUrls(newProps), }); }, + onRoomStateEvents: function(ev) { + if (ev.getRoomId() !== this.props.room.roomId || + ev.getType() !== 'm.room.avatar' + ) return; + + this.setState({ + urls: this.getImageUrls(this.props), + }); + }, + getImageUrls: function(props) { return [ ContentRepo.getHttpUriForMxc( diff --git a/src/components/views/rooms/RoomList.js b/src/components/views/rooms/RoomList.js index e48f19d143c..6ccf04c5d8c 100644 --- a/src/components/views/rooms/RoomList.js +++ b/src/components/views/rooms/RoomList.js @@ -77,9 +77,7 @@ module.exports = React.createClass({ cli.on("Room", this.onRoom); cli.on("deleteRoom", this.onDeleteRoom); - cli.on("Room.name", this.onRoomName); cli.on("Room.receipt", this.onRoomReceipt); - cli.on("RoomState.events", this.onRoomStateEvents); cli.on("RoomMember.name", this.onRoomMemberName); cli.on("Event.decrypted", this.onEventDecrypted); cli.on("accountData", this.onAccountData); @@ -161,12 +159,6 @@ module.exports = React.createClass({ }); } break; - case 'on_room_read': - // Force an update because the notif count state is too deep to cause - // an update. This forces the local echo of reading notifs to be - // reflected by the RoomTiles. - this.forceUpdate(); - break; } }, @@ -177,9 +169,7 @@ module.exports = React.createClass({ if (MatrixClientPeg.get()) { MatrixClientPeg.get().removeListener("Room", this.onRoom); MatrixClientPeg.get().removeListener("deleteRoom", this.onDeleteRoom); - MatrixClientPeg.get().removeListener("Room.name", this.onRoomName); MatrixClientPeg.get().removeListener("Room.receipt", this.onRoomReceipt); - MatrixClientPeg.get().removeListener("RoomState.events", this.onRoomStateEvents); MatrixClientPeg.get().removeListener("RoomMember.name", this.onRoomMemberName); MatrixClientPeg.get().removeListener("Event.decrypted", this.onEventDecrypted); MatrixClientPeg.get().removeListener("accountData", this.onAccountData); @@ -243,14 +233,6 @@ module.exports = React.createClass({ } }, - onRoomName: function(room) { - this._delayedRefreshRoomList(); - }, - - onRoomStateEvents: function(ev, state) { - this._delayedRefreshRoomList(); - }, - onRoomMemberName: function(ev, member) { this._delayedRefreshRoomList(); }, diff --git a/src/components/views/rooms/RoomTile.js b/src/components/views/rooms/RoomTile.js index 6c0c2497b31..448810e2eae 100644 --- a/src/components/views/rooms/RoomTile.js +++ b/src/components/views/rooms/RoomTile.js @@ -21,6 +21,7 @@ const React = require('react'); const ReactDOM = require("react-dom"); import PropTypes from 'prop-types'; const classNames = require('classnames'); +import dis from '../../../dispatcher'; const MatrixClientPeg = require('../../../MatrixClientPeg'); import DMRoomMap from '../../../utils/DMRoomMap'; const sdk = require('../../../index'); @@ -58,7 +59,9 @@ module.exports = React.createClass({ hover: false, badgeHover: false, menuDisplayed: false, + roomName: this.props.room.name, notifState: RoomNotifs.getRoomNotifsState(this.props.room.roomId), + notificationCount: this.props.room.getUnreadNotificationCount(), selected: this.props.room.roomId === RoomViewStore.getRoomId(), }); }, @@ -81,6 +84,20 @@ module.exports = React.createClass({ } }, + onRoomTimeline: function(ev, room) { + if (room !== this.props.room) return; + this.setState({ + notificationCount: this.props.room.getUnreadNotificationCount(), + }); + }, + + onRoomName: function(room) { + if (room !== this.props.room) return; + this.setState({ + roomName: this.props.room.name, + }); + }, + onAccountData: function(accountDataEvent) { if (accountDataEvent.getType() == 'm.push_rules') { this.setState({ @@ -89,6 +106,21 @@ module.exports = React.createClass({ } }, + onAction: function(payload) { + switch (payload.action) { + // XXX: slight hack in order to zero the notification count when a room + // is read. Ideally this state would be given to this via props (as we + // do with `unread`). This is still better than forceUpdating the entire + // RoomList when a room is read. + case 'on_room_read': + if (payload.roomId !== this.props.room.roomId) break; + this.setState({ + notificationCount: this.props.room.getUnreadNotificationCount(), + }); + break; + } + }, + _onActiveRoomChange: function() { this.setState({ selected: this.props.room.roomId === RoomViewStore.getRoomId(), @@ -97,15 +129,37 @@ module.exports = React.createClass({ componentWillMount: function() { MatrixClientPeg.get().on("accountData", this.onAccountData); + MatrixClientPeg.get().on("Room.timeline", this.onRoomTimeline); + MatrixClientPeg.get().on("Room.name", this.onRoomName); ActiveRoomObserver.addListener(this.props.room.roomId, this._onActiveRoomChange); + this.dispatcherRef = dis.register(this.onAction); }, componentWillUnmount: function() { const cli = MatrixClientPeg.get(); if (cli) { MatrixClientPeg.get().removeListener("accountData", this.onAccountData); + MatrixClientPeg.get().removeListener("Room.timeline", this.onRoomTimeline); + MatrixClientPeg.get().removeListener("Room.name", this.onRoomName); } ActiveRoomObserver.removeListener(this.props.room.roomId, this._onActiveRoomChange); + dis.unregister(this.dispatcherRef); + }, + + // Do a simple shallow comparison of props and state to avoid unnecessary + // renders. The assumption made here is that only state and props are used + // in rendering this component and children. + // + // RoomList is frequently made to forceUpdate, so this decreases number of + // RoomTile renderings. + shouldComponentUpdate: function(newProps, newState) { + if (Object.keys(newProps).some((k) => newProps[k] !== this.props[k])) { + return true; + } + if (Object.keys(newState).some((k) => newState[k] !== this.state[k])) { + return true; + } + return false; }, onClick: function(ev) { @@ -174,7 +228,7 @@ module.exports = React.createClass({ const myUserId = MatrixClientPeg.get().credentials.userId; const me = this.props.room.currentState.members[myUserId]; - const notificationCount = this.props.room.getUnreadNotificationCount(); + const notificationCount = this.state.notificationCount; // var highlightCount = this.props.room.getUnreadNotificationCount("highlight"); const notifBadges = notificationCount > 0 && this._shouldShowNotifBadge(); @@ -202,9 +256,7 @@ module.exports = React.createClass({ 'mx_RoomTile_badgeButton': this.state.badgeHover || this.state.menuDisplayed, }); - // XXX: We should never display raw room IDs, but sometimes the - // room name js sdk gives is undefined (cannot repro this -- k) - let name = this.props.room.name || this.props.room.roomId; + let name = this.state.roomName; name = name.replace(":", ":\u200b"); // add a zero-width space to allow linewrapping after the colon let badge; From c903cc63756d83a087f00a6709fa843868804874 Mon Sep 17 00:00:00 2001 From: Luke Barnard Date: Wed, 14 Mar 2018 14:55:36 +0000 Subject: [PATCH 2/2] Fix tests --- src/components/views/avatars/RoomAvatar.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/components/views/avatars/RoomAvatar.js b/src/components/views/avatars/RoomAvatar.js index 0ccf9c66fe0..e547cf0fa72 100644 --- a/src/components/views/avatars/RoomAvatar.js +++ b/src/components/views/avatars/RoomAvatar.js @@ -53,7 +53,10 @@ module.exports = React.createClass({ }, componentWillUnmount: function() { - MatrixClientPeg.get().removeListener("RoomState.events", this.onRoomStateEvents); + const cli = MatrixClientPeg.get(); + if (cli) { + cli.removeListener("RoomState.events", this.onRoomStateEvents); + } }, componentWillReceiveProps: function(newProps) {