Address review comments

Rearrange a couple of things for clarity, and add some comments.

lib/timeline-window.js

@@ -64,6 +64,9 @@ function TimelineWindow(client, room, opts) {
     this._client = client;
     this._room = room;
 
+    // these will be TimelineIndex objects; they delineate the 'start' and
+    // 'end' of the window.
+    //
     // _start.index is inclusive; _end.index is exclusive.
     this._start = null;
     this._end = null;
@@ -93,7 +96,7 @@ TimelineWindow.prototype.load = function(initialEventId, initialWindowSize) {
                 // make sure that our window includes the event
                 for (var i = 0; i < tl.getEvents().length; i++) {
                     if (tl.getEvents()[i].getId() == initialEventId) {
-                        return [tl, i];
+                        return {timeline: tl, index: i};
                     }
                 }
                 throw new Error("getEventTimeline result didn't include requested event");
@@ -103,11 +106,11 @@ TimelineWindow.prototype.load = function(initialEventId, initialWindowSize) {
 
         // start with the most recent events
         var tl = this._room.getLiveTimeline();
-        prom = q([tl, tl.getEvents().length]);
+        prom = q({timeline: tl, index: tl.getEvents().length});
     }
 
     prom = prom.then(function(v) {
-        var tl = v[0], eventIndex = v[1];
+        var tl = v.timeline, eventIndex = v.index;
 
         var endIndex = Math.min(tl.getEvents().length,
                                 eventIndex + initialWindowSize / 2);
@@ -145,8 +148,8 @@ TimelineWindow.prototype.canPaginate = function(direction) {
         throw new Error("Invalid direction '" + direction + "'");
     }
 
-    return tl.timeline.getNeighbouringTimeline(direction) ||
-        tl.timeline.getPaginationToken(direction);
+    return Boolean(tl.timeline.getNeighbouringTimeline(direction) ||
+                   tl.timeline.getPaginationToken(direction));
 };
 
 /**
@@ -159,8 +162,11 @@ TimelineWindow.prototype.canPaginate = function(direction) {
  *    number are immediately available, then we return immediately rather than
  *    making an API call.
  *
- * @param {boolean} [makeRequest = true]  whether we should make API calls to
- *    fetch further events if we don't have any.
+ * @param {boolean} [makeRequest = true] whether we should make API calls to
+ *    fetch further events if we don't have any at all. (This has no effect if
+ *    the room already knows about additional events in the relevant direction,
+ *    even if there are fewer than 'size' of them, as we will just return those
+ *    we already know about.)
  *
  * @return {module:client.Promise} Resolves to a boolean which is true if more events
  *    were successfully retrieved.
@@ -200,8 +206,9 @@ TimelineWindow.prototype.paginate = function(direction, size, makeRequest) {
         debuglog("TimelineWindow: increased cap by " + count +
                  " (now " + this._eventCount + ")");
         // remove some events from the other end, if necessary
-        if (this._eventCount > this._windowLimit) {
-            this._unpaginate(direction != EventTimeline.BACKWARDS);
+        var excess = this._eventCount - this._windowLimit;
+        if (excess > 0) {
+            this._unpaginate(excess, direction != EventTimeline.BACKWARDS);
         }
         return q(true);
     }
@@ -240,15 +247,20 @@ TimelineWindow.prototype.paginate = function(direction, size, makeRequest) {
 /**
  * Trim the window to the windowlimit
  *
+ * @param {number}  delta           number of events to remove from the timeline
  * @param {boolean} startOfTimeline if events should be removed from the start
  *     of the timeline.
  *
  * @private
  */
-TimelineWindow.prototype._unpaginate = function(startOfTimeline) {
+TimelineWindow.prototype._unpaginate = function(delta, startOfTimeline) {
     var tl = startOfTimeline ? this._start : this._end;
 
-    var delta = this._eventCount - this._windowLimit;
+    // sanity-check the delta
+    if (delta > this._eventCount || delta < 0) {
+        throw new Error("Attemting to unpaginate " + delta + " events, but " +
+                        "only have " + this._eventCount + " in the timeline");
+    }
 
     while (delta > 0) {
         var count = startOfTimeline ? tl.advance(delta) : tl.retreat(delta);
@@ -280,10 +292,20 @@ TimelineWindow.prototype.getEvents = function() {
 
     var result = [];
 
-    for (var timeline = this._start.timeline; timeline !== null;
-        timeline = timeline.getNeighbouringTimeline(EventTimeline.FORWARDS)) {
-
+    // iterate through each timeline between this._start and this._end
+    // (inclusive).
+    var timeline = this._start.timeline;
+    while (true) {
         var events = timeline.getEvents();
+
+        // For the first timeline in the chain, we want to start at
+        // this._start.index. For the last timeline in the chain, we want to
+        // stop before this._end.index. Otherwise, we want to copy all of the
+        // events in the timeline.
+        //
+        // (Note that both this._start.index and this._end.index are relative
+        // to their respective timelines' BaseIndex).
+        //
         var startIndex = 0, endIndex = events.length;
         if (timeline === this._start.timeline) {
             startIndex = this._start.index + timeline.getBaseIndex();
@@ -295,7 +317,15 @@ TimelineWindow.prototype.getEvents = function() {
         for (var i = startIndex; i < endIndex; i++) {
             result.push(events[i]);
         }
+
+        // if we're not done, iterate to the next timeline.
+        if (timeline === this._end.timeline) {
+            break;
+        } else {
+            timeline = timeline.getNeighbouringTimeline(EventTimeline.FORWARDS);
+        }
     }
+
     return result;
 };
 
@@ -320,7 +350,7 @@ function TimelineIndex(timeline, index) {
  *    timeline
  */
 TimelineIndex.prototype.minIndex = function() {
-    return -this.timeline.getBaseIndex();
+    return this.timeline.getBaseIndex() * -1;
 };
 
 /**
@@ -338,25 +368,40 @@ TimelineIndex.prototype.maxIndex = function() {
  * @return {number} number of events successfully advanced by
  */
 TimelineIndex.prototype.advance = function(delta) {
-    // first try moving the cap
-    var x;
-    if (delta === 0) {
+    if (!delta) {
         return 0;
-    } else if (delta < 0) {
-        x = Math.max(delta, this.minIndex() - this.index);
-        if (x < 0) {
-            this.index += x;
-            return x;
+    }
+
+    // first try moving the index in the current timeline. See if there is room
+    // to do so.
+    var cappedDelta;
+    if (delta < 0) {
+        // we want to wind the index backwards.
+        //
+        // (this.minIndex() - this.index) is a negative number whose magnitude
+        // is the amount of room we have to wind back the index in the current
+        // timeline. We cap delta to this quantity.
+        cappedDelta = Math.max(delta, this.minIndex() - this.index);
+        if (cappedDelta < 0) {
+            this.index += cappedDelta;
+            return cappedDelta;
         }
     } else {
-        x = Math.min(delta, this.maxIndex() - this.index);
-        if (x > 0) {
-            this.index += x;
-            return x;
+        // we want to wind the index forwards.
+        //
+        // (this.maxIndex() - this.index) is a (positive) number whose magnitude
+        // is the amount of room we have to wind forward the index in the current
+        // timeline. We cap delta to this quantity.
+        cappedDelta = Math.min(cappedDelta, this.maxIndex() - this.index);
+        if (cappedDelta > 0) {
+            this.index += cappedDelta;
+            return cappedDelta;
         }
     }
 
-    // next see if there is a neighbouring timeline to switch to
+    // the index is already at the start/end of the current timeline.
+    //
+    // next see if there is a neighbouring timeline to switch to.
     var neighbour = this.timeline.getNeighbouringTimeline(
         delta < 0 ? EventTimeline.BACKWARDS : EventTimeline.FORWARDS);
     if (neighbour) {