From c82b364ca84c429f0151293738e3fa01ceef7e19 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Wed, 3 Feb 2016 08:03:10 +0000 Subject: [PATCH] Address review comments Mostly renaming things and adding comments. --- src/components/structures/MatrixChat.js | 12 +--- src/components/structures/RoomView.js | 70 +++++++++++++++--------- src/components/structures/ScrollPanel.js | 52 ++++++++++++------ src/components/views/rooms/EventTile.js | 4 +- 4 files changed, 83 insertions(+), 55 deletions(-) diff --git a/src/components/structures/MatrixChat.js b/src/components/structures/MatrixChat.js index 9d9e7758b6..325b2ca459 100644 --- a/src/components/structures/MatrixChat.js +++ b/src/components/structures/MatrixChat.js @@ -657,15 +657,9 @@ module.exports = React.createClass({ action: 'start_post_registration', }); } else if (screen.indexOf('room/') == 0) { - var roomString = screen.substring(5); - var eventId; - - // extract event id, if one is given - var idx = roomString.indexOf('/'); - if (idx >= 0) { - eventId = roomString.substring(idx+1); - roomString = roomString.substring(0, idx); - } + var segments = screen.substring(5).split('/'); + var roomString = segments[0]; + var eventId = segments[1]; // undefined if no event id given if (roomString[0] == '#') { if (this.state.logged_in) { diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index be690e4cf2..f0c2e61e58 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -102,7 +102,11 @@ module.exports = React.createClass({ canPeek: false, readMarkerEventId: room ? room.getEventReadUpTo(MatrixClientPeg.get().credentials.userId) : null, readMarkerGhostEventId: undefined, - atBottom: true, + + // this is true if we are fully scrolled-down, and are looking at + // the end of the live timeline. It has the effect of hiding the + // 'scroll to bottom' knob, among a couple of other things. + atEndOfLiveTimeline: true, } }, @@ -360,7 +364,7 @@ module.exports = React.createClass({ if (ev.getSender() !== MatrixClientPeg.get().credentials.userId) { // update unread count when scrolled up - if (!this.state.searchResults && this.state.atBottom) { + if (!this.state.searchResults && this.state.atEndOfLiveTimeline) { // no change } else { @@ -467,7 +471,7 @@ module.exports = React.createClass({ // // we do this here as well as in sendReadReceipt to deal with // people using two clients at once. - if (this.refs.messagePanel && this.state.atBottom) { + if (this.refs.messagePanel && this.state.atEndOfLiveTimeline) { this.refs.messagePanel.scrollToToken(readMarkerEventId); } } @@ -700,14 +704,14 @@ module.exports = React.createClass({ if (this.state.numUnreadMessages != 0) { this.setState({ numUnreadMessages: 0 }); } - if (!this.state.atBottom) { - this.setState({ atBottom: true }); + if (!this.state.atEndOfLiveTimeline) { + this.setState({ atEndOfLiveTimeline: true }); } } else { - if (this.state.atBottom) { - this.setState({ atBottom: false }); - } + if (this.state.atEndOfLiveTimeline) { + this.setState({ atEndOfLiveTimeline: false }); + } } }, @@ -994,7 +998,7 @@ module.exports = React.createClass({ ret.push(
  • + last={last} isSelectedEvent={highlight}/>
    • ); @@ -1285,11 +1289,18 @@ module.exports = React.createClass({ var currentReadUpToEventIndex = this._indexForEventId(currentReadUpToEventId); // We want to avoid sending out read receipts when we are looking at - // events in the past. + // events in the past which are before the latest RR. + // + // For now, let's apply a heuristic: if (a) the event corresponding to + // the latest RR (either from the server, or sent by ourselves) doesn't + // appear in our timeline, and (b) we could forward-paginate the event + // timeline, then don't send any more RRs. + // + // This isn't watertight, as we could be looking at a section of + // timeline which is *after* the latest RR (so we should actually send + // RRs) - but that is a bit of a niche case. It will sort itself out when + // the user eventually hits the live timeline. // - // For now, let's apply a heuristic: if (a) the server has a - // readUpToEvent for us, (b) we can't find it, and (c) we could - // forward-paginate the event timeline, then suppress read receipts. if (currentReadUpToEventId && currentReadUpToEventIndex === null && this._timelineWindow.canPaginate(EventTimeline.FORWARDS)) { return; @@ -1317,7 +1328,7 @@ module.exports = React.createClass({ // // we do this here as well as in onRoomReceipt to cater for guest users // (which do not send out read receipts). - if (this.state.atBottom) { + if (this.state.atEndOfLiveTimeline) { this.refs.messagePanel.scrollToToken(lastReadEvent.getId()); } } @@ -1444,7 +1455,8 @@ module.exports = React.createClass({ return this.state.numUnreadMessages + " new message" + (this.state.numUnreadMessages > 1 ? "s" : ""); }, - scrollToBottom: function() { + // jump down to the bottom of this room, where new events are arriving + jumpToLiveTimeline: function() { // if we can't forward-paginate the existing timeline, then there // is no point reloading it - just jump straight to the bottom. // @@ -1479,16 +1491,20 @@ module.exports = React.createClass({ var scrollState = messagePanel.getScrollState(); - if (scrollState.atBottom) { + if (scrollState.stuckAtBottom) { // we don't really expect to be in this state, but it will - // occasionally happen when we are in a transition. Treat it the - // same as having no saved state (which will cause us to scroll to - // last unread on reload). + // occasionally happen when no scroll state has been set on the + // messagePanel (ie, we didn't have an initial event (so it's + // probably a new room), there has been no user-initiated scroll, and + // no read-receipts have arrived to update the scroll position). + // + // Return null, which will cause us to scroll to last unread on + // reload. return null; } return { - focussedEvent: scrollState.lastDisplayedScrollToken, + focussedEvent: scrollState.trackedScrollToken, pixelOffset: scrollState.pixelOffset, }; }, @@ -1731,7 +1747,7 @@ module.exports = React.createClass({ // set when you've scrolled up else if (unreadMsgs) { statusBar = ( -
    +
    {unreadMsgs}
    @@ -1745,9 +1761,9 @@ module.exports = React.createClass({
    ); } - else if (!this.state.atBottom) { + else if (!this.state.atEndOfLiveTimeline) { statusBar = ( -
    +
    Scroll to bottom of page
    ); @@ -1888,8 +1904,10 @@ module.exports = React.createClass({ // just show a spinner while the timeline loads. // - // put it in a div of the right class so that the order in the - // roomview flexbox is correct. + // put it in a div of the right class (mx_RoomView_messagePanel) so + // that the order in the roomview flexbox is correct, and + // mx_RoomView_messageListWrapper to position the inner div in the + // right place. // // Note that the click-on-search-result functionality relies on the // fact that the messagePanel is hidden while the timeline reloads, @@ -1897,7 +1915,7 @@ module.exports = React.createClass({ // exist. if (this.state.timelineLoading) { messagePanel = ( -
    +
    ); diff --git a/src/components/structures/ScrollPanel.js b/src/components/structures/ScrollPanel.js index a577be2e52..514937f877 100644 --- a/src/components/structures/ScrollPanel.js +++ b/src/components/structures/ScrollPanel.js @@ -38,16 +38,17 @@ if (DEBUG_SCROLL) { * when we get close to the start or end of the list. * * Each child element should have a 'data-scroll-token'. This token is used to - * serialise the scroll state, and returned as the 'lastDisplayedScrollToken' + * serialise the scroll state, and returned as the 'trackedScrollToken' * attribute by getScrollState(). * * Some notes about the implementation: * * The saved 'scrollState' can exist in one of two states: * - * - atBottom: (the default, and restored by resetScrollState): the viewport - * is scrolled down as far as it can be. When the children are updated, the - * scroll position will be updated to ensure it is still at the bottom. + * - stuckAtBottom: (the default, and restored by resetScrollState): the + * viewport is scrolled down as far as it can be. When the children are + * updated, the scroll position will be updated to ensure it is still at + * the bottom. * * - fixed, in which the viewport is conceptually tied at a specific scroll * offset. We don't save the absolute scroll offset, because that would be @@ -59,9 +60,9 @@ if (DEBUG_SCROLL) { * The 'stickyBottom' property controls the behaviour when we reach the bottom * of the window (either through a user-initiated scroll, or by calling * scrollToBottom). If stickyBottom is enabled, the scrollState will enter - * 'atBottom' state - ensuring that new additions cause the window to scroll - * down further. If stickyBottom is disabled, we just save the scroll offset as - * normal. + * 'stuckAtBottom' state - ensuring that new additions cause the window to + * scroll down further. If stickyBottom is disabled, we just save the scroll + * offset as normal. */ module.exports = React.createClass({ displayName: 'ScrollPanel', @@ -178,6 +179,10 @@ module.exports = React.createClass({ }, // return true if the content is fully scrolled down right now; else false. + // + // note that this is independent of the 'stuckAtBottom' state - it is simply + // about whether the the content is scrolled down right now, irrespective of + // whether it will stay that way when the children update. isAtBottom: function() { var sn = this._getScrollNode(); @@ -268,8 +273,19 @@ module.exports = React.createClass({ }).done(); }, - // get the current scroll position of the room, so that it can be - // restored later + /* get the current scroll state. This returns an object with the following + * properties: + * + * boolean stuckAtBottom: true if we are tracking the bottom of the + * scroll. false if we are tracking a particular child. + * + * string trackedScrollToken: undefined if stuckAtBottom is true; if it is + * false, the data-scroll-token of the child which we are tracking. + * + * number pixelOffset: undefined if stuckAtBottom is true; if it is false, + * the number of pixels the bottom of the tracked child is above the + * bottom of the scroll panel. + */ getScrollState: function() { return this.scrollState; }, @@ -287,7 +303,7 @@ module.exports = React.createClass({ * child list.) */ resetScrollState: function() { - this.scrollState = {atBottom: true}; + this.scrollState = {stuckAtBottom: true}; }, scrollToBottom: function() { @@ -322,8 +338,8 @@ module.exports = React.createClass({ // timeline and need to do more pagination); we want to save the // *desired* scroll state rather than what we end up achieving. this.scrollState = { - atBottom: false, - lastDisplayedScrollToken: scrollToken, + stuckAtBottom: false, + trackedScrollToken: scrollToken, pixelOffset: pixelOffset }; @@ -370,7 +386,7 @@ module.exports = React.createClass({ _saveScrollState: function() { if (this.props.stickyBottom && this.isAtBottom()) { - this.scrollState = { atBottom: true }; + this.scrollState = { stuckAtBottom: true }; debuglog("Saved scroll state", this.scrollState); return; } @@ -386,8 +402,8 @@ module.exports = React.createClass({ var boundingRect = node.getBoundingClientRect(); if (boundingRect.bottom < wrapperRect.bottom) { this.scrollState = { - atBottom: false, - lastDisplayedScrollToken: node.dataset.scrollToken, + stuckAtBottom: false, + trackedScrollToken: node.dataset.scrollToken, pixelOffset: wrapperRect.bottom - boundingRect.bottom, } debuglog("Saved scroll state", this.scrollState); @@ -402,11 +418,11 @@ module.exports = React.createClass({ var scrollState = this.scrollState; var scrollNode = this._getScrollNode(); - if (scrollState.atBottom) { + if (scrollState.stuckAtBottom) { scrollNode.scrollTop = scrollNode.scrollHeight; debuglog("Scrolled to bottom; offset now", scrollNode.scrollTop); - } else if (scrollState.lastDisplayedScrollToken) { - this._scrollToToken(scrollState.lastDisplayedScrollToken, + } else if (scrollState.trackedScrollToken) { + this._scrollToToken(scrollState.trackedScrollToken, scrollState.pixelOffset); } this._lastSetScroll = scrollNode.scrollTop; diff --git a/src/components/views/rooms/EventTile.js b/src/components/views/rooms/EventTile.js index 6d786294ad..f580686128 100644 --- a/src/components/views/rooms/EventTile.js +++ b/src/components/views/rooms/EventTile.js @@ -100,7 +100,7 @@ module.exports = React.createClass({ onHighlightClick: React.PropTypes.func, /* is this the focussed event */ - selectedEvent: React.PropTypes.bool, + isSelectedEvent: React.PropTypes.bool, }, getInitialState: function() { @@ -276,7 +276,7 @@ module.exports = React.createClass({ ) !== -1, mx_EventTile_notSent: this.props.mxEvent.status == 'not_sent', mx_EventTile_highlight: this.shouldHighlight(), - mx_EventTile_selected: this.props.selectedEvent, + mx_EventTile_selected: this.props.isSelectedEvent, mx_EventTile_continuation: this.props.continuation, mx_EventTile_last: this.props.last, mx_EventTile_contextual: this.props.contextual,