Widget API: initial skeleton #2420

daniel-abramov · 2023-08-17T18:11:04Z

today, we had a sync-up with @toger5 on the current state of the Client Widget API implementation in the Rust SDK (we've been working on it for a few weeks). If my understanding is correct (@toger5, please correct me if I'm wrong), you wanted to see some "top-level boilerplate" that you could use in order to develop the FFI part of the Widget Client API inside the Rust SDK and check if it matches your expectations from the interface of the API.

We've implemented the part of the spec that should be sufficient for the integration of Element Call in Element X and currently we're working on the final steps to add proper error handling, ensuring that we did not miss anything, and testing it (@toger5 is on it). The latest state could be seen here: https://github.com/toger5/matrix-rust-sdk/blob/client-widget-api/crates/matrix-sdk/src/widget_api/mod.rs

This PR is a small subset of our branch which only includes the minimalistic "upper layer" of the API. We decided to create this small PR as if our understanding is right, you would prefer to see a simple interface before digging into 1.3k+ LOC PR for now 🙂 Additionally, our implementation in a branch misses some documentation and probably would need a round of clippy fixes to make sure that we have a clean implementation that would pass the CI.

codecov · 2023-08-17T18:38:35Z

Codecov Report

Patch coverage has no change and project coverage change: +0.02% 🎉

Comparison is base (92284a3) 77.11% compared to head (02cde35) 77.13%.
Report is 42 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2420      +/-   ##
==========================================
+ Coverage   77.11%   77.13%   +0.02%     
==========================================
  Files         181      181              
  Lines       19166    19166              
==========================================
+ Hits        14779    14784       +5     
+ Misses       4387     4382       -5

Files Changed	Coverage Δ
crates/matrix-sdk/src/lib.rs	`100.00% <ø> (ø)`

... and 1 file with indirect coverage changes

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

jplatte

Thanks for the PR! I have a lot of feedback, but it should all be pretty straight-forward. If anything is unclear or you disagree with one of my suggestions, please reply instead of changing things in what seems to be the most appropriate way (this may be obvious, but I haven't interacted with you before and have had the latter happen multiple times 😅).

jplatte · 2023-08-17T18:22:50Z

crates/matrix-sdk/src/lib.rs

 #[cfg(feature = "experimental-sliding-sync")]
 pub mod sliding_sync;

+#[cfg(feature = "widget-api")]
+pub mod widget_api;
+
 #[cfg(feature = "e2e-encryption")]
 pub mod encryption;


Can you remove the blank lines between all of the module declarations, and instead have just one to separate the mods from the pub uses?

Sure. One question though: should I also remove the empty lines around the sliding_sync. Rationale: I added pub mod widget_api there since it was following the experimental sliding sync module which also was separated by spaces.

I.e. it was:

#[cfg(feature = "experimental-oidc")] pub mod oidc; pub mod room; pub mod sync; #[cfg(feature = "experimental-sliding-sync")] pub mod sliding_sync; #[cfg(feature = "e2e-encryption")] pub mod encryption;

So I placed the pub mod widget_api right after the pub mod sliding_sync. I've pushed a change, hopefully I got it right.

crates/matrix-sdk/Cargo.toml

crates/matrix-sdk/src/widget_api/mod.rs

crates/matrix-sdk/src/widget_api/permissions.rs

jplatte · 2023-08-17T18:31:35Z

crates/matrix-sdk/src/widget_api/permissions.rs

+}
+
+/// An alias for an actual type of the messages that is going to be used with a permission request.
+pub type MessageType = String;


This is for a Matrix event type, and widgets might interact with both message events and state events, right? If yes, this type alias doesn't need to exist, we can use TimelineEventType from ruma::events.

Right (if I remember correctly). I actually wanted to also use stronger types here and had a discussion with @toger5, but as far as I can remember, @toger5's suggestion was that it can be any event type in theory and that there was no simple way to map them (no enum with a function f: EventType -> String and f: String -> EventType transformation). @toger5, could you chime in? 🙂

There are other "event" types that aren't part of TimelineEventType, but I'm 99.5% certain that those are out of scope for the widget API. They are ephemeral or mutable things like typing and receipt events, account data and presence. Often when people talk about events, those are implicitly excluded (see also matrix-org/matrix-spec#897).

Widgets are allowed to introduce new unspecced message types. Just arbitrary strings for their own usecases. Does TimelineEevntType have a "other" enum case. If so this would very much be a better solution. If not it would be a shame if a widget is working with its own timeline event types but the widget driver is giving a parsing error.

Does TimelineEevntType have a "other" enum case.

Yes,any string can be converted .into() that type. The variant is hidden / opaque, but to check for a custom type, one can also convert the other way using .to_string().

Nice! Updated.

crates/matrix-sdk/src/widget_api/widget.rs

crates/matrix-sdk/src/widget_api/mod.rs

daniel-abramov · 2023-08-17T19:01:33Z

Thanks for the PR! I have a lot of feedback, but it should all be pretty straight-forward. If anything is unclear or you disagree with one of my suggestions, please reply instead of changing things in what seems to be the most appropriate way (this may be obvious, but I haven't interacted with you before and have had the latter happen multiple times 😅).

Awesome! Btw, based on the feedback so far I very much appreciate the diligence and attention to detail! 👍 I was worried I'm the only one paying attention to such things 😄

P.S.: Apologies for not running this through rustfmt and clippy, I was probably too rushed to submit "something".

jplatte · 2023-08-17T19:02:55Z

Oh, maybe you ran rustfmt, but not nightly rustfmt. We use some rustfmt options that are unfortunately still unstable and only applied when running nightly rustfmt. Try cargo +nightly fmt.

Co-authored-by: Jonas Platte <jplatte+git@posteo.de>

Hywan · 2023-08-21T15:59:26Z

crates/matrix-sdk/src/widget_api/mod.rs

I would rename this module widget instead of widget_api. Just a personal taste compared to the rest of the SDK.

Though in that case, we should not have a widget sub-module. Which is fine by me really, the items from that could be part of this module just as well.

I also prefer short and concise names, I think the only reason we chose widget_api originally was that we thought that if we call it widget, it might be confused with the widget implementation (not the API, in our case the client widget API), but I'm totally fine with renaming it to widget if that's your preference folks!

The client widget API couldn't live in this crate though, right? In that case, let's do the rename as suggested by Ivan.

Updated.

The client widget API couldn't live in this crate though, right?

The actual implementation of the run() function and its internals. Originally I thought that it would live there, but maybe you're right and it's better to place the client widget API in a separate module or something like this.

Okay, then I guess I misunderstood what you meant by "client widget API". Anyways, let's figure this out when it comes to the next PR.

jplatte

I think the conclusion from the discussion on Matrix is simply that we should replace timeline events by message-like event in the EventFilter?

jplatte · 2023-08-23T10:45:33Z

crates/matrix-sdk/src/widget/permissions.rs

+
+use async_trait::async_trait;
+
+use crate::ruma::events::{StateEventType, TimelineEventType};


Suggested change

use crate::ruma::events::{StateEventType, TimelineEventType};

use crate::ruma::events::{MessageLikeEventType, StateEventType};

jplatte · 2023-08-23T10:45:34Z

crates/matrix-sdk/src/widget/permissions.rs

+pub enum EventFilter {
+    /// Filters for the timeline events, the `data` field is used to store the
+    /// `msgtype`.
+    Timeline(FilterContent<TimelineEventType>),


Suggested change

Timeline(FilterContent<TimelineEventType>),

MessageLike(FilterContent<MessageLikeEventType>),

jplatte · 2023-08-23T10:45:55Z

crates/matrix-sdk/src/widget/permissions.rs

+/// sending or for receiving).
+#[derive(Debug)]
+pub enum EventFilter {
+    /// Filters for the timeline events, the `data` field is used to store the


Suggested change

/// Filters for the timeline events, the `data` field is used to store the

/// Filters for message-like events, the `data` field is used to store the

crates/matrix-sdk/src/widget/permissions.rs

jplatte

Actually, I am now realizing that this filtering API is also suffering from the confusion between message events and m.room.message events. Not every message(-like) event has a msgtype, so please can you clarify what the role of the "timeline" filter is? Are widgets only supposed to interact with m.room.message events, or state events? (an example of a different message-like event would be m.reaction)

Co-authored-by: Jonas Platte <jplatte+git@posteo.de>

daniel-abramov · 2023-08-23T12:25:55Z

Actually, I am now realizing that this filtering API is also suffering from the confusion between message events and m.room.message events. Not every message(-like) event has a msgtype, so please can you clarify what the role of the "timeline" filter is? Are widgets only supposed to interact with m.room.message events, or state events? (an example of a different message-like event would be m.reaction)

These are very valid questions!

Are widgets only supposed to interact with m.room.message events, or state events?

From what I understand, it can interact with both state and message-like events, though among message-like events it only handles m.room.message event at the moment (EC does not need more than that), though it feels like strictly speaking it could interact with any other message-like events as well.

What about this:

pub struct EventFilter {
    pub event_type: TimelineEventType,
    pub event_kind: EventKind,
}

pub enum EventKind {
    StateEvent {
        key: String,
    },
    MessageLikeEvent {
        msgtype: Option<String>,
    },
}

Does this look sensible to you?

jplatte · 2023-08-23T12:49:08Z

I'd propose this instead:

pub enum EventFilter {
    MessageLike {
        event_type: MessageLikeEventType,
        /// Additional filter for the msgtype, only used for `m.room.message`.
        msgtype: Option<String>,
    }
    State {
        event_type: StateEventType,
        state_key: String,
    }
}

And, final question: Should the state_key be optional too? For example, could a widget ask for permission to write m.room.member for any state key?

daniel-abramov · 2023-08-23T13:41:07Z

I'd propose this instead:

pub enum EventFilter {
    MessageLike {
        event_type: MessageLikeEventType,
        /// Additional filter for the msgtype, only used for `m.room.message`.
        msgtype: Option<String>,
    }
    State {
        event_type: StateEventType,
        state_key: String,
    }
}

And, final question: Should the state_key be optional too? For example, could a widget ask for permission to write m.room.member for any state key?

The proposal makes sense 👍

Indeed, it can request permissions to read/send any state events. So, in this case, I suppose that the state_key should either be Option<String>, where None would be interpreted as "no limitations, any state key", unless we want to explicitly create an enum with the following variants StateKey = StateKey { key } | AnyStateKey. Wdyt?

jplatte · 2023-08-23T13:42:36Z

I think Option<String> is good enough, the docs can explain its meaning in detail.

toger5 · 2023-08-24T13:36:32Z

crates/matrix-sdk/src/widget/mod.rs

+    /// Whether or not the widget should be initialized on load message
+    /// (`ContentLoad` message), or upon creation/attaching of the widget to
+    /// the SDK's state machine that drives the API.
+    pub init_on_load: bool,


I think we should call it init_on_content_loaded even with the doc text I can see how one interprets it as:
init_on_load -> the widget initializes when the I frame is attached/created/"loaded".
(But we want this bool to define define, that the widget is initialized once it sends us the content loaded action.

widget_api: add initial skeleton

a5df9c5

daniel-abramov requested a review from a team as a code owner August 17, 2023 18:11

daniel-abramov requested review from Hywan and removed request for a team August 17, 2023 18:11

daniel-abramov marked this pull request as draft August 17, 2023 18:11

jplatte reviewed Aug 17, 2023

View reviewed changes

daniel-abramov and others added 6 commits August 17, 2023 21:02

widget_api: change order of mods and rename feat

8bed3a7

widget_api: change wording of the doc comment

1247f0c

Co-authored-by: Jonas Platte <jplatte+git@posteo.de>

widget_api: change wording of the doc comment

090f425

Co-authored-by: Jonas Platte <jplatte+git@posteo.de>

widget_api: change wording of the doc comment

d3fe2f8

Co-authored-by: Jonas Platte <jplatte+git@posteo.de>

widget_api: make mods private

d25c800

Co-authored-by: Jonas Platte <jplatte+git@posteo.de>

widget_api: change the doc comment

73ee5bf

Hywan reviewed Aug 21, 2023

View reviewed changes

daniel-abramov added 2 commits August 22, 2023 12:18

widget_api: introduce event filter types

012a215

widget_api: run rustup run nightly cargo fmt

463ea60

daniel-abramov marked this pull request as ready for review August 22, 2023 10:28

widget_api: rename widget_api -> widget

7f90b28

jplatte changed the title ~~widget_api: initial skeleton~~ Widget API: initial skeleton Aug 23, 2023

daniel-abramov added 2 commits August 23, 2023 11:46

widget: move widget::widget -> widget

583ce4f

widget: fix the left-overs that did not compile

448435d

jplatte reviewed Aug 23, 2023

View reviewed changes

jplatte requested changes Aug 23, 2023

View reviewed changes

widget: fix typos in the documentation

7793bcf

Co-authored-by: Jonas Platte <jplatte+git@posteo.de>

widget: properly spec event filters structures

02cde35

jplatte approved these changes Aug 23, 2023

View reviewed changes

jplatte enabled auto-merge (squash) August 23, 2023 14:55

jplatte merged commit 6ecd640 into matrix-org:main Aug 23, 2023

daniel-abramov deleted the client-widget-api-surface branch August 23, 2023 15:08

toger5 reviewed Aug 24, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Widget API: initial skeleton #2420

Widget API: initial skeleton #2420

daniel-abramov commented Aug 17, 2023

codecov bot commented Aug 17, 2023 •

edited

Loading

jplatte left a comment

jplatte Aug 17, 2023

daniel-abramov Aug 17, 2023 •

edited

Loading

jplatte Aug 17, 2023

daniel-abramov Aug 17, 2023

jplatte Aug 17, 2023 •

edited

Loading

toger5 Aug 18, 2023

jplatte Aug 18, 2023

daniel-abramov Aug 22, 2023

daniel-abramov commented Aug 17, 2023

jplatte commented Aug 17, 2023

Hywan Aug 21, 2023

jplatte Aug 21, 2023

daniel-abramov Aug 22, 2023

jplatte Aug 22, 2023

daniel-abramov Aug 23, 2023 •

edited

Loading

jplatte Aug 23, 2023

jplatte left a comment

jplatte Aug 23, 2023

jplatte Aug 23, 2023

jplatte Aug 23, 2023

jplatte left a comment •

edited

Loading

daniel-abramov commented Aug 23, 2023

jplatte commented Aug 23, 2023

daniel-abramov commented Aug 23, 2023

jplatte commented Aug 23, 2023

toger5 Aug 24, 2023


		use async_trait::async_trait;

		use crate::ruma::events::{StateEventType, TimelineEventType};

	use crate::ruma::events::{StateEventType, TimelineEventType};
	use crate::ruma::events::{MessageLikeEventType, StateEventType};

	Timeline(FilterContent<TimelineEventType>),
	MessageLike(FilterContent<MessageLikeEventType>),

	/// Filters for the timeline events, the `data` field is used to store the
	/// Filters for message-like events, the `data` field is used to store the

Widget API: initial skeleton #2420

Widget API: initial skeleton #2420

Conversation

daniel-abramov commented Aug 17, 2023

codecov bot commented Aug 17, 2023 • edited Loading

Codecov Report

jplatte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-abramov Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jplatte Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-abramov commented Aug 17, 2023

jplatte commented Aug 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-abramov Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jplatte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jplatte left a comment • edited Loading

Choose a reason for hiding this comment

daniel-abramov commented Aug 23, 2023

jplatte commented Aug 23, 2023

daniel-abramov commented Aug 23, 2023

jplatte commented Aug 23, 2023

Choose a reason for hiding this comment

codecov bot commented Aug 17, 2023 •

edited

Loading

daniel-abramov Aug 17, 2023 •

edited

Loading

jplatte Aug 17, 2023 •

edited

Loading

daniel-abramov Aug 23, 2023 •

edited

Loading

jplatte left a comment •

edited

Loading