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.

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

feat(sdk): Implement Client::observe_events and Client::observe_room_events #4253

Merged
merged 3 commits into from
Nov 13, 2024

Conversation

Hywan
Copy link
Member

@Hywan Hywan commented Nov 12, 2024

This patch introduces a mechanism similar to
Client::add_event_handler and Client::add_room_event_handler
but with a reactive programming pattern. This patch adds
Client::observe_events and Client::observe_room_events.

// Get an observer.
let observer =
    client.observe_events::<SyncRoomMessageEvent, (Room, Vec<Action>)>();

// Subscribe to the observer.
let mut subscriber = observer.subscribe();

// Use the subscriber as a `Stream`.
let (message_event, (room, push_actions)) = subscriber.next().await.unwrap();

When calling observe_events, one has to specify the type of event
(in the example, SyncRoomMessageEvent) and a context (in the example,
(Room, Vec<Action>), respectively for the room and the push actions).

Read the documentation of the ObservableEventHandler and
EventHandlerSubscriber to learn more about how it works.

In order to achieve that, it was necessary to implement EventHandlerContext
on tuples where each part implements EventHandlerContext. Why? Because,
while EventHandler is implemented for FnOnce() with 0 to 8 arguments
(e.g. with 2 arguments client.add_event_handler(|ev: E, ctx1: C1, ctx2: C2| { … })),
it is not possible to have “variadic” generic parameters. Example, it's not possible
to write client.observe_events::<E, C1, C2>(). The trick is to use tuples (!), like:
client.observe_events::<E, (C1, C2)>(): the type of observe_events remains
“constant”:

pub fn observe_events<Ev, Ctx>(&self) -> ObservableEventHandler<(Ev, Ctx)>
where
    Ev: SyncEvent + DeserializeOwned + Send + Sync + 'static,
    Ctx: EventHandlerContext + Send + Sync + 'static,
{}

@Hywan Hywan requested a review from a team as a code owner November 12, 2024 17:16
@Hywan Hywan requested review from jmartinesp and poljar and removed request for a team and jmartinesp November 12, 2024 17:16
@Hywan Hywan force-pushed the feat-sdk-event-handler-to-stream branch 2 times, most recently from 53b11d5 to 174191e Compare November 12, 2024 17:21
@Hywan Hywan changed the title feat(sdk): Implement Client::observe_events and Client::observe_room_events feat(sdk): Implement Client::observe_events and Client::observe_room_events Nov 12, 2024
This patch implements `EventHandlerContext` for tuples where each part
implements `EventHandlerContext` itself.
@Hywan Hywan force-pushed the feat-sdk-event-handler-to-stream branch from 174191e to e817a85 Compare November 12, 2024 17:23
Copy link

codecov bot commented Nov 12, 2024

Codecov Report

Attention: Patch coverage is 82.35294% with 6 lines in your changes missing coverage. Please review.

Project coverage is 84.93%. Comparing base (36b96cc) to head (e2b1818).
Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
crates/matrix-sdk/src/event_handler/mod.rs 76.47% 4 Missing ⚠️
crates/matrix-sdk/src/event_handler/context.rs 60.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4253      +/-   ##
==========================================
+ Coverage   84.90%   84.93%   +0.02%     
==========================================
  Files         274      274              
  Lines       29761    29795      +34     
==========================================
+ Hits        25270    25307      +37     
+ Misses       4491     4488       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Hywan Hywan force-pushed the feat-sdk-event-handler-to-stream branch 2 times, most recently from bc2d18d to e0d6d21 Compare November 13, 2024 07:24
Copy link
Contributor

@jmartinesp jmartinesp left a comment

Choose a reason for hiding this comment

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

Thanks, the changes LGTM in general since I assume it's correct, but maybe a second pair of more experienced eyes wouldn't hurt since I'm not familiar with some parts of it.

Copy link
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

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

Very nice. Quite the clever trick on how to make this pattern generic, great work.

I left some small nits about the documentation but otherwise this is good to go.

crates/matrix-sdk/src/client/mod.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/client/mod.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/client/mod.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/client/mod.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/client/mod.rs Show resolved Hide resolved
crates/matrix-sdk/src/client/mod.rs Outdated Show resolved Hide resolved
…om_events`.

Changelog: This patch introduces a mechanism similar to
 `Client::add_event_handler` and `Client::add_room_event_handler`
 but with a reactive programming pattern. This patch adds
 `Client::observe_events` and `Client::observe_room_events`.

 ```rust
 // Get an observer.
 let observer =
     client.observe_events::<SyncRoomMessageEvent, (Room, Vec<Action>)>();

 // Subscribe to the observer.
 let mut subscriber = observer.subscribe();

 // Use the subscriber as a `Stream`.
 let (message_event, (room, push_actions)) = subscriber.next().await.unwrap();
 ```

 When calling `observe_events`, one has to specify the type of event
 (in the example, `SyncRoomMessageEvent`) and a context (in the example,
 `(Room, Vec<Action>)`, respectively for the room and the push actions).
@Hywan Hywan force-pushed the feat-sdk-event-handler-to-stream branch 2 times, most recently from ab4b0d0 to 5649704 Compare November 13, 2024 09:54
@Hywan Hywan force-pushed the feat-sdk-event-handler-to-stream branch from 5649704 to e2b1818 Compare November 13, 2024 09:58
@Hywan Hywan merged commit 0509236 into matrix-org:main Nov 13, 2024
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants