Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Only feed state events to widgets if we believe they update the state #28422

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from
+38 −8
Draft

Only feed state events to widgets if we believe they update the state #28422

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
46 changes: 38 additions & 8 deletions src/stores/widgets/StopGapWidget.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,14 @@
* Please see LICENSE files in the repository root for full details.
*/

import { Room, MatrixEvent, MatrixEventEvent, MatrixClient, ClientEvent } from "matrix-js-sdk/src/matrix";
import {
Room,
MatrixEvent,
MatrixEventEvent,
MatrixClient,
ClientEvent,
RoomStateEvent,
} from "matrix-js-sdk/src/matrix";
import { KnownMembership } from "matrix-js-sdk/src/types";
import {
ClientWidgetApi,
Expand Down Expand Up @@ -333,6 +340,7 @@ export class StopGapWidget extends EventEmitter {
// Attach listeners for feeding events - the underlying widget classes handle permissions for us
this.client.on(ClientEvent.Event, this.onEvent);
this.client.on(MatrixEventEvent.Decrypted, this.onEventDecrypted);
this.client.on(RoomStateEvent.Events, this.onStateEvent);
this.client.on(ClientEvent.ToDeviceEvent, this.onToDeviceEvent);

this.messaging.on(
Expand Down Expand Up @@ -463,15 +471,33 @@ export class StopGapWidget extends EventEmitter {

this.client.off(ClientEvent.Event, this.onEvent);
Copy link
Contributor

Choose a reason for hiding this comment

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

Change to RoomEvent.Timeline

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you help me understand why RoomEvent.Timeline would be more appropriate here? I'm looking at:

/**
 * Fires whenever the SDK receives a new event.
 * <p>
 * This is only fired for live events received via /sync - it is not fired for
 * events received over context, search, or pagination APIs.
 *
 * @param event - The matrix event which caused this event to fire.
 * @example
 * ```
 * matrixClient.on("event", function(event){
 *   var sender = event.getSender();
 * });
 * ```
 */
[ClientEvent.Event]: (event: MatrixEvent) => void;

versus

/**
 * Fires whenever the timeline in a room is updated.
 * @param event - The matrix event which caused this event to fire.
 * @param room - The room, if any, whose timeline was updated.
 * @param toStartOfTimeline - True if this event was added to the start
 * @param removed - True if this event has just been removed from the timeline
 * (beginning; oldest) of the timeline e.g. due to pagination.
 *
 * @param data - more data about the event
 *
 * @example
 * ```
 * matrixClient.on("Room.timeline",
 *                 function(event, room, toStartOfTimeline, removed, data) {
 *   if (!toStartOfTimeline && data.liveEvent) {
 *     var messageToAppend = room.timeline.[room.timeline.length - 1];
 *   }
 * });
 * ```
 */
[RoomEvent.Timeline]: (
    event: MatrixEvent,
    room: Room | undefined,
    toStartOfTimeline: boolean | undefined,
    removed: boolean,
    data: IRoomTimelineData,
) => void;

and from these comments it sounds like ClientEvent.Event, receiving new live events down /sync, is exactly what we want.

Copy link
Member

Choose a reason for hiding this comment

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

ClientEvent.Event fires on every single event the SDK ever sees, state, account data, presence, all before those events are processed and without any metadata on whether that event is state which should be applied (state_after), or state which should be ignored (timeline).

Copy link
Member Author

Choose a reason for hiding this comment

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

Right okay, and we don't seem to require any metadata or processing in this spot

Copy link
Member

Choose a reason for hiding this comment

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

We do if we want to tell the embedded client if the event is in the timeline or state, isState() doesn't tell you that.

Copy link
Member Author

@robintown robintown Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

For now we're not yet trying to tell widgets that. This is deliberately a very blunt, "filter all state events out of the timeline and then finally send through just those state events that cause a RoomStateEvent.Events" change. (which is why it's in draft)

this.client.off(MatrixEventEvent.Decrypted, this.onEventDecrypted);
this.client.off(RoomStateEvent.Events, this.onStateEvent);
this.client.off(ClientEvent.ToDeviceEvent, this.onToDeviceEvent);
}

private onEvent = (ev: MatrixEvent): void => {
this.client.decryptEventIfNeeded(ev);
this.feedEvent(ev);
// Only process non-state events here; we don't want to confuse the
// widget with a state event from the timeline that might not have
// actually updated the room's state
if (!ev.isState()) this.handleEvent(ev);
};

private onEventDecrypted = (ev: MatrixEvent): void => {
this.handleEvent(ev);
};

private onStateEvent = (ev: MatrixEvent): void => {
// State events get to skip all the checks of handleEvent and be fed
// directly to the widget. When it comes to state events, we don't care
// so much about getting the order and contents of the timeline right as
// we care about state updates reliably getting through to the widget so
// that it sees the same state as what the server calculated.
// TODO: We can provide widgets with a more complete timeline stream
// while also getting state updates right if we create a separate widget
// action for communicating state deltas, similar to how the 'state'
// sections of sync responses in Simplified Sliding Sync and MSC4222
// work.
this.feedEvent(ev);
};

Expand Down Expand Up @@ -544,8 +570,7 @@ export class StopGapWidget extends EventEmitter {
return false;
}

private feedEvent(ev: MatrixEvent): void {
if (this.messaging === null) return;
private handleEvent(ev: MatrixEvent): void {
if (
// If we had decided earlier to feed this event to the widget, but
// it just wasn't ready, give it another try
Expand Down Expand Up @@ -573,11 +598,16 @@ export class StopGapWidget extends EventEmitter {
if (ev.isBeingDecrypted() || ev.isDecryptionFailure()) {
this.eventsToFeed.add(ev);
} else {
const raw = ev.getEffectiveEvent();
this.messaging.feedEvent(raw as IRoomEvent, this.eventListenerRoomId!).catch((e) => {
logger.error("Error sending event to widget: ", e);
});
this.feedEvent(ev);
}
}
}

private feedEvent(ev: MatrixEvent): void {
if (this.messaging === null) return;
const raw = ev.getEffectiveEvent();
this.messaging.feedEvent(raw as IRoomEvent, this.eventListenerRoomId!).catch((e) => {
logger.error("Error sending event to widget: ", e);
});
}
}
Loading