From 1be35a77ec996a3409e544d3e7a11ecde6652137 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 8 Sep 2017 17:06:46 +0100 Subject: [PATCH 1/4] Don't wait for setState to run onHaveRoom onHaveRoom sets some more state (among other things) so putting it in the setState callback so it could observe the new state caused us to have to re-render again unnecessarily. Just give it the new state as a parameter. --- src/components/structures/RoomView.js | 29 +++++++++++++++------------ 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index 2de8496b50..037cb736cd 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -211,16 +211,19 @@ module.exports = React.createClass({ newState.searchResults = null; } - this.setState(newState, () => { - // At this point, this.state.roomId could be null (e.g. the alias might not - // have been resolved yet) so anything called here must handle this case. - if (initial) { - this._onHaveRoom(); - } - }); + this.setState(newState); + // At this point, newState.roomId could be null (e.g. the alias might not + // have been resolved yet) so anything called here must handle this case. + // We pass the new state into this function for it to read: it needs to + // observe the new state but we don't want to put it in the setState + // callback because this would prevent the setStates from being batched, + // ie. cause it to render RoomView twice rather than the once that is necessary. + if (initial) { + this._onHaveRoom(newState); + } }, - _onHaveRoom: function() { + _onHaveRoom: function(state) { // if this is an unknown room then we're in one of three states: // - This is a room we can peek into (search engine) (we can /peek) // - This is a room we can publicly join or were invited to. (we can /join) @@ -236,7 +239,7 @@ module.exports = React.createClass({ // about it). We don't peek in the historical case where we were joined but are // now not joined because the js-sdk peeking API will clobber our historical room, // making it impossible to indicate a newly joined room. - const room = this.state.room; + const room = state.room; if (room) { this.setState({ unsentMessageError: this._getUnsentMessageError(room), @@ -244,15 +247,15 @@ module.exports = React.createClass({ }); this._onRoomLoaded(room); } - if (!this.state.joining && this.state.roomId) { + if (!state.joining && state.roomId) { if (this.props.autoJoin) { this.onJoinButtonClicked(); - } else if (!room && this.state.shouldPeek) { - console.log("Attempting to peek into room %s", this.state.roomId); + } else if (!room && state.shouldPeek) { + console.log("Attempting to peek into room %s", state.roomId); this.setState({ peekLoading: true, }); - MatrixClientPeg.get().peekInRoom(this.state.roomId).then((room) => { + MatrixClientPeg.get().peekInRoom(state.roomId).then((room) => { this.setState({ room: room, peekLoading: false, From 03dcded72f98108a9f59fe8bb5e01b42299112b4 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 8 Sep 2017 17:39:10 +0100 Subject: [PATCH 2/4] Blank line to make comment clearer --- src/components/structures/RoomView.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index 037cb736cd..6b76ad4ae1 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -214,6 +214,7 @@ module.exports = React.createClass({ this.setState(newState); // At this point, newState.roomId could be null (e.g. the alias might not // have been resolved yet) so anything called here must handle this case. + // We pass the new state into this function for it to read: it needs to // observe the new state but we don't want to put it in the setState // callback because this would prevent the setStates from being batched, From bf982004f6e06934df18ddc0fdcd51dbd863e7c2 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 8 Sep 2017 17:56:53 +0100 Subject: [PATCH 3/4] Give onHaveRoom the info it needs explicitly Rather than giving it a state object which is not actually the whole state but happens to be everything it actually wants (currently) --- src/components/structures/RoomView.js | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index 6b76ad4ae1..56c0d1d281 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -220,11 +220,11 @@ module.exports = React.createClass({ // callback because this would prevent the setStates from being batched, // ie. cause it to render RoomView twice rather than the once that is necessary. if (initial) { - this._onHaveRoom(newState); + this._onHaveRoom(newState.room, newState.roomId, newState.joining, newState.shouldPeek); } }, - _onHaveRoom: function(state) { + _onHaveRoom: function(room, roomId, joining, shouldPeek) { // if this is an unknown room then we're in one of three states: // - This is a room we can peek into (search engine) (we can /peek) // - This is a room we can publicly join or were invited to. (we can /join) @@ -240,7 +240,6 @@ module.exports = React.createClass({ // about it). We don't peek in the historical case where we were joined but are // now not joined because the js-sdk peeking API will clobber our historical room, // making it impossible to indicate a newly joined room. - const room = state.room; if (room) { this.setState({ unsentMessageError: this._getUnsentMessageError(room), @@ -248,15 +247,15 @@ module.exports = React.createClass({ }); this._onRoomLoaded(room); } - if (!state.joining && state.roomId) { + if (!joining && roomId) { if (this.props.autoJoin) { this.onJoinButtonClicked(); - } else if (!room && state.shouldPeek) { - console.log("Attempting to peek into room %s", state.roomId); + } else if (!room && shouldPeek) { + console.log("Attempting to peek into room %s", roomId); this.setState({ peekLoading: true, }); - MatrixClientPeg.get().peekInRoom(state.roomId).then((room) => { + MatrixClientPeg.get().peekInRoom(roomId).then((room) => { this.setState({ room: room, peekLoading: false, From aee2f3cdefaf4a787f7bbdfedf226f3927a10301 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 8 Sep 2017 18:11:13 +0100 Subject: [PATCH 4/4] Rename onHaveRoom And move some code out of it which didn't really have any reason to be hanging out there rather than just be where we set the room a few lines above. --- src/components/structures/RoomView.js | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index 56c0d1d281..8a0eeb50b9 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -189,6 +189,11 @@ module.exports = React.createClass({ // the RoomView instance if (initial) { newState.room = MatrixClientPeg.get().getRoom(newState.roomId); + if (newState.room) { + newState.unsentMessageError = this._getUnsentMessageError(newState.room); + newState.showApps = this._shouldShowApps(newState.room); + this._onRoomLoaded(newState.room); + } } if (this.state.roomId === null && newState.roomId !== null) { @@ -220,11 +225,11 @@ module.exports = React.createClass({ // callback because this would prevent the setStates from being batched, // ie. cause it to render RoomView twice rather than the once that is necessary. if (initial) { - this._onHaveRoom(newState.room, newState.roomId, newState.joining, newState.shouldPeek); + this._setupRoom(newState.room, newState.roomId, newState.joining, newState.shouldPeek); } }, - _onHaveRoom: function(room, roomId, joining, shouldPeek) { + _setupRoom: function(room, roomId, joining, shouldPeek) { // if this is an unknown room then we're in one of three states: // - This is a room we can peek into (search engine) (we can /peek) // - This is a room we can publicly join or were invited to. (we can /join) @@ -240,13 +245,6 @@ module.exports = React.createClass({ // about it). We don't peek in the historical case where we were joined but are // now not joined because the js-sdk peeking API will clobber our historical room, // making it impossible to indicate a newly joined room. - if (room) { - this.setState({ - unsentMessageError: this._getUnsentMessageError(room), - showApps: this._shouldShowApps(room), - }); - this._onRoomLoaded(room); - } if (!joining && roomId) { if (this.props.autoJoin) { this.onJoinButtonClicked();