From f2a24521dc4b7f9e1568755c8ea5ed529a41e12a Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 24 Dec 2015 00:10:21 +0000 Subject: [PATCH 1/3] Make ScrollPanel keep track of when fill requests are happening The dance to avoid doing repeated fill requests on every update is common, so add it to ScrollPanel. Let onFillRequest return a promise, which prevents any updates until it completes. --- src/components/structures/RoomView.js | 27 +++++----- src/components/structures/ScrollPanel.js | 63 +++++++++++++++++++++++- 2 files changed, 72 insertions(+), 18 deletions(-) diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index 127fe2fdb8..fc909f69ab 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -330,8 +330,6 @@ module.exports = React.createClass({ this.scrollToBottom(); this.sendReadReceipt(); - - this.refs.messagePanel.checkFillState(); }, componentDidUpdate: function() { @@ -353,30 +351,25 @@ module.exports = React.createClass({ }); this.setState({paginating: false}); - - // we might not have got enough (or, indeed, any) results from the - // pagination request, so give the messagePanel a chance to set off - // another. - - this.refs.messagePanel.checkFillState(); }, onSearchResultsFillRequest: function(backwards) { - if (!backwards || this.state.searchInProgress) - return; + if (!backwards) + return false; if (this.nextSearchBatch) { if (DEBUG_SCROLL) console.log("requesting more search results"); - this._getSearchBatch(this.state.searchTerm, - this.state.searchScope); + return this._getSearchBatch(this.state.searchTerm, + this.state.searchScope).then(true); } else { if (DEBUG_SCROLL) console.log("no more search results"); + return false; } }, // set off a pagination request. onMessageListFillRequest: function(backwards) { - if (!backwards || this.state.paginating) + if (!backwards) return; // Either wind back the message cap (if there are enough events in the @@ -386,11 +379,13 @@ module.exports = React.createClass({ var cap = Math.min(this.state.messageCap + PAGINATE_SIZE, this.state.room.timeline.length); if (DEBUG_SCROLL) console.log("winding back message cap to", cap); this.setState({messageCap: cap}); + return true; } else if(this.state.room.oldState.paginationToken) { var cap = this.state.messageCap + PAGINATE_SIZE; if (DEBUG_SCROLL) console.log("starting paginate to cap", cap); this.setState({messageCap: cap, paginating: true}); - MatrixClientPeg.get().scrollback(this.state.room, PAGINATE_SIZE).finally(this._paginateCompleted).done(); + return MatrixClientPeg.get().scrollback(this.state.room, PAGINATE_SIZE). + finally(this._paginateCompleted).then(true); } }, @@ -515,8 +510,8 @@ module.exports = React.createClass({ var self = this; if (DEBUG_SCROLL) console.log("sending search request"); - MatrixClientPeg.get().search({ body: this._getSearchCondition(term, scope), - next_batch: this.nextSearchBatch }) + return MatrixClientPeg.get().search({ body: this._getSearchCondition(term, scope), + next_batch: this.nextSearchBatch }) .then(function(data) { if (DEBUG_SCROLL) console.log("search complete"); if (!self.state.searching || self.searchId != searchId) { diff --git a/src/components/structures/ScrollPanel.js b/src/components/structures/ScrollPanel.js index 2c68562ada..fc0b630f26 100644 --- a/src/components/structures/ScrollPanel.js +++ b/src/components/structures/ScrollPanel.js @@ -17,6 +17,7 @@ limitations under the License. var React = require("react"); var ReactDOM = require("react-dom"); var GeminiScrollbar = require('react-gemini-scrollbar'); +var q = require("q"); var DEBUG_SCROLL = false; @@ -51,7 +52,15 @@ module.exports = React.createClass({ /* onFillRequest(backwards): a callback which is called on scroll when * the user nears the start (backwards = true) or end (backwards = - * false) of the list + * false) of the list. + * + * This should return true if the pagination was successful, or false if + * there is no more data in this directon (at this time) - which will + * stop the pagination cycle until the user scrolls again. + * + * This can return a promise; if it does, no more calls will be made + * until the promise completes. The promise should resolve to true or + * false as above. */ onFillRequest: React.PropTypes.func, @@ -77,14 +86,22 @@ module.exports = React.createClass({ }, componentWillMount: function() { + this._pendingFillRequests = {b: null, f: null}; this.resetScrollState(); }, + componentDidMount: function() { + this.checkFillState(); + }, + componentDidUpdate: function() { // after adding event tiles, we may need to tweak the scroll (either to // keep at the bottom of the timeline, or to maintain the view after // adding events to the top). this._restoreSavedScrollState(); + + // we also re-check the fill state, in case the paginate was inadequate + this.checkFillState(); }, onScroll: function(ev) { @@ -131,10 +148,52 @@ module.exports = React.createClass({ if (sn.scrollTop < sn.clientHeight) { // there's less than a screenful of messages left - try to get some // more messages. - this.props.onFillRequest(true); + this._maybeFill(true); } }, + // check if there is already a pending fill request. If not, set one off. + _maybeFill: function(backwards) { + var dir = backwards ? 'b' : 'f'; + if (this._pendingFillRequests[dir]) { + if (DEBUG_SCROLL) { + console.log("ScrollPanel: Already a "+dir+" fill in progress - not starting another"); + } + return; + } + + if (DEBUG_SCROLL) { + console.log("ScrollPanel: starting "+dir+" fill"); + } + + // onFillRequest can end up calling us recursively (via onScroll + // events) so make sure we set this before firing off the call. That + // does present the risk that we might not ever actually fire off the + // fill request, so wrap it in a try/catch. + this._pendingFillRequests[dir] = true; + var r; + try { + r = this.props.onFillRequest(backwards); + } catch (e) { + this._pendingFillRequests[dir] = false; + throw e; + } + + q.finally(r, () => { + if (DEBUG_SCROLL) { + console.log("ScrollPanel: "+dir+" fill complete"); + } + this._pendingFillRequests[dir] = false; + }).then((res) => { + if (res) { + // further pagination requests have been disabled until now, so + // it's time to check the fill state again in case the pagination + // was insufficient. + this.checkFillState(); + } + }).done(); + }, + // get the current scroll position of the room, so that it can be // restored later getScrollState: function() { From 93e7f90ae4f286394391857175803d3ed4c73012 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 24 Dec 2015 00:08:17 +0000 Subject: [PATCH 2/3] ScrollPanel: implement forward-fill --- src/components/structures/ScrollPanel.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/components/structures/ScrollPanel.js b/src/components/structures/ScrollPanel.js index fc0b630f26..80d35aea65 100644 --- a/src/components/structures/ScrollPanel.js +++ b/src/components/structures/ScrollPanel.js @@ -150,6 +150,9 @@ module.exports = React.createClass({ // more messages. this._maybeFill(true); } + if (sn.scrollTop > sn.scrollHeight - sn.clientHeight * 2) { + this._maybeFill(false); + } }, // check if there is already a pending fill request. If not, set one off. From b5eae891b4f148f9988d7524f97455420bd9aa77 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 4 Jan 2016 16:28:32 +0000 Subject: [PATCH 3/3] Address review comments Make onFillRequest always return a promise --- src/components/structures/RoomView.js | 33 +++++---- src/components/structures/ScrollPanel.js | 88 +++++++++++++++--------- 2 files changed, 77 insertions(+), 44 deletions(-) diff --git a/src/components/structures/RoomView.js b/src/components/structures/RoomView.js index fc909f69ab..150c39fb70 100644 --- a/src/components/structures/RoomView.js +++ b/src/components/structures/RoomView.js @@ -43,6 +43,13 @@ var INITIAL_SIZE = 20; var DEBUG_SCROLL = false; +if (DEBUG_SCROLL) { + // using bind means that we get to keep useful line numbers in the console + var debuglog = console.log.bind(console); +} else { + var debuglog = function () {}; +} + module.exports = React.createClass({ displayName: 'RoomView', propTypes: { @@ -344,7 +351,7 @@ module.exports = React.createClass({ }, _paginateCompleted: function() { - if (DEBUG_SCROLL) console.log("paginate complete"); + debuglog("paginate complete"); this.setState({ room: MatrixClientPeg.get().getRoom(this.props.roomId) @@ -355,34 +362,34 @@ module.exports = React.createClass({ onSearchResultsFillRequest: function(backwards) { if (!backwards) - return false; + return q(false); if (this.nextSearchBatch) { - if (DEBUG_SCROLL) console.log("requesting more search results"); + debuglog("requesting more search results"); return this._getSearchBatch(this.state.searchTerm, this.state.searchScope).then(true); } else { - if (DEBUG_SCROLL) console.log("no more search results"); - return false; + debuglog("no more search results"); + return q(false); } }, // set off a pagination request. onMessageListFillRequest: function(backwards) { if (!backwards) - return; + return q(false); // Either wind back the message cap (if there are enough events in the // timeline to do so), or fire off a pagination request. if (this.state.messageCap < this.state.room.timeline.length) { var cap = Math.min(this.state.messageCap + PAGINATE_SIZE, this.state.room.timeline.length); - if (DEBUG_SCROLL) console.log("winding back message cap to", cap); + debuglog("winding back message cap to", cap); this.setState({messageCap: cap}); - return true; + return q(true); } else if(this.state.room.oldState.paginationToken) { var cap = this.state.messageCap + PAGINATE_SIZE; - if (DEBUG_SCROLL) console.log("starting paginate to cap", cap); + debuglog("starting paginate to cap", cap); this.setState({messageCap: cap, paginating: true}); return MatrixClientPeg.get().scrollback(this.state.room, PAGINATE_SIZE). finally(this._paginateCompleted).then(true); @@ -492,7 +499,7 @@ module.exports = React.createClass({ } this.nextSearchBatch = null; - this._getSearchBatch(term, scope); + this._getSearchBatch(term, scope).done(); }, // fire off a request for a batch of search results @@ -509,11 +516,11 @@ module.exports = React.createClass({ var self = this; - if (DEBUG_SCROLL) console.log("sending search request"); + debuglog("sending search request"); return MatrixClientPeg.get().search({ body: this._getSearchCondition(term, scope), next_batch: this.nextSearchBatch }) .then(function(data) { - if (DEBUG_SCROLL) console.log("search complete"); + debuglog("search complete"); if (!self.state.searching || self.searchId != searchId) { console.error("Discarding stale search results"); return; @@ -559,7 +566,7 @@ module.exports = React.createClass({ self.setState({ searchInProgress: false }); - }).done(); + }); }, _getSearchCondition: function(term, scope) { diff --git a/src/components/structures/ScrollPanel.js b/src/components/structures/ScrollPanel.js index 80d35aea65..072d397d10 100644 --- a/src/components/structures/ScrollPanel.js +++ b/src/components/structures/ScrollPanel.js @@ -21,6 +21,13 @@ var q = require("q"); var DEBUG_SCROLL = false; +if (DEBUG_SCROLL) { + // using bind means that we get to keep useful line numbers in the console + var debuglog = console.log.bind(console); +} else { + var debuglog = function () {}; +} + /* This component implements an intelligent scrolling list. * * It wraps a list of
  • children; when items are added to the start or end @@ -54,13 +61,14 @@ module.exports = React.createClass({ * the user nears the start (backwards = true) or end (backwards = * false) of the list. * - * This should return true if the pagination was successful, or false if - * there is no more data in this directon (at this time) - which will - * stop the pagination cycle until the user scrolls again. + * This should return a promise; no more calls will be made until the + * promise completes. * - * This can return a promise; if it does, no more calls will be made - * until the promise completes. The promise should resolve to true or - * false as above. + * The promise should resolve to true if there is more data to be + * retrieved in this direction (in which case onFillRequest may be + * called again immediately), or false if there is no more data in this + * directon (at this time) - which will stop the pagination cycle until + * the user scrolls again. */ onFillRequest: React.PropTypes.func, @@ -80,7 +88,7 @@ module.exports = React.createClass({ getDefaultProps: function() { return { stickyBottom: true, - onFillRequest: function(backwards) {}, + onFillRequest: function(backwards) { return q(false); }, onScroll: function() {}, }; }, @@ -106,7 +114,7 @@ module.exports = React.createClass({ onScroll: function(ev) { var sn = this._getScrollNode(); - if (DEBUG_SCROLL) console.log("Scroll event: offset now:", sn.scrollTop, "recentEventScroll:", this.recentEventScroll); + debuglog("Scroll event: offset now:", sn.scrollTop, "recentEventScroll:", this.recentEventScroll); // Sometimes we see attempts to write to scrollTop essentially being // ignored. (Or rather, it is successfully written, but on the next @@ -130,7 +138,7 @@ module.exports = React.createClass({ } this.scrollState = this._calculateScrollState(); - if (DEBUG_SCROLL) console.log("Saved scroll state", this.scrollState); + debuglog("Saved scroll state", this.scrollState); this.props.onScroll(ev); @@ -145,12 +153,36 @@ module.exports = React.createClass({ checkFillState: function() { var sn = this._getScrollNode(); + // if there is less than a screenful of messages above or below the + // viewport, try to get some more messages. + // + // scrollTop is the number of pixels between the top of the content and + // the top of the viewport. + // + // scrollHeight is the total height of the content. + // + // clientHeight is the height of the viewport (excluding borders, + // margins, and scrollbars). + // + // + // .---------. - - + // | | | scrollTop | + // .-+---------+-. - - | + // | | | | | | + // | | | | | clientHeight | scrollHeight + // | | | | | | + // `-+---------+-' - | + // | | | + // | | | + // `---------' - + // + if (sn.scrollTop < sn.clientHeight) { - // there's less than a screenful of messages left - try to get some - // more messages. + // need to back-fill this._maybeFill(true); } if (sn.scrollTop > sn.scrollHeight - sn.clientHeight * 2) { + // need to forward-fill this._maybeFill(false); } }, @@ -159,36 +191,30 @@ module.exports = React.createClass({ _maybeFill: function(backwards) { var dir = backwards ? 'b' : 'f'; if (this._pendingFillRequests[dir]) { - if (DEBUG_SCROLL) { - console.log("ScrollPanel: Already a "+dir+" fill in progress - not starting another"); - } + debuglog("ScrollPanel: Already a "+dir+" fill in progress - not starting another"); return; } - if (DEBUG_SCROLL) { - console.log("ScrollPanel: starting "+dir+" fill"); - } + debuglog("ScrollPanel: starting "+dir+" fill"); // onFillRequest can end up calling us recursively (via onScroll // events) so make sure we set this before firing off the call. That // does present the risk that we might not ever actually fire off the // fill request, so wrap it in a try/catch. this._pendingFillRequests[dir] = true; - var r; + var fillPromise; try { - r = this.props.onFillRequest(backwards); + fillPromise = this.props.onFillRequest(backwards); } catch (e) { this._pendingFillRequests[dir] = false; throw e; } - q.finally(r, () => { - if (DEBUG_SCROLL) { - console.log("ScrollPanel: "+dir+" fill complete"); - } + q.finally(fillPromise, () => { + debuglog("ScrollPanel: "+dir+" fill complete"); this._pendingFillRequests[dir] = false; - }).then((res) => { - if (res) { + }).then((hasMoreResults) => { + if (hasMoreResults) { // further pagination requests have been disabled until now, so // it's time to check the fill state again in case the pagination // was insufficient. @@ -218,13 +244,13 @@ module.exports = React.createClass({ scrollToTop: function() { this._getScrollNode().scrollTop = 0; - if (DEBUG_SCROLL) console.log("Scrolled to top"); + debuglog("Scrolled to top"); }, scrollToBottom: function() { var scrollNode = this._getScrollNode(); scrollNode.scrollTop = scrollNode.scrollHeight; - if (DEBUG_SCROLL) console.log("Scrolled to bottom; offset now", scrollNode.scrollTop); + debuglog("Scrolled to bottom; offset now", scrollNode.scrollTop); }, // scroll the message list to the node with the given scrollToken. See @@ -261,10 +287,10 @@ module.exports = React.createClass({ this.recentEventScroll = scrollNode.scrollTop; } - if (DEBUG_SCROLL) { - console.log("Scrolled to token", node.dataset.scrollToken, "+", pixelOffset+":", scrollNode.scrollTop, "(delta: "+scrollDelta+")"); - console.log("recentEventScroll now "+this.recentEventScroll); - } + debuglog("Scrolled to token", node.dataset.scrollToken, "+", + pixelOffset+":", scrollNode.scrollTop, + "(delta: "+scrollDelta+")"); + debuglog("recentEventScroll now "+this.recentEventScroll); }, _calculateScrollState: function() {