Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Enable pagination for overlay timelines #10757

Merged
merged 3 commits into from
May 12, 2023
Merged

Enable pagination for overlay timelines #10757

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1617493
Update @types/jest to 29.2.6
robintown May 5, 2023
5c1d78c
Enable pagination for overlay timelines
robintown May 5, 2023
76dcaed
Merge branch 'develop' into overlay-pagination
robintown May 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@
"@types/fs-extra": "^11.0.0",
"@types/geojson": "^7946.0.8",
"@types/glob-to-regexp": "^0.4.1",
"@types/jest": "29.2.5",
"@types/jest": "29.2.6",
"@types/katex": "^0.16.0",
"@types/lodash": "^4.14.168",
"@types/modernizr": "^3.5.3",
Expand Down
202 changes: 152 additions & 50 deletions src/components/structures/TimelinePanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import { TimelineWindow } from "matrix-js-sdk/src/timeline-window";
import { EventType, RelationType } from "matrix-js-sdk/src/@types/event";
import { SyncState } from "matrix-js-sdk/src/sync";
import { RoomMember, RoomMemberEvent } from "matrix-js-sdk/src/models/room-member";
import { debounce, throttle } from "lodash";
import { debounce, findLastIndex, throttle } from "lodash";
import { logger } from "matrix-js-sdk/src/logger";
import { ClientEvent } from "matrix-js-sdk/src/client";
import { Thread, ThreadEvent } from "matrix-js-sdk/src/models/thread";
Expand Down Expand Up @@ -73,6 +73,12 @@ const debuglog = (...args: any[]): void => {
}
};

const overlaysBefore = (overlayEvent: MatrixEvent, mainEvent: MatrixEvent): boolean =>
overlayEvent.localTimestamp < mainEvent.localTimestamp;

const overlaysAfter = (overlayEvent: MatrixEvent, mainEvent: MatrixEvent): boolean =>
overlayEvent.localTimestamp >= mainEvent.localTimestamp;
Comment on lines +76 to +80
Copy link
Member

@t3chguy t3chguy May 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach isn't really right for aligning timelines - matrix-org/matrix-js-sdk#3325

localTimestamp is based on age which can even be negative

The time in milliseconds that has elapsed since the event was sent. This field is generated by the local homeserver, and may be incorrect if the local time on at least one of the two servers is out of sync, which can cause the age to either be negative or greater than it actually is.

Copy link
Member Author

@robintown robintown May 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately we do need to use a timestamp-based approach to event ordering here, since the events are from entirely different rooms, and as such the server doesn't give us any hints as to what order it received them in.

My understanding is that localTimestamp is the most consistent measure we have for creating an order relative to the local clock. Or would it be better to use origin_server_ts instead here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this warrants a spec discussion on how this is meant to be done

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've raised this as a problem needing further consideration in the spec repo: matrix-org/matrix-spec#1520

For now though, I would note that the localTimestamp approach to aligning overlay events is nothing new introduced by this PR, it's only been refactored here. My intent in extracting the comparison into these two functions, was so that we could easily switch to a better approach if one eventually presents itself. There's also some internal context to this which I'll send you in a DM


interface IProps {
// The js-sdk EventTimelineSet object for the timeline sequence we are
// representing. This may or may not have a room, depending on what it's
Expand All @@ -83,7 +89,6 @@ interface IProps {
// added to support virtual rooms
// events from the overlay timeline set will be added by localTimestamp
// into the main timeline
// back paging not yet supported
overlayTimelineSet?: EventTimelineSet;
// filter events from overlay timeline
overlayTimelineSetFilter?: (event: MatrixEvent) => boolean;
Expand Down Expand Up @@ -506,30 +511,64 @@ class TimelinePanel extends React.Component<IProps, IState> {
// this particular event should be the first or last to be unpaginated.
const eventId = scrollToken;

const marker = this.state.events.findIndex((ev) => {
return ev.getId() === eventId;
});
// The event in question could belong to either the main timeline or
// overlay timeline; let's check both
const mainEvents = this.timelineWindow!.getEvents();
const overlayEvents = this.overlayTimelineWindow?.getEvents() ?? [];

let marker = mainEvents.findIndex((ev) => ev.getId() === eventId);
let overlayMarker: number;
if (marker === -1) {
// The event must be from the overlay timeline instead
overlayMarker = overlayEvents.findIndex((ev) => ev.getId() === eventId);
marker = backwards
? findLastIndex(mainEvents, (ev) => overlaysAfter(overlayEvents[overlayMarker], ev))
: mainEvents.findIndex((ev) => overlaysBefore(overlayEvents[overlayMarker], ev));
} else {
overlayMarker = backwards
? findLastIndex(overlayEvents, (ev) => overlaysBefore(ev, mainEvents[marker]))
: overlayEvents.findIndex((ev) => overlaysAfter(ev, mainEvents[marker]));
}

// The number of events to unpaginate from the main timeline
let count: number;
if (marker === -1) {
count = 0;
} else {
count = backwards ? marker + 1 : mainEvents.length - marker;
}

const count = backwards ? marker + 1 : this.state.events.length - marker;
// The number of events to unpaginate from the overlay timeline
let overlayCount: number;
if (overlayMarker === -1) {
overlayCount = 0;
} else {
overlayCount = backwards ? overlayMarker + 1 : overlayEvents.length - overlayMarker;
}

if (count > 0) {
debuglog("Unpaginating", count, "in direction", dir);
this.timelineWindow?.unpaginate(count, backwards);
this.timelineWindow!.unpaginate(count, backwards);
}

const { events, liveEvents, firstVisibleEventIndex } = this.getEvents();
this.buildLegacyCallEventGroupers(events);
this.setState({
events,
liveEvents,
firstVisibleEventIndex,
});
if (overlayCount > 0) {
debuglog("Unpaginating", count, "from overlay timeline in direction", dir);
this.overlayTimelineWindow!.unpaginate(overlayCount, backwards);
}

// We can now paginate in the unpaginated direction
if (backwards) {
this.setState({ canBackPaginate: true });
} else {
this.setState({ canForwardPaginate: true });
}
const { events, liveEvents, firstVisibleEventIndex } = this.getEvents();
this.buildLegacyCallEventGroupers(events);
this.setState({
events,
liveEvents,
firstVisibleEventIndex,
});

// We can now paginate in the unpaginated direction
if (backwards) {
this.setState({ canBackPaginate: true });
} else {
this.setState({ canForwardPaginate: true });
}
};

Expand Down Expand Up @@ -572,11 +611,15 @@ class TimelinePanel extends React.Component<IProps, IState> {
debuglog("Initiating paginate; backwards:" + backwards);
this.setState<null>({ [paginatingKey]: true });

return this.onPaginationRequest(this.timelineWindow, dir, PAGINATE_SIZE).then((r) => {
return this.onPaginationRequest(this.timelineWindow, dir, PAGINATE_SIZE).then(async (r) => {
if (this.unmounted) {
return false;
}

if (this.overlayTimelineWindow) {
await this.extendOverlayWindowToCoverMainWindow();
}

debuglog("paginate complete backwards:" + backwards + "; success:" + r);

const { events, liveEvents, firstVisibleEventIndex } = this.getEvents();
Expand Down Expand Up @@ -769,8 +812,15 @@ class TimelinePanel extends React.Component<IProps, IState> {
});
};

private hasTimelineSetFor(roomId: string | undefined): boolean {
return (
(roomId !== undefined && roomId === this.props.timelineSet.room?.roomId) ||
roomId === this.props.overlayTimelineSet?.room?.roomId
);
}

private onRoomTimelineReset = (room: Room | undefined, timelineSet: EventTimelineSet): void => {
if (timelineSet !== this.props.timelineSet) return;
if (timelineSet !== this.props.timelineSet && timelineSet !== this.props.overlayTimelineSet) return;

if (this.canResetTimeline()) {
this.loadTimeline();
Expand All @@ -783,7 +833,7 @@ class TimelinePanel extends React.Component<IProps, IState> {
if (this.unmounted) return;

// ignore events for other rooms
if (room !== this.props.timelineSet.room) return;
if (!this.hasTimelineSetFor(room.roomId)) return;

// we could skip an update if the event isn't in our timeline,
// but that's probably an early optimisation.
Expand All @@ -796,10 +846,7 @@ class TimelinePanel extends React.Component<IProps, IState> {
}

// ignore events for other rooms
const roomId = thread.roomId;
if (roomId !== this.props.timelineSet.room?.roomId) {
return;
}
if (!this.hasTimelineSetFor(thread.roomId)) return;

// we could skip an update if the event isn't in our timeline,
// but that's probably an early optimisation.
Expand All @@ -817,10 +864,7 @@ class TimelinePanel extends React.Component<IProps, IState> {
}

// ignore events for other rooms
const roomId = ev.getRoomId();
if (roomId !== this.props.timelineSet.room?.roomId) {
return;
}
if (!this.hasTimelineSetFor(ev.getRoomId())) return;

// we could skip an update if the event isn't in our timeline,
// but that's probably an early optimisation.
Expand All @@ -834,7 +878,7 @@ class TimelinePanel extends React.Component<IProps, IState> {
if (this.unmounted) return;

// ignore events for other rooms
if (member.roomId !== this.props.timelineSet.room?.roomId) return;
if (!this.hasTimelineSetFor(member.roomId)) return;

// ignore events for other users
if (member.userId != MatrixClientPeg.get().credentials?.userId) return;
Expand All @@ -857,7 +901,7 @@ class TimelinePanel extends React.Component<IProps, IState> {
if (this.unmounted) return;

// ignore events for other rooms
if (replacedEvent.getRoomId() !== this.props.timelineSet.room?.roomId) return;
if (!this.hasTimelineSetFor(replacedEvent.getRoomId())) return;

// we could skip an update if the event isn't in our timeline,
// but that's probably an early optimisation.
Expand All @@ -877,7 +921,7 @@ class TimelinePanel extends React.Component<IProps, IState> {
if (this.unmounted) return;

// ignore events for other rooms
if (room !== this.props.timelineSet.room) return;
if (!this.hasTimelineSetFor(room.roomId)) return;

this.reloadEvents();
};
Expand Down Expand Up @@ -905,7 +949,7 @@ class TimelinePanel extends React.Component<IProps, IState> {
// Can be null for the notification timeline, etc.
if (!this.props.timelineSet.room) return;

if (ev.getRoomId() !== this.props.timelineSet.room.roomId) return;
if (!this.hasTimelineSetFor(ev.getRoomId())) return;

if (!this.state.events.includes(ev)) return;

Expand Down Expand Up @@ -1380,6 +1424,48 @@ class TimelinePanel extends React.Component<IProps, IState> {
});
}

private async extendOverlayWindowToCoverMainWindow(): Promise<void> {
const mainWindow = this.timelineWindow!;
const overlayWindow = this.overlayTimelineWindow!;
const mainEvents = mainWindow.getEvents();

if (mainEvents.length > 0) {
let paginationRequests: Promise<unknown>[];

// Keep paginating until the main window is covered
do {
paginationRequests = [];
const overlayEvents = overlayWindow.getEvents();

if (
overlayWindow.canPaginate(EventTimeline.BACKWARDS) &&
(overlayEvents.length === 0 ||
overlaysAfter(overlayEvents[0], mainEvents[0]) ||
!mainWindow.canPaginate(EventTimeline.BACKWARDS))
) {
// Paginating backwards could reveal more events to be overlaid in the main window
paginationRequests.push(
this.onPaginationRequest(overlayWindow, EventTimeline.BACKWARDS, PAGINATE_SIZE),
);
}

if (
overlayWindow.canPaginate(EventTimeline.FORWARDS) &&
(overlayEvents.length === 0 ||
overlaysBefore(overlayEvents.at(-1)!, mainEvents.at(-1)!) ||
!mainWindow.canPaginate(EventTimeline.FORWARDS))
) {
// Paginating forwards could reveal more events to be overlaid in the main window
paginationRequests.push(
this.onPaginationRequest(overlayWindow, EventTimeline.FORWARDS, PAGINATE_SIZE),
);
}

await Promise.all(paginationRequests);
} while (paginationRequests.length > 0);
}
}

/**
* (re)-load the event timeline, and initialise the scroll state, centered
* around the given event.
Expand Down Expand Up @@ -1417,8 +1503,14 @@ class TimelinePanel extends React.Component<IProps, IState> {

this.setState(
{
canBackPaginate: !!this.timelineWindow?.canPaginate(EventTimeline.BACKWARDS),
canForwardPaginate: !!this.timelineWindow?.canPaginate(EventTimeline.FORWARDS),
canBackPaginate:
(this.timelineWindow?.canPaginate(EventTimeline.BACKWARDS) ||
this.overlayTimelineWindow?.canPaginate(EventTimeline.BACKWARDS)) ??
false,
canForwardPaginate:
(this.timelineWindow?.canPaginate(EventTimeline.FORWARDS) ||
this.overlayTimelineWindow?.canPaginate(EventTimeline.FORWARDS)) ??
false,
timelineLoading: false,
},
() => {
Expand Down Expand Up @@ -1494,21 +1586,21 @@ class TimelinePanel extends React.Component<IProps, IState> {
// This is a hot-path optimization by skipping a promise tick
// by repeating a no-op sync branch in
// TimelineSet.getTimelineForEvent & MatrixClient.getEventTimeline
if (this.props.timelineSet.getTimelineForEvent(eventId)) {
if (this.props.timelineSet.getTimelineForEvent(eventId) && !this.overlayTimelineWindow) {
// if we've got an eventId, and the timeline exists, we can skip
// the promise tick.
this.timelineWindow.load(eventId, INITIAL_SIZE);
this.overlayTimelineWindow?.load(undefined, INITIAL_SIZE);
// in this branch this method will happen in sync time
onLoaded();
return;
}

const prom = this.timelineWindow.load(eventId, INITIAL_SIZE).then(async (): Promise<void> => {
if (this.overlayTimelineWindow) {
// @TODO(kerrya) use timestampToEvent to load the overlay timeline
// TODO: use timestampToEvent to load the overlay timeline
// with more correct position when main TL eventId is truthy
await this.overlayTimelineWindow.load(undefined, INITIAL_SIZE);
await this.extendOverlayWindowToCoverMainWindow();
}
});
this.buildLegacyCallEventGroupers();
Expand Down Expand Up @@ -1541,23 +1633,33 @@ class TimelinePanel extends React.Component<IProps, IState> {
this.reloadEvents();
}

// get the list of events from the timeline window and the pending event list
// get the list of events from the timeline windows and the pending event list
private getEvents(): Pick<IState, "events" | "liveEvents" | "firstVisibleEventIndex"> {
const mainEvents: MatrixEvent[] = this.timelineWindow?.getEvents() || [];
const eventFilter = this.props.overlayTimelineSetFilter || Boolean;
const overlayEvents = this.overlayTimelineWindow?.getEvents().filter(eventFilter) || [];
const mainEvents = this.timelineWindow!.getEvents();
let overlayEvents = this.overlayTimelineWindow?.getEvents() ?? [];
if (this.props.overlayTimelineSetFilter !== undefined) {
overlayEvents = overlayEvents.filter(this.props.overlayTimelineSetFilter);
}

// maintain the main timeline event order as returned from the HS
// merge overlay events at approximately the right position based on local timestamp
const events = overlayEvents.reduce(
(acc: MatrixEvent[], overlayEvent: MatrixEvent) => {
// find the first main tl event with a later timestamp
const index = acc.findIndex((event) => event.localTimestamp > overlayEvent.localTimestamp);
const index = acc.findIndex((event) => overlaysBefore(overlayEvent, event));
// insert overlay event into timeline at approximately the right place
if (index > -1) {
acc.splice(index, 0, overlayEvent);
// if it's beyond the edge of the main window, hide it so that expanding
// the main window doesn't cause new events to pop in and change its position
if (index === -1) {
if (!this.timelineWindow!.canPaginate(EventTimeline.FORWARDS)) {
acc.push(overlayEvent);
}
} else if (index === 0) {
if (!this.timelineWindow!.canPaginate(EventTimeline.BACKWARDS)) {
acc.unshift(overlayEvent);
}
} else {
acc.push(overlayEvent);
acc.splice(index, 0, overlayEvent);
}
return acc;
},
Expand All @@ -1574,14 +1676,14 @@ class TimelinePanel extends React.Component<IProps, IState> {
client.decryptEventIfNeeded(event);
});

const firstVisibleEventIndex = this.checkForPreJoinUISI(mainEvents);
const firstVisibleEventIndex = this.checkForPreJoinUISI(events);

// Hold onto the live events separately. The read receipt and read marker
// should use this list, so that they don't advance into pending events.
const liveEvents = [...events];

// if we're at the end of the live timeline, append the pending events
if (!this.timelineWindow?.canPaginate(EventTimeline.FORWARDS)) {
if (!this.timelineWindow!.canPaginate(EventTimeline.FORWARDS)) {
const pendingEvents = this.props.timelineSet.getPendingEvents();
events.push(
...pendingEvents.filter((event) => {
Expand Down
Loading