Fix infinite loop when there are a lot of invisible events (#554)
Instead of using a window of a fixed number of events, unpaginate based on the distance of the viewport from the end of the scroll range. The ScrollPanel uses the scrollTokens to convey to its parent (the TimelinePanel, in this case) the point to unpaginate up to. The TimelinePanel then takes a chunk of events off the front or back of `this.state.events` using `timelineWindow.unpaginate`. Fixes https://github.com/vector-im/vector-web/issues/2020
This commit is contained in:
Luke Barnard
Richard van der Hoff
parent
6ccc825f0d
commit
b718f1542c
4 changed files with 137 additions and 6 deletions
|
|
@ -321,8 +321,11 @@ module.exports = React.createClass({
|
|||
}
|
||||
|
||||
ret.push(
|
||||
<MemberEventListSummary key={mxEv.getId()} events={summarisedEvents}>
|
||||
{eventTiles}
|
||||
<MemberEventListSummary
|
||||
key={mxEv.getId()}
|
||||
events={summarisedEvents}
|
||||
data-scroll-token={eventId}>
|
||||
{eventTiles}
|
||||
</MemberEventListSummary>
|
||||
);
|
||||
continue;
|
||||
|
|
@ -564,6 +567,7 @@ module.exports = React.createClass({
|
|||
onScroll={ this.props.onScroll }
|
||||
onResize={ this.onResize }
|
||||
onFillRequest={ this.props.onFillRequest }
|
||||
onUnfillRequest={ this.props.onUnfillRequest }
|
||||
style={ style }
|
||||
stickyBottom={ this.props.stickyBottom }>
|
||||
{topSpinner}
|
||||
|
|
|
|||
|
|
@ -23,6 +23,10 @@ var KeyCode = require('../../KeyCode');
|
|||
var DEBUG_SCROLL = false;
|
||||
// var DEBUG_SCROLL = true;
|
||||
|
||||
// The amount of extra scroll distance to allow prior to unfilling.
|
||||
// See _getExcessHeight.
|
||||
const UNPAGINATION_PADDING = 500;
|
||||
|
||||
if (DEBUG_SCROLL) {
|
||||
// using bind means that we get to keep useful line numbers in the console
|
||||
var debuglog = console.log.bind(console);
|
||||
|
|
@ -101,6 +105,17 @@ module.exports = React.createClass({
|
|||
*/
|
||||
onFillRequest: React.PropTypes.func,
|
||||
|
||||
/* onUnfillRequest(backwards): a callback which is called on scroll when
|
||||
* there are children elements that are far out of view and could be removed
|
||||
* without causing pagination to occur.
|
||||
*
|
||||
* This function should accept a boolean, which is true to indicate the back/top
|
||||
* of the panel and false otherwise, and a scroll token, which refers to the
|
||||
* first element to remove if removing from the front/bottom, and last element
|
||||
* to remove if removing from the back/top.
|
||||
*/
|
||||
onUnfillRequest: React.PropTypes.func,
|
||||
|
||||
/* onScroll: a callback which is called whenever any scroll happens.
|
||||
*/
|
||||
onScroll: React.PropTypes.func,
|
||||
|
|
@ -124,6 +139,7 @@ module.exports = React.createClass({
|
|||
stickyBottom: true,
|
||||
startAtBottom: true,
|
||||
onFillRequest: function(backwards) { return q(false); },
|
||||
onUnfillRequest: function(backwards, scrollToken) {},
|
||||
onScroll: function() {},
|
||||
};
|
||||
},
|
||||
|
|
@ -226,6 +242,46 @@ module.exports = React.createClass({
|
|||
return sn.scrollHeight - Math.ceil(sn.scrollTop) <= sn.clientHeight + 3;
|
||||
},
|
||||
|
||||
// returns the vertical height in the given direction that can be removed from
|
||||
// the content box (which has a height of scrollHeight, see checkFillState) without
|
||||
// pagination occuring.
|
||||
//
|
||||
// padding* = UNPAGINATION_PADDING
|
||||
//
|
||||
// ### Region determined as excess.
|
||||
//
|
||||
// .---------. - -
|
||||
// |#########| | |
|
||||
// |#########| - | scrollTop |
|
||||
// | | | padding* | |
|
||||
// | | | | |
|
||||
// .-+---------+-. - - | |
|
||||
// : | | : | | |
|
||||
// : | | : | clientHeight | |
|
||||
// : | | : | | |
|
||||
// .-+---------+-. - - |
|
||||
// | | | | | |
|
||||
// | | | | | clientHeight | scrollHeight
|
||||
// | | | | | |
|
||||
// `-+---------+-' - |
|
||||
// : | | : | |
|
||||
// : | | : | clientHeight |
|
||||
// : | | : | |
|
||||
// `-+---------+-' - - |
|
||||
// | | | padding* |
|
||||
// | | | |
|
||||
// |#########| - |
|
||||
// |#########| |
|
||||
// `---------' -
|
||||
_getExcessHeight: function(backwards) {
|
||||
var sn = this._getScrollNode();
|
||||
if (backwards) {
|
||||
return sn.scrollTop - sn.clientHeight - UNPAGINATION_PADDING;
|
||||
} else {
|
||||
return sn.scrollHeight - (sn.scrollTop + 2*sn.clientHeight) - UNPAGINATION_PADDING;
|
||||
}
|
||||
},
|
||||
|
||||
// check the scroll state and send out backfill requests if necessary.
|
||||
checkFillState: function() {
|
||||
if (this.unmounted) {
|
||||
|
|
@ -268,6 +324,47 @@ module.exports = React.createClass({
|
|||
}
|
||||
},
|
||||
|
||||
// check if unfilling is possible and send an unfill request if necessary
|
||||
_checkUnfillState: function(backwards) {
|
||||
let excessHeight = this._getExcessHeight(backwards);
|
||||
if (excessHeight <= 0) {
|
||||
return;
|
||||
}
|
||||
var itemlist = this.refs.itemlist;
|
||||
var tiles = itemlist.children;
|
||||
|
||||
// The scroll token of the first/last tile to be unpaginated
|
||||
let markerScrollToken = null;
|
||||
|
||||
// Subtract clientHeights to simulate the events being unpaginated whilst counting
|
||||
// the events to be unpaginated.
|
||||
if (backwards) {
|
||||
// Iterate forwards from start of tiles, subtracting event tile height
|
||||
let i = 0;
|
||||
while (i < tiles.length && excessHeight > tiles[i].clientHeight) {
|
||||
excessHeight -= tiles[i].clientHeight;
|
||||
if (tiles[i].dataset.scrollToken) {
|
||||
markerScrollToken = tiles[i].dataset.scrollToken;
|
||||
}
|
||||
i++;
|
||||
}
|
||||
} else {
|
||||
// Iterate backwards from end of tiles, subtracting event tile height
|
||||
let i = tiles.length - 1;
|
||||
while (i > 0 && excessHeight > tiles[i].clientHeight) {
|
||||
excessHeight -= tiles[i].clientHeight;
|
||||
if (tiles[i].dataset.scrollToken) {
|
||||
markerScrollToken = tiles[i].dataset.scrollToken;
|
||||
}
|
||||
i--;
|
||||
}
|
||||
}
|
||||
|
||||
if (markerScrollToken) {
|
||||
this.props.onUnfillRequest(backwards, markerScrollToken);
|
||||
}
|
||||
},
|
||||
|
||||
// check if there is already a pending fill request. If not, set one off.
|
||||
_maybeFill: function(backwards) {
|
||||
var dir = backwards ? 'b' : 'f';
|
||||
|
|
@ -285,7 +382,7 @@ module.exports = React.createClass({
|
|||
this._pendingFillRequests[dir] = true;
|
||||
var fillPromise;
|
||||
try {
|
||||
fillPromise = this.props.onFillRequest(backwards);
|
||||
fillPromise = this.props.onFillRequest(backwards);
|
||||
} catch (e) {
|
||||
this._pendingFillRequests[dir] = false;
|
||||
throw e;
|
||||
|
|
@ -294,6 +391,9 @@ module.exports = React.createClass({
|
|||
q.finally(fillPromise, () => {
|
||||
this._pendingFillRequests[dir] = false;
|
||||
}).then((hasMoreResults) => {
|
||||
// Unpaginate once filling is complete
|
||||
this._checkUnfillState(!backwards);
|
||||
|
||||
debuglog("ScrollPanel: "+dir+" fill complete; hasMoreResults:"+hasMoreResults);
|
||||
if (hasMoreResults) {
|
||||
// further pagination requests have been disabled until now, so
|
||||
|
|
|
|||
|
|
@ -108,7 +108,9 @@ var TimelinePanel = React.createClass({
|
|||
|
||||
getDefaultProps: function() {
|
||||
return {
|
||||
timelineCap: 250,
|
||||
// By default, disable the timelineCap in favour of unpaginating based on
|
||||
// event tile heights. (See _unpaginateEvents)
|
||||
timelineCap: Number.MAX_VALUE,
|
||||
className: 'mx_RoomView_messagePanel',
|
||||
};
|
||||
},
|
||||
|
|
@ -245,6 +247,30 @@ var TimelinePanel = React.createClass({
|
|||
}
|
||||
},
|
||||
|
||||
onMessageListUnfillRequest: function(backwards, scrollToken) {
|
||||
let dir = backwards ? EventTimeline.BACKWARDS : EventTimeline.FORWARDS;
|
||||
debuglog("TimelinePanel: unpaginating events in direction", dir);
|
||||
|
||||
// All tiles are inserted by MessagePanel to have a scrollToken === eventId
|
||||
let eventId = scrollToken;
|
||||
|
||||
let marker = this.state.events.findIndex(
|
||||
(ev) => {
|
||||
return ev.getId() === eventId;
|
||||
}
|
||||
);
|
||||
|
||||
let count = backwards ? marker + 1 : this.state.events.length - marker;
|
||||
|
||||
if (count > 0) {
|
||||
debuglog("TimelinePanel: Unpaginating", count, "in direction", dir);
|
||||
this._timelineWindow._unpaginate(count, backwards);
|
||||
this.setState({
|
||||
events: this._getEvents(),
|
||||
});
|
||||
}
|
||||
},
|
||||
|
||||
// set off a pagination request.
|
||||
onMessageListFillRequest: function(backwards) {
|
||||
var dir = backwards ? EventTimeline.BACKWARDS : EventTimeline.FORWARDS;
|
||||
|
|
@ -984,6 +1010,7 @@ var TimelinePanel = React.createClass({
|
|||
stickyBottom={ stickyBottom }
|
||||
onScroll={ this.onMessageListScroll }
|
||||
onFillRequest={ this.onMessageListFillRequest }
|
||||
onUnfillRequest={ this.onMessageListUnfillRequest }
|
||||
opacity={ this.props.opacity }
|
||||
className={ this.props.className }
|
||||
tileShape={ this.props.tileShape }
|
||||
|
|
|
|||
|
|
@ -321,7 +321,7 @@ describe('TimelinePanel', function() {
|
|||
expect(messagePanel.props.suppressFirstDateSeparator).toBe(false);
|
||||
var events = scryEventTiles(panel);
|
||||
expect(events[0].props.mxEvent).toBe(timeline.getEvents()[0]);
|
||||
expect(events.length).toEqual(TIMELINE_CAP);
|
||||
expect(events.length).toBeLessThanOrEqualTo(TIMELINE_CAP);
|
||||
|
||||
// we should now be able to scroll down, and paginate in the other
|
||||
// direction.
|
||||
|
|
@ -339,7 +339,7 @@ describe('TimelinePanel', function() {
|
|||
expect(messagePanel.props.suppressFirstDateSeparator).toBe(true);
|
||||
|
||||
var events = scryEventTiles(panel);
|
||||
expect(events.length).toEqual(TIMELINE_CAP);
|
||||
expect(events.length).toBeLessThanOrEqualTo(TIMELINE_CAP);
|
||||
|
||||
// we don't really know what the first event tile will be, since that
|
||||
// depends on how much the timelinepanel decides to paginate.
|
||||
|
|
|
|||
Loading…
Reference in a new issue