Skip to content

Commit

Permalink
refactor(sdk): Remove SlidingSyncRoomInner::client.
Browse files Browse the repository at this point in the history
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!
  • Loading branch information
Hywan committed Jan 8, 2025
1 parent 3f6c202 commit ca9daef
Show file tree
Hide file tree
Showing 6 changed files with 24 additions and 53 deletions.
24 changes: 13 additions & 11 deletions crates/matrix-sdk-ui/tests/integration/timeline/sliding_sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -223,7 +223,9 @@ macro_rules! assert_timeline_stream {

pub(crate) use assert_timeline_stream;

async fn new_sliding_sync(lists: Vec<SlidingSyncListBuilder>) -> Result<(MockServer, SlidingSync)> {
async fn new_sliding_sync(
lists: Vec<SlidingSyncListBuilder>,
) -> Result<(Client, MockServer, SlidingSync)> {
let (client, server) = logged_in_client_with_server().await;

let mut sliding_sync_builder = client.sliding_sync("integration-test")?;
Expand All @@ -234,7 +236,7 @@ async fn new_sliding_sync(lists: Vec<SlidingSyncListBuilder>) -> Result<(MockSer

let sliding_sync = sliding_sync_builder.build().await?;

Ok((server, sliding_sync))
Ok((client, server, sliding_sync))
}

async fn create_one_room(
Expand Down Expand Up @@ -268,13 +270,13 @@ async fn create_one_room(
}

async fn timeline_test_helper(
client: &Client,
sliding_sync: &SlidingSync,
room_id: &RoomId,
) -> Result<(Vector<Arc<TimelineItem>>, impl Stream<Item = VectorDiff<Arc<TimelineItem>>>)> {
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")
})?;
Expand All @@ -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?;

Expand All @@ -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.
Expand Down Expand Up @@ -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?;

Expand All @@ -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.
{
Expand Down Expand Up @@ -423,7 +425,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?;

Expand All @@ -437,7 +439,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.
Expand Down
9 changes: 3 additions & 6 deletions crates/matrix-sdk/src/sliding_sync/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down Expand Up @@ -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()),
);
}

Expand Down
2 changes: 1 addition & 1 deletion crates/matrix-sdk/src/sliding_sync/error.rs
Original file line number Diff line number Diff line change
@@ -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)]
Expand Down
12 changes: 1 addition & 11 deletions crates/matrix-sdk/src/sliding_sync/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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()],
Expand All @@ -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()],
Expand All @@ -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()],
Expand All @@ -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()],
Expand All @@ -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()],
Expand All @@ -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()],
Expand Down
26 changes: 3 additions & 23 deletions crates/matrix-sdk/src/sliding_sync/room.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -40,14 +38,12 @@ pub struct SlidingSyncRoom {
impl SlidingSyncRoom {
/// Create a new `SlidingSyncRoom`.
pub fn new(
client: Client,
room_id: OwnedRoomId,
prev_batch: Option<String>,
timeline: Vec<SyncTimelineEvent>,
) -> Self {
Self {
inner: Arc::new(SlidingSyncRoomInner {
client,
room_id,
state: RwLock::new(SlidingSyncRoomState::NotLoaded),
prev_batch: RwLock::new(prev_batch),
Expand All @@ -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,
Expand Down Expand Up @@ -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),
Expand All @@ -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,

Expand Down Expand Up @@ -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 )+ ) => {
Expand All @@ -257,10 +240,7 @@ mod tests {
inner: http::response::Room,
timeline: Vec<SyncTimelineEvent>,
) -> 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]
Expand Down
4 changes: 3 additions & 1 deletion crates/matrix-sdk/src/sliding_sync/utils.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
//! Moaaar features for Sliding Sync.
#![cfg(feature = "e2e-encryption")]

use std::{
future::Future,
pin::Pin,
Expand All @@ -23,7 +25,7 @@ impl<T> Drop for AbortOnDrop<T> {
}
}

impl<T> Future for AbortOnDrop<T> {
impl<T: 'static> Future for AbortOnDrop<T> {
type Output = Result<T, JoinError>;

fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
Expand Down

0 comments on commit ca9daef

Please sign in to comment.