Merge pull request #2168 from matrix-org/bwindels/fixstalerr

Fix stale RR and improve LL reliability in RoomView & MemberList.

src/actions/MatrixActionCreators.js

@@ -144,26 +144,25 @@ function createRoomTimelineAction(matrixClient, timelineEvent, room, toStartOfTi
 /**
  * @typedef RoomMembershipAction
  * @type {Object}
- * @property {string} action 'MatrixActions.RoomMember.membership'.
+ * @property {string} action 'MatrixActions.Room.myMembership'.
- * @property {RoomMember} member the member whose membership was updated.
+ * @property {Room} room to room for which the self-membership changed.
+ * @property {string} membership the new membership
+ * @property {string} oldMembership the previous membership, can be null.
  */
 
 /**
- * Create a MatrixActions.Room.selfMembership action that represents
+ * Create a MatrixActions.Room.myMembership action that represents
- * a MatrixClient `RoomMember.membership` matrix event for the syncing user,
+ * a MatrixClient `Room.myMembership` event for the syncing user,
- * emitted when the member's membership is updated.
+ * emitted when the syncing user's membership is updated for a room.
  *
  * @param {MatrixClient} matrixClient the matrix client.
- * @param {MatrixEvent} membershipEvent the m.room.member event.
+ * @param {Room} room to room for which the self-membership changed.
- * @param {RoomMember} member the member whose membership was updated.
+ * @param {string} membership the new membership
- * @param {string} oldMembership the member's previous membership.
+ * @param {string} oldMembership the previous membership, can be null.
- * @returns {RoomMembershipAction} an action of type `MatrixActions.RoomMember.membership`.
+ * @returns {RoomMembershipAction} an action of type `MatrixActions.Room.myMembership`.
  */
-function createSelfRoomMembershipAction(matrixClient, membershipEvent, member, oldMembership) {
+function createSelfMembershipAction(matrixClient, room, membership, oldMembership) {
-    if (member.userId === matrixClient.getUserId()) {
+    return { action: 'MatrixActions.Room.myMembership', room, membership, oldMembership};
-        return { action: 'MatrixActions.Room.selfMembership', member };
-    }
-    return null;
 }
 
 /**
@@ -205,7 +204,7 @@ export default {
         this._addMatrixClientListener(matrixClient, 'Room', createRoomAction);
         this._addMatrixClientListener(matrixClient, 'Room.tags', createRoomTagsAction);
         this._addMatrixClientListener(matrixClient, 'Room.timeline', createRoomTimelineAction);
-        this._addMatrixClientListener(matrixClient, 'RoomMember.membership', createSelfRoomMembershipAction);
+        this._addMatrixClientListener(matrixClient, 'Room.myMembership', createSelfMembershipAction);
         this._addMatrixClientListener(matrixClient, 'Event.decrypted', createEventDecryptedAction);
     },
 

src/components/structures/RoomView.js

@@ -91,13 +91,16 @@ module.exports = React.createClass({
     },
 
     getInitialState: function() {
+        const llMembers = MatrixClientPeg.get().hasLazyLoadMembersEnabled();
         return {
             room: null,
             roomId: null,
             roomLoading: true,
             peekLoading: false,
             shouldPeek: true,
-
+            // used to trigger a rerender in TimelinePanel once the members are loaded,
+            // so RR are rendered again (now with the members available), ...
+            membersLoaded: !llMembers,
             // The event to be scrolled to initially
             initialEventId: null,
             // The offset in pixels from the event with which to scroll vertically
@@ -148,7 +151,7 @@ module.exports = React.createClass({
         MatrixClientPeg.get().on("Room.name", this.onRoomName);
         MatrixClientPeg.get().on("Room.accountData", this.onRoomAccountData);
         MatrixClientPeg.get().on("RoomState.members", this.onRoomStateMember);
-        MatrixClientPeg.get().on("RoomMember.membership", this.onRoomMemberMembership);
+        MatrixClientPeg.get().on("Room.myMembership", this.onMyMembership);
         MatrixClientPeg.get().on("accountData", this.onAccountData);
 
         // Start listening for RoomViewStore updates
@@ -412,8 +415,8 @@ module.exports = React.createClass({
             MatrixClientPeg.get().removeListener("Room.timeline", this.onRoomTimeline);
             MatrixClientPeg.get().removeListener("Room.name", this.onRoomName);
             MatrixClientPeg.get().removeListener("Room.accountData", this.onRoomAccountData);
+            MatrixClientPeg.get().removeListener("Room.myMembership", this.onMyMembership);
             MatrixClientPeg.get().removeListener("RoomState.members", this.onRoomStateMember);
-            MatrixClientPeg.get().removeListener("RoomMember.membership", this.onRoomMemberMembership);
             MatrixClientPeg.get().removeListener("accountData", this.onAccountData);
         }
 
@@ -582,21 +585,25 @@ module.exports = React.createClass({
         this._warnAboutEncryption(room);
         this._calculatePeekRules(room);
         this._updatePreviewUrlVisibility(room);
-        this._loadMembersIfJoined();
+        this._loadMembersIfJoined(room);
     },
 
-    _loadMembersIfJoined: function() {
+    _loadMembersIfJoined: async function(room) {
         // lazy load members if enabled
-        if (SettingsStore.isFeatureEnabled('feature_lazyloading')) {
         const cli = MatrixClientPeg.get();
-            const room = cli.getRoom(this.state.roomId);
+        if (cli.hasLazyLoadMembersEnabled()) {
             if (room && room.getMyMembership() === 'join') {
-                room.loadMembersIfNeeded().catch((err) => {
+                try {
+                    await room.loadMembersIfNeeded();
+                    if (!this.unmounted) {
+                        this.setState({membersLoaded: true});
+                    }
+                } catch (err) {
                     const errorMessage = `Fetching room members for ${room.roomId} failed.` +
                         " Room members will appear incomplete.";
                     console.error(errorMessage);
                     console.error(err);
-                });
+                }
             }
         }
     },
@@ -710,10 +717,10 @@ module.exports = React.createClass({
         this._updateRoomMembers();
     },
 
-    onRoomMemberMembership: function(ev, member, oldMembership) {
+    onMyMembership: function(room, membership, oldMembership) {
-        if (member.userId == MatrixClientPeg.get().credentials.userId) {
+        if (room.roomId === this.state.roomId) {
-            this._loadMembersIfJoined();
             this.forceUpdate();
+            this._loadMembersIfJoined(room);
         }
     },
 
@@ -1761,6 +1768,7 @@ module.exports = React.createClass({
                 onReadMarkerUpdated={this._updateTopUnreadMessagesBar}
                 showUrlPreview = {this.state.showUrlPreview}
                 className="mx_RoomView_messagePanel"
+                membersLoaded={this.state.membersLoaded}
             />);
 
         let topUnreadMessagesBar = null;

src/components/views/rooms/MemberList.js

@@ -34,20 +34,23 @@ module.exports = React.createClass({
     getInitialState: function() {
         const cli = MatrixClientPeg.get();
         if (cli.hasLazyLoadMembersEnabled()) {
-            return {loading: true};
+            // show an empty list
+            return this._getMembersState([]);
         } else {
-            return this._getMembersState();
+            return this._getMembersState(this.roomMembers());
         }
     },
 
     componentWillMount: function() {
-        this._mounted = false;
+        this._mounted = true;
         const cli = MatrixClientPeg.get();
-        if (!cli.hasLazyLoadMembersEnabled()) {
+        if (cli.hasLazyLoadMembersEnabled()) {
+            this._showMembersAccordingToMembershipWithLL();
+            cli.on("Room.myMembership", this.onMyMembership);
+        } else {
             this._listenForMembersChanges();
         }
         cli.on("Room", this.onRoom); // invites & joining after peek
-        cli.on("RoomMember.membership", this.onRoomMembership); // update when accepting an invite
         const enablePresenceByHsUrl = SdkConfig.get()["enable_presence_by_hs_url"];
         const hsUrl = MatrixClientPeg.get().baseUrl;
         this._showPresence = true;
@@ -68,51 +71,50 @@ module.exports = React.createClass({
         // cli.on("Room.timeline", this.onRoomTimeline);
     },
 
-    componentDidMount: async function() {
-        this._mounted = true;
-        this._loadMembersIfNeeded(true);
-    },
-
     componentWillUnmount: function() {
         this._mounted = false;
         const cli = MatrixClientPeg.get();
         if (cli) {
             cli.removeListener("RoomState.members", this.onRoomStateMember);
             cli.removeListener("RoomMember.name", this.onRoomMemberName);
-            cli.removeListener("RoomMember.membership", this.onRoomMembership);
+            cli.removeListener("Room.myMembership", this.onMyMembership);
             cli.removeListener("RoomState.events", this.onRoomStateEvent);
             cli.removeListener("Room", this.onRoom);
             cli.removeListener("User.lastPresenceTs", this.onUserLastPresenceTs);
-            // cli.removeListener("Room.timeline", this.onRoomTimeline);
         }
 
         // cancel any pending calls to the rate_limited_funcs
         this._updateList.cancelPendingCall();
     },
 
-    _loadMembersIfNeeded: async function(initial) {
+    /**
+     * If lazy loading is enabled, either:
+     * show a spinner and load the members if the user is joined,
+     * or show the members available so far if the user is invited
+     */
+    _showMembersAccordingToMembershipWithLL: async function() {
         const cli = MatrixClientPeg.get();
         if (cli.hasLazyLoadMembersEnabled()) {
             const cli = MatrixClientPeg.get();
             const room = cli.getRoom(this.props.roomId);
-            if (room && room.getMyMembership() === 'join') {
+            const membership = room && room.getMyMembership();
+            if (membership === "join") {
                 this.setState({loading: true});
                 try {
                     await room.loadMembersIfNeeded();
-                } catch(ex) {/* already logged in RoomView */}
+                } catch (ex) {/* already logged in RoomView */}
                 if (this._mounted) {
-                    this.setState(this._getMembersState());
+                    this.setState(this._getMembersState(this.roomMembers()));
                     this._listenForMembersChanges();
                 }
-            } else if(initial) {
+            } else if (membership === "invite") {
-                // show the members we've got
+                // show the members we've got when invited
-                this.setState(this._getMembersState());
+                this.setState(this._getMembersState(this.roomMembers()));
             }
         }
     },
 
-    _getMembersState: function() {
+    _getMembersState: function(members) {
-        const members = this.roomMembers();
         // set the state after determining _showPresence to make sure it's
         // taken into account while rerendering
         return {
@@ -129,34 +131,6 @@ module.exports = React.createClass({
         };
     },
 
-/*
-    onRoomTimeline: function(ev, room, toStartOfTimeline, removed, data) {
-        // ignore anything but real-time updates at the end of the room:
-        // updates from pagination will happen when the paginate completes.
-        if (toStartOfTimeline || !data || !data.liveEvent) return;
-
-        // treat any activity from a user as implicit presence to update the
-        // ordering of the list whenever someone says something.
-        // Except right now we're not tiebreaking "active now" users in this way
-        // so don't bother for now.
-        if (ev.getSender()) {
-            // console.log("implicit presence from " + ev.getSender());
-
-            var tile = this.refs[ev.getSender()];
-            if (tile) {
-                // work around a race where you might have a room member object
-                // before the user object exists.  XXX: why does this ever happen?
-                var all_members = room.currentState.members;
-                var userId = ev.getSender();
-                if (all_members[userId].user === null) {
-                    all_members[userId].user = MatrixClientPeg.get().getUser(userId);
-                }
-                this._updateList(); // reorder the membership list
-            }
-        }
-    },
-*/
-
     onUserLastPresenceTs(event, user) {
         // Attach a SINGLE listener for global presence changes then locate the
         // member tile and re-render it. This is more efficient than every tile
@@ -175,20 +149,12 @@ module.exports = React.createClass({
         // We listen for room events because when we accept an invite
         // we need to wait till the room is fully populated with state
         // before refreshing the member list else we get a stale list.
-
+        this._showMembersAccordingToMembershipWithLL();
-        // also when peeking, we need to await the members being loaded
-        // before showing them.
-
-        this._loadMembersIfNeeded();
     },
 
-    onRoomMembership: function(ev, member, oldMembership) {
+    onMyMembership: function(room, membership, oldMembership) {
-        const cli = MatrixClientPeg.get();
+        if (room.roomId === this.props.roomId && membership === "join") {
-        const myId = cli.getUserId();
+            this._showMembersAccordingToMembershipWithLL();
-        if (member.userId === myId && oldMembership !== "join" && member.membership === "join") {
-            // once we've joined, no need to listen for membership anymore
-            cli.removeListener("RoomMember.membership", this.onRoomMembership);
-            this._loadMembersIfNeeded();
         }
     },
 

src/stores/RoomListStore.js

@@ -120,7 +120,7 @@ class RoomListStore extends Store {
                 this._generateRoomLists();
             }
             break;
-            case 'MatrixActions.Room.selfMembership': {
+            case 'MatrixActions.Room.myMembership': {
                 this._generateRoomLists();
             }
             break;

test/components/views/rooms/RoomList-test.js

@@ -74,6 +74,7 @@ describe('RoomList', () => {
         // Mock joined member
         myMember = new RoomMember(movingRoomId, myUserId);
         myMember.membership = 'join';
+        movingRoom.updateMyMembership('join');
         movingRoom.getMember = (userId) => ({
             [client.credentials.userId]: myMember,
         }[userId]);
@@ -81,6 +82,7 @@ describe('RoomList', () => {
         otherRoom = createRoom({name: 'Other room'});
         myOtherMember = new RoomMember(otherRoom.roomId, myUserId);
         myOtherMember.membership = 'join';
+        otherRoom.updateMyMembership('join');
         otherRoom.getMember = (userId) => ({
             [client.credentials.userId]: myOtherMember,
         }[userId]);