New timeline API #940
New timeline API #940
Conversation
|
Things I would like to discuss at the current stage:
|
I think we should just remove the feature from the codebase. The example should be updated to use the new feature. For the FFI I would expect the FFI crate to be updated and hopefully tested in Element X as part of this PR. This does mean some coordination with the Element X team and live testing but should overall assure us that the PR works and is what Element X wants.
In both cases you can't be too sure if the client actually wants to actually render the individual item, i.e. is the message in view or not. Seems like any decision on whether we should fetch additional data from the server or not should be delegated to the client itself. In both cases the client can and should display a placeholder until it has all the data to render things fully. |
|
Re.
This is a good point. I guess we can still have the three-states thing though, have the client call |
jplatte
force-pushed
the
jplatte/new-timeline
branch
4 times, most recently
from
8d6c890
to
7af4457
Compare
This comment was marked as resolved.
This comment was marked as resolved.
jplatte
force-pushed
the
jplatte/new-timeline
branch
from
7af4457
to
a59ab1c
Compare
This comment was marked as resolved.
This comment was marked as resolved.
|
I'm guessing all timeline event types will be supported later? What is the plan with events that are not successfully deserialized? |
|
Yeah, the plan is to support all specced event types. Events that aren't successfully deserialized haven't been discussed so far, but I'm pretty sure we'll want to cover that somehow. Element Web has a dev mode thing where reactions (and I think edits too) show up as greyed-out extra entries in the timeline, I think we'll want to support something like that, maybe in the form of a "raw" timeline item that is added whenever an event doesn't otherwise produce a new timeline item. |
|
We plan to do the same thing in Fractal in debug builds, that's why I was asking. This seems like a good solution. One basic thing currently missing for |
jplatte
force-pushed
the
jplatte/new-timeline
branch
6 times, most recently
from
8780377
to
fb44613
Compare
|
I want to see whether bindings are generating correctly, but this isn't actually ready for review. |
jplatte
force-pushed
the
jplatte/new-timeline
branch
3 times, most recently
from
2bb9970
to
518785d
Compare
Codecov ReportBase: 72.77% // Head: 73.25% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #940 +/- ##
==========================================
+ Coverage 72.77% 73.25% +0.48%
==========================================
Files 106 109 +3
Lines 11867 12145 +278
==========================================
+ Hits 8636 8897 +261
- Misses 3231 3248 +17
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
jplatte
force-pushed
the
jplatte/new-timeline
branch
from
518785d
to
7965e63
Compare
|
This is currently used in this MR on Fractal: fractal!1136. I'll try to update it frequently with new changes from this PR. |
jplatte
force-pushed
the
jplatte/new-timeline
branch
from
7965e63
to
72bfb69
Compare
jplatte
force-pushed
the
jplatte/new-timeline
branch
9 times, most recently
from
8cf8f34
to
2d6db64
Compare
jplatte
force-pushed
the
jplatte/new-timeline
branch
4 times, most recently
from
f7f0cb4
to
b76484d
Compare
|
This is now ready for review! I have removed the FFI from this branch, and will post it as a separate PR. |
There was a problem hiding this comment.
This is a bit longish so bear with me, I didn't yet look at the implementation in detail but these things did catch my attention while I tried to use the new API.
Docs
Getting started with this was non-trivial, the docs are very generic and just
point you towards the docs for the StreamExt API, these have a lot of methods and there's no real way to know what you want.
Example isn't that nice
The new timeline feature is now used for multiple things:
- Consuming events that are part of the timeline
- Scrolling through the timeline
The old one was only used for point no 2, and the timeline example reflects
that, we should split that example into two, one for each timeline feature.
What the first example could look like:
diff --git a/examples/timeline/src/main.rs b/examples/timeline/src/main.rs
index ea35976a0b..345fc1d6a8 100644
--- a/examples/timeline/src/main.rs
+++ b/examples/timeline/src/main.rs
@@ -1,124 +1,81 @@
-use std::{env, process::exit, sync::Mutex, time::Duration};
-
+use anyhow::Result;
+use clap::Parser;
use futures::StreamExt;
use futures_signals::signal_vec::SignalVecExt;
-use matrix_sdk::{
- self,
- config::SyncSettings,
- room::Room,
- ruma::{
- api::client::filter::{FilterDefinition, LazyLoadOptions},
- events::{AnySyncMessageLikeEvent, AnySyncTimelineEvent, SyncMessageLikeEvent},
- uint,
- },
- store::make_store_config,
- Client, LoopCtrl,
-};
-use tokio::sync::oneshot;
+use matrix_sdk::{self, config::SyncSettings, ruma::OwnedRoomId, Client};
use url::Url;
-async fn login(homeserver_url: String, username: &str, password: &str) -> Client {
- let homeserver_url = Url::parse(&homeserver_url).expect("Couldn't parse the homeserver URL");
- let path = "./";
- let store_config = make_store_config(path, Some("some password")).unwrap();
- let client = Client::builder()
- .homeserver_url(homeserver_url)
- .store_config(store_config)
- .build()
- .await
- .unwrap();
+#[derive(Parser, Debug)]
+struct Cli {
+ /// The homeserver to connect to.
+ #[clap(value_parser)]
+ homeserver: Url,
- client
- .login_username(username, password)
- .initial_device_display_name("rust-sdk")
- .send()
- .await
- .unwrap();
- client
-}
+ /// The user name that should be used for the login.
+ #[clap(value_parser)]
+ user_name: String,
+
+ /// The password that should be used for the login.
+ #[clap(value_parser)]
+ password: String,
+
+ /// Set the proxy that should be used for the connection.
+ #[clap(short, long)]
+ proxy: Option<Url>,
-fn _event_content(event: AnySyncTimelineEvent) -> Option<String> {
- if let AnySyncTimelineEvent::MessageLike(AnySyncMessageLikeEvent::RoomMessage(
- SyncMessageLikeEvent::Original(event),
- )) = event
- {
- Some(event.content.msgtype.body().to_owned())
- } else {
- None
- }
+ /// Enable verbose logging output.
+ #[clap(short, long, action)]
+ verbose: bool,
+
+ /// The room id that we should listen for the,
+ #[clap(value_parser)]
+ room_id: OwnedRoomId,
}
-async fn print_timeline(room: Room) {
- let timeline = room.timeline();
- let mut timeline_stream = timeline.signal().to_stream();
- tokio::spawn(async move {
- while let Some(_diff) = timeline_stream.next().await {
- // Is a straight-forward CLI example of dynamic timeline items
- // possible?? let event = event.unwrap();
- //if let Some(content) =
- // event_content(event.event.deserialize().unwrap()) {
- // println!("{content}");
- //}
- }
- });
+async fn login(cli: Cli) -> Result<Client> {
+ let builder = Client::builder().homeserver_url(cli.homeserver);
- loop {
- match timeline.paginate_backwards(uint!(10)).await {
- Ok(outcome) if !outcome.more_messages => break,
- Ok(_) => {}
- Err(e) => {
- eprintln!("error paginating: {e}");
- }
- }
- }
+ let builder = if let Some(proxy) = cli.proxy { builder.proxy(proxy) } else { builder };
+
+ let client = builder.build().await?;
+
+ client
+ .login_username(&cli.user_name, &cli.password)
+ .initial_device_display_name("rust-sdk")
+ .send()
+ .await?;
+
+ Ok(client)
}
#[tokio::main]
-async fn main() -> anyhow::Result<()> {
+async fn main() -> Result<()> {
tracing_subscriber::fmt::init();
- let (homeserver_url, username, password, room_id) =
- match (env::args().nth(1), env::args().nth(2), env::args().nth(3), env::args().nth(4)) {
- (Some(a), Some(b), Some(c), Some(d)) => (a, b, c, d),
- _ => {
- eprintln!(
- "Usage: {} <homeserver_url> <username> <password> <room_id>",
- env::args().next().unwrap()
- );
- exit(1)
- }
- };
-
- let client = login(homeserver_url, &username, &password).await;
-
- let mut filter = FilterDefinition::default();
- filter.room.include_leave = true;
- filter.room.state.lazy_load_options =
- LazyLoadOptions::Enabled { include_redundant_members: false };
-
- let sync_settings = SyncSettings::new().timeout(Duration::from_secs(30)).filter(filter.into());
- let (sender, receiver) = oneshot::channel::<()>();
- let sender = Mutex::new(Some(sender));
- let client_clone = client.clone();
- tokio::spawn(async move {
- client_clone
- .sync_with_callback(sync_settings, |_| async {
- if let Some(sender) = sender.lock().unwrap().take() {
- sender.send(()).unwrap();
- }
- LoopCtrl::Continue
- })
- .await
- .unwrap();
- });
+ let cli = Cli::parse();
+ let room_id = cli.room_id.clone();
+ let client = login(cli).await?;
+
+ let sync_settings = SyncSettings::default();
// Wait for the first sync response
println!("Wait for the first sync");
- receiver.await.unwrap();
- let room = client.get_room(room_id.as_str().try_into().unwrap()).unwrap();
+ client.sync_once(sync_settings.clone()).await?;
+
+ // Get the timeline stream and listen to it.
+ let room = client.get_room(&room_id).unwrap();
+ let timeline = room.timeline();
+ let mut timeline_stream = timeline.signal().to_stream();
+
+ tokio::spawn(async move {
+ while let Some(diff) = timeline_stream.next().await {
+ println!("Received a timeline diff {diff:#?}");
+ }
+ });
- print_timeline(room).await;
+ // Sync forever
+ client.sync(sync_settings).await?;
Ok(())
}Incidentally, putting something along those lines into the docs would have
helped quite a bit.
Local echo support hard to use
Sending a message means:
VecDiff::Push(TimelineKey::TransactionId)- The sending of the eventVecDiff::UpdateAt(_, TimelineKey::TransactionId)- The receival of the/sendresponse. First remote echo.VecDiff::UpdateAt(_, TimelineKey::EventId)- The remote echo through the/synccall.
Clients might want to do one of the following:
- Render only at no. 1 or no. 3 - No local echo support
- Render at no. 1 and 2 - Simple local echo support, i.e. grey message or a single checkmark.
- Render at no. 1, 2, and 3 - What element does, two checkmarks.
The first two can be connected without additional state, while the last one
can't easily.
You need to pull out the Raw event and look for the unsigned field and if it contains the TransactionId
No. 2, the first VecDiff::UpdateAt, should contain the EventId, since this is what the response gives us. This way we can already mark the event as completed.
No 3, the second VecDif::UpdateAt should have a simple accessor for the TransactionId that's part of the unsigned field.
Looking at this further, this seems to be a bit low level and generic, or perhaps just not explicit enough.
If I pretend to now know the Matrix spec, I would be wholly confused by the TransactionId and EventId timeline keys. But if I saw an instant messenger I could guess what a local echo is.
So as a client developer that wants to add local echo I would like my
VecDiff tell me explicitly this TimelineItem is one of the three scenarios mentioned above. As it is now, I have to guestimate what the different TimelineKeys mean and how to render them.
I think that this would also let us to document things much better. Edits seem to have a similar problem, though an is_edit() method exists which let's you figure this out a bit easier.
Things to fix
The PR seems to work well for what it tries to achieve. It is much easier to
consume events using this API, but I still think we might tweak the API to make it even simpler. There are some things I would like to see fixed before we merge this:
-
Support for
m.room.encryptedevents. E2EE is one of our best selling points yet we always treat it as an afterthought. After Sliding Sync timeline events and extensions #1054 is merged, Element X will need this anyways. -
We don't have access to the
redacted_becauseor ratherRedactedUnsignedfield when we receive a diff for aTimelineItemContent::RedactedMessage. -
VecDiffs should at least let you have the event id at the first remote echo stage, step 2. Later on I would like to see a more explicit local echo API.
|
Like I said on matrix, currently we don't get the aggregated reactions. However if we forward the |
jplatte
force-pushed
the
jplatte/new-timeline
branch
from
b76484d
to
fddec6e
Compare
|
I can confirm that I get all the reactions now. |
| TimelineKey, | ||
| }; | ||
|
|
||
| pub(super) fn handle_live_event( |
There was a problem hiding this comment.
Shouldn't this be part of TimelineInner?
There was a problem hiding this comment.
TimelineInner is a fairly recent addition compared to the rest of this module but yeah it sounds reasonable.
| _ => {} | ||
| } | ||
|
|
||
| if !self.event_added { |
There was a problem hiding this comment.
Can't this be handled like the did_update flag? Where we return the value instead of mutating things in some magic place somewhere else?
There was a problem hiding this comment.
Well, there is only a single place where it is set, so I think it's still obvious enough what happens and this way the method signatures are shorter. We can try out the other way though.
| } | ||
| } | ||
|
|
||
| fn find_event( |
There was a problem hiding this comment.
Hmm, don't we expect this to be problematic? Do we ever remove items from the timeline? I don't think we do. How many items do we want to support? Don't we want to let the caller configure how many items we keep?
We'll likely need an index that maps from the key to the index in any case. Let's at least add a fixme.
There was a problem hiding this comment.
I'm a little confused, do you mean this is problematic if we keeps those indices around? The intention of this function is just to find the right index to update, it is only meant to be valid for a very short duration.
We could have a separate index so we don't have to do a linear scan, but that would be more complex and I don't think we need it right now (maybe never if we put a reasonable upper bound on the amount of entries that can be in the timeline).
There was a problem hiding this comment.
No, I meant the growth of the MutableVec is unbounded thus the rfind() and in turn the find_event() function will take an increasing amount of time in the worst case.
| } | ||
| } | ||
|
|
||
| /// Add more events to the start of the timeline. |
There was a problem hiding this comment.
I wonder if calling this operation scrolling would add more clarity.
|
|
||
| self.maybe_update_timeline_item(event_id, "edit", |item| { | ||
| if self.meta.sender != item.sender() { | ||
| info!( |
There was a problem hiding this comment.
All the log lines seem to be missing the room id, this will be quite annoying to debug if we don't know the room the log line applies to.
There was a problem hiding this comment.
I've added it for local echo and back-pagination, for live events I think we should just add a useful span for all event handlers, not just the timeline ones.
| inner: TimelineInner, | ||
| room: room::Common, | ||
| start_token: Mutex<Option<String>>, | ||
| _end_token: Mutex<Option<String>>, |
There was a problem hiding this comment.
what is this for? Is that for future usage?
There was a problem hiding this comment.
Yeah, future forwards pagination support.
|
|
||
| pub(super) fn to_redacted(&self) -> Self { | ||
| build!(Self { | ||
| // FIXME: Change when we support state events |
There was a problem hiding this comment.
Tracking issue for the FIXME's? There's quite a few in this file...
|
|
||
| /// The reactions. | ||
| /// | ||
| /// Key: The reaction, usually an emoji.\ |
There was a problem hiding this comment.
| /// Key: The reaction, usually an emoji.\ | |
| /// Key: The reaction, usually an emoji. |
There was a problem hiding this comment.
This is actually load-bearing, it inserts a line break before the Value line without putting them on entirely separate paragraphs (the same way as double space at the end of the line, but that is stripped by many editors).
| } | ||
|
|
||
| let mut lock = self.timeline.items.lock_mut(); | ||
| if let Some((idx, item)) = find_event(&lock, event_id) { |
There was a problem hiding this comment.
I wonder if that could become a bottle neck if we have scrolled back a room for a bit ... but I'll put that off as a future problem.
jplatte
force-pushed
the
jplatte/new-timeline
branch
from
ff8f732
to
533484f
Compare
Status:
Not planned for this PR, but planned for later: