From 5675ac7f46fee634ca98d285348ae5c9f2bc7535 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 8 Jan 2025 15:10:33 +0100 Subject: [PATCH] refactor(sdk): Remove `SlidingSyncRoomInner::client`. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch removes `SlidingSyncRoomInner::client` because, first off, it's not `Send`, and second, it's useless. Nobody uses it, it's basically dead code… annoying dead code… bad dead code! --- .../integration/timeline/sliding_sync.rs | 24 ++--- crates/matrix-sdk/src/sliding_sync/cache.rs | 9 +- crates/matrix-sdk/src/sliding_sync/error.rs | 2 +- crates/matrix-sdk/src/sliding_sync/mod.rs | 12 +-- crates/matrix-sdk/src/sliding_sync/room.rs | 99 +++++++------------ crates/matrix-sdk/src/sliding_sync/utils.rs | 4 +- 6 files changed, 56 insertions(+), 94 deletions(-) diff --git a/crates/matrix-sdk-ui/tests/integration/timeline/sliding_sync.rs b/crates/matrix-sdk-ui/tests/integration/timeline/sliding_sync.rs index 9a1301acd..75a116479 100644 --- a/crates/matrix-sdk-ui/tests/integration/timeline/sliding_sync.rs +++ b/crates/matrix-sdk-ui/tests/integration/timeline/sliding_sync.rs @@ -20,8 +20,8 @@ use assert_matches2::assert_let; use eyeball_im::{Vector, VectorDiff}; use futures_util::{pin_mut, FutureExt, Stream, StreamExt}; use matrix_sdk::{ - test_utils::logged_in_client_with_server, SlidingSync, SlidingSyncList, SlidingSyncListBuilder, - SlidingSyncMode, UpdateSummary, + test_utils::{logged_in_client, logged_in_client_with_server}, + Client, SlidingSync, SlidingSyncList, SlidingSyncListBuilder, SlidingSyncMode, UpdateSummary, }; use matrix_sdk_test::{async_test, mocks::mock_encryption_state}; use matrix_sdk_ui::{ @@ -223,7 +223,9 @@ macro_rules! assert_timeline_stream { pub(crate) use assert_timeline_stream; -async fn new_sliding_sync(lists: Vec) -> Result<(MockServer, SlidingSync)> { +async fn new_sliding_sync( + lists: Vec, +) -> Result<(Client, MockServer, SlidingSync)> { let (client, server) = logged_in_client_with_server().await; let mut sliding_sync_builder = client.sliding_sync("integration-test")?; @@ -234,7 +236,7 @@ async fn new_sliding_sync(lists: Vec) -> Result<(MockSer let sliding_sync = sliding_sync_builder.build().await?; - Ok((server, sliding_sync)) + Ok((client, server, sliding_sync)) } async fn create_one_room( @@ -268,13 +270,13 @@ async fn create_one_room( } async fn timeline_test_helper( + client: &Client, sliding_sync: &SlidingSync, room_id: &RoomId, ) -> Result<(Vector>, impl Stream>>)> { let sliding_sync_room = sliding_sync.get_room(room_id).await.unwrap(); let room_id = sliding_sync_room.room_id(); - let client = sliding_sync_room.client(); let sdk_room = client.get_room(room_id).ok_or_else(|| { anyhow::anyhow!("Room {room_id} not found in client. Can't provide a timeline for it") })?; @@ -295,7 +297,7 @@ impl Match for SlidingSyncMatcher { #[async_test] async fn test_timeline_basic() -> Result<()> { - let (server, sliding_sync) = new_sliding_sync(vec![SlidingSyncList::builder("foo") + let (client, server, sliding_sync) = new_sliding_sync(vec![SlidingSyncList::builder("foo") .sync_mode(SlidingSyncMode::new_selective().add_range(0..=10))]) .await?; @@ -309,7 +311,7 @@ async fn test_timeline_basic() -> Result<()> { mock_encryption_state(&server, false).await; let (timeline_items, mut timeline_stream) = - timeline_test_helper(&sliding_sync, room_id).await?; + timeline_test_helper(&client, &sliding_sync, room_id).await?; assert!(timeline_items.is_empty()); // Receiving a bunch of events. @@ -344,7 +346,7 @@ async fn test_timeline_basic() -> Result<()> { #[async_test] async fn test_timeline_duplicated_events() -> Result<()> { - let (server, sliding_sync) = new_sliding_sync(vec![SlidingSyncList::builder("foo") + let (client, server, sliding_sync) = new_sliding_sync(vec![SlidingSyncList::builder("foo") .sync_mode(SlidingSyncMode::new_selective().add_range(0..=10))]) .await?; @@ -357,7 +359,7 @@ async fn test_timeline_duplicated_events() -> Result<()> { mock_encryption_state(&server, false).await; - let (_, mut timeline_stream) = timeline_test_helper(&sliding_sync, room_id).await?; + let (_, mut timeline_stream) = timeline_test_helper(&client, &sliding_sync, room_id).await?; // Receiving events. { @@ -422,7 +424,7 @@ async fn test_timeline_duplicated_events() -> Result<()> { #[async_test] async fn test_timeline_read_receipts_are_updated_live() -> Result<()> { - let (server, sliding_sync) = new_sliding_sync(vec![SlidingSyncList::builder("foo") + let (client, server, sliding_sync) = new_sliding_sync(vec![SlidingSyncList::builder("foo") .sync_mode(SlidingSyncMode::new_selective().add_range(0..=10))]) .await?; @@ -436,7 +438,7 @@ async fn test_timeline_read_receipts_are_updated_live() -> Result<()> { mock_encryption_state(&server, false).await; let (timeline_items, mut timeline_stream) = - timeline_test_helper(&sliding_sync, room_id).await?; + timeline_test_helper(&client, &sliding_sync, room_id).await?; assert!(timeline_items.is_empty()); // Receiving initial events. diff --git a/crates/matrix-sdk/src/sliding_sync/cache.rs b/crates/matrix-sdk/src/sliding_sync/cache.rs index 22f52f27b..a882e0e6d 100644 --- a/crates/matrix-sdk/src/sliding_sync/cache.rs +++ b/crates/matrix-sdk/src/sliding_sync/cache.rs @@ -248,10 +248,7 @@ pub(super) async fn restore_sliding_sync_state( restored_fields.rooms = frozen_rooms .into_iter() .map(|frozen_room| { - ( - frozen_room.room_id.clone(), - SlidingSyncRoom::from_frozen(frozen_room, client.clone()), - ) + (frozen_room.room_id.clone(), SlidingSyncRoom::from_frozen(frozen_room)) }) .collect(); } @@ -355,11 +352,11 @@ mod tests { rooms.insert( room_id1.clone(), - SlidingSyncRoom::new(client.clone(), room_id1.clone(), None, Vec::new()), + SlidingSyncRoom::new(room_id1.clone(), None, Vec::new()), ); rooms.insert( room_id2.clone(), - SlidingSyncRoom::new(client.clone(), room_id2.clone(), None, Vec::new()), + SlidingSyncRoom::new(room_id2.clone(), None, Vec::new()), ); } diff --git a/crates/matrix-sdk/src/sliding_sync/error.rs b/crates/matrix-sdk/src/sliding_sync/error.rs index c72d5333e..774e31510 100644 --- a/crates/matrix-sdk/src/sliding_sync/error.rs +++ b/crates/matrix-sdk/src/sliding_sync/error.rs @@ -1,7 +1,7 @@ //! Sliding Sync errors. +use matrix_sdk_common::executor::JoinError; use thiserror::Error; -use tokio::task::JoinError; /// Internal representation of errors in Sliding Sync. #[derive(Error, Debug)] diff --git a/crates/matrix-sdk/src/sliding_sync/mod.rs b/crates/matrix-sdk/src/sliding_sync/mod.rs index 11a3d7858..cfdf45105 100644 --- a/crates/matrix-sdk/src/sliding_sync/mod.rs +++ b/crates/matrix-sdk/src/sliding_sync/mod.rs @@ -359,7 +359,6 @@ impl SlidingSync { rooms_map.insert( room_id.clone(), SlidingSyncRoom::new( - self.inner.client.clone(), room_id.clone(), room_data.prev_batch, timeline, @@ -2269,9 +2268,6 @@ mod tests { #[async_test] async fn test_limited_flag_computation() { - let server = MockServer::start().await; - let client = logged_in_client(Some(server.uri())).await; - let make_event = |event_id: &str| -> SyncTimelineEvent { SyncTimelineEvent::new( Raw::from_json_string( @@ -2313,7 +2309,6 @@ mod tests { // it's not marked as initial in the response. not_initial.to_owned(), SlidingSyncRoom::new( - client.clone(), no_overlap.to_owned(), None, vec![event_a.clone(), event_b.clone()], @@ -2323,7 +2318,6 @@ mod tests { // This has no events overlapping with the response timeline, hence limited. no_overlap.to_owned(), SlidingSyncRoom::new( - client.clone(), no_overlap.to_owned(), None, vec![event_a.clone(), event_b.clone()], @@ -2333,7 +2327,6 @@ mod tests { // This has event_c in common with the response timeline. partial_overlap.to_owned(), SlidingSyncRoom::new( - client.clone(), partial_overlap.to_owned(), None, vec![event_a.clone(), event_b.clone(), event_c.clone()], @@ -2343,7 +2336,6 @@ mod tests { // This has all events in common with the response timeline. complete_overlap.to_owned(), SlidingSyncRoom::new( - client.clone(), partial_overlap.to_owned(), None, vec![event_c.clone(), event_d.clone()], @@ -2354,7 +2346,6 @@ mod tests { // limited. no_remote_events.to_owned(), SlidingSyncRoom::new( - client.clone(), no_remote_events.to_owned(), None, vec![event_c.clone(), event_d.clone()], @@ -2364,14 +2355,13 @@ mod tests { // We don't have events for this room locally, and even if the remote room contains // some events, it's not a limited sync. no_local_events.to_owned(), - SlidingSyncRoom::new(client.clone(), no_local_events.to_owned(), None, vec![]), + SlidingSyncRoom::new(no_local_events.to_owned(), None, vec![]), ), ( // Already limited, but would be marked limited if the flag wasn't ignored (same as // partial overlap). already_limited.to_owned(), SlidingSyncRoom::new( - client, already_limited.to_owned(), None, vec![event_a, event_b, event_c.clone()], diff --git a/crates/matrix-sdk/src/sliding_sync/room.rs b/crates/matrix-sdk/src/sliding_sync/room.rs index 1f4b4dbec..b78cdc147 100644 --- a/crates/matrix-sdk/src/sliding_sync/room.rs +++ b/crates/matrix-sdk/src/sliding_sync/room.rs @@ -8,8 +8,6 @@ use matrix_sdk_base::{deserialized_responses::SyncTimelineEvent, sliding_sync::h use ruma::{OwnedRoomId, RoomId}; use serde::{Deserialize, Serialize}; -use crate::Client; - /// The state of a [`SlidingSyncRoom`]. #[derive(Copy, Clone, Debug, Default, PartialEq)] pub enum SlidingSyncRoomState { @@ -40,14 +38,12 @@ pub struct SlidingSyncRoom { impl SlidingSyncRoom { /// Create a new `SlidingSyncRoom`. pub fn new( - client: Client, room_id: OwnedRoomId, prev_batch: Option, timeline: Vec, ) -> Self { Self { inner: Arc::new(SlidingSyncRoomInner { - client, room_id, state: RwLock::new(SlidingSyncRoomState::NotLoaded), prev_batch: RwLock::new(prev_batch), @@ -74,11 +70,6 @@ impl SlidingSyncRoom { self.inner.timeline_queue.read().unwrap().clone() } - /// Get a clone of the associated client. - pub fn client(&self) -> Client { - self.inner.client.clone() - } - pub(super) fn update( &mut self, room_data: http::response::Room, @@ -129,12 +120,11 @@ impl SlidingSyncRoom { *state = SlidingSyncRoomState::Loaded; } - pub(super) fn from_frozen(frozen_room: FrozenSlidingSyncRoom, client: Client) -> Self { + pub(super) fn from_frozen(frozen_room: FrozenSlidingSyncRoom) -> Self { let FrozenSlidingSyncRoom { room_id, prev_batch, timeline_queue } = frozen_room; Self { inner: Arc::new(SlidingSyncRoomInner { - client, room_id, prev_batch: RwLock::new(prev_batch), state: RwLock::new(SlidingSyncRoomState::Preloaded), @@ -157,9 +147,6 @@ impl SlidingSyncRoom { #[derive(Debug)] struct SlidingSyncRoomInner { - /// The client, used to fetch [`Room`][crate::Room]. - client: Client, - /// The room ID. room_id: OwnedRoomId, @@ -232,13 +219,9 @@ mod tests { use matrix_sdk_test::async_test; use ruma::{events::room::message::RoomMessageEventContent, room_id, serde::Raw, RoomId}; use serde_json::json; - use wiremock::MockServer; use super::{http, NUMBER_OF_TIMELINE_EVENTS_TO_KEEP_FOR_THE_CACHE}; - use crate::{ - sliding_sync::{FrozenSlidingSyncRoom, SlidingSyncRoom, SlidingSyncRoomState}, - test_utils::logged_in_client, - }; + use crate::sliding_sync::{FrozenSlidingSyncRoom, SlidingSyncRoom, SlidingSyncRoomState}; macro_rules! room_response { ( $( $json:tt )+ ) => { @@ -248,24 +231,21 @@ mod tests { }; } - async fn new_room(room_id: &RoomId, inner: http::response::Room) -> SlidingSyncRoom { - new_room_with_timeline(room_id, inner, vec![]).await + fn new_room(room_id: &RoomId, inner: http::response::Room) -> SlidingSyncRoom { + new_room_with_timeline(room_id, inner, vec![]) } - async fn new_room_with_timeline( + fn new_room_with_timeline( room_id: &RoomId, inner: http::response::Room, timeline: Vec, ) -> SlidingSyncRoom { - let server = MockServer::start().await; - let client = logged_in_client(Some(server.uri())).await; - - SlidingSyncRoom::new(client, room_id.to_owned(), inner.prev_batch, timeline) + SlidingSyncRoom::new(room_id.to_owned(), inner.prev_batch, timeline) } #[async_test] async fn test_state_from_not_loaded() { - let mut room = new_room(room_id!("!foo:bar.org"), room_response!({})).await; + let mut room = new_room(room_id!("!foo:bar.org"), room_response!({})); assert_eq!(room.state(), SlidingSyncRoomState::NotLoaded); @@ -277,7 +257,7 @@ mod tests { #[async_test] async fn test_state_from_preloaded() { - let mut room = new_room(room_id!("!foo:bar.org"), room_response!({})).await; + let mut room = new_room(room_id!("!foo:bar.org"), room_response!({})); room.set_state(SlidingSyncRoomState::Preloaded); @@ -290,7 +270,7 @@ mod tests { #[async_test] async fn test_room_room_id() { let room_id = room_id!("!foo:bar.org"); - let room = new_room(room_id, room_response!({})).await; + let room = new_room(room_id, room_response!({})); assert_eq!(room.room_id(), room_id); } @@ -299,7 +279,7 @@ mod tests { async fn test_prev_batch() { // Default value. { - let room = new_room(room_id!("!foo:bar.org"), room_response!({})).await; + let room = new_room(room_id!("!foo:bar.org"), room_response!({})); assert_eq!(room.prev_batch(), None); } @@ -307,15 +287,14 @@ mod tests { // Some value when initializing. { let room = - new_room(room_id!("!foo:bar.org"), room_response!({"prev_batch": "t111_222_333"})) - .await; + new_room(room_id!("!foo:bar.org"), room_response!({"prev_batch": "t111_222_333"})); assert_eq!(room.prev_batch(), Some("t111_222_333".to_owned())); } // Some value when updating. { - let mut room = new_room(room_id!("!foo:bar.org"), room_response!({})).await; + let mut room = new_room(room_id!("!foo:bar.org"), room_response!({})); assert_eq!(room.prev_batch(), None); @@ -329,7 +308,7 @@ mod tests { #[async_test] async fn test_timeline_queue_initially_empty() { - let room = new_room(room_id!("!foo:bar.org"), room_response!({})).await; + let room = new_room(room_id!("!foo:bar.org"), room_response!({})); assert!(room.timeline_queue().is_empty()); } @@ -368,8 +347,8 @@ mod tests { }; } - #[async_test] - async fn test_timeline_queue_initially_not_empty() { + #[test] + fn test_timeline_queue_initially_not_empty() { let room = new_room_with_timeline( room_id!("!foo:bar.org"), room_response!({}), @@ -377,8 +356,7 @@ mod tests { timeline_event!(from "@alice:baz.org" with id "$x0:baz.org" at 0: "message 0"), timeline_event!(from "@alice:baz.org" with id "$x1:baz.org" at 1: "message 1"), ], - ) - .await; + ); { let timeline_queue = room.timeline_queue(); @@ -394,8 +372,8 @@ mod tests { } } - #[async_test] - async fn test_timeline_queue_update_with_empty_timeline() { + #[test] + fn test_timeline_queue_update_with_empty_timeline() { let mut room = new_room_with_timeline( room_id!("!foo:bar.org"), room_response!({}), @@ -403,8 +381,7 @@ mod tests { timeline_event!(from "@alice:baz.org" with id "$x0:baz.org" at 0: "message 0"), timeline_event!(from "@alice:baz.org" with id "$x1:baz.org" at 1: "message 1"), ], - ) - .await; + ); { let timeline_queue = room.timeline_queue(); @@ -436,8 +413,8 @@ mod tests { } } - #[async_test] - async fn test_timeline_queue_update_with_empty_timeline_and_with_limited() { + #[test] + fn test_timeline_queue_update_with_empty_timeline_and_with_limited() { let mut room = new_room_with_timeline( room_id!("!foo:bar.org"), room_response!({}), @@ -445,8 +422,7 @@ mod tests { timeline_event!(from "@alice:baz.org" with id "$x0:baz.org" at 0: "message 0"), timeline_event!(from "@alice:baz.org" with id "$x1:baz.org" at 1: "message 1"), ], - ) - .await; + ); { let timeline_queue = room.timeline_queue(); @@ -477,8 +453,8 @@ mod tests { } } - #[async_test] - async fn test_timeline_queue_update_from_preloaded() { + #[test] + fn test_timeline_queue_update_from_preloaded() { let mut room = new_room_with_timeline( room_id!("!foo:bar.org"), room_response!({}), @@ -486,8 +462,7 @@ mod tests { timeline_event!(from "@alice:baz.org" with id "$x0:baz.org" at 0: "message 0"), timeline_event!(from "@alice:baz.org" with id "$x1:baz.org" at 1: "message 1"), ], - ) - .await; + ); room.set_state(SlidingSyncRoomState::Preloaded); @@ -527,8 +502,8 @@ mod tests { } } - #[async_test] - async fn test_timeline_queue_update_from_not_loaded() { + #[test] + fn test_timeline_queue_update_from_not_loaded() { let mut room = new_room_with_timeline( room_id!("!foo:bar.org"), room_response!({}), @@ -536,8 +511,7 @@ mod tests { timeline_event!(from "@alice:baz.org" with id "$x0:baz.org" at 0: "message 0"), timeline_event!(from "@alice:baz.org" with id "$x1:baz.org" at 1: "message 1"), ], - ) - .await; + ); { let timeline_queue = room.timeline_queue(); @@ -577,8 +551,8 @@ mod tests { } } - #[async_test] - async fn test_timeline_queue_update_from_not_loaded_with_limited() { + #[test] + fn test_timeline_queue_update_from_not_loaded_with_limited() { let mut room = new_room_with_timeline( room_id!("!foo:bar.org"), room_response!({}), @@ -586,8 +560,7 @@ mod tests { timeline_event!(from "@alice:baz.org" with id "$x0:baz.org" at 0: "message 0"), timeline_event!(from "@alice:baz.org" with id "$x1:baz.org" at 1: "message 1"), ], - ) - .await; + ); { let timeline_queue = room.timeline_queue(); @@ -678,8 +651,8 @@ mod tests { assert_eq!(deserialized.room_id, frozen_room.room_id); } - #[async_test] - async fn test_frozen_sliding_sync_room_has_a_capped_version_of_the_timeline() { + #[test] + fn test_frozen_sliding_sync_room_has_a_capped_version_of_the_timeline() { // Just below the limit. { let max = NUMBER_OF_TIMELINE_EVENTS_TO_KEEP_FOR_THE_CACHE - 1; @@ -705,8 +678,7 @@ mod tests { room_id!("!foo:bar.org"), room_response!({}), timeline_events, - ) - .await; + ); let frozen_room = FrozenSlidingSyncRoom::from(&room); assert_eq!(frozen_room.timeline_queue.len(), max + 1); @@ -743,8 +715,7 @@ mod tests { room_id!("!foo:bar.org"), room_response!({}), timeline_events, - ) - .await; + ); let frozen_room = FrozenSlidingSyncRoom::from(&room); assert_eq!( diff --git a/crates/matrix-sdk/src/sliding_sync/utils.rs b/crates/matrix-sdk/src/sliding_sync/utils.rs index 98d2619b6..6c699a16a 100644 --- a/crates/matrix-sdk/src/sliding_sync/utils.rs +++ b/crates/matrix-sdk/src/sliding_sync/utils.rs @@ -1,5 +1,7 @@ //! Moaaar features for Sliding Sync. +#![cfg(feature = "e2e-encryption")] + use std::{ future::Future, pin::Pin, @@ -23,7 +25,7 @@ impl Drop for AbortOnDrop { } } -impl Future for AbortOnDrop { +impl Future for AbortOnDrop { type Output = Result; fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll {