Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Widget API: initial skeleton #2420
Widget API: initial skeleton #2420
Changes from 1 commit
a5df9c5
8bed3a7
1247f0c
090f425
d3fe2f8
d25c800
73ee5bf
012a215
463ea60
7f90b28
583ce4f
448435d
7793bcf
02cde35
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove the blank lines between all of the module declarations, and instead have just one to separate the
mod
s from thepub use
s?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. One question though: should I also remove the empty lines around the
sliding_sync
. Rationale: I addedpub mod widget_api
there since it was following the experimental sliding sync module which also was separated by spaces.I.e. it was:
So I placed the
pub mod widget_api
right after thepub mod sliding_sync
. I've pushed a change, hopefully I got it right.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rename this module
widget
instead ofwidget_api
. Just a personal taste compared to the rest of the SDK.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 itwidget
, 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 towidget
if that's your preference folks!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The client widget API couldn't live in this crate though, right? In that case, let's do the rename as suggested by Ivan.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
fromruma::events
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
andf: String -> EventType
transformation). @toger5, could you chime in? 🙂There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Updated.