From 6f37c232f7023c7f2922225ed7f93a1af1dd2bac Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 8 Apr 2021 12:56:24 +0200 Subject: [PATCH 1/3] Don't cache members that haven't been written yet - fixes #271 --- src/matrix/room/timeline/persistence/MemberWriter.js | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/matrix/room/timeline/persistence/MemberWriter.js b/src/matrix/room/timeline/persistence/MemberWriter.js index 9143acc9..dc6f621d 100644 --- a/src/matrix/room/timeline/persistence/MemberWriter.js +++ b/src/matrix/room/timeline/persistence/MemberWriter.js @@ -91,9 +91,6 @@ export class MemberWriter { }); if (memberEvent) { member = RoomMember.fromMemberEvent(this._roomId, memberEvent); - // adding it to the cache, but not storing it for now; - // we'll do that when we get to the event - this._cache.set(member); } } return member; @@ -222,5 +219,14 @@ export function tests() { assert.equal(change.member.membership, "join"); assert.equal(txn.members.get(alice).displayName, "Alice"); }, + "newly joined member causes a change with lookup done first": async assert => { + const event = createMemberEvent("join", alice, "Alice"); + const writer = new MemberWriter(roomId); + const txn = createStorage(); + const member = await writer.lookupSenderMember(event, [event], txn); + assert(member); + const change = await writer.writeTimelineMemberEvent(event, txn); + assert(change); + }, }; } From f67ccc18f4f9fef56a6ad674ef269ce281634ec2 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 8 Apr 2021 12:57:10 +0200 Subject: [PATCH 2/3] take most recent member rather than first in timeline for inline lookup noticed this while inspecting the code, looks related to #269 --- .../room/timeline/persistence/MemberWriter.js | 30 +++++++++++++++---- .../room/timeline/persistence/SyncWriter.js | 2 +- 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/src/matrix/room/timeline/persistence/MemberWriter.js b/src/matrix/room/timeline/persistence/MemberWriter.js index dc6f621d..73da64cd 100644 --- a/src/matrix/room/timeline/persistence/MemberWriter.js +++ b/src/matrix/room/timeline/persistence/MemberWriter.js @@ -73,7 +73,8 @@ export class MemberWriter { } } - async lookupMember(userId, timelineEvents, txn) { + async lookupSenderMember(event, timelineEvents, txn) { + const userId = event.sender; let member = this._cache.get(userId); if (!member) { const memberData = await txn.roomMembers.get(this._roomId, userId); @@ -83,12 +84,21 @@ export class MemberWriter { } } if (!member) { + let memberEvent; // sometimes the member event isn't included in state, but rather in the timeline, - // even if it is not the first event in the timeline. In this case, go look for the - // first occurence - const memberEvent = timelineEvents.find(e => { - return e.type === MEMBER_EVENT_TYPE && e.state_key === userId; - }); + // even if it is not the first event in the timeline. In this case, go look for + // the last one before the event + let foundEvent = false; + for (let i = timelineEvents.length - 1; i >= 0; i -= 1) { + const e = timelineEvents[i]; + if (!foundEvent && e.event_id === event.event_id) { + foundEvent = true; + } + if (foundEvent && e.type === MEMBER_EVENT_TYPE && e.state_key === userId) { + memberEvent = e; + break; + } + } if (memberEvent) { member = RoomMember.fromMemberEvent(this._roomId, memberEvent); } @@ -228,5 +238,13 @@ export function tests() { const change = await writer.writeTimelineMemberEvent(event, txn); assert(change); }, + "lookupSenderMember returns closest member in the past": async assert => { + const event1 = createMemberEvent("join", alice, "Alice"); + const event2 = createMemberEvent("join", alice, "Alies"); + const writer = new MemberWriter(roomId); + const txn = createStorage(); + const member = await writer.lookupSenderMember(event2, [event1, event2], txn); + assert.equal(member.displayName, "Alies"); + }, }; } diff --git a/src/matrix/room/timeline/persistence/SyncWriter.js b/src/matrix/room/timeline/persistence/SyncWriter.js index a7675993..1f251fce 100644 --- a/src/matrix/room/timeline/persistence/SyncWriter.js +++ b/src/matrix/room/timeline/persistence/SyncWriter.js @@ -162,7 +162,7 @@ export class SyncWriter { // store event in timeline currentKey = currentKey.nextKey(); const entry = createEventEntry(currentKey, this._roomId, event); - let member = await this._memberWriter.lookupMember(event.sender, events, txn); + let member = await this._memberWriter.lookupSenderMember(event, events, txn); if (member) { entry.displayName = member.displayName; entry.avatarUrl = member.avatarUrl; From 813be758d7b69e7f42d5fbf99bd8df6a8b0888a8 Mon Sep 17 00:00:00 2001 From: Bruno Windels Date: Thu, 8 Apr 2021 15:33:21 +0200 Subject: [PATCH 3/3] we should prefer to not pick the event itself if it's a member event but still fall back to that if it's a new join --- .../room/timeline/persistence/MemberWriter.js | 41 +++++++++++++------ .../room/timeline/persistence/SyncWriter.js | 2 +- 2 files changed, 29 insertions(+), 14 deletions(-) diff --git a/src/matrix/room/timeline/persistence/MemberWriter.js b/src/matrix/room/timeline/persistence/MemberWriter.js index 73da64cd..db649f68 100644 --- a/src/matrix/room/timeline/persistence/MemberWriter.js +++ b/src/matrix/room/timeline/persistence/MemberWriter.js @@ -73,8 +73,7 @@ export class MemberWriter { } } - async lookupSenderMember(event, timelineEvents, txn) { - const userId = event.sender; + async lookupMember(userId, event, timelineEvents, txn) { let member = this._cache.get(userId); if (!member) { const memberData = await txn.roomMembers.get(this._roomId, userId); @@ -84,23 +83,38 @@ export class MemberWriter { } } if (!member) { - let memberEvent; // sometimes the member event isn't included in state, but rather in the timeline, // even if it is not the first event in the timeline. In this case, go look for - // the last one before the event + // the last one before the event, or if none is found, + // the least recent matching member event in the timeline. + // The latter is needed because of new joins picking up their own display name let foundEvent = false; + let memberEventBefore; + let firstMemberEvent; for (let i = timelineEvents.length - 1; i >= 0; i -= 1) { const e = timelineEvents[i]; - if (!foundEvent && e.event_id === event.event_id) { - foundEvent = true; + let matchingEvent; + if (e.type === MEMBER_EVENT_TYPE && e.state_key === userId) { + matchingEvent = e; + firstMemberEvent = matchingEvent; } - if (foundEvent && e.type === MEMBER_EVENT_TYPE && e.state_key === userId) { - memberEvent = e; + if (!foundEvent) { + if (e.event_id === event.event_id) { + foundEvent = true; + } + } else if (matchingEvent) { + memberEventBefore = matchingEvent; break; } } - if (memberEvent) { - member = RoomMember.fromMemberEvent(this._roomId, memberEvent); + // first see if we found a member event before the event we're looking up the sender for + if (memberEventBefore) { + member = RoomMember.fromMemberEvent(this._roomId, memberEventBefore); + } + // and only if we didn't, fall back to the first member event, + // regardless of where it is positioned relative to the lookup event + else if (firstMemberEvent) { + member = RoomMember.fromMemberEvent(this._roomId, firstMemberEvent); } } return member; @@ -233,17 +247,18 @@ export function tests() { const event = createMemberEvent("join", alice, "Alice"); const writer = new MemberWriter(roomId); const txn = createStorage(); - const member = await writer.lookupSenderMember(event, [event], txn); + const member = await writer.lookupMember(event.sender, event, [event], txn); assert(member); const change = await writer.writeTimelineMemberEvent(event, txn); assert(change); }, - "lookupSenderMember returns closest member in the past": async assert => { + "lookupMember returns closest member in the past": async assert => { const event1 = createMemberEvent("join", alice, "Alice"); const event2 = createMemberEvent("join", alice, "Alies"); + const event3 = createMemberEvent("join", alice, "Alys"); const writer = new MemberWriter(roomId); const txn = createStorage(); - const member = await writer.lookupSenderMember(event2, [event1, event2], txn); + const member = await writer.lookupMember(event3.sender, event3, [event1, event2, event3], txn); assert.equal(member.displayName, "Alies"); }, }; diff --git a/src/matrix/room/timeline/persistence/SyncWriter.js b/src/matrix/room/timeline/persistence/SyncWriter.js index 1f251fce..4913ac53 100644 --- a/src/matrix/room/timeline/persistence/SyncWriter.js +++ b/src/matrix/room/timeline/persistence/SyncWriter.js @@ -162,7 +162,7 @@ export class SyncWriter { // store event in timeline currentKey = currentKey.nextKey(); const entry = createEventEntry(currentKey, this._roomId, event); - let member = await this._memberWriter.lookupSenderMember(event, events, txn); + let member = await this._memberWriter.lookupMember(event.sender, event, events, txn); if (member) { entry.displayName = member.displayName; entry.avatarUrl = member.avatarUrl;