Address a number of review comments.

Make sure that room state is copied correctly when resetting the live
timeline.

Also comments and bits.

lib/models/event-timeline.js

@@ -5,6 +5,8 @@
  */
 
 var RoomState = require("./room-state");
+var utils = require("../utils");
+var MatrixEvent = require("./event").MatrixEvent;
 
 /**
  * Construct a new EventTimeline
@@ -31,7 +33,9 @@ function EventTimeline(roomId) {
     this._events = [];
     this._baseIndex = -1;
     this._startState = new RoomState(roomId);
+    this._startState.paginationToken = null;
     this._endState = new RoomState(roomId);
+    this._endState.paginationToken = null;
 
     this._prevTimeline = null;
     this._nextTimeline = null;
@@ -54,8 +58,14 @@ EventTimeline.prototype.initialiseState = function(stateEvents) {
         throw new Error("Cannot initialise state after events are added");
     }
 
-    // do we need to copy here? sync thinks we do but I can't see why
+    // we deep-copy the events here, in case they get changed later - we don't
-    this._startState.setStateEvents(stateEvents);
+    // want changes to the start state leaking through to the end state.
+    var oldStateEvents = utils.map(
+        utils.deepCopy(
+            stateEvents.map(function(mxEvent) { return mxEvent.event; })
+        ), function(ev) { return new MatrixEvent(ev); });
+
+    this._startState.setStateEvents(oldStateEvents);
     this._endState.setStateEvents(stateEvents);
 };
 
@@ -72,7 +82,9 @@ EventTimeline.prototype.getRoomId = function() {
  *
  * <p>This is an index which is incremented when events are prepended to the
  * timeline. An individual event therefore stays at the same index in the array
- * relative to the base index.
+ * relative to the base index (although note that a given event's index may
+ * well be less than the base index, thus giving that event a negative relative
+ * index).
  *
  * @return {number}
  */

lib/models/room.js

@@ -177,8 +177,20 @@ Room.prototype.resetLiveTimeline = function() {
     }
 
     if (this._liveTimeline) {
+        // we have an existing timeline. This will always be true, except when
+        // this method is called by our own constructor.
+
         // initialise the state in the new timeline from our last known state
-        newTimeline.initialiseState(this._liveTimeline.getState(false).events);
+        var evMap = this._liveTimeline.getState(false).events;
+        var events = [];
+        for (var evtype in evMap) {
+            if (!evMap.hasOwnProperty(evtype)) { continue; }
+            for (var stateKey in evMap[evtype]) {
+                if (!evMap[evtype].hasOwnProperty(stateKey)) { continue; }
+                events.push(evMap[evtype][stateKey]);
+            }
+        }
+        newTimeline.initialiseState(events);
     }
     this._liveTimeline = newTimeline;
 
@@ -196,13 +208,14 @@ Room.prototype.resetLiveTimeline = function() {
  *
  * @param {string} eventId  event ID to look for
  * @return {?module:models/event-timeline~EventTimeline} timeline containing
- * the given event, or undefined if unknown
+ * the given event, or null if unknown
  */
 Room.prototype.getTimelineForEvent = function(eventId) {
-    return this._eventIdToTimeline[eventId];
+    var res = this._eventIdToTimeline[eventId];
+    return (res === undefined) ? null : res;
 };
 
-/*
+/**
  * Get one of the notification counts for this room
  * @param {String} type The type of notification count to get. default: 'total'
  * @return {Number} The notification count, or undefined if there is no count
@@ -364,94 +377,109 @@ Room.prototype.addEventsToTimeline = function(events, toStartOfTimeline,
         return;
     }
 
-    // we need to handle the following case:
+    // Adding events to timelines can be quite complicated. The following
+    // illustrates some of the corner-cases.
     //
-    // We already know about two timelines. One of these contains just
+    // Let's say we start by knowing about four timelines. timeline3 and
-    // event M, and one contains just event P:
+    // timeline4 are neighbours:
     //
-    //    timeline1      timeline2
+    //    timeline1    timeline2    timeline3    timeline4
-    //       [M]            [P]
+    //      [M]          [P]          [S] <------> [T]
     //
-    // We are now processing a new pagination response from the server.
+    // Now we paginate timeline1, and get the following events from the server:
-    // This contains the following events: [M, N, P, R, S].
+    // [M, N, P, R, S, T, U].
     //
     // 1. First, we ignore event M, since we already know about it.
     //
     // 2. Next, we append N to timeline 1.
     //
     // 3. Next, we don't add event P, since we already know about it,
-    //    but we do link togehter the timelines. We now have:
+    //    but we do link together the timelines. We now have:
     //
-    //    timeline1      timeline2
+    //    timeline1    timeline2    timeline3    timeline4
-    //     [M, N] <----> [P]
+    //      [M, N] <---> [P]          [S] <------> [T]
     //
-    // 4. We now need to append R and S to timeline2. So finally, we have:
+    // 4. Now we add event R to timeline2:
     //
-    //    timeline1      timeline2
+    //    timeline1    timeline2    timeline3    timeline4
-    //     [M, N] <----> [P, R, S]
+    //      [M, N] <---> [P, R]       [S] <------> [T]
     //
-    // The important thing to note in the above is that we switched the
+    //    Note that we have switched the timeline we are working on from
-    // timeline we were adding events to, after we found a neighbour.
+    //    timeline1 to timeline2.
-
+    //
-    // XXX: TODO: but timeline2 might already be joined to timeline3: we need
+    // 5. We ignore event S, but again join the timelines:
-    // to skip all the way to the end of the linked list!
+    //
+    //    timeline1    timeline2    timeline3    timeline4
+    //      [M, N] <---> [P, R] <---> [S] <------> [T]
+    //
+    // 6. We ignore event T, and the timelines are already joined, so there
+    //    is nothing to do.
+    //
+    // 7. Finally, we add event U to timeline4:
+    //
+    //    timeline1    timeline2    timeline3    timeline4
+    //      [M, N] <---> [P, R] <---> [S] <------> [T, U]
+    //
+    // The important thing to note in the above is what happened when we
+    // already knew about a given event:
+    //
+    //   - if it was appropriate, we joined up the timelines (steps 3, 5).
+    //   - in any case, we started adding further events to the timeline which
+    //       contained the event we knew about (steps 3, 5, 6).
+    //
+    //
+    // Finally, consider what we want to do with the pagination token. In the
+    // case above, we will be given a pagination token which tells us how to
+    // get events beyond 'U' - in this case, it makes sense to store this
+    // against timeline4. But what if timeline4 already had 'U' and beyond? in
+    // that case, our best bet is to throw away the pagination token we were
+    // given and stick with whatever token timeline4 had previously. In short,
+    // we want to only store the pagination token if the last event we receive
+    // is one we didn't previously know about.
 
     var updateToken = false;
     for (var i = 0; i < events.length; i++) {
-        var existingTimeline = this._checkForNewEventInExistingTimeline(
+        var event = events[i];
-            events[i], timeline, toStartOfTimeline);
-
-        if (existingTimeline) {
-            timeline = existingTimeline;
-            updateToken = false;
-        } else {
-            this._addEventToTimeline(events[i], timeline, toStartOfTimeline);
-            updateToken = true;
-        }
-    }
-    if (updateToken) {
-        timeline.setPaginationToken(paginationToken, toStartOfTimeline);
-    }
-};
-
-/**
- * Check if this event is already in a timeline, and join up the timelines if
- * necessary
- *
- * @param {MatrixEvent} event           event to add
- * @param {EventTimeline} timeline      timeline we think we should add to
- * @param {boolean} toStartOfTimeline   true if we're adding to the start of
- *    the timeline
- * @return {?EventTimeline} the timeline with the event already in, or null if
- *    none
- * @private
- */
-Room.prototype._checkForNewEventInExistingTimeline = function(
-           event, timeline, toStartOfTimeline) {
         var eventId = event.getId();
 
         var existingTimeline = this._eventIdToTimeline[eventId];
+
         if (!existingTimeline) {
-        return null;
+            // we don't know about this event yet. Just add it to the timeline.
+            this._addEventToTimeline(event, timeline, toStartOfTimeline);
+            updateToken = true;
+            continue;
         }
 
-    // we already know about this event. Hopefully it's in this timeline, or
+        // we already know about this event. We don't want to use the pagination
-    // its neighbour
+        // token if this is the last event, as per the text above.
+        updateToken = false;
+
         if (existingTimeline == timeline) {
             console.log("Event " + eventId + " already in timeline " + timeline);
-        return timeline;
+            continue;
         }
 
         var neighbour = timeline.getNeighbouringTimeline(toStartOfTimeline);
         if (neighbour) {
+            // this timeline already has a neighbour in the relevant direction;
+            // let's assume the timelines are already correctly linked up, and
+            // skip over to it.
+            //
+            // there's probably some edge-case here where we end up with an
+            // event which is in a timeline a way down the chain, and there is
+            // a break in the chain somewhere. But I can't really imagine how
+            // that would happen, so I'm going to ignore it for now.
+            //
             if (existingTimeline == neighbour) {
                 console.log("Event " + eventId + " in neighbouring timeline - " +
                             "switching to " + existingTimeline);
             } else {
-            console.warn("Event " + eventId + " already in a different " +
+                console.log("Event " + eventId + " already in a different " +
                             "timeline " + existingTimeline);
             }
-        return existingTimeline;
+            timeline = existingTimeline;
+            continue;
         }
 
         // time to join the timelines.
@@ -460,7 +488,12 @@ Room.prototype._checkForNewEventInExistingTimeline = function(
                      existingTimeline);
         timeline.setNeighbouringTimeline(existingTimeline, toStartOfTimeline);
         existingTimeline.setNeighbouringTimeline(timeline, !toStartOfTimeline);
-    return existingTimeline;
+        timeline = existingTimeline;
+    }
+
+    if (updateToken) {
+        timeline.setPaginationToken(paginationToken, toStartOfTimeline);
+    }
 };
 
 /**
@@ -472,7 +505,11 @@ Room.prototype._checkForNewEventInExistingTimeline = function(
  * @param {MatrixEvent} event
  * @param {EventTimeline} timeline
  * @param {boolean} toStartOfTimeline
- * @param {boolean} spliceBeforeLocalEcho
+ *
+ * @param {boolean} spliceBeforeLocalEcho   if true, insert this event before
+ *     any localecho events at the end of the timeline. Ignored if
+ *     toStartOfTimeline == true.
+ *
  * @fires module:client~MatrixClient#event:"Room.timeline"
  *
  * @private

lib/sync.js

@@ -216,6 +216,13 @@ SyncApi.prototype.peek = function(roomId) {
             response.messages.chunk, client.getEventMapper()
         );
 
+        // set the pagination token before adding the events in case people
+        // fire off pagination requests in response to the Room.timeline
+        // events.
+        if (response.messages.start) {
+            peekRoom.oldState.paginationToken = response.messages.start;
+        }
+
         // set the state of the room to as it was after the timeline executes
         peekRoom.oldState.setStateEvents(oldStateEvents);
         peekRoom.currentState.setStateEvents(stateEvents);
@@ -223,14 +230,11 @@ SyncApi.prototype.peek = function(roomId) {
         self._resolveInvites(peekRoom);
         peekRoom.recalculate(self.client.credentials.userId);
 
-        // roll backwards to diverge old state:
+        // roll backwards to diverge old state. addEventsToTimeline
-        peekRoom.addEventsToTimeline(messages.reverse(), true);
+        // will overwrite the pagination token, so make sure it overwrites
-
+        // it with the right thing.
-        // set the pagination token. Make sure this happens after adding
+        peekRoom.addEventsToTimeline(messages.reverse(), true,
-        // events to the timeline, otherwise it will get reset.
+                                     undefined, response.messages.start);
-        if (response.messages.start) {
-            peekRoom.oldState.paginationToken = response.messages.start;
-        }
 
         client.store.storeRoom(peekRoom);
         client.emit("Room", peekRoom);

spec/integ/matrix-client-room-timeline.spec.js

@@ -481,4 +481,36 @@ describe("MatrixClient room timelines", function() {
             httpBackend.flush("/sync", 1);
         });
     });
+
+    describe("gappy sync", function() {
+        it("should copy the last known state to the new timeline", function(done) {
+            var eventData = [
+                utils.mkMessage({user: userId, room: roomId}),
+            ];
+            setNextSyncData(eventData);
+            NEXT_SYNC_DATA.rooms.join[roomId].timeline.limited = true;
+
+            client.on("sync", function(state) {
+                if (state !== "PREPARED") { return; }
+                var room = client.getRoom(roomId);
+
+                httpBackend.flush("/messages", 1);
+                httpBackend.flush("/sync", 1).done(function() {
+                    expect(room.timeline.length).toEqual(1);
+                    expect(room.timeline[0].event).toEqual(eventData[0]);
+                    expect(room.currentState.getMembers().length).toEqual(2);
+                    expect(room.currentState.getMember(userId).name).toEqual(userName);
+                    expect(room.currentState.getMember(userId).membership).toEqual(
+                        "join"
+                    );
+                    expect(room.currentState.getMember(otherUserId).name).toEqual("Bob");
+                    expect(room.currentState.getMember(otherUserId).membership).toEqual(
+                        "join"
+                    );
+                    done();
+                });
+            });
+            httpBackend.flush("/sync", 1);
+        });
+    });
 });

spec/unit/event-timeline.spec.js

@@ -54,11 +54,12 @@ describe("EventTimeline", function() {
                     event: true,
                 });
 
-            var state =
+            var state = [
                 utils.mkMembership({
                     room: roomId, mship: "invite", user: userB, skey: userA,
                     event: true,
-                });
+                })
+            ];
 
             expect(function() { timeline.initialiseState(state); }).not.toThrow();
             timeline.addEvent(event, false);