From f3e5e0300909813724979b0ae2486eaa07547926 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 4 Feb 2016 15:31:12 +0000 Subject: [PATCH] Give precedence to later Read Receipts In order to resolve the conflict between local and remote read-receipts, try to give precedence to the read-receipt which refers to the most recent event. Also fix the read-receipt synthesis in _addLiveEvents so that it actually works (drop the spurious MatrixEvent wrapper), and remove the synthesis in recalculate() (which appears to be redundant). --- lib/client.js | 6 + lib/models/room.js | 142 +++++++++++++++-------- spec/integ/matrix-client-syncing.spec.js | 7 +- spec/unit/room.spec.js | 127 ++++++++++++++++++++ 4 files changed, 234 insertions(+), 48 deletions(-) diff --git a/lib/client.js b/lib/client.js index b7a4d7f09..9ba942c84 100644 --- a/lib/client.js +++ b/lib/client.js @@ -1180,6 +1180,12 @@ function _sendEvent(client, room, event, callback) { matchingEvent.status = null; // make sure it's still marked as sent } else { + // best way to make sure the room timeline structures are updated + // correctly is to remove the event and add it again with the right + // ID. + // + // This will also make us synthesize our own read receipt for the + // sent message. room.removeEvents([localEventId]); event.event.event_id = res.event_id; event.status = null; diff --git a/lib/models/room.js b/lib/models/room.js index 838a28888..5b14e2e16 100644 --- a/lib/models/room.js +++ b/lib/models/room.js @@ -27,6 +27,7 @@ var ContentRepo = require("../content-repo"); var EventTimeline = require("./event-timeline"); function synthesizeReceipt(userId, event, receiptType) { + // console.log("synthesizing receipt for "+event.getId()); // This is really ugly because JS has no way to express an object literal // where the name of a key comes from an expression var fakeReceipt = { @@ -609,13 +610,13 @@ Room.prototype._addLiveEvents = function(events) { // synthesize and inject implicit read receipts // Done after adding the event because otherwise the app would get a read receipt // pointing to an event that wasn't yet in the timeline - - // This is really ugly because JS has no way to express an object literal - // where the name of a key comes from an expression - if (events[i].sender) { - this.addReceipt(new MatrixEvent(synthesizeReceipt( + // + // (we don't do this for local echoes, as they have temporary event + // ids, which don't make much sense as RRs). + if (events[i].sender && !isLocalEcho) { + this.addReceipt(synthesizeReceipt( events[i].sender.userId, events[i], "m.read" - ))); + )); } } }; @@ -722,6 +723,79 @@ Room.prototype.removeEvent = function(eventId) { return removed; }; +/** + * Determine where two events appear in the timeline relative to one another + * + * @param {string} eventId1 The id of the first event + * @param {string} eventId2 The id of the second event + + * @return {?number} a number less than zero if eventId1 precedes eventId2, and + * greater than zero if eventId1 succeeds eventId2. zero if they are the + * same event; null if we can't tell (either because we don't know about one + * of the events, or because they are in separate timelines which don't join + * up). + */ +Room.prototype.compareEventOrdering = function(eventId1, eventId2) { + if (eventId1 == eventId2) { + // optimise this case + return 0; + } + + var timeline1 = this._eventIdToTimeline[eventId1]; + var timeline2 = this._eventIdToTimeline[eventId2]; + + if (timeline1 === undefined) { + return null; + } + if (timeline2 === undefined) { + return null; + } + + if (timeline1 === timeline2) { + // both events are in the same timeline - figure out their + // relative indices + var idx1, idx2; + var events = timeline1.getEvents(); + for (var idx = 0; idx < events.length && + (idx1 === undefined || idx2 === undefined); idx++) { + var evId = events[idx].getId(); + if (evId == eventId1) { + idx1 = idx; + } + if (evId == eventId2) { + idx2 = idx; + } + } + return idx1 - idx2; + } + + // the events are in different timelines. Iterate through the + // linkedlist to see which comes first. + + // first work forwards from timeline1 + var tl = timeline1; + while (tl) { + if (tl === timeline2) { + // timeline1 is before timeline2 + return -1; + } + tl = tl.getNeighbouringTimeline(EventTimeline.FORWARDS); + } + + // now try backwards from timeline1 + tl = timeline1; + while (tl) { + if (tl === timeline2) { + // timeline2 is before timeline1 + return 1; + } + tl = tl.getNeighbouringTimeline(EventTimeline.BACKWARDS); + } + + // the timelines are not contiguous. + return null; +}; + /** * Recalculate various aspects of the room, including the room name and * room summary. Call this any time the room's current state is modified. @@ -767,45 +841,6 @@ Room.prototype.recalculate = function(userId) { if (oldName !== this.name) { this.emit("Room.name", this); } - - - - // recalculate read receipts, adding implicit ones where necessary - // NB. This is a duplication of logic for injecting implicit receipts, - // it would be technically possible to only ever generate these - // receipts in addEventsToTimeline but doing so means correctly - // choosing whether to keep or replace the existing receipt which - // is complex and slow. This is faster and more understandable. - - var usersFound = {}; - for (var i = this.timeline.length - 1; i >= 0; --i) { - // loop through the timeline backwards looking for either an - // event sent by each user or a real receipt from them. - // Replace the read receipt for that user with whichever - // occurs later in the timeline (ie. first because we're going - // backwards). - var e = this.timeline[i]; - - var readReceiptsForEvent = this.getReceiptsForEvent(e); - - for (var receiptIt = 0; receiptIt < readReceiptsForEvent.length; ++receiptIt) { - var receipt = readReceiptsForEvent[receiptIt]; - if (receipt.type !== "m.read") { continue; } - - if (usersFound[receipt.userId]) { continue; } - - // Then this is the receipt we keep for this user - usersFound[receipt.userId] = 1; - } - - if (e.sender && usersFound[e.sender.userId] === undefined) { - // no receipt yet for this sender, so we synthesize one. - - this.addReceipt(synthesizeReceipt(e.sender.userId, e, "m.read")); - - usersFound[e.sender.userId] = 1; - } - } }; @@ -870,12 +905,27 @@ Room.prototype.addReceipt = function(event) { utils.keys(event.getContent()[eventId][receiptType]).forEach( function(userId) { var receipt = event.getContent()[eventId][receiptType][userId]; + if (!self._receipts[receiptType]) { self._receipts[receiptType] = {}; } - if (!self._receipts[receiptType][userId]) { + + var existingReceipt = self._receipts[receiptType][userId]; + + if (!existingReceipt) { self._receipts[receiptType][userId] = {}; + } else { + // we only want to add this receipt if we think it is later + // than the one we already have. (This is managed + // server-side, but because we synthesize RRs locally we + // have to do it here too.) + var ordering = self.compareEventOrdering( + existingReceipt.eventId, eventId); + if (ordering !== null && ordering >= 0) { + return; + } } + self._receipts[receiptType][userId] = { eventId: eventId, data: receipt diff --git a/spec/integ/matrix-client-syncing.spec.js b/spec/integ/matrix-client-syncing.spec.js index ba222db2b..dc50a3af2 100644 --- a/spec/integ/matrix-client-syncing.spec.js +++ b/spec/integ/matrix-client-syncing.spec.js @@ -434,6 +434,9 @@ describe("MatrixClient syncing", function() { events: [ utils.mkMessage({ room: roomOne, user: otherUserId, msg: "hello" + }), + utils.mkMessage({ + room: roomOne, user: otherUserId, msg: "world" }) ] }, @@ -473,7 +476,7 @@ describe("MatrixClient syncing", function() { receipt[ackEvent.event_id] = { "m.read": {} }; - receipt[ackEvent.event_id]["m.read"][otherUserId] = { + receipt[ackEvent.event_id]["m.read"][userC] = { ts: 176592842636 }; syncData.rooms.join[roomOne].ephemeral.events = [{ @@ -489,7 +492,7 @@ describe("MatrixClient syncing", function() { var room = client.getRoom(roomOne); expect(room.getReceiptsForEvent(new MatrixEvent(ackEvent))).toEqual([{ type: "m.read", - userId: otherUserId, + userId: userC, data: { ts: 176592842636 } diff --git a/spec/unit/room.spec.js b/spec/unit/room.spec.js index 676fdb52a..f82c0c564 100644 --- a/spec/unit/room.spec.js +++ b/spec/unit/room.spec.js @@ -311,6 +311,96 @@ describe("Room", function() { expect(events[1].forwardLooking).toBe(false); expect(room.currentState.setStateEvents).not.toHaveBeenCalled(); }); + + it("should synthesize read receipts for the senders of events", function() { + var sentinel = { + userId: userA, + membership: "join", + name: "Alice" + }; + room.currentState.getSentinelMember.andCallFake(function(uid) { + if (uid === userA) { + return sentinel; + } + return null; + }); + room.addEventsToTimeline(events); + expect(room.getEventReadUpTo(userA)).toEqual(events[1].getId()); + }); + }); + + describe("compareEventOrdering", function() { + beforeEach(function() { + room = new Room(roomId, {timelineSupport: true}); + }); + + var events = [ + utils.mkMessage({ + room: roomId, user: userA, msg: "1111", event: true + }), + utils.mkMessage({ + room: roomId, user: userA, msg: "2222", event: true + }), + utils.mkMessage({ + room: roomId, user: userA, msg: "3333", event: true + }), + ]; + + it("should handle events in the same timeline", function() { + room.addEventsToTimeline(events); + + expect(room.compareEventOrdering(events[0].getId(), + events[1].getId())) + .toBeLessThan(0); + expect(room.compareEventOrdering(events[2].getId(), + events[1].getId())) + .toBeGreaterThan(0); + expect(room.compareEventOrdering(events[1].getId(), + events[1].getId())) + .toEqual(0); + }); + + it("should handle events in adjacent timelines", function() { + var oldTimeline = room.addTimeline(); + oldTimeline.setNeighbouringTimeline(room.getLiveTimeline(), 'f'); + room.getLiveTimeline().setNeighbouringTimeline(oldTimeline, 'b'); + + room.addEventsToTimeline([events[0]], false, oldTimeline); + room.addEventsToTimeline([events[1]]); + + expect(room.compareEventOrdering(events[0].getId(), + events[1].getId())) + .toBeLessThan(0); + expect(room.compareEventOrdering(events[1].getId(), + events[0].getId())) + .toBeGreaterThan(0); + }); + + it("should return null for events in non-adjacent timelines", function() { + var oldTimeline = room.addTimeline(); + + room.addEventsToTimeline([events[0]], false, oldTimeline); + room.addEventsToTimeline([events[1]]); + + expect(room.compareEventOrdering(events[0].getId(), + events[1].getId())) + .toBe(null); + expect(room.compareEventOrdering(events[1].getId(), + events[0].getId())) + .toBe(null); + }); + + it("should return null for unknown events", function() { + room.addEventsToTimeline(events); + + expect(room.compareEventOrdering(events[0].getId(), "xxx")) + .toBe(null); + expect(room.compareEventOrdering("xxx", events[0].getId())) + .toBe(null); + expect(room.compareEventOrdering(events[0].getId(), + events[0].getId())) + .toBe(0); + }); }); describe("getJoinedMembers", function() { @@ -792,6 +882,43 @@ describe("Room", function() { ]); }); + it("should prioritise the most recent event", function() { + var events = [ + utils.mkMessage({ + room: roomId, user: userA, msg: "1111", + event: true + }), + utils.mkMessage({ + room: roomId, user: userA, msg: "2222", + event: true + }), + utils.mkMessage({ + room: roomId, user: userA, msg: "3333", + event: true + }), + ]; + + room.addEventsToTimeline(events); + var ts = 13787898424; + + // check it initialises correctly + room.addReceipt(mkReceipt(roomId, [ + mkRecord(events[0].getId(), "m.read", userB, ts), + ])); + expect(room.getEventReadUpTo(userB)).toEqual(events[0].getId()); + + // 2>0, so it should move forward + room.addReceipt(mkReceipt(roomId, [ + mkRecord(events[2].getId(), "m.read", userB, ts), + ])); + expect(room.getEventReadUpTo(userB)).toEqual(events[2].getId()); + + // 1<2, so it should stay put + room.addReceipt(mkReceipt(roomId, [ + mkRecord(events[1].getId(), "m.read", userB, ts), + ])); + expect(room.getEventReadUpTo(userB)).toEqual(events[2].getId()); + }); }); describe("getUsersReadUpTo", function() {