From b33a47e2534108f89d9de8fdecfce522113ec45d Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 19 Jul 2017 11:49:20 +0100 Subject: [PATCH 01/11] Fix member events breaking on timeline reset, 2 Re-use the same RoomState from the old live timeline so we re-use all the same member objects etc, so all the listeners stay attached --- src/client.js | 2 +- src/models/event-timeline-set.js | 43 +++++++++++++++++++++----------- src/models/room.js | 8 +++--- src/sync.js | 6 +---- 4 files changed, 35 insertions(+), 24 deletions(-) diff --git a/src/client.js b/src/client.js index e35f331f4..d206f6a40 100644 --- a/src/client.js +++ b/src/client.js @@ -2089,7 +2089,7 @@ MatrixClient.prototype.resetNotifTimelineSet = function() { // know about /notifications, so we have no choice but to start paginating // from the current point in time. This may well overlap with historical // notifs which are then inserted into the timeline by /sync responses. - this._notifTimelineSet.resetLiveTimeline('end', true); + this._notifTimelineSet.resetLiveTimeline('end', null); // we could try to paginate a single event at this point in order to get // a more valid pagination token, but it just ends up with an out of order diff --git a/src/models/event-timeline-set.js b/src/models/event-timeline-set.js index 4b46eccf5..16ad21d72 100644 --- a/src/models/event-timeline-set.js +++ b/src/models/event-timeline-set.js @@ -149,13 +149,14 @@ EventTimelineSet.prototype.replaceEventId = function(oldEventId, newEventId) { *

This is used when /sync returns a 'limited' timeline. * * @param {string=} backPaginationToken token for back-paginating the new timeline - * @param {?bool} flush Whether to flush the non-live timelines too. + * @param {string=} forwardPaginationToken token for forward-paginating the old live timeline, + * if absent or null, all timelines are reset. * * @fires module:client~MatrixClient#event:"Room.timelineReset" */ -EventTimelineSet.prototype.resetLiveTimeline = function(backPaginationToken, flush) { +EventTimelineSet.prototype.resetLiveTimeline = function(backPaginationToken, forwardPaginationToken) { // if timeline support is disabled, forget about the old timelines - const resetAllTimelines = !this._timelineSupport || flush; + const resetAllTimelines = !this._timelineSupport || !forwardPaginationToken; let newTimeline; if (resetAllTimelines) { @@ -166,21 +167,35 @@ EventTimelineSet.prototype.resetLiveTimeline = function(backPaginationToken, flu newTimeline = this.addTimeline(); } - // initialise the state in the new timeline from our last known state - const evMap = this._liveTimeline.getState(EventTimeline.FORWARDS).events; - const events = []; - for (const evtype in evMap) { - if (!evMap.hasOwnProperty(evtype)) { - continue; - } - for (const stateKey in evMap[evtype]) { - if (!evMap[evtype].hasOwnProperty(stateKey)) { + // move the state objects over from the old live timeline, then we'll + // keep using the same RoomMember objects for the 'live' set of members + newTimeline._startState = this._liveTimeline._startState; + newTimeline._endState = this._liveTimeline._endState; + + if (!resetAllTimelines) { + // Now regenerate the state for the previously-live timeline, because + // we just stole it and put it on the new live timeline + // (If we're resetting all timelines, don't bother because the old live + // timeline is about to be thrown away anyway. + this._liveTimeline._startState = new RoomState(this._roomId); + this._liveTimeline._endState = new RoomState(this._roomId); + const evMap = this._liveTimeline.getState(EventTimeline.FORWARDS).events; + const events = []; + for (const evtype in evMap) { + if (!evMap.hasOwnProperty(evtype)) { continue; } - events.push(evMap[evtype][stateKey]); + for (const stateKey in evMap[evtype]) { + if (!evMap[evtype].hasOwnProperty(stateKey)) { + continue; + } + events.push(evMap[evtype][stateKey]); + } } + this._liveTimeline.initialiseState(events); + + this._liveTimeline.setPaginationToken(forwardPaginationToken, EventTimeline.FORWARDS); } - newTimeline.initialiseState(events); // make sure we set the pagination token before firing timelineReset, // otherwise clients which start back-paginating will fail, and then get diff --git a/src/models/room.js b/src/models/room.js index a1f88cd7c..6c47dd24c 100644 --- a/src/models/room.js +++ b/src/models/room.js @@ -205,12 +205,12 @@ Room.prototype.getLiveTimeline = function() { *

This is used when /sync returns a 'limited' timeline. * * @param {string=} backPaginationToken token for back-paginating the new timeline - * @param {boolean=} flush True to remove all events in all timelines. If false, only - * the live timeline is reset. + * @param {string=} forwardPaginationToken token for forward-paginating the old live timeline, + * if absent or null, all timelines are reset. */ -Room.prototype.resetLiveTimeline = function(backPaginationToken, flush) { +Room.prototype.resetLiveTimeline = function(backPaginationToken, forwardPaginationToken) { for (let i = 0; i < this._timelineSets.length; i++) { - this._timelineSets[i].resetLiveTimeline(backPaginationToken, flush); + this._timelineSets[i].resetLiveTimeline(backPaginationToken, forwardPaginationToken); } this._fixUpLegacyTimelineFields(); diff --git a/src/sync.js b/src/sync.js index 284b967ef..54536fc6c 100644 --- a/src/sync.js +++ b/src/sync.js @@ -884,14 +884,10 @@ SyncApi.prototype._processSyncResponse = function(syncToken, data) { } if (limited) { - // save the old 'next_batch' token as the - // forward-pagination token for the previously-active - // timeline. - room.currentState.paginationToken = syncToken; self._deregisterStateListeners(room); room.resetLiveTimeline( joinObj.timeline.prev_batch, - self.opts.canResetEntireTimeline(room.roomId), + self.opts.canResetEntireTimeline(room.roomId) ? null : syncToken, ); // We have to assume any gap in any timeline is From f4b25b59e57c94fed6af37ec287d4a07fd5a9a9d Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 19 Jul 2017 11:56:58 +0100 Subject: [PATCH 02/11] Lint --- src/models/event-timeline-set.js | 9 +++++++-- src/models/room.js | 4 +++- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/src/models/event-timeline-set.js b/src/models/event-timeline-set.js index 16ad21d72..ad58f23dc 100644 --- a/src/models/event-timeline-set.js +++ b/src/models/event-timeline-set.js @@ -20,6 +20,7 @@ limitations under the License. const EventEmitter = require("events").EventEmitter; const utils = require("../utils"); const EventTimeline = require("./event-timeline"); +const RoomState = require("./room-state"); // var DEBUG = false; const DEBUG = true; @@ -154,7 +155,9 @@ EventTimelineSet.prototype.replaceEventId = function(oldEventId, newEventId) { * * @fires module:client~MatrixClient#event:"Room.timelineReset" */ -EventTimelineSet.prototype.resetLiveTimeline = function(backPaginationToken, forwardPaginationToken) { +EventTimelineSet.prototype.resetLiveTimeline = function( + backPaginationToken, forwardPaginationToken, +) { // if timeline support is disabled, forget about the old timelines const resetAllTimelines = !this._timelineSupport || !forwardPaginationToken; @@ -194,7 +197,9 @@ EventTimelineSet.prototype.resetLiveTimeline = function(backPaginationToken, for } this._liveTimeline.initialiseState(events); - this._liveTimeline.setPaginationToken(forwardPaginationToken, EventTimeline.FORWARDS); + this._liveTimeline.setPaginationToken( + forwardPaginationToken, EventTimeline.FORWARDS, + ); } // make sure we set the pagination token before firing timelineReset, diff --git a/src/models/room.js b/src/models/room.js index 6c47dd24c..e3623ad3a 100644 --- a/src/models/room.js +++ b/src/models/room.js @@ -210,7 +210,9 @@ Room.prototype.getLiveTimeline = function() { */ Room.prototype.resetLiveTimeline = function(backPaginationToken, forwardPaginationToken) { for (let i = 0; i < this._timelineSets.length; i++) { - this._timelineSets[i].resetLiveTimeline(backPaginationToken, forwardPaginationToken); + this._timelineSets[i].resetLiveTimeline( + backPaginationToken, forwardPaginationToken, + ); } this._fixUpLegacyTimelineFields(); From 1ce4977a70a9993ed6720534d663c59ce0e808e1 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 19 Jul 2017 12:00:04 +0100 Subject: [PATCH 03/11] get state events before we nuke the roomstate --- src/models/event-timeline-set.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/models/event-timeline-set.js b/src/models/event-timeline-set.js index ad58f23dc..db4659eff 100644 --- a/src/models/event-timeline-set.js +++ b/src/models/event-timeline-set.js @@ -180,8 +180,6 @@ EventTimelineSet.prototype.resetLiveTimeline = function( // we just stole it and put it on the new live timeline // (If we're resetting all timelines, don't bother because the old live // timeline is about to be thrown away anyway. - this._liveTimeline._startState = new RoomState(this._roomId); - this._liveTimeline._endState = new RoomState(this._roomId); const evMap = this._liveTimeline.getState(EventTimeline.FORWARDS).events; const events = []; for (const evtype in evMap) { @@ -195,6 +193,8 @@ EventTimelineSet.prototype.resetLiveTimeline = function( events.push(evMap[evtype][stateKey]); } } + this._liveTimeline._startState = new RoomState(this._roomId); + this._liveTimeline._endState = new RoomState(this._roomId); this._liveTimeline.initialiseState(events); this._liveTimeline.setPaginationToken( From f91293c6c551e3d2b1e8715af0b0671abf105ea2 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 19 Jul 2017 14:52:27 +0100 Subject: [PATCH 04/11] Set the start state of the new timeline correctly --- src/models/event-timeline-set.js | 47 ++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 21 deletions(-) diff --git a/src/models/event-timeline-set.js b/src/models/event-timeline-set.js index db4659eff..7ab4eab06 100644 --- a/src/models/event-timeline-set.js +++ b/src/models/event-timeline-set.js @@ -170,32 +170,37 @@ EventTimelineSet.prototype.resetLiveTimeline = function( newTimeline = this.addTimeline(); } - // move the state objects over from the old live timeline, then we'll - // keep using the same RoomMember objects for the 'live' set of members - newTimeline._startState = this._liveTimeline._startState; + // Collect the state events from the old timeline + const evMap = this._liveTimeline.getState(EventTimeline.FORWARDS).events; + const events = []; + for (const evtype in evMap) { + if (!evMap.hasOwnProperty(evtype)) { + continue; + } + for (const stateKey in evMap[evtype]) { + if (!evMap[evtype].hasOwnProperty(stateKey)) { + continue; + } + events.push(evMap[evtype][stateKey]); + } + } + + // Use those events to initialise the state of the new live timeline + newTimeline.initialiseState(events); + + const freshEndState = newTimeline._endState; + // Now clobber the end state with that from the previous live timeline. + // It will be identical except that we'll keep using the same RoomMember + // objects for the 'live' set of members with any listeners still attached newTimeline._endState = this._liveTimeline._endState; if (!resetAllTimelines) { - // Now regenerate the state for the previously-live timeline, because - // we just stole it and put it on the new live timeline + // We just stole the old timeline's end state, so it needs a new one. + // Just swap them around and give it the one we just generated for the + // new live timeline. // (If we're resetting all timelines, don't bother because the old live // timeline is about to be thrown away anyway. - const evMap = this._liveTimeline.getState(EventTimeline.FORWARDS).events; - const events = []; - for (const evtype in evMap) { - if (!evMap.hasOwnProperty(evtype)) { - continue; - } - for (const stateKey in evMap[evtype]) { - if (!evMap[evtype].hasOwnProperty(stateKey)) { - continue; - } - events.push(evMap[evtype][stateKey]); - } - } - this._liveTimeline._startState = new RoomState(this._roomId); - this._liveTimeline._endState = new RoomState(this._roomId); - this._liveTimeline.initialiseState(events); + this._liveTimeline._endState = freshEndState; this._liveTimeline.setPaginationToken( forwardPaginationToken, EventTimeline.FORWARDS, From 342f5c01e058666aa3400ef50f36511f5d32ee3a Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 19 Jul 2017 14:54:18 +0100 Subject: [PATCH 05/11] Update tests for new resetLiveTimeline interface --- spec/unit/room.spec.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/unit/room.spec.js b/spec/unit/room.spec.js index 887e60e1b..d6aad8229 100644 --- a/spec/unit/room.spec.js +++ b/spec/unit/room.spec.js @@ -404,7 +404,7 @@ describe("Room", function() { it("should copy state from previous timeline", function() { room.addLiveEvents([events[0], events[1]]); expect(room.getLiveTimeline().getEvents().length).toEqual(2); - room.resetLiveTimeline(); + room.resetLiveTimeline('sometoken', 'someothertoken'); room.addLiveEvents([events[2]]); const oldState = room.getLiveTimeline().getState(EventTimeline.BACKWARDS); @@ -417,7 +417,7 @@ describe("Room", function() { it("should reset the legacy timeline fields", function() { room.addLiveEvents([events[0], events[1]]); expect(room.timeline.length).toEqual(2); - room.resetLiveTimeline(); + room.resetLiveTimeline('sometoken', 'someothertoken'); room.addLiveEvents([events[2]]); const newLiveTimeline = room.getLiveTimeline(); @@ -451,7 +451,7 @@ describe("Room", function() { room.addLiveEvents([events[0]]); expect(room.timeline.length).toEqual(1); const firstLiveTimeline = room.getLiveTimeline(); - room.resetLiveTimeline(); + room.resetLiveTimeline('sometoken', 'someothertoken'); const tl = room.getTimelineForEvent(events[0].getId()); expect(tl).toBe(timelineSupport ? firstLiveTimeline : null); From 39d694de8c1292ada7bf8eec3d16f45cac376228 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 19 Jul 2017 14:58:18 +0100 Subject: [PATCH 06/11] No longer need RoomState --- src/models/event-timeline-set.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/models/event-timeline-set.js b/src/models/event-timeline-set.js index 7ab4eab06..87d19bbf3 100644 --- a/src/models/event-timeline-set.js +++ b/src/models/event-timeline-set.js @@ -20,7 +20,6 @@ limitations under the License. const EventEmitter = require("events").EventEmitter; const utils = require("../utils"); const EventTimeline = require("./event-timeline"); -const RoomState = require("./room-state"); // var DEBUG = false; const DEBUG = true; From 5e4cd6cf1196c8a633fb198b1b9d56a540d8d0ca Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 19 Jul 2017 16:20:40 +0100 Subject: [PATCH 07/11] Add hopefully clearer comments --- src/models/event-timeline-set.js | 5 ++--- src/models/room.js | 4 +++- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/models/event-timeline-set.js b/src/models/event-timeline-set.js index 87d19bbf3..90ab4cd70 100644 --- a/src/models/event-timeline-set.js +++ b/src/models/event-timeline-set.js @@ -193,12 +193,11 @@ EventTimelineSet.prototype.resetLiveTimeline = function( // objects for the 'live' set of members with any listeners still attached newTimeline._endState = this._liveTimeline._endState; + // If we're not resetting all timelines, we need to fix up the old live timeline if (!resetAllTimelines) { - // We just stole the old timeline's end state, so it needs a new one. + // Firstly, we just stole the old timeline's end state, so it needs a new one. // Just swap them around and give it the one we just generated for the // new live timeline. - // (If we're resetting all timelines, don't bother because the old live - // timeline is about to be thrown away anyway. this._liveTimeline._endState = freshEndState; this._liveTimeline.setPaginationToken( diff --git a/src/models/room.js b/src/models/room.js index e3623ad3a..54823840b 100644 --- a/src/models/room.js +++ b/src/models/room.js @@ -206,7 +206,9 @@ Room.prototype.getLiveTimeline = function() { * * @param {string=} backPaginationToken token for back-paginating the new timeline * @param {string=} forwardPaginationToken token for forward-paginating the old live timeline, - * if absent or null, all timelines are reset. + * if absent or null, all timelines are reset, removing old ones (including the previous live + * timeline which would otherwise be unable to paginate forwards without this token). + * Removing just the old live timeline whilst preserving previous ones is not supported. */ Room.prototype.resetLiveTimeline = function(backPaginationToken, forwardPaginationToken) { for (let i = 0; i < this._timelineSets.length; i++) { From 8ac15068ee7a601d1531dd745b7a9941d5dd0c9f Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 19 Jul 2017 16:24:42 +0100 Subject: [PATCH 08/11] more comments --- src/models/event-timeline-set.js | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/models/event-timeline-set.js b/src/models/event-timeline-set.js index 90ab4cd70..95b76d9cf 100644 --- a/src/models/event-timeline-set.js +++ b/src/models/event-timeline-set.js @@ -188,9 +188,10 @@ EventTimelineSet.prototype.resetLiveTimeline = function( newTimeline.initialiseState(events); const freshEndState = newTimeline._endState; - // Now clobber the end state with that from the previous live timeline. - // It will be identical except that we'll keep using the same RoomMember - // objects for the 'live' set of members with any listeners still attached + // Now clobber the end state of the new live timeline with that from the + // previous live timeline. It will be identical except that we'll keep + // using the same RoomMember objects for the 'live' set of members with any + // listeners still attached newTimeline._endState = this._liveTimeline._endState; // If we're not resetting all timelines, we need to fix up the old live timeline @@ -200,6 +201,8 @@ EventTimelineSet.prototype.resetLiveTimeline = function( // new live timeline. this._liveTimeline._endState = freshEndState; + // Now set the forward pagination token on the old live timeline + // so it can be forward-paginated. this._liveTimeline.setPaginationToken( forwardPaginationToken, EventTimeline.FORWARDS, ); @@ -210,6 +213,7 @@ EventTimelineSet.prototype.resetLiveTimeline = function( // stuck without realising that they *can* back-paginate. newTimeline.setPaginationToken(backPaginationToken, EventTimeline.BACKWARDS); + // Now we can swap the live timeline to the new one. this._liveTimeline = newTimeline; this.emit("Room.timelineReset", this.room, this, resetAllTimelines); }; From 2999603b28a2e505b48a26fd3461eff15ae984c9 Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 20 Jul 2017 10:43:47 +0100 Subject: [PATCH 09/11] more commentage --- src/models/event-timeline-set.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/models/event-timeline-set.js b/src/models/event-timeline-set.js index 95b76d9cf..571305337 100644 --- a/src/models/event-timeline-set.js +++ b/src/models/event-timeline-set.js @@ -157,6 +157,14 @@ EventTimelineSet.prototype.replaceEventId = function(oldEventId, newEventId) { EventTimelineSet.prototype.resetLiveTimeline = function( backPaginationToken, forwardPaginationToken, ) { + // Each EventTimeline has RoomState objects tracking the state at the start + // and end of that timeline. The copies at the end of the live timeline are + // special because they will have listeners attached to monitor changes to + // the current room state, so we move this RoomState from the end of the + // current live timeline to the end of the new one and, if necessary, + // replace it with the fresh one created for the new timeline. We also make + // a copy for the start of the new timeline. + // if timeline support is disabled, forget about the old timelines const resetAllTimelines = !this._timelineSupport || !forwardPaginationToken; From ff685e33d5d532b91e9b1d34c0f052b2dbcc6b8c Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 20 Jul 2017 11:00:50 +0100 Subject: [PATCH 10/11] clarify comment --- src/models/event-timeline-set.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/models/event-timeline-set.js b/src/models/event-timeline-set.js index 571305337..6de61cf62 100644 --- a/src/models/event-timeline-set.js +++ b/src/models/event-timeline-set.js @@ -162,8 +162,8 @@ EventTimelineSet.prototype.resetLiveTimeline = function( // special because they will have listeners attached to monitor changes to // the current room state, so we move this RoomState from the end of the // current live timeline to the end of the new one and, if necessary, - // replace it with the fresh one created for the new timeline. We also make - // a copy for the start of the new timeline. + // replace it with a newly created one. We also make a copy for the start + // of the new timeline. // if timeline support is disabled, forget about the old timelines const resetAllTimelines = !this._timelineSupport || !forwardPaginationToken; From 34adaae5af110392c37410f9548feac74ce176db Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 20 Jul 2017 14:17:14 +0100 Subject: [PATCH 11/11] Add helpfully named variable for old timeline --- src/models/event-timeline-set.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/models/event-timeline-set.js b/src/models/event-timeline-set.js index 6de61cf62..2777ea313 100644 --- a/src/models/event-timeline-set.js +++ b/src/models/event-timeline-set.js @@ -177,8 +177,10 @@ EventTimelineSet.prototype.resetLiveTimeline = function( newTimeline = this.addTimeline(); } + const oldTimeline = this._liveTimeline; + // Collect the state events from the old timeline - const evMap = this._liveTimeline.getState(EventTimeline.FORWARDS).events; + const evMap = oldTimeline.getState(EventTimeline.FORWARDS).events; const events = []; for (const evtype in evMap) { if (!evMap.hasOwnProperty(evtype)) { @@ -200,18 +202,18 @@ EventTimelineSet.prototype.resetLiveTimeline = function( // previous live timeline. It will be identical except that we'll keep // using the same RoomMember objects for the 'live' set of members with any // listeners still attached - newTimeline._endState = this._liveTimeline._endState; + newTimeline._endState = oldTimeline._endState; // If we're not resetting all timelines, we need to fix up the old live timeline if (!resetAllTimelines) { // Firstly, we just stole the old timeline's end state, so it needs a new one. // Just swap them around and give it the one we just generated for the // new live timeline. - this._liveTimeline._endState = freshEndState; + oldTimeline._endState = freshEndState; // Now set the forward pagination token on the old live timeline // so it can be forward-paginated. - this._liveTimeline.setPaginationToken( + oldTimeline.setPaginationToken( forwardPaginationToken, EventTimeline.FORWARDS, ); }