Rewrite event handler abstraction

[jplatte]

Blocked on #310.

Fixes #269.

Left to do:

• Add EncryptionInfo context where available (not required for feature parity with the old EventHandler)
• "Port" prev_content workaround
• Use an sdk-specific SyncEventKind enum instead of ruma_events::EventKind (need to distinguish between initial / stripped / regular state events)
• Also introduce notification handler!?

[jplatte]

event_handler::static_events needs to be replaced with some code in Ruma, some in matrix-sdk (currently I'm thinking Ruma will just provide StaticEventContent for now, with matrix-sdk then being able to easily exclude the non-sync event types from StaticEvent by implementing it for Sync*Event<impl StaticEventContent> only.

Otherwise I think this is pretty much ready. Maybe an implementation of EventHandlerContext for Appservice would be helpful?

[MTRNord]

Hm as a consumer of this crate I have just 2 things to mention:

a) How do I have multiple handlers now? The examples dont seem clear to me or am I missing something here?

b) Honestly I prefer the EventHandler trait way way more than this. To a point where I might actually end up using a fork that preserves EventHandler usage. :( So personal opinion is that I am not too much of a fan of this change

[jplatte]

a) How do I have multiple handlers now?

You simply call .register_event_handler multiple times.

b) Honestly I prefer the EventHandler trait way way more than this. To a point where I might actually end up using a fork that preserves EventHandler usage. :( So personal opinion is that I am not too much of a fan of this change

Can you try to elaborate why you like the previous approach over this one?

[MTRNord]

a) How do I have multiple handlers now?

You simply call .register_event_handler multiple times.

Ah ok :)

b) Honestly I prefer the EventHandler trait way way more than this. To a point where I might actually end up using a fork that preserves EventHandler usage. :( So personal opinion is that I am not too much of a fan of this change

Can you try to elaborate why you like the previous approach over this one?

It mostly boils down to the fact that it to me feels more organized. As in you have everything related to sync handling in one place that is in itself organized by being a trait. With the new approach for this you would need modules and imho more comment work to organize these. They now feel like loose functions to me while they with the trait were clearly related to the sdk. Though I also see that people may like using a tree of modules or have issues with the trait due to for example restrictions with gui crates. So its mostly a taste issue of having a well organized trait/interface vs loose functions. Imho a trait here very much represents an interface and to me is probably more object oriented (while obviously rust has no objects) while this now is more "function oriented" coding. I just like having my methods that are related to a feature kept together under a trait or struct as thats the reason rust has those imho.

[jplatte]

Actually I forgot that there's (at least) one more thing to do related to the Ruma / StaticEvent changes mentioned and that is checking more event fields including redacted_because instead of calling an event handler based on the event type alone.

[jplatte]

It mostly boils down to the fact that it to me feels more organized. As in you have everything related to sync handling in one place that is in itself organized by being a trait.

Well you can still have all the event handlers in one place with this.. Just create a handlers module or similar with all the actual logic and when creating your client in some other module, e.g. main.rs, you do

client.register_event_handler(handlers::on_room_message);
client.register_event_handler(handlers::on_room_topic);
client.register_event_handler(handlers::on_room_avatar);

very similar to what you would do with a web framework. (we should probably allow these to be chained)

In case of wanting to have them as methods you could also do

client.register_event_handler({
    let my_app = my_app.clone();
    move |ev, room| my_app.clone().on_room_message(ev, room)
});
client.register_event_handler({
    let my_app = my_app.clone();
    move |ev, room| my_app.clone().on_room_topic(ev, room)
});
client.register_event_handler({
    let my_app = my_app.clone();
    move |ev, room| my_app.clone().on_room_avatar(ev, room)
});

or some variation of that, possibly shortened via macros.

I just like having my methods that are related to a feature kept together under a trait or struct as thats the reason rust has those imho.

I would disagree actually; a trait like the previous EventHandler is highly unusual in Rust. Usually you have a handful of methods and multiple implementations, possibly over various crates in one dependency graph. The previous EventHandler had (way too) many methods with normally only one implementation. Trait are for abstracting over different types IMHO.

There is also some more advantages of this new approach that you might not have realized? Maybe this will change your opinion a little bit: With the new API,

• you can add multiple event handlers for the same event type (no need to have one method shell out to multiple actual handler fns)
• event handlers can return different return types (currently (), Result<(), impl std::error::Error> and anyhow::Result<()>); if an error happens it is automatically logged
• event types not defined by Ruma (either customized types for standard events, or just types for custom events) are supported exactly the same way as event types from Ruma: just use the ruma::events::macros::EventContent derive, same as within Ruma, on your content type and you can register an event handler for it

[jplatte]

One more open question: Should one event handler only react to a certain event in one context? Ruma currently supports event content types having multiple "kinds", although there is only a single type that makes use of this: EncryptedEventContent.

Is there any point in being able define an event handler for a certain event type that would fire regardless of where that event was seen (e.g. to-device section or timeline)? My feeling is no, it shouldn't. And it's likely better to have exactly one type per (event kind, event type) in Ruma too. Looking at the current EncryptedEventContent definition, I'm very skeptical about the definition even making sense for to-device events (it includes a relates_to field, which I would expect only ever to be Some(_) when it's a message event.

[poljar]

Is there any point in being able define an event handler for a certain event type that would fire regardless of where that event was seen (e.g. to-device section or timeline)? My feeling is no, it
shouldn't.

It might make sense for the verification events, people might want to handle in-room verifications the same way they handle to-device verifications. Though people can easily do so even if there are separate handlers as well so that's probably not necessary.

Agree, I don't think it makes sense for the EncryptedEventContent.

And it's likely better to have exactly one type per (event kind, event type) in Ruma too. Looking at the current EncryptedEventContent definition, I'm very skeptical about the definition even making sense for to-device events (it includes a relates_to field, which I would expect only ever to be Some(_) when it's a message event.

Yeah, relates_to field is only used in rooms. We mostly have different content types for to-device events vs in-room events, so it might make sense to do so here as well.

[jplatte]

It might make sense for the verification events, people might want to handle in-room verifications the same way they handle to-device verifications. Though people can easily do so even if there are separate handlers as well so that's probably not necessary.

Well, for those the to-device and room types are already separate. Having one event type for use in an event handler that can handle both would be extra work and so I think at least for now this is something to be handled in user code.

Yeah, relates_to field is only used in rooms. We mostly have different content types for to-device events vs in-room events, so it might make sense to do so here as well.

👍🏼

[jplatte]

Slightly related: Created ruma/ruma#698 for a small API addition in Ruma that I think would improve event handler ergonomics (both new & old event handler).

[jplatte]

I've made the notification handler thing much more minimal than the event handler stuff. The room & client args there could be made optional in the future by using the same kind of API as event handlers, but for now I was mostly interested in reaching parity with the previous interface and not grow that one commit more and more.

I did include a few refactoring commits though, I hope that's fine :)

[poljar]

This looks and works great, thanks.

There are some dead doc links and I think the docs in general could be a bit improved before we can merge this.

[poljar]

Looks good, please just add a event handler test to the matrix-sdk crate, copying the one from the app service crate and maybe modifying it a bit should be enough.

I think we can merge after that.

[jplatte]

Added the test, as well as .register_notification_handler that I had forgotten, plus allowing chaining which I noticed would be nice when writing the test :)

[poljar]

Thanks, merged.