Dima/matrix-js-sdk
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge pull request #75 from matrix-org/rav/read_receipt_ordering

Browse Source 
Give precedence to later Read Receipts
This commit is contained in:
Richard van der Hoff
2016-02-04 16:50:59 +01:00
parent 1b43e5ed98 f3e5e03009
commit 122867e8ee
4 changed files with 234 additions and 48 deletions
Download Patch File Download Diff File Expand all files Collapse all files
lib/client.js
+6
View File
@@ -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;
lib/models/room.js
+96 -46
View File
@@ -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
spec/integ/matrix-client-syncing.spec.js
+5 -2
View File
@@ -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
}
spec/unit/room.spec.js
+127
View File
@@ -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() {