Address review comments

Improve comments and naming.

lib/client.js

@@ -2072,8 +2072,8 @@ MatrixClient.prototype.paginateEventContext = function(eventContext, opts) {
  * Get an EventTimeline for the given event
  *
  * <p>If the room object already has the given event in its store, the
- * corresponding timeline will be returned. Otherwise, we make a /context
+ * corresponding timeline will be returned. Otherwise, a /context request is
- * request, and use it to construct an EventTimeline.
+ * made, and used to construct an EventTimeline.
  *
  * @param {Room} room  The room to look for the event in
  * @param {string} eventId  The ID of the event to look for
@@ -2085,7 +2085,7 @@ MatrixClient.prototype.paginateEventContext = function(eventContext, opts) {
 MatrixClient.prototype.getEventTimeline = function(room, eventId) {
     // don't allow any timeline support unless it's been enabled.
     if (!this.timelineSupport) {
-        throw Error("timeline support is disabled. Set the 'timelineSupport'" +
+        throw new Error("timeline support is disabled. Set the 'timelineSupport'" +
                     " parameter to true when creating MatrixClient to enable" +
                     " it.");
     }
@@ -2108,7 +2108,7 @@ MatrixClient.prototype.getEventTimeline = function(room, eventId) {
         self._http.authedRequest(undefined, "GET", path
     ).then(function(res) {
         if (!res.event) {
-            throw Error("'event' not in '/context' result - homeserver too old?");
+            throw new Error("'event' not in '/context' result - homeserver too old?");
         }
 
         // by the time the request completes, the event might have ended up in
@@ -2127,7 +2127,7 @@ MatrixClient.prototype.getEventTimeline = function(room, eventId) {
 
         var timeline = room.getTimelineForEvent(matrixEvents[0].getId());
         if (!timeline) {
-            timeline = room.createTimeline();
+            timeline = room.addTimeline();
             timeline.initialiseState(utils.map(res.state,
                                                self.getEventMapper()));
             timeline.getState(false).paginationToken = res.end;
@@ -2155,8 +2155,8 @@ MatrixClient.prototype.getEventTimeline = function(room, eventId) {
  *    false to go forwards
  * @param {number}   [opts.limit = 30]         number of events to request
  *
- * @return {module:client.Promise} Resolves: false if there are no events and
+ * @return {module:client.Promise} Resolves to a boolean: false if there are no
- *    we reached the end of the timeline; else true. Rejects: Error with an error response
+ *    events and we reached either end of the timeline; else true.
  */
 MatrixClient.prototype.paginateEventTimeline = function(eventTimeline, opts) {
     // TODO: we should implement a backoff (as per scrollback()) to deal more
@@ -2164,10 +2164,15 @@ MatrixClient.prototype.paginateEventTimeline = function(eventTimeline, opts) {
     opts = opts || {};
     var backwards = opts.backwards || false;
 
+    var room = this.getRoom(eventTimeline.getRoomId());
+    if (!room) {
+        throw new Error("Unknown room " + eventTimeline.getRoomId());
+    }
+
     var token = eventTimeline.getPaginationToken(backwards);
     if (!token) {
-        // no more results.
+        // no token - no results.
-        return q.reject(new Error("No paginate token"));
+        return q(false);
     }
 
     var dir = backwards ? 'b' : 'f';
@@ -2193,9 +2198,6 @@ MatrixClient.prototype.paginateEventTimeline = function(eventTimeline, opts) {
         this._http.authedRequest(undefined, "GET", path, params
     ).then(function(res) {
         var token = res.end;
-        console.log("client: completed paginate; backwards=" + backwards +
-                    "; token=" + token);
-        var room = self.getRoom(eventTimeline.getRoomId());
         var matrixEvents = utils.map(res.chunk, self.getEventMapper());
         room.addEventsToTimeline(matrixEvents, backwards, eventTimeline, token);
 

lib/models/event-timeline.js

@@ -15,12 +15,12 @@ var RoomState = require("./room-state");
  * the room at the beginning and end of the timeline, and pagination tokens for
  * going backwards and forwards in the timeline.
  *
- * <p>In order that clients can meaningfully maintain an index into a timeline, we
+ * <p>In order that clients can meaningfully maintain an index into a timeline,
- * track a 'baseIndex'. This starts at zero, but is incremented when events are
+ * the EventTimeline object tracks a 'baseIndex'. This starts at zero, but is
- * prepended to the timeline. The index of an event relative to baseIndex
+ * incremented when events are prepended to the timeline. The index of an event
- * therefore remains constant.
+ * relative to baseIndex therefore remains constant.
  *
- * <p>Once a timeline joins up with its neighbour, we link them together into a
+ * <p>Once a timeline joins up with its neighbour, they are linked together into a
  * doubly-linked list.
  *
  * @param {string} roomId    the ID of the room where this timeline came from
@@ -45,8 +45,9 @@ function EventTimeline(roomId) {
  *
  * <p>This can only be called before any events are added.
  *
- * @param {MatrixEvent[]} stateEvents list of state events to intialise the
+ * @param {MatrixEvent[]} stateEvents list of state events to initialise the
  * state with.
+ * @throws {Error} if an attempt is made to call this after addEvent is called.
  */
 EventTimeline.prototype.initialiseState = function(stateEvents) {
     if (this._events.length > 0) {
@@ -67,7 +68,11 @@ EventTimeline.prototype.getRoomId = function() {
 };
 
 /**
- * Get the base index
+ * Get the base index.
+ *
+ * <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.
  *
  * @return {number}
  */
@@ -137,6 +142,9 @@ EventTimeline.prototype.getNeighbouringTimeline = function(before) {
  *
  * @param {boolean} before true to set the previous timeline; false to set
  * following one.
+ *
+ * @throws {Error} if an attempt is made to set the neighbouring timeline when
+ * it is already set.
  */
 EventTimeline.prototype.setNeighbouringTimeline = function(neighbour, before) {
     if (this.getNeighbouringTimeline(before)) {

lib/models/room.js

@@ -171,7 +171,7 @@ Room.prototype.resetLiveTimeline = function() {
         this._timelines = [newTimeline];
         this._eventIdToTimeline = {};
     } else {
-        newTimeline = this.createTimeline();
+        newTimeline = this.addTimeline();
     }
 
     if (this._liveTimeline) {
@@ -292,12 +292,11 @@ Room.prototype.getAvatarUrl = function(baseUrl, width, height, resizeMethod,
  };
 
 /**
- * Create a new timeline for this room
+ * Add a new timeline to this room
  *
- * @param {Array<MatrixEvent>} stateBeforeEvent state of the room before this event
  * @return {module:models/event-timeline~EventTimeline} newly-created timeline
  */
-Room.prototype.createTimeline = function() {
+Room.prototype.addTimeline = function() {
     if (!this._timelineSupport) {
         throw Error("timeline support is disabled. Set the 'timelineSupport'" +
                     " parameter to true when creating MatrixClient to enable" +
@@ -341,12 +340,44 @@ Room.prototype.addEventsToTimeline = function(events, toStartOfTimeline,
         return;
     }
 
+    // we need to handle the following case:
+    //
+    // We already know about two timelines. One of these contains just
+    // event M, and one contains just event P:
+    //
+    //    timeline1      timeline2
+    //       [M]            [P]
+    //
+    // We are now processing a new pagination response from the server.
+    // This contains the following events: [M, N, P, R, S].
+    //
+    // 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:
+    //
+    //    timeline1      timeline2
+    //     [M, N] <----> [P]
+    //
+    // 4. We now need to append R and S to timeline2. So finally, we have:
+    //
+    //    timeline1      timeline2
+    //     [M, N] <----> [P, R, S]
+    //
+    // The important thing to note in the above is that we switched the
+    // timeline we were adding events to, after we found a neighbour.
+
+    // XXX: TODO: but timeline2 might already be joined to timeline3: we need
+    // to skip all the way to the end of the linked list!
+
     var updateToken = false;
     for (var i = 0; i < events.length; i++) {
-        var existingTimeline = this._checkExistingTimeline(events[i], timeline,
+        var existingTimeline = this._checkForNewEventInExistingTimeline(
-                                                           toStartOfTimeline);
+            events[i], timeline, toStartOfTimeline);
+
         if (existingTimeline) {
-            // switch to the other timeline
             timeline = existingTimeline;
             updateToken = false;
         } else {
@@ -371,8 +402,8 @@ Room.prototype.addEventsToTimeline = function(events, toStartOfTimeline,
  *    none
  * @private
  */
-Room.prototype._checkExistingTimeline = function(event, timeline,
+Room.prototype._checkForNewEventInExistingTimeline = function(
-                                                 toStartOfTimeline) {
+           event, timeline, toStartOfTimeline) {
     var eventId = event.getId();
 
     var existingTimeline = this._eventIdToTimeline[eventId];
@@ -410,7 +441,7 @@ Room.prototype._checkExistingTimeline = function(event, timeline,
 
 /**
  * Check for redactions, and otherwise add event to the given timeline. Assumes
- * we have already checked we don't lnow about this event.
+ * we have already checked we don't know about this event.
  *
  * Will fire "Room.timeline" for each event added.
  *
@@ -592,13 +623,16 @@ Room.prototype.removeEvents = function(event_ids) {
 
 /**
  * Removes a single event from this room.
+ *
  * @param {String} eventId  The id of the event to remove
- * @return {?MatrixEvent} the removed event, or null if none
+ *
+ * @return {?MatrixEvent} the removed event, or null if the event was not found
+ * in this room.
  */
 Room.prototype.removeEvent = function(eventId) {
     var timeline = this._eventIdToTimeline[eventId];
     if (!timeline) {
-        return false;
+        return null;
     }
 
     var removed = timeline.removeEvent(eventId);