This repository has been archived by the owner on Sep 11, 2024. It is now read-only.
Ask to refresh timeline when historical messages are imported (MSC2716) #8303
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -79,6 +79,7 @@ interface IState { | |
| syncStateData: ISyncStateData; | ||
| unsentMessages: MatrixEvent[]; | ||
| isResending: boolean; | ||
| timelineNeedsRefresh: boolean; | ||
| } | ||
|
|
||
| export default class RoomStatusBar extends React.PureComponent<IProps, IState> { | ||
|
|
@@ -93,13 +94,15 @@ export default class RoomStatusBar extends React.PureComponent<IProps, IState> { | |
| syncStateData: this.context.getSyncStateData(), | ||
| unsentMessages: getUnsentMessages(this.props.room), | ||
| isResending: false, | ||
| timelineNeedsRefresh: true // TODO: Put this back to this.props.room.getTimelineNeedsRefresh(), | ||
| }; | ||
| } | ||
|
|
||
| public componentDidMount(): void { | ||
| const client = this.context; | ||
| client.on("sync", this.onSyncStateChange); | ||
| client.on("Room.localEchoUpdated", this.onRoomLocalEchoUpdated); | ||
| client.on("Room.historyImportedWithinTimeline", this.onRoomHistoryImportedWithinTimeline); | ||
|
|
||
| this.checkSize(); | ||
| } | ||
|
|
@@ -115,6 +118,7 @@ export default class RoomStatusBar extends React.PureComponent<IProps, IState> { | |
| if (client) { | ||
| client.removeListener("sync", this.onSyncStateChange); | ||
| client.removeListener("Room.localEchoUpdated", this.onRoomLocalEchoUpdated); | ||
| client.removeListener("Room.historyImportedWithinTimeline", this.onRoomHistoryImportedWithinTimeline); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -142,6 +146,22 @@ export default class RoomStatusBar extends React.PureComponent<IProps, IState> { | |
| dis.fire(Action.FocusSendMessageComposer); | ||
| }; | ||
|
|
||
| private onRefreshTimelineClick = (): void => { | ||
| console.log('TODO: Refresh timeline'); | ||
| // TODO: What's the best way to refresh the timeline? Something like | ||
| // `room.resetLiveTimeline(null, null);` although this just seems to | ||
| // clear the timeline. I also tried to split out | ||
| // `scrollbackFromPaginationToken` from the `scrollback` method in to | ||
| // paginate from the beginning of the room but it's just not right. | ||
|
There was a problem hiding this comment. What's the best way to refresh the timeline? Something like I also tried to split out the logic from scrollbackFromPaginationToken refactor /**
* Retrieve older messages from the given room and put them in the timeline.
*
* If this is called multiple times whilst a request is ongoing, the <i>same</i>
* Promise will be returned. If there was a problem requesting scrollback, there
* will be a small delay before another request can be made (to prevent tight-looping
* when there is no connection).
*
* @param {Room} room The room to get older messages in.
* @param {Integer} limit Optional. The maximum number of previous events to
* pull in. Default: 30.
* @param {module:client.callback} callback Optional.
* @return {Promise} Resolves: Room. If you are at the beginning
* of the timeline, <code>Room.oldState.paginationToken</code> will be
* <code>null</code>.
* @return {module:http-api.MatrixError} Rejects: with an error response.
*/
public scrollback(room: Room, limit = 30, callback?: Callback): Promise<Room> {
if (utils.isFunction(limit)) {
callback = limit as any as Callback; // legacy
limit = undefined;
}
let timeToWaitMs = 0;
let info = this.ongoingScrollbacks[room.roomId] || {};
if (info.promise) {
return info.promise;
} else if (info.errorTs) {
const timeWaitedMs = Date.now() - info.errorTs;
timeToWaitMs = Math.max(SCROLLBACK_DELAY_MS - timeWaitedMs, 0);
}
if (room.oldState.paginationToken === null) {
return Promise.resolve(room); // already at the start.
}
// attempt to grab more events from the store first
const numAdded = this.store.scrollback(room, limit).length;
if (numAdded === limit) {
// store contained everything we needed.
return Promise.resolve(room);
}
// reduce the required number of events appropriately
limit = limit - numAdded;
const prom = Promise.resolve().then(async () => {
try {
// wait for a time before doing this request
// (which may be 0 in order not to special case the code paths)
await sleep(timeToWaitMs)
await this.scrollbackFromPaginationToken({
room,
fromToken: room.oldState.paginationToken,
direction: Direction.Backward,
limit,
})
this.ongoingScrollbacks[room.roomId] = null;
callback?.(null, room);
return room;
} catch(err) {
this.ongoingScrollbacks[room.roomId] = {
errorTs: Date.now(),
};
callback?.(err);
throw err;
}
});
info = {
promise: prom,
errorTs: null,
};
this.ongoingScrollbacks[room.roomId] = info;
return prom;
}
public async scrollbackFromPaginationToken({
room,
fromToken,
direction,
limit,
}: {
room: Room,
fromToken: string | null,
direction: Direction,
limit?: number,
}) {
const res: IMessagesResponse = await this.createMessagesRequest(
room.roomId,
fromToken,
limit,
direction,
);
const matrixEvents = res.chunk.map(this.getEventMapper());
if (res.state) {
const stateEvents = res.state.map(this.getEventMapper());
room.currentState.setUnknownStateEvents(stateEvents);
}
const [timelineEvents, threadedEvents] = this.partitionThreadedEvents(room, matrixEvents);
room.addEventsToTimeline(timelineEvents, true, room.getLiveTimeline());
await this.processThreadEvents(room, threadedEvents, true);
room.oldState.paginationToken = res.end;
if (res.chunk.length === 0) {
room.oldState.paginationToken = null;
}
this.store.storeEvents(room, matrixEvents, res.end, true);
}There was a problem hiding this comment. Assigned review to get some advice for this question ^ Otherwise just need to add tests for this PR |
||
| this.props.room.refreshLiveTimeline(); | ||
| //timelinePanel.refreshTimeline(); | ||
|
|
||
| // TODO: Uncomment | ||
| // this.setState({ | ||
| // timelineNeedsRefresh: false, | ||
| // }); | ||
| }; | ||
|
There was a problem hiding this comment. I think the behavior of all of this would be best served by an end-to-end test. It looks like we just added Cypress recently but it's missing all of the utility necessary to complete something like this.
I can't find an overall issue describing the need/want to use Cypress so it's unclear how much we want to move forward with it. I've had many troubles using Cypress with Gitter. It seems like we have other e2e tests using Puppeteer but I'm guessing we want to move all of these to Cypress. Better place I should be adding some e2e tests or approaching this? There was a problem hiding this comment. Conversation continued at #8354 (comment) |
||
|
|
||
| private onRoomLocalEchoUpdated = (ev: MatrixEvent, room: Room) => { | ||
| if (room.roomId !== this.props.room.roomId) return; | ||
| const messages = getUnsentMessages(this.props.room); | ||
|
|
@@ -151,6 +171,14 @@ export default class RoomStatusBar extends React.PureComponent<IProps, IState> { | |
| }); | ||
| }; | ||
|
|
||
| private onRoomHistoryImportedWithinTimeline = (markerEv: MatrixEvent, room: Room) => { | ||
| if (room.roomId !== this.props.room.roomId) return; | ||
|
|
||
| this.setState({ | ||
| timelineNeedsRefresh: room.getTimelineNeedsRefresh(), | ||
| }); | ||
| }; | ||
|
|
||
| // Check whether current size is greater than 0, if yes call props.onVisible | ||
| private checkSize(): void { | ||
| if (this.getSize()) { | ||
|
|
@@ -166,7 +194,11 @@ export default class RoomStatusBar extends React.PureComponent<IProps, IState> { | |
| private getSize(): number { | ||
| if (this.shouldShowConnectionError()) { | ||
| return STATUS_BAR_EXPANDED; | ||
| } else if ( | ||
| this.state.unsentMessages.length > 0 || | ||
| this.state.isResending || | ||
| this.state.timelineNeedsRefresh | ||
| ) { | ||
| return STATUS_BAR_EXPANDED_LARGE; | ||
| } | ||
| return STATUS_BAR_HIDDEN; | ||
|
|
@@ -306,6 +338,37 @@ export default class RoomStatusBar extends React.PureComponent<IProps, IState> { | |
| return this.getUnsentMessageContent(); | ||
| } | ||
|
|
||
| if (this.state.timelineNeedsRefresh) { | ||
| return ( | ||
| <div className="mx_RoomStatusBar mx_RoomStatusBar_unsentMessages"> | ||
| <div role="alert"> | ||
| <div className="mx_RoomStatusBar_unsentBadge"> | ||
| <img | ||
| src={require("../../../res/img/feather-customised/warning-triangle.svg").default} | ||
| width="24" | ||
| height="24" | ||
| title="/!\ " | ||
| alt="/!\ " /> | ||
MadLittleMods marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| </div> | ||
| <div> | ||
| <div className="mx_RoomStatusBar_unsentTitle"> | ||
| { _t("History import detected.") } | ||
| </div> | ||
| <div className="mx_RoomStatusBar_unsentDescription"> | ||
| { _t("History was just imported somewhere in the room. " + | ||
| "In order to see the historical messages, refresh your timeline.") } | ||
| </div> | ||
| </div> | ||
| <div className="mx_RoomStatusBar_unsentButtonBar"> | ||
| <AccessibleButton onClick={this.onRefreshTimelineClick} className="mx_RoomStatusBar_refreshTimelineBtn"> | ||
| { _t("Refresh timeline") } | ||
| </AccessibleButton> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -280,6 +280,7 @@ class TimelinePanel extends React.Component<IProps, IState> { | |
| const cli = MatrixClientPeg.get(); | ||
| cli.on(RoomEvent.Timeline, this.onRoomTimeline); | ||
| cli.on(RoomEvent.TimelineReset, this.onRoomTimelineReset); | ||
| this.props.timelineSet.room.on(RoomEvent.TimelineRefresh, this.onRoomTimelineRefresh); | ||
| cli.on(RoomEvent.Redaction, this.onRoomRedaction); | ||
| if (SettingsStore.getValue("feature_msc3531_hide_messages_pending_moderation")) { | ||
| // Make sure that events are re-rendered when their visibility-pending-moderation changes. | ||
|
|
@@ -370,6 +371,8 @@ class TimelinePanel extends React.Component<IProps, IState> { | |
| client.removeListener(MatrixEventEvent.VisibilityChange, this.onEventVisibilityChange); | ||
| client.removeListener(ClientEvent.Sync, this.onSync); | ||
| } | ||
|
|
||
| this.props.timelineSet.room.removeListener(RoomEvent.TimelineRefresh, this.onRoomTimelineRefresh); | ||
| } | ||
|
|
||
| private onMessageListUnfillRequest = (backwards: boolean, scrollToken: string): void => { | ||
|
|
@@ -627,10 +630,18 @@ class TimelinePanel extends React.Component<IProps, IState> { | |
| }); | ||
| }; | ||
|
|
||
| private onRoomTimelineRefresh = (room: Room, timelineSet: EventTimelineSet): void => { | ||
| console.log(`onRoomTimelineRefresh skipping=${timelineSet !== this.props.timelineSet}`); | ||
| if (timelineSet !== this.props.timelineSet) return; | ||
|
|
||
| this.refreshTimeline(); | ||
| }; | ||
|
|
||
| private onRoomTimelineReset = (room: Room, timelineSet: EventTimelineSet): void => { | ||
| console.log(`onRoomTimelineReset skipping=${timelineSet !== this.props.timelineSet} skippingBecauseAtBottom=${this.canResetTimeline()}`); | ||
| if (timelineSet !== this.props.timelineSet) return; | ||
|
|
||
| if (this.canResetTimeline()) { | ||
|
There was a problem hiding this comment. This seems like a missed refactor. matrix-react-sdk/src/components/structures/RoomView.tsx Lines 1004 to 1009 in 27118a9 |
||
| this.loadTimeline(); | ||
| } | ||
| }; | ||
|
|
@@ -1181,6 +1192,9 @@ class TimelinePanel extends React.Component<IProps, IState> { | |
| * @param {boolean?} scrollIntoView whether to scroll the event into view. | ||
| */ | ||
| private loadTimeline(eventId?: string, pixelOffset?: number, offsetBase?: number, scrollIntoView = true): void { | ||
| console.log('TimelinePanel: loadTimeline', this.props.timelineSet.getTimelines(), this.props.timelineSet.getTimelines().map((timeline) => { | ||
| return timeline.getEvents().length; | ||
| })) | ||
| this.timelineWindow = new TimelineWindow( | ||
| MatrixClientPeg.get(), this.props.timelineSet, | ||
| { windowLimit: this.props.timelineCap }); | ||
|
|
@@ -1319,6 +1333,7 @@ class TimelinePanel extends React.Component<IProps, IState> { | |
| // get the list of events from the timeline window and the pending event list | ||
| private getEvents(): Pick<IState, "events" | "liveEvents" | "firstVisibleEventIndex"> { | ||
| const events: MatrixEvent[] = this.timelineWindow.getEvents(); | ||
| console.log('TimelinePanel: getEvents', events.length); | ||
|
|
||
| // `arrayFastClone` performs a shallow copy of the array | ||
| // we want the last event to be decrypted first but displayed last | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the context type definition around line L87 as string emitter keys are no longer allowed in TypedEventEmitter, TS thinks
this.contextisanyhere thoughThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also not listen on the room directly, re-emitters suck
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I don't guess, what should we do? 🙇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.props.room.on(...);but with componentDidUpdate comparing this.props.room with prevProps.room to remove old listener and setup a new one. This is all a lot nicer in hooks where its a one-liner.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated 👍
I'm not sure what to do here exactly. I've stopped using string keys. Is there anything further?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conversation continued at #8354 (comment)