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

Ensure consistency when rendering the sent event indicator #11314

Merged
merged 4 commits into from
Jul 25, 2023
Merged

Ensure consistency when rendering the sent event indicator #11314

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e9d7519
Ensure consistency when considering where to render the sent event in…
t3chguy Jul 25, 2023
16131a8
Add test
t3chguy Jul 25, 2023
4667759
Fix redacted edge case
t3chguy Jul 25, 2023
b6eca4a
Comments
t3chguy Jul 25, 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
128 changes: 64 additions & 64 deletions src/components/structures/MessagePanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -588,9 +588,9 @@ export default class MessagePanel extends React.Component<IProps, IState> {
* the tile.
*/
private getNextEventInfo(
events: EventAndShouldShow[],
events: WrappedEvent[],
i: number,
): { nextEventAndShouldShow: EventAndShouldShow | null; nextTile: MatrixEvent | null } {
): { nextEventAndShouldShow: WrappedEvent | null; nextTile: MatrixEvent | null } {
// WARNING: this method is on a hot path.

const nextEventAndShouldShow = i < events.length - 1 ? events[i + 1] : null;
Expand Down Expand Up @@ -626,13 +626,13 @@ export default class MessagePanel extends React.Component<IProps, IState> {
// we also need to figure out which is the last event we show which isn't
// a local echo, to manage the read-marker.
let lastShownEvent: MatrixEvent | undefined;
const events: EventAndShouldShow[] = this.props.events.map((event) => {
const events: WrappedEvent[] = this.props.events.map((event) => {
return { event, shouldShow: this.shouldShowEvent(event) };
});

const userId = MatrixClientPeg.safeGet().getSafeUserId();

let lastSuccessfulEventIndex = -1;
let foundLastSuccessfulWeSent = false;
let lastShownNonLocalEchoIndex = -1;
// Find the indices of the last successful event we sent and the last non-local-echo events shown
for (let i = events.length - 1; i >= 0; i--) {
Expand All @@ -645,15 +645,16 @@ export default class MessagePanel extends React.Component<IProps, IState> {
lastShownEvent = event;
}

if (lastSuccessfulEventIndex < 0 && this.isSentState(event) && isEligibleForSpecialReceipt(event, userId)) {
lastSuccessfulEventIndex = i;
if (!foundLastSuccessfulWeSent && this.isSentState(event) && isEligibleForSpecialReceipt(event, userId)) {
events[i].lastSuccessfulWeSent = true;
foundLastSuccessfulWeSent = true;
}

if (lastShownNonLocalEchoIndex < 0 && !event.status) {
lastShownNonLocalEchoIndex = i;
}

if (lastShownNonLocalEchoIndex >= 0 && lastSuccessfulEventIndex >= 0) {
if (lastShownNonLocalEchoIndex >= 0 && foundLastSuccessfulWeSent) {
break;
}
}
Expand All @@ -672,15 +673,15 @@ export default class MessagePanel extends React.Component<IProps, IState> {
let grouper: BaseGrouper | null = null;

for (let i = 0; i < events.length; i++) {
const eventAndShouldShow = events[i];
const { event, shouldShow } = eventAndShouldShow;
const wrappedEvent = events[i];
const { event, shouldShow } = wrappedEvent;
const eventId = event.getId()!;
const last = event === lastShownEvent;
const { nextEventAndShouldShow, nextTile } = this.getNextEventInfo(events, i);

if (grouper) {
if (grouper.shouldGroup(eventAndShouldShow)) {
grouper.add(eventAndShouldShow);
if (grouper.shouldGroup(wrappedEvent)) {
grouper.add(wrappedEvent);
continue;
} else {
// not part of group, so get the group tiles, close the
Expand All @@ -692,10 +693,10 @@ export default class MessagePanel extends React.Component<IProps, IState> {
}

for (const Grouper of groupers) {
if (Grouper.canStartGroup(this, eventAndShouldShow) && !this.props.disableGrouping) {
if (Grouper.canStartGroup(this, wrappedEvent) && !this.props.disableGrouping) {
grouper = new Grouper(
this,
eventAndShouldShow,
wrappedEvent,
prevEvent,
lastShownEvent,
nextEventAndShouldShow,
Expand All @@ -713,12 +714,11 @@ export default class MessagePanel extends React.Component<IProps, IState> {
ret.push(
...this.getTilesForEvent(
prevEvent,
event,
wrappedEvent,
last,
false,
nextEventAndShouldShow,
nextTile,
i === lastSuccessfulEventIndex,
),
);
prevEvent = event;
Expand All @@ -738,13 +738,13 @@ export default class MessagePanel extends React.Component<IProps, IState> {

public getTilesForEvent(
prevEvent: MatrixEvent | null,
mxEv: MatrixEvent,
wrappedEvent: WrappedEvent,
last = false,
isGrouped = false,
nextEvent: EventAndShouldShow | null = null,
nextEvent: WrappedEvent | null = null,
nextEventWithTile: MatrixEvent | null = null,
isLastSuccessful = false,
): ReactNode[] {
const mxEv = wrappedEvent.event;
const ret: ReactNode[] = [];

const isEditing = this.props.editState?.getEvent().getId() === mxEv.getId();
Expand Down Expand Up @@ -812,7 +812,7 @@ export default class MessagePanel extends React.Component<IProps, IState> {
permalinkCreator={this.props.permalinkCreator}
last={last}
lastInSection={lastInSection}
lastSuccessful={isLastSuccessful}
lastSuccessful={wrappedEvent.lastSuccessfulWeSent}
isSelectedEvent={highlight}
getRelationsForEvent={this.props.getRelationsForEvent}
showReactions={this.props.showReactions}
Expand Down Expand Up @@ -880,7 +880,7 @@ export default class MessagePanel extends React.Component<IProps, IState> {
// Get an object that maps from event ID to a list of read receipts that
// should be shown next to that event. If a hidden event has read receipts,
// they are folded into the receipts of the last shown event.
private getReadReceiptsByShownEvent(events: EventAndShouldShow[]): Map<string, IReadReceiptProps[]> {
private getReadReceiptsByShownEvent(events: WrappedEvent[]): Map<string, IReadReceiptProps[]> {
const receiptsByEvent: Map<string, IReadReceiptProps[]> = new Map();
const receiptsByUserId: Map<string, IReadReceiptForUser> = new Map();

Expand Down Expand Up @@ -1069,25 +1069,26 @@ export default class MessagePanel extends React.Component<IProps, IState> {
* Holds on to an event, caching the information about whether it should be
t3chguy marked this conversation as resolved.
Show resolved Hide resolved
* shown. Avoids calling shouldShowEvent more times than we need to.
*/
interface EventAndShouldShow {
interface WrappedEvent {
event: MatrixEvent;
shouldShow: boolean;
shouldShow?: boolean;
lastSuccessfulWeSent?: boolean;
}

abstract class BaseGrouper {
public static canStartGroup = (_panel: MessagePanel, _ev: EventAndShouldShow): boolean => true;
public static canStartGroup = (_panel: MessagePanel, _ev: WrappedEvent): boolean => true;

public events: MatrixEvent[] = [];
public events: WrappedEvent[] = [];
// events that we include in the group but then eject out and place above the group.
public ejectedEvents: MatrixEvent[] = [];
public ejectedEvents: WrappedEvent[] = [];
public readMarker: ReactNode;

public constructor(
public readonly panel: MessagePanel,
public readonly firstEventAndShouldShow: EventAndShouldShow,
public readonly firstEventAndShouldShow: WrappedEvent,
public readonly prevEvent: MatrixEvent | null,
public readonly lastShownEvent: MatrixEvent | undefined,
public readonly nextEvent: EventAndShouldShow | null,
public readonly nextEvent: WrappedEvent | null,
public readonly nextEventTile?: MatrixEvent | null,
) {
this.readMarker = panel.readMarkerForEvent(
Expand All @@ -1096,8 +1097,8 @@ abstract class BaseGrouper {
);
}

public abstract shouldGroup(ev: EventAndShouldShow): boolean;
public abstract add(ev: EventAndShouldShow): void;
public abstract shouldGroup(ev: WrappedEvent): boolean;
public abstract add(ev: WrappedEvent): void;
public abstract getTiles(): ReactNode[];
public abstract getNewPrevEvent(): MatrixEvent;
}
Expand All @@ -1118,11 +1119,11 @@ abstract class BaseGrouper {
// Grouping only events sent by the same user that sent the `m.room.create` and only until
// the first non-state event, beacon_info event or membership event which is not regarding the sender of the `m.room.create` event
class CreationGrouper extends BaseGrouper {
public static canStartGroup = function (_panel: MessagePanel, { event }: EventAndShouldShow): boolean {
public static canStartGroup = function (_panel: MessagePanel, { event }: WrappedEvent): boolean {
return event.getType() === EventType.RoomCreate;
};

public shouldGroup({ event, shouldShow }: EventAndShouldShow): boolean {
public shouldGroup({ event, shouldShow }: WrappedEvent): boolean {
const panel = this.panel;
const createEvent = this.firstEventAndShouldShow.event;
if (!shouldShow) {
Expand Down Expand Up @@ -1157,16 +1158,17 @@ class CreationGrouper extends BaseGrouper {
return false;
}

public add({ event: ev, shouldShow }: EventAndShouldShow): void {
public add(wrappedEvent: WrappedEvent): void {
const { event: ev, shouldShow } = wrappedEvent;
const panel = this.panel;
this.readMarker = this.readMarker || panel.readMarkerForEvent(ev.getId()!, ev === this.lastShownEvent);
if (!shouldShow) {
return;
}
if (ev.getType() === EventType.RoomEncryption) {
this.ejectedEvents.push(ev);
this.ejectedEvents.push(wrappedEvent);
} else {
this.events.push(ev);
this.events.push(wrappedEvent);
}
}

Expand Down Expand Up @@ -1194,7 +1196,7 @@ class CreationGrouper extends BaseGrouper {
// If this m.room.create event should be shown (room upgrade) then show it before the summary
if (createEvent.shouldShow) {
// pass in the createEvent as prevEvent as well so no extra DateSeparator is rendered
ret.push(...panel.getTilesForEvent(createEvent.event, createEvent.event));
ret.push(...panel.getTilesForEvent(createEvent.event, createEvent));
}

for (const ejected of this.ejectedEvents) {
Expand All @@ -1209,11 +1211,11 @@ class CreationGrouper extends BaseGrouper {
// of GenericEventListSummary, render each member event as if the previous
// one was itself. This way, the timestamp of the previous event === the
// timestamp of the current event, and no DateSeparator is inserted.
return panel.getTilesForEvent(e, e, e === lastShownEvent, isGrouped);
return panel.getTilesForEvent(e.event, e, e.event === lastShownEvent, isGrouped);
})
.reduce((a, b) => a.concat(b), []);
// Get sender profile from the latest event in the summary as the m.room.create doesn't contain one
const ev = this.events[this.events.length - 1];
const ev = this.events[this.events.length - 1].event;

let summaryText: string;
const roomId = ev.getRoomId();
Expand All @@ -1229,7 +1231,7 @@ class CreationGrouper extends BaseGrouper {
ret.push(
<GenericEventListSummary
key="roomcreationsummary"
events={this.events}
events={this.events.map((e) => e.event)}
onToggle={panel.onHeightChanged} // Update scroll state
summaryMembers={ev.sender ? [ev.sender] : undefined}
summaryText={summaryText}
Expand All @@ -1253,10 +1255,7 @@ class CreationGrouper extends BaseGrouper {

// Wrap consecutive grouped events in a ListSummary
class MainGrouper extends BaseGrouper {
public static canStartGroup = function (
panel: MessagePanel,
{ event: ev, shouldShow }: EventAndShouldShow,
): boolean {
public static canStartGroup = function (panel: MessagePanel, { event: ev, shouldShow }: WrappedEvent): boolean {
if (!shouldShow) return false;

if (ev.isState() && groupedStateEvents.includes(ev.getType() as EventType)) {
Expand All @@ -1276,22 +1275,22 @@ class MainGrouper extends BaseGrouper {

public constructor(
public readonly panel: MessagePanel,
public readonly firstEventAndShouldShow: EventAndShouldShow,
public readonly firstEventAndShouldShow: WrappedEvent,
public readonly prevEvent: MatrixEvent | null,
public readonly lastShownEvent: MatrixEvent | undefined,
nextEvent: EventAndShouldShow | null,
nextEvent: WrappedEvent | null,
nextEventTile: MatrixEvent | null,
) {
super(panel, firstEventAndShouldShow, prevEvent, lastShownEvent, nextEvent, nextEventTile);
this.events = [firstEventAndShouldShow.event];
this.events = [firstEventAndShouldShow];
}

public shouldGroup({ event: ev, shouldShow }: EventAndShouldShow): boolean {
public shouldGroup({ event: ev, shouldShow }: WrappedEvent): boolean {
if (!shouldShow) {
// absorb hidden events so that they do not break up streams of messages & redaction events being grouped
return true;
}
if (this.panel.wantsDateSeparator(this.events[0], ev.getDate())) {
if (this.panel.wantsDateSeparator(this.events[0].event, ev.getDate())) {
return false;
}
if (ev.isState() && groupedStateEvents.includes(ev.getType() as EventType)) {
Expand All @@ -1306,7 +1305,8 @@ class MainGrouper extends BaseGrouper {
return false;
}

public add({ event: ev, shouldShow }: EventAndShouldShow): void {
public add(wrappedEvent: WrappedEvent): void {
const { event: ev, shouldShow } = wrappedEvent;
if (ev.getType() === EventType.RoomMember) {
// We can ignore any events that don't actually have a message to display
if (!hasText(ev, MatrixClientPeg.safeGet(), this.panel.showHiddenEvents)) return;
Expand All @@ -1316,11 +1316,11 @@ class MainGrouper extends BaseGrouper {
// absorb hidden events to not split the summary
return;
}
this.events.push(ev);
this.events.push(wrappedEvent);
}

private generateKey(): string {
return "eventlistsummary-" + this.events[0].getId();
return "eventlistsummary-" + this.events[0].event.getId();
}

public getTiles(): ReactNode[] {
Expand All @@ -1334,41 +1334,41 @@ class MainGrouper extends BaseGrouper {
const lastShownEvent = this.lastShownEvent;
const ret: ReactNode[] = [];

if (panel.wantsDateSeparator(this.prevEvent, this.events[0].getDate())) {
const ts = this.events[0].getTs();
if (panel.wantsDateSeparator(this.prevEvent, this.events[0].event.getDate())) {
const ts = this.events[0].event.getTs();
ret.push(
<li key={ts + "~"}>
<DateSeparator roomId={this.events[0].getRoomId()!} ts={ts} />
<DateSeparator roomId={this.events[0].event.getRoomId()!} ts={ts} />
</li>,
);
}

// Ensure that the key of the EventListSummary does not change with new events in either direction.
// This will prevent it from being re-created unnecessarily, and instead will allow new props to be provided.
// In turn, the shouldComponentUpdate method on ELS can be used to prevent unnecessary renderings.
const keyEvent = this.events.find((e) => this.panel.grouperKeyMap.get(e));
const keyEvent = this.events.find((e) => this.panel.grouperKeyMap.get(e.event));
const key =
keyEvent && this.panel.grouperKeyMap.has(keyEvent)
? this.panel.grouperKeyMap.get(keyEvent)!
keyEvent && this.panel.grouperKeyMap.has(keyEvent.event)
? this.panel.grouperKeyMap.get(keyEvent.event)!
: this.generateKey();
if (!keyEvent) {
// Populate the weak map with the key.
// Note that we only set the key on the specific event it refers to, since this group might get
// split up in the future by other intervening events. If we were to set the key on all events
// currently in the group, we would risk later giving the same key to multiple groups.
this.panel.grouperKeyMap.set(this.events[0], key);
this.panel.grouperKeyMap.set(this.events[0].event, key);
}

let highlightInSummary = false;
let eventTiles: ReactNode[] | null = this.events
.map((e, i) => {
if (e.getId() === panel.props.highlightedEventId) {
if (e.event.getId() === panel.props.highlightedEventId) {
highlightInSummary = true;
}
return panel.getTilesForEvent(
i === 0 ? this.prevEvent : this.events[i - 1],
i === 0 ? this.prevEvent : this.events[i - 1].event,
e,
e === lastShownEvent,
e.event === lastShownEvent,
isGrouped,
this.nextEvent,
this.nextEventTile,
Expand All @@ -1390,7 +1390,7 @@ class MainGrouper extends BaseGrouper {
<EventListSummary
key={key}
data-testid={key}
events={this.events}
events={this.events.map((e) => e.event)}
onToggle={panel.onHeightChanged} // Update scroll state
startExpanded={highlightInSummary}
layout={this.panel.props.layout}
Expand All @@ -1407,18 +1407,18 @@ class MainGrouper extends BaseGrouper {
}

public getNewPrevEvent(): MatrixEvent {
return this.events[this.events.length - 1];
return this.events[this.events.length - 1].event;
}
}

// all the grouper classes that we use, ordered by priority
const groupers = [CreationGrouper, MainGrouper];

/**
* Look through the supplied list of EventAndShouldShow, and return the first
* Look through the supplied list of WrappedEvent, and return the first
* event that is >start items through the list, and is shown.
*/
function findFirstShownAfter(start: number, events: EventAndShouldShow[]): MatrixEvent | null {
function findFirstShownAfter(start: number, events: WrappedEvent[]): MatrixEvent | null {
// Note: this could be done with something like:
// events.slice(i + 1).find((e) => e.shouldShow)?.event ?? null;
// but it is ~10% slower, and this is on the critical path.
Expand Down
Loading
Loading