From 408b8c18ea89d0c2fa714f4e34ec82e455273d42 Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 7 Sep 2017 17:08:36 +0100 Subject: [PATCH 1/5] Introduce a RoomScrollStateStore to keep the place we're scrolled to in rooms. This mainly eleimates the extra, superfluous onRoomViewStoreUpdate callback that happened when the previous room saved back its scroll state. Moving the scroll state to a separate store means we can have this not emit events because nothing needs to know when the scroll state changes. --- src/components/structures/RoomView.js | 42 ++++++++++++++------------- src/stores/RoomScrollStateStore.js | 37 +++++++++++++++++++++++ src/stores/RoomViewStore.js | 30 ------------------- 3 files changed, 59 insertions(+), 50 deletions(-) create mode 100644 src/stores/RoomScrollStateStore.js diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index 87bed1ed08..02a333ee82 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -47,6 +47,7 @@ import KeyCode from '../../KeyCode'; import UserProvider from '../../autocomplete/UserProvider'; import RoomViewStore from '../../stores/RoomViewStore'; +import RoomScrollStateStore from '../../stores/RoomScrollStateStore'; let DEBUG = false; let debuglog = function() {}; @@ -163,7 +164,6 @@ module.exports = React.createClass({ roomLoadError: RoomViewStore.getRoomLoadError(), joining: RoomViewStore.isJoining(), initialEventId: RoomViewStore.getInitialEventId(), - initialEventPixelOffset: RoomViewStore.getInitialEventPixelOffset(), isInitialEventHighlighted: RoomViewStore.isInitialEventHighlighted(), forwardingEvent: RoomViewStore.getForwardingEvent(), shouldPeek: RoomViewStore.shouldPeek(), @@ -191,18 +191,32 @@ module.exports = React.createClass({ newState.room = MatrixClientPeg.get().getRoom(newState.roomId); } + if (this.state.roomId !== newState.roomId) { + // Store the scroll state for the previous room so that we can return to this + // position when viewing this room in future. + if (this.state.roomId) { + RoomScrollStateStore.setScrollState(this.state.roomId, this._getScrollState()); + } + + // ...and get the scroll state for the new room + + // If an event ID wasn't specified, default to the one saved for this room + // via update_scroll_state. Assume initialEventPixelOffset should be set. + if (!newState.initialEventId) { + const roomScrollState = RoomScrollStateStore.getScrollState(newState.roomId); + if (roomScrollState) { + newState.initialEventId = roomScrollState.focussedEvent; + newState.initialEventPixelOffset = roomScrollState.pixelOffset; + } + } + } + // Clear the search results when clicking a search result (which changes the // currently scrolled to event, this.state.initialEventId). if (this.state.initialEventId !== newState.initialEventId) { newState.searchResults = null; } - // Store the scroll state for the previous room so that we can return to this - // position when viewing this room in future. - if (this.state.roomId !== newState.roomId) { - this._updateScrollMap(this.state.roomId); - } - 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. @@ -340,7 +354,7 @@ module.exports = React.createClass({ this.unmounted = true; // update the scroll map before we get unmounted - this._updateScrollMap(this.state.roomId); + RoomScrollStateStore.setScrollState(this.state.roomId, this._getScrollState()); if (this.refs.roomView) { // disconnect the D&D event listeners from the room view. This @@ -617,18 +631,6 @@ module.exports = React.createClass({ }); }, - _updateScrollMap(roomId) { - // No point updating scroll state if the room ID hasn't been resolved yet - if (!roomId) { - return; - } - dis.dispatch({ - action: 'update_scroll_state', - room_id: roomId, - scroll_state: this._getScrollState(), - }); - }, - onRoom: function(room) { if (!room || room.roomId !== this.state.roomId) { return; diff --git a/src/stores/RoomScrollStateStore.js b/src/stores/RoomScrollStateStore.js new file mode 100644 index 0000000000..a1d46a29b8 --- /dev/null +++ b/src/stores/RoomScrollStateStore.js @@ -0,0 +1,37 @@ +/* +Copyright 2017 New Vector Ltd + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +/** + * Stores where the user has scrolled to in each room + */ +class RoomScrollStateStore { + constructor() { + this._scrollStateMap = {}; + } + + getScrollState(roomId) { + return this._scrollStateMap[roomId]; + } + + setScrollState(roomId, scrollState) { + this._scrollStateMap[roomId] = scrollState; + } +} + +if (global.mx_RoomScrollStateStore === undefined) { + global.mx_RoomScrollStateStore = new RoomScrollStateStore(); +} +export default global.mx_RoomScrollStateStore; diff --git a/src/stores/RoomViewStore.js b/src/stores/RoomViewStore.js index bd9d3ea0fa..bd1fff600d 100644 --- a/src/stores/RoomViewStore.js +++ b/src/stores/RoomViewStore.js @@ -30,8 +30,6 @@ const INITIAL_STATE = { // The event to scroll to when the room is first viewed initialEventId: null, - // The offset to display the initial event at (see scrollStateMap) - initialEventPixelOffset: null, // Whether to highlight the initial event isInitialEventHighlighted: false, @@ -41,20 +39,6 @@ const INITIAL_STATE = { roomLoading: false, // Any error that has occurred during loading roomLoadError: null, - // A map from room id to scroll state. - // - // If there is no special scroll state (ie, we are following the live - // timeline), the scroll state is null. Otherwise, it is an object with - // the following properties: - // - // focussedEvent: the ID of the 'focussed' event. Typically this is - // the last event fully visible in the viewport, though if we - // have done an explicit scroll to an explicit event, it will be - // that event. - // - // pixelOffset: the number of pixels the window is scrolled down - // from the focussedEvent. - scrollStateMap: {}, forwardingEvent: null, }; @@ -115,9 +99,6 @@ class RoomViewStore extends Store { case 'on_logged_out': this.reset(); break; - case 'update_scroll_state': - this._updateScrollState(payload); - break; case 'forward_event': this._setState({ forwardingEvent: payload.event, @@ -132,7 +113,6 @@ class RoomViewStore extends Store { roomId: payload.room_id, roomAlias: payload.room_alias, initialEventId: payload.event_id, - initialEventPixelOffset: undefined, isInitialEventHighlighted: payload.highlighted, forwardingEvent: null, roomLoading: false, @@ -145,16 +125,6 @@ class RoomViewStore extends Store { newState.joining = false; } - // If an event ID wasn't specified, default to the one saved for this room - // via update_scroll_state. Assume initialEventPixelOffset should be set. - if (!newState.initialEventId) { - const roomScrollState = this._state.scrollStateMap[payload.room_id]; - if (roomScrollState) { - newState.initialEventId = roomScrollState.focussedEvent; - newState.initialEventPixelOffset = roomScrollState.pixelOffset; - } - } - if (this._state.forwardingEvent) { dis.dispatch({ action: 'send_event', From 7f44ac740346b06934850834214e6506b4a4e62a Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 7 Sep 2017 17:15:19 +0100 Subject: [PATCH 2/5] Remove now unused stuff from RoomViewStore --- src/stores/RoomViewStore.js | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/src/stores/RoomViewStore.js b/src/stores/RoomViewStore.js index bd1fff600d..17fcc97160 100644 --- a/src/stores/RoomViewStore.js +++ b/src/stores/RoomViewStore.js @@ -211,15 +211,6 @@ class RoomViewStore extends Store { }); } - _updateScrollState(payload) { - // Clobber existing scroll state for the given room ID - const newScrollStateMap = this._state.scrollStateMap; - newScrollStateMap[payload.room_id] = payload.scroll_state; - this._setState({ - scrollStateMap: newScrollStateMap, - }); - } - reset() { this._state = Object.assign({}, INITIAL_STATE); } @@ -234,11 +225,6 @@ class RoomViewStore extends Store { return this._state.initialEventId; } - // The offset to display the initial event at (see scrollStateMap) - getInitialEventPixelOffset() { - return this._state.initialEventPixelOffset; - } - // Whether to highlight the initial event isInitialEventHighlighted() { return this._state.isInitialEventHighlighted; From 82d1afcc47e7515ba093c5be56ef68f357411540 Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 7 Sep 2017 17:16:32 +0100 Subject: [PATCH 3/5] Correct comment --- src/components/structures/RoomView.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index 02a333ee82..69760499a1 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -201,7 +201,7 @@ module.exports = React.createClass({ // ...and get the scroll state for the new room // If an event ID wasn't specified, default to the one saved for this room - // via update_scroll_state. Assume initialEventPixelOffset should be set. + // in the scroll state store. Assume initialEventPixelOffset should be set. if (!newState.initialEventId) { const roomScrollState = RoomScrollStateStore.getScrollState(newState.roomId); if (roomScrollState) { From d714291aa139b5a7b65edd3d1615f19a8027ac0f Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 8 Sep 2017 13:27:14 +0100 Subject: [PATCH 4/5] Re-add doc on scroll state map structure --- src/stores/RoomScrollStateStore.js | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/stores/RoomScrollStateStore.js b/src/stores/RoomScrollStateStore.js index a1d46a29b8..07848283d1 100644 --- a/src/stores/RoomScrollStateStore.js +++ b/src/stores/RoomScrollStateStore.js @@ -19,6 +19,19 @@ limitations under the License. */ class RoomScrollStateStore { constructor() { + // A map from room id to scroll state. + // + // If there is no special scroll state (ie, we are following the live + // timeline), the scroll state is null. Otherwise, it is an object with + // the following properties: + // + // focussedEvent: the ID of the 'focussed' event. Typically this is + // the last event fully visible in the viewport, though if we + // have done an explicit scroll to an explicit event, it will be + // that event. + // + // pixelOffset: the number of pixels the window is scrolled down + // from the focussedEvent. this._scrollStateMap = {}; } From 59c54d3756153e3378ba2198115e946eee599159 Mon Sep 17 00:00:00 2001 From: David Baker Date: Fri, 8 Sep 2017 13:39:22 +0100 Subject: [PATCH 5/5] Remove redundant code --- src/components/structures/RoomView.js | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index 69760499a1..2de8496b50 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -191,14 +191,8 @@ module.exports = React.createClass({ newState.room = MatrixClientPeg.get().getRoom(newState.roomId); } - if (this.state.roomId !== newState.roomId) { - // Store the scroll state for the previous room so that we can return to this - // position when viewing this room in future. - if (this.state.roomId) { - RoomScrollStateStore.setScrollState(this.state.roomId, this._getScrollState()); - } - - // ...and get the scroll state for the new room + if (this.state.roomId === null && newState.roomId !== null) { + // Get the scroll state for the new room // If an event ID wasn't specified, default to the one saved for this room // in the scroll state store. Assume initialEventPixelOffset should be set. @@ -354,7 +348,9 @@ module.exports = React.createClass({ this.unmounted = true; // update the scroll map before we get unmounted - RoomScrollStateStore.setScrollState(this.state.roomId, this._getScrollState()); + if (this.state.roomId) { + RoomScrollStateStore.setScrollState(this.state.roomId, this._getScrollState()); + } if (this.refs.roomView) { // disconnect the D&D event listeners from the room view. This