Implement 'pendingEventList'

The existing 'pendingEventOrdering'=='end' semantics had been substantially
broken by the introduction of timelines and gappy syncs: after a gappy
sync, pending events would get stuck in the old timeline section. (Part of
https://github.com/vector-im/vector-web/issues/1120).

lib/models/event-timeline.js

@@ -211,10 +211,8 @@ EventTimeline.prototype.setNeighbouringTimeline = function(neighbour, direction)
  *
  * @param {MatrixEvent} event   new event
  * @param {boolean}  atStart     true to insert new event at the start
- * @param {boolean}  [spliceBeforeLocalEcho = false] insert this event before any
- *     localecho events at the end of the timeline. Ignored if atStart == true
  */
-EventTimeline.prototype.addEvent = function(event, atStart, spliceBeforeLocalEcho) {
+EventTimeline.prototype.addEvent = function(event, atStart) {
     var stateContext = atStart ? this._startState : this._endState;
 
     setEventMetadata(event, stateContext, atStart);
@@ -243,17 +241,6 @@ EventTimeline.prototype.addEvent = function(event, atStart, spliceBeforeLocalEch
         insertIndex = 0;
     } else {
         insertIndex = this._events.length;
-
-        // if this is a real event, we might need to splice it in before any pending
-        // local echo events.
-        if (spliceBeforeLocalEcho) {
-            for (var j = this._events.length - 1; j >= 0; j--) {
-                if (!this._events[j].status) { // real events don't have a status
-                    insertIndex = j + 1;
-                    break;
-                }
-            }
-        }
     }
 
     this._events.splice(insertIndex, 0, event); // insert element

lib/models/room.js

@@ -68,11 +68,13 @@ function synthesizeReceipt(userId, event, receiptType) {
  * @param {*} opts.storageToken Optional. The token which a data store can use
  * to remember the state of the room. What this means is dependent on the store
  * implementation.
- * @param {String=} opts.pendingEventOrdering Controls where pending messages appear
+ *
- * in a room's timeline. If "<b>chronological</b>", messages will appear in the timeline
+ * @param {String=} opts.pendingEventOrdering Controls where pending messages
- * when the call to <code>sendEvent</code> was made. If "<b>end</b>", pending messages
+ * appear in a room's timeline. If "<b>chronological</b>", messages will appear
- * will always appear at the end of the timeline (multiple pending messages will be sorted
+ * in the timeline when the call to <code>sendEvent</code> was made. If
- * chronologically). Default: "chronological".
+ * "<b>detached</b>", pending messages will appear in the
+ * 'pendingEventList'. Default: "chronological".
+
  * @param {boolean} [opts.timelineSupport = false] Set to true to enable improved
  * timeline support.
  *
@@ -99,10 +101,10 @@ function Room(roomId, opts) {
     opts = opts || {};
     opts.pendingEventOrdering = opts.pendingEventOrdering || "chronological";
 
-    if (["chronological", "end"].indexOf(opts.pendingEventOrdering) === -1) {
+    if (["chronological", "detached"].indexOf(opts.pendingEventOrdering) === -1) {
         throw new Error(
             "opts.pendingEventOrdering MUST be either 'chronological' or " +
-            "'end'. Got: '" + opts.pendingEventOrdering + "'"
+            "'detached'. Got: '" + opts.pendingEventOrdering + "'"
         );
     }
 
@@ -151,9 +153,31 @@ function Room(roomId, opts) {
     this._eventIdToTimeline = {};
     this._timelineSupport = Boolean(opts.timelineSupport);
 
+    if (this._opts.pendingEventOrdering == "detached") {
+        this._pendingEventList = [];
+    }
 }
 utils.inherits(Room, EventEmitter);
 
+/**
+ * Get the list of pending sent events for this room
+ *
+ * @return {module:models/event~MatrixEvent[]} A list of the sent events
+ * waiting for remote echo.
+ *
+ * @throws If <code>opts.pendingEventOrdering</code> was not 'detached'
+ */
+Room.prototype.getPendingEvents = function() {
+    if (this._opts.pendingEventOrdering !== "detached") {
+        throw new Error(
+            "Cannot call getPendingEventList with pendingEventOrdering == " +
+                this._opts.pendingEventOrdering);
+    }
+
+    return this._pendingEventList;
+};
+
+
 /**
  * Get the live timeline for this room.
  *
@@ -558,18 +582,13 @@ Room.prototype.addEventsToTimeline = function(events, toStartOfTimeline,
  * @param {EventTimeline} timeline
  * @param {boolean} toStartOfTimeline
  *
- * @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
  */
-Room.prototype._addEventToTimeline = function(event, timeline, toStartOfTimeline,
+Room.prototype._addEventToTimeline = function(event, timeline, toStartOfTimeline) {
-                                              spliceBeforeLocalEcho) {
     var eventId = event.getId();
-    timeline.addEvent(event, toStartOfTimeline, spliceBeforeLocalEcho);
+    timeline.addEvent(event, toStartOfTimeline);
     this._eventIdToTimeline[eventId] = timeline;
 
     var data = {
@@ -589,8 +608,6 @@ Room.prototype._addEventToTimeline = function(event, timeline, toStartOfTimeline
  * @private
  */
 Room.prototype._addLiveEvents = function(events) {
-    var addLocalEchoToEnd = this._opts.pendingEventOrdering === "end";
-
     for (var i = 0; i < events.length; i++) {
         if (events[i].getType() === "m.room.redaction") {
             var redactId = events[i].event.redacts;
@@ -624,8 +641,7 @@ Room.prototype._addLiveEvents = function(events) {
 
         if (!this._eventIdToTimeline[events[i].getId()]) {
             // TODO: pass through filter to see if this should be added to the timeline.
-            this._addEventToTimeline(events[i], this._liveTimeline, false,
+            this._addEventToTimeline(events[i], this._liveTimeline, false);
-                                     addLocalEchoToEnd);
         }
 
         // synthesize and inject implicit read receipts
@@ -643,6 +659,9 @@ Room.prototype._addLiveEvents = function(events) {
 /**
  * Add a pending outgoing event to this room.
  *
+ * <p>The event is added to either the pendingEventList, or the live timeline,
+ * depending on the setting of opts.pendingEventOrdering.
+ *
  * <p>This is an internal method, intended for use by MatrixClient.
  *
  * @param {module:models/event~MatrixEvent} event The event to add.
@@ -674,7 +693,11 @@ Room.prototype.addPendingEvent = function(event, txnId) {
 
     this._txnToEvent[txnId] = event;
 
-    this._addEventToTimeline(event, this._liveTimeline, false);
+    if (this._opts.pendingEventOrdering == "detached") {
+        this._pendingEventList.push(event);
+    } else {
+        this._addEventToTimeline(event, this._liveTimeline, false);
+    }
 
     this.emit("Room.localEchoUpdated", event, this, null, null);
 };
@@ -682,10 +705,13 @@ Room.prototype.addPendingEvent = function(event, txnId) {
 /**
  * Deal with the echo of a message we sent.
  *
+ * <p>We move the event to the live timeline if it isn't there already, and
+ * update it.
+ *
  * @param {module:models/event~MatrixEvent} remoteEvent   The event received from
  *    /sync
  * @param {module:models/event~MatrixEvent} localEvent    The local echo, which
- *    should already be in the timeline.
+ *    should be either in the _pendingEventList or the timeline.
  *
  * @fires module:client~MatrixClient#event:"Room.localEchoUpdated"
  * @private
@@ -698,6 +724,15 @@ Room.prototype._handleRemoteEcho = function(remoteEvent, localEvent) {
     // no longer pending
     delete this._txnToEvent[remoteEvent.transaction_id];
 
+    // if it's in the pending list, remove it
+    if (this._pendingEventList) {
+        utils.removeElement(
+            this._pendingEventList,
+            function(ev) { return ev.getId() == oldEventId; },
+            false
+        );
+    }
+
     // replace the event source, but preserve the original content
     // and type in case it was encrypted (we won't be able to
     // decrypt it, even though we sent it.)
@@ -709,18 +744,19 @@ Room.prototype._handleRemoteEcho = function(remoteEvent, localEvent) {
     // successfully sent.
     localEvent.status = null;
 
-    // Update the timeline map.
+    // if it's already in the timeline, update the timeline map. If it's not, add it.
     var existingTimeline = this._eventIdToTimeline[oldEventId];
     if (existingTimeline) {
         delete this._eventIdToTimeline[oldEventId];
         this._eventIdToTimeline[newEventId] = existingTimeline;
+    } else {
+        this._addEventToTimeline(localEvent, this._liveTimeline, false);
     }
 
     this.emit("Room.localEchoUpdated", localEvent, this,
               oldEventId, oldStatus);
 };
 
-
 /* a map from current event status to a list of allowed next statuses
  */
 var ALLOWED_TRANSITIONS = {};
@@ -750,10 +786,6 @@ ALLOWED_TRANSITIONS[EventStatus.NOT_SENT] =
  * @fires module:client~MatrixClient#event:"Room.localEchoUpdated"
  */
 Room.prototype.updatePendingEvent = function(event, newStatus, newEventId) {
-    if (!this.getTimelineForEvent(event.getId())) {
-        throw new Error("updateLocalEchoStatus called on an unknown event.");
-    }
-
     // if the message was sent, we expect an event id
     if (newStatus == EventStatus.SENT && !newEventId) {
         throw new Error("updatePendingEvent called with status=SENT, " +
@@ -790,7 +822,9 @@ Room.prototype.updatePendingEvent = function(event, newStatus, newEventId) {
         // update the event id
         event.event.event_id = newEventId;
 
-        // Update the timeline map
+        // if the event was already in the timeline (which will be the case if
+        // opts.pendingEventOrdering==chronological), we need to update the
+        // timeline map.
         var existingTimeline = this._eventIdToTimeline[oldEventId];
         if (existingTimeline) {
             delete this._eventIdToTimeline[oldEventId];

spec/unit/room.spec.js

@@ -333,7 +333,6 @@ describe("Room", function() {
             var localEvent = utils.mkMessage({
                 room: roomId, user: userA, event: true,
             });
-            localEvent._txnId = "TXN_ID";
             localEvent.status = EventStatus.SENDING;
             var localEventId = localEvent.getId();
 
@@ -1144,10 +1143,11 @@ describe("Room", function() {
         });
     });
 
-    describe("pendingEventOrdering", function() {
+    describe("addPendingEvent", function() {
-        it("should sort pending events to the end of the timeline if 'end'", function() {
+        it("should add pending events to the pendingEventList if " +
+                      "pendingEventOrdering == 'detached'", function() {
             var room = new Room(roomId, {
-                pendingEventOrdering: "end"
+                pendingEventOrdering: "detached"
             });
             var eventA = utils.mkMessage({
                 room: roomId, user: userA, msg: "remote 1", event: true
@@ -1155,17 +1155,24 @@ describe("Room", function() {
             var eventB = utils.mkMessage({
                 room: roomId, user: userA, msg: "local 1", event: true
             });
+            eventB._txnId = "TXN1";
             eventB.status = EventStatus.SENDING;
             var eventC = utils.mkMessage({
                 room: roomId, user: userA, msg: "remote 2", event: true
             });
-            room.addEvents([eventA, eventB, eventC]);
+            room.addEvents([eventA]);
+            room.addPendingEvent(eventB);
+            room.addEvents([eventC]);
             expect(room.timeline).toEqual(
-                [eventA, eventC, eventB]
+                [eventA, eventC]
+            );
+            expect(room.getPendingEvents()).toEqual(
+                [eventB]
             );
         });
 
-        it("should sort pending events chronologically if 'chronological'", function() {
+        it("should add pending events to the timeline if " +
+                      "pendingEventOrdering == 'chronological'", function() {
             room = new Room(roomId, {
                 pendingEventOrdering: "chronological"
             });
@@ -1175,54 +1182,17 @@ describe("Room", function() {
             var eventB = utils.mkMessage({
                 room: roomId, user: userA, msg: "local 1", event: true
             });
+            eventB._txnId = "TXN1";
             eventB.status = EventStatus.SENDING;
             var eventC = utils.mkMessage({
                 room: roomId, user: userA, msg: "remote 2", event: true
             });
-            room.addEvents([eventA, eventB, eventC]);
+            room.addEvents([eventA]);
+            room.addPendingEvent(eventB);
+            room.addEvents([eventC]);
             expect(room.timeline).toEqual(
                 [eventA, eventB, eventC]
             );
         });
-
-        it("should treat NOT_SENT events as local echo", function() {
-            var room = new Room(roomId, {
-                pendingEventOrdering: "end"
-            });
-            var eventA = utils.mkMessage({
-                room: roomId, user: userA, msg: "remote 1", event: true
-            });
-            var eventB = utils.mkMessage({
-                room: roomId, user: userA, msg: "local 1", event: true
-            });
-            eventB.status = EventStatus.NOT_SENT;
-            var eventC = utils.mkMessage({
-                room: roomId, user: userA, msg: "remote 2", event: true
-            });
-            room.addEvents([eventA, eventB, eventC]);
-            expect(room.timeline).toEqual(
-                [eventA, eventC, eventB]
-            );
-        });
-
-        it("should treat QUEUED events as local echo", function() {
-            var room = new Room(roomId, {
-                pendingEventOrdering: "end"
-            });
-            var eventA = utils.mkMessage({
-                room: roomId, user: userA, msg: "remote 1", event: true
-            });
-            var eventB = utils.mkMessage({
-                room: roomId, user: userA, msg: "local 1", event: true
-            });
-            eventB.status = EventStatus.QUEUED;
-            var eventC = utils.mkMessage({
-                room: roomId, user: userA, msg: "remote 2", event: true
-            });
-            room.addEvents([eventA, eventB, eventC]);
-            expect(room.timeline).toEqual(
-                [eventA, eventC, eventB]
-            );
-        });
     });
 });