From 43a54060cffb0a79884dc0911b35f06d80c58f8f Mon Sep 17 00:00:00 2001 From: Michael Weimann Date: Wed, 26 Apr 2023 17:19:12 +0200 Subject: [PATCH 1/5] Add room.getLastLiveEvent and remove room.lastThread --- spec/unit/room.spec.ts | 85 +++++++++++++++++++++++++++++++++++++++++- src/models/room.ts | 35 ++++++++++++----- 2 files changed, 109 insertions(+), 11 deletions(-) diff --git a/spec/unit/room.spec.ts b/spec/unit/room.spec.ts index df1b060403d..f3122636250 100644 --- a/spec/unit/room.spec.ts +++ b/spec/unit/room.spec.ts @@ -54,6 +54,7 @@ import { Crypto } from "../../src/crypto"; import { mkThread } from "../test-utils/thread"; import { getMockClientWithEventEmitter, mockClientMethodsUser } from "../test-utils/client"; import { logger } from "../../src/logger"; +import { IMessageOpts } from "../test-utils/test-utils"; describe("Room", function () { const roomId = "!foo:bar"; @@ -63,9 +64,10 @@ describe("Room", function () { const userD = "@dorothy:bar"; let room: Room; - const mkMessage = () => + const mkMessage = (opts?: Partial) => utils.mkMessage( { + ...opts, event: true, user: userA, room: roomId, @@ -113,9 +115,10 @@ describe("Room", function () { room.client, ); - const mkThreadResponse = (root: MatrixEvent) => + const mkThreadResponse = (root: MatrixEvent, opts?: Partial) => utils.mkEvent( { + ...opts, event: true, type: EventType.RoomMessage, user: userA, @@ -165,6 +168,32 @@ describe("Room", function () { room.client, ); + const addRoomMainAndThreadMessages = ( + room: Room, + tsMain?: number, + tsThread?: number, + ): { mainEvent?: MatrixEvent; threadEvent?: MatrixEvent } => { + const result: { mainEvent?: MatrixEvent; threadEvent?: MatrixEvent } = {}; + + if (tsMain) { + result.mainEvent = mkMessage({ ts: tsMain }); + room.addLiveEvents([result.mainEvent]); + } + + if (tsThread) { + const { rootEvent, thread } = mkThread({ + room, + client: new TestClient().client, + authorId: "@bob:example.org", + participantUserIds: ["@bob:example.org"], + }); + result.threadEvent = mkThreadResponse(rootEvent, { ts: tsThread }); + thread.liveTimeline.addEvent(result.threadEvent, true); + } + + return result; + }; + beforeEach(function () { room = new Room(roomId, new TestClient(userA, "device").client, userA); // mock RoomStates @@ -3475,4 +3504,56 @@ describe("Room", function () { expect(room.findPredecessor()).toBeNull(); }); }); + + describe("getLastLiveEvent", () => { + let lastEventInMainTimeline: MatrixEvent; + let lastEventInThread: MatrixEvent; + + it("when there are no events, it should return undefined", () => { + expect(room.getLastLiveEvent()).toBeUndefined(); + }); + + describe("when there is only an event in the main timeline and there are no threads", () => { + beforeEach(() => { + lastEventInMainTimeline = addRoomMainAndThreadMessages(room, 23).mainEvent!; + room.addLiveEvents([lastEventInMainTimeline]); + }); + + it("should return the last event from the main timeline", () => { + expect(room.getLastLiveEvent()).toBe(lastEventInMainTimeline); + }); + }); + + describe("when there is no event in the room live timeline, but in a thread", () => { + beforeEach(() => { + lastEventInThread = addRoomMainAndThreadMessages(room, undefined, 42).threadEvent!; + }); + + it("should return the last event from the thread", () => { + expect(room.getLastLiveEvent()).toBe(lastEventInThread); + }); + }); + + describe("when there are events in both, the main timeline and threads", () => { + describe("and the last event is in a thread", () => { + beforeEach(() => { + lastEventInThread = addRoomMainAndThreadMessages(room, 23, 42).threadEvent!; + }); + + it("should return the last event from the thread", () => { + expect(room.getLastLiveEvent()).toBe(lastEventInThread); + }); + }); + + describe("and the last event is in the main timeline", () => { + beforeEach(() => { + lastEventInMainTimeline = addRoomMainAndThreadMessages(room, 42, 23).mainEvent!; + }); + + it("should return the last event from the main timeline", () => { + expect(room.getLastLiveEvent()).toBe(lastEventInMainTimeline); + }); + }); + }); + }); }); diff --git a/src/models/room.ts b/src/models/room.ts index af5f9f0b06e..3ecae4a1fc2 100644 --- a/src/models/room.ts +++ b/src/models/room.ts @@ -396,7 +396,6 @@ export class Room extends ReadReceipt { * This is not a comprehensive list of the threads that exist in this room */ private threads = new Map(); - public lastThread?: Thread; /** * A mapping of eventId to all visibility changes to apply @@ -785,6 +784,32 @@ export class Room extends ReadReceipt { } } + /** + * Returns the last live event of this room. + * "last" means latest timestamp. + * "live of this room" means from all live timelines: the room and the threads. + * + * @returns MatrixEvent if there is a last event; else undefined. + */ + public getLastLiveEvent(): MatrixEvent | undefined { + const roomEvents = this.getLiveTimeline().getEvents(); + const lastRoomEvent = roomEvents[roomEvents.length - 1] as MatrixEvent | undefined; + + return this.getThreads().reduce( + (lastEventSoFar: MatrixEvent | undefined, thread: Thread) => { + const lastThreadEvent = thread.events[thread.events.length - 1]; + + if (!lastEventSoFar || (lastThreadEvent?.getTs() ?? 0) > (lastEventSoFar?.getTs() ?? 0)) { + // no last-event-so-far or last thread event is newer than the last-event-so-far + return lastThreadEvent; + } + + return lastEventSoFar; + }, + lastRoomEvent, + ); + } + /** * @returns the membership type (join | leave | invite) for the logged in user */ @@ -2212,14 +2237,6 @@ export class Room extends ReadReceipt { RoomEvent.Timeline, RoomEvent.TimelineReset, ]); - const isNewer = - this.lastThread?.rootEvent && - rootEvent?.localTimestamp && - this.lastThread.rootEvent?.localTimestamp < rootEvent?.localTimestamp; - - if (!this.lastThread || isNewer) { - this.lastThread = thread; - } if (this.threadsReady) { this.updateThreadRootEvents(thread, toStartOfTimeline, false); From bc209e652819abdd3ea19813e8a2ecb729477ef6 Mon Sep 17 00:00:00 2001 From: Michael Weimann Date: Thu, 27 Apr 2023 10:39:03 +0200 Subject: [PATCH 2/5] Deprecate Room.lastThread --- spec/unit/room.spec.ts | 69 ++++++++++++++++++++++++++++++++++++++++-- src/models/room.ts | 48 ++++++++++++++++++++++------- 2 files changed, 104 insertions(+), 13 deletions(-) diff --git a/spec/unit/room.spec.ts b/spec/unit/room.spec.ts index f3122636250..385fbbcfee5 100644 --- a/spec/unit/room.spec.ts +++ b/spec/unit/room.spec.ts @@ -19,7 +19,7 @@ limitations under the License. */ import { mocked } from "jest-mock"; -import { M_POLL_KIND_DISCLOSED, M_POLL_RESPONSE, M_POLL_START, PollStartEvent } from "matrix-events-sdk"; +import { M_POLL_KIND_DISCLOSED, M_POLL_RESPONSE, M_POLL_START, Optional, PollStartEvent } from "matrix-events-sdk"; import * as utils from "../test-utils/test-utils"; import { emitPromise } from "../test-utils/test-utils"; @@ -188,7 +188,41 @@ describe("Room", function () { participantUserIds: ["@bob:example.org"], }); result.threadEvent = mkThreadResponse(rootEvent, { ts: tsThread }); - thread.liveTimeline.addEvent(result.threadEvent, true); + thread.liveTimeline.addEvent(result.threadEvent, { toStartOfTimeline: true }); + } + + return result; + }; + + const addRoomThreads = ( + room: Room, + thread1EventTs: Optional, + thread2EventTs: Optional, + ): { thread1?: Thread; thread2?: Thread } => { + const result: { thread1?: Thread; thread2?: Thread } = {}; + + if (thread1EventTs !== null) { + const { rootEvent: thread1RootEvent, thread: thread1 } = mkThread({ + room, + client: new TestClient().client, + authorId: "@bob:example.org", + participantUserIds: ["@bob:example.org"], + }); + const thread1Event = mkThreadResponse(thread1RootEvent, { ts: thread1EventTs }); + thread1.liveTimeline.addEvent(thread1Event, { toStartOfTimeline: true }); + result.thread1 = thread1; + } + + if (thread2EventTs !== null) { + const { rootEvent: thread2RootEvent, thread: thread2 } = mkThread({ + room, + client: new TestClient().client, + authorId: "@bob:example.org", + participantUserIds: ["@bob:example.org"], + }); + const thread2Event = mkThreadResponse(thread2RootEvent, { ts: thread2EventTs }); + thread2.liveTimeline.addEvent(thread2Event, { toStartOfTimeline: true }); + result.thread2 = thread2; } return result; @@ -3556,4 +3590,35 @@ describe("Room", function () { }); }); }); + + describe("getLastThread", () => { + it("when there is no thread, it should return undefined", () => { + expect(room.getLastThread()).toBeUndefined(); + }); + + it("when there is only one thread, it should return this one", () => { + const { thread1 } = addRoomThreads(room, 23, null); + expect(room.getLastThread()).toBe(thread1); + }); + + it("when there are tho threads, it should return the one with the recent event I", () => { + const { thread2 } = addRoomThreads(room, 23, 42); + expect(room.getLastThread()).toBe(thread2); + }); + + it("when there are tho threads, it should return the one with the recent event II", () => { + const { thread1 } = addRoomThreads(room, 42, 23); + expect(room.getLastThread()).toBe(thread1); + }); + + it("when there is a thread with the last event ts undefined, it should return the thread with the defined event ts", () => { + const { thread2 } = addRoomThreads(room, undefined, 23); + expect(room.getLastThread()).toBe(thread2); + }); + + it("when the last event ts of all threads is undefined, it should return the last added thread", () => { + const { thread2 } = addRoomThreads(room, undefined, undefined); + expect(room.getLastThread()).toBe(thread2); + }); + }); }); diff --git a/src/models/room.ts b/src/models/room.ts index 3ecae4a1fc2..aa2e0f0705c 100644 --- a/src/models/room.ts +++ b/src/models/room.ts @@ -397,6 +397,11 @@ export class Room extends ReadReceipt { */ private threads = new Map(); + /** + * @deprecated use {@link Room.getLastThread} instead + */ + public lastThread?: Thread; + /** * A mapping of eventId to all visibility changes to apply * to the event, by chronological order, as per @@ -794,20 +799,33 @@ export class Room extends ReadReceipt { public getLastLiveEvent(): MatrixEvent | undefined { const roomEvents = this.getLiveTimeline().getEvents(); const lastRoomEvent = roomEvents[roomEvents.length - 1] as MatrixEvent | undefined; + const lastThread = this.getLastThread(); - return this.getThreads().reduce( - (lastEventSoFar: MatrixEvent | undefined, thread: Thread) => { - const lastThreadEvent = thread.events[thread.events.length - 1]; + if (!lastThread) return lastRoomEvent; - if (!lastEventSoFar || (lastThreadEvent?.getTs() ?? 0) > (lastEventSoFar?.getTs() ?? 0)) { - // no last-event-so-far or last thread event is newer than the last-event-so-far - return lastThreadEvent; - } + const lastThreadEvent = lastThread.events[lastThread.events.length - 1]; - return lastEventSoFar; - }, - lastRoomEvent, - ); + return (lastRoomEvent?.getTs() ?? 0) > (lastThreadEvent.getTs() ?? 0) ? lastRoomEvent : lastThreadEvent; + } + + /** + * @returns the thread with the most recent event in its live time line. undefined if there is no thread. + */ + public getLastThread(): Thread | undefined { + return this.getThreads().reduce((lastThread: Thread | undefined, thread: Thread) => { + if (!lastThread) return thread; + + const threadEvent = thread.events[thread.events.length - 1]; + const lastThreadEvent = lastThread.events[lastThread.events.length - 1]; + + if ((threadEvent?.getTs() ?? 0) >= (lastThreadEvent?.getTs() ?? 0)) { + // Last message of current thread is newer → new last thread. + // Equal also means newer, because it was added to the thread map later. + return thread; + } + + return lastThread; + }, undefined); } /** @@ -2237,6 +2255,14 @@ export class Room extends ReadReceipt { RoomEvent.Timeline, RoomEvent.TimelineReset, ]); + const isNewer = + this.lastThread?.rootEvent && + rootEvent?.localTimestamp && + this.lastThread.rootEvent?.localTimestamp < rootEvent?.localTimestamp; + + if (!this.lastThread || isNewer) { + this.lastThread = thread; + } if (this.threadsReady) { this.updateThreadRootEvents(thread, toStartOfTimeline, false); From 24b78e5f677d19464f17ed190b8995c433e25bee Mon Sep 17 00:00:00 2001 From: Michael Weimann Date: Thu, 27 Apr 2023 10:52:22 +0200 Subject: [PATCH 3/5] Add comments about timestamps --- src/models/room.ts | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/models/room.ts b/src/models/room.ts index aa2e0f0705c..9ee62c74b9c 100644 --- a/src/models/room.ts +++ b/src/models/room.ts @@ -792,6 +792,9 @@ export class Room extends ReadReceipt { /** * Returns the last live event of this room. * "last" means latest timestamp. + * Instead of using timestamps, it would be better to do the comparison based on the order of the homeserver DAG. + * Unfortunately, this information is currently not available in the client. + * See {@link https://github.com/matrix-org/matrix-js-sdk/issues/3325}. * "live of this room" means from all live timelines: the room and the threads. * * @returns MatrixEvent if there is a last event; else undefined. @@ -809,6 +812,12 @@ export class Room extends ReadReceipt { } /** + * Returns the last thread of this room. + * "last" means latest timestamp of the last thread event. + * Instead of using timestamps, it would be better to do the comparison based on the order of the homeserver DAG. + * Unfortunately, this information is currently not available in the client. + * See {@link https://github.com/matrix-org/matrix-js-sdk/issues/3325}. + * * @returns the thread with the most recent event in its live time line. undefined if there is no thread. */ public getLastThread(): Thread | undefined { From 130c518f4e0253a257ed8b46cee7130fbd0472ca Mon Sep 17 00:00:00 2001 From: Michael Weimann Date: Thu, 27 Apr 2023 11:14:28 +0200 Subject: [PATCH 4/5] Improve lastThread prop doc --- src/models/room.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/models/room.ts b/src/models/room.ts index 9ee62c74b9c..439bd681998 100644 --- a/src/models/room.ts +++ b/src/models/room.ts @@ -398,7 +398,8 @@ export class Room extends ReadReceipt { private threads = new Map(); /** - * @deprecated use {@link Room.getLastThread} instead + * @deprecated This value is unreliable. It may not contain the last thread. + * Use {@link Room.getLastThread} instead. */ public lastThread?: Thread; From 06f44f6d28909cea3d33448aca83983b3c49fc27 Mon Sep 17 00:00:00 2001 From: Michael Weimann Date: Thu, 27 Apr 2023 11:51:42 +0200 Subject: [PATCH 5/5] Simplify test structure --- spec/unit/room.spec.ts | 46 ++++++++++++------------------------------ 1 file changed, 13 insertions(+), 33 deletions(-) diff --git a/spec/unit/room.spec.ts b/spec/unit/room.spec.ts index 385fbbcfee5..8caca90bee8 100644 --- a/spec/unit/room.spec.ts +++ b/spec/unit/room.spec.ts @@ -3547,46 +3547,26 @@ describe("Room", function () { expect(room.getLastLiveEvent()).toBeUndefined(); }); - describe("when there is only an event in the main timeline and there are no threads", () => { - beforeEach(() => { - lastEventInMainTimeline = addRoomMainAndThreadMessages(room, 23).mainEvent!; - room.addLiveEvents([lastEventInMainTimeline]); - }); - - it("should return the last event from the main timeline", () => { - expect(room.getLastLiveEvent()).toBe(lastEventInMainTimeline); - }); + it("when there is only an event in the main timeline and there are no threads, it should return the last event from the main timeline", () => { + lastEventInMainTimeline = addRoomMainAndThreadMessages(room, 23).mainEvent!; + room.addLiveEvents([lastEventInMainTimeline]); + expect(room.getLastLiveEvent()).toBe(lastEventInMainTimeline); }); - describe("when there is no event in the room live timeline, but in a thread", () => { - beforeEach(() => { - lastEventInThread = addRoomMainAndThreadMessages(room, undefined, 42).threadEvent!; - }); - - it("should return the last event from the thread", () => { - expect(room.getLastLiveEvent()).toBe(lastEventInThread); - }); + it("when there is no event in the room live timeline but in a thread, it should return the last event from the thread", () => { + lastEventInThread = addRoomMainAndThreadMessages(room, undefined, 42).threadEvent!; + expect(room.getLastLiveEvent()).toBe(lastEventInThread); }); describe("when there are events in both, the main timeline and threads", () => { - describe("and the last event is in a thread", () => { - beforeEach(() => { - lastEventInThread = addRoomMainAndThreadMessages(room, 23, 42).threadEvent!; - }); - - it("should return the last event from the thread", () => { - expect(room.getLastLiveEvent()).toBe(lastEventInThread); - }); + it("and the last event is in a thread, it should return the last event from the thread", () => { + lastEventInThread = addRoomMainAndThreadMessages(room, 23, 42).threadEvent!; + expect(room.getLastLiveEvent()).toBe(lastEventInThread); }); - describe("and the last event is in the main timeline", () => { - beforeEach(() => { - lastEventInMainTimeline = addRoomMainAndThreadMessages(room, 42, 23).mainEvent!; - }); - - it("should return the last event from the main timeline", () => { - expect(room.getLastLiveEvent()).toBe(lastEventInMainTimeline); - }); + it("and the last event is in the main timeline, it should return the last event from the main timeline", () => { + lastEventInMainTimeline = addRoomMainAndThreadMessages(room, 42, 23).mainEvent!; + expect(room.getLastLiveEvent()).toBe(lastEventInMainTimeline); }); }); });