From de09b781ec29ca163f677fc96da7aa35b18298dc Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 28 Jun 2023 11:11:36 +0200 Subject: [PATCH] feat(ui): Do not update the viewport of the room list if it hasn't changed. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Imagine the room list has the viewport set to the range `0..=19`. The user scrolls quickly to `50..=69` to see something below and immediately scrolls back to its initial position, without stopping the scroll at any moment in between. The app using this API might update the viewport from `0..=19` to… `0..=19`. Updating the viewport, updates the `visible_rooms` sliding sync list. Since a list is modified, the current sync-loop iteration is skipped over and a new one restarts, i.e. the current in-flight request is cancelled… for nothing. This patch prevents this situation. The current viewport ranges is stored in `RoomListService`. `RoomListService::apply_input` used to return `Result<(), Error>`, now it returns `Result`. The `InputResult` enum is a new type. It represents whether an input has been applied or ignored, which must be differentiate from errors. So now, if the viewport changes and it's not different from the previous value, `InputResult::Ignored` is returned. --- crates/matrix-sdk-ui/src/room_list/mod.rs | 63 ++++++++++++++----- .../tests/integration/room_list.rs | 23 +++++-- 2 files changed, 66 insertions(+), 20 deletions(-) diff --git a/crates/matrix-sdk-ui/src/room_list/mod.rs b/crates/matrix-sdk-ui/src/room_list/mod.rs index 8fe36d50a..4977d1e88 100644 --- a/crates/matrix-sdk-ui/src/room_list/mod.rs +++ b/crates/matrix-sdk-ui/src/room_list/mod.rs @@ -87,15 +87,27 @@ use ruma::{ }; pub use state::*; use thiserror::Error; -use tokio::sync::RwLock; +use tokio::sync::{Mutex, RwLock}; /// The [`RoomListService`] type. See the module's documentation to learn more. #[derive(Debug)] pub struct RoomListService { + /// The Sliding Sync instance. sliding_sync: Arc, + + /// The current state of the `RoomListService`. + /// + /// `RoomListService` is a simple state-machine. + state: Observable, + /// Room cache, to avoid recreating `Room`s everytime users fetch them. rooms: Arc>>, - state: Observable, + + /// The current viewport ranges. + /// + /// This is useful to avoid resetting the ranges to the same value, + /// which would cancel the current in-flight sync request. + viewport_ranges: Mutex, } impl RoomListService { @@ -158,7 +170,12 @@ impl RoomListService { .map(Arc::new) .map_err(Error::SlidingSync)?; - Ok(Self { sliding_sync, state: Observable::new(State::Init), rooms: Default::default() }) + Ok(Self { + sliding_sync, + state: Observable::new(State::Init), + rooms: Default::default(), + viewport_ranges: Mutex::new(vec![state::VISIBLE_ROOMS_DEFAULT_RANGE]), + }) } /// Start to sync the room list. @@ -258,19 +275,23 @@ impl RoomListService { } /// Pass an [`Input`] onto the state machine. - pub async fn apply_input(&self, input: Input) -> Result<(), Error> { + pub async fn apply_input(&self, input: Input) -> Result { use Input::*; match input { - Viewport(ranges) => { - self.update_viewport(ranges).await?; - } + Viewport(ranges) => self.update_viewport(ranges).await, } - - Ok(()) } - async fn update_viewport(&self, ranges: Ranges) -> Result<(), Error> { + async fn update_viewport(&self, ranges: Ranges) -> Result { + let mut viewport_ranges = self.viewport_ranges.lock().await; + + // Is it worth updating the viewport? + // The viewport has the same ranges. Don't update it. + if *viewport_ranges == ranges { + return Ok(InputResult::Ignored); + } + self.sliding_sync .on_list(VISIBLE_ROOMS_LIST_NAME, |list| { list.set_sync_mode(SlidingSyncMode::new_selective().add_ranges(ranges.clone())); @@ -278,9 +299,11 @@ impl RoomListService { ready(()) }) .await - .ok_or_else(|| Error::InputHasNotBeenApplied(Input::Viewport(ranges)))?; + .ok_or_else(|| Error::InputCannotBeApplied(Input::Viewport(ranges.clone())))?; - Ok(()) + *viewport_ranges = ranges; + + Ok(InputResult::Applied) } /// Get a [`Room`] if it exists. @@ -320,8 +343,8 @@ pub enum Error { UnknownList(String), /// An input was asked to be applied but it wasn't possible to apply it. - #[error("The input has been not applied")] - InputHasNotBeenApplied(Input), + #[error("The input cannot be applied")] + InputCannotBeApplied(Input), /// The requested room doesn't exist. #[error("Room `{0}` not found")] @@ -342,6 +365,18 @@ pub enum Input { Viewport(Ranges), } +/// An [`Input`] Ok result: whether it's been applied, or ignored. +#[derive(Debug, Eq, PartialEq)] +pub enum InputResult { + /// The input has been applied. + Applied, + + /// The input has been ignored. + /// + /// Note that this is not an error. The input was valid, but simply ignored. + Ignored, +} + #[cfg(test)] mod tests { use matrix_sdk::{ diff --git a/crates/matrix-sdk-ui/tests/integration/room_list.rs b/crates/matrix-sdk-ui/tests/integration/room_list.rs index 4da17629a..17bf3e76f 100644 --- a/crates/matrix-sdk-ui/tests/integration/room_list.rs +++ b/crates/matrix-sdk-ui/tests/integration/room_list.rs @@ -7,8 +7,9 @@ use imbl::vector; use matrix_sdk_test::async_test; use matrix_sdk_ui::{ room_list::{ - Error, Input, RoomListEntry, RoomListLoadingState, State, ALL_ROOMS_LIST_NAME as ALL_ROOMS, - INVITES_LIST_NAME as INVITES, VISIBLE_ROOMS_LIST_NAME as VISIBLE_ROOMS, + Error, Input, InputResult, RoomListEntry, RoomListLoadingState, State, + ALL_ROOMS_LIST_NAME as ALL_ROOMS, INVITES_LIST_NAME as INVITES, + VISIBLE_ROOMS_LIST_NAME as VISIBLE_ROOMS, }, timeline::{TimelineItem, VirtualTimelineItem}, RoomListService, @@ -658,7 +659,7 @@ async fn test_sync_resumes_from_error() -> Result<(), Error> { pin_mut!(sync); // Update the viewport, just to be sure it's not reset later. - room_list.apply_input(Input::Viewport(vec![5..=10])).await?; + assert_eq!(room_list.apply_input(Input::Viewport(vec![5..=10])).await?, InputResult::Applied); // Do a regular sync from the `Error` state. sync_then_assert_request_and_fake_response! { @@ -950,7 +951,7 @@ async fn test_sync_resumes_from_terminated() -> Result<(), Error> { pin_mut!(sync); // Update the viewport, just to be sure it's not reset later. - room_list.apply_input(Input::Viewport(vec![5..=10])).await?; + assert_eq!(room_list.apply_input(Input::Viewport(vec![5..=10])).await?, InputResult::Applied); // Do a regular sync from the `Terminated` state. sync_then_assert_request_and_fake_response! { @@ -2141,7 +2142,7 @@ async fn test_input_viewport() -> Result<(), Error> { // present. assert_matches!( room_list.apply_input(Input::Viewport(vec![10..=15])).await, - Err(Error::InputHasNotBeenApplied(_)) + Err(Error::InputCannotBeApplied(_)) ); sync_then_assert_request_and_fake_response! { @@ -2185,7 +2186,17 @@ async fn test_input_viewport() -> Result<(), Error> { }, }; - assert!(room_list.apply_input(Input::Viewport(vec![10..=15, 20..=25])).await.is_ok()); + // Now we can change the viewport.. + assert_eq!( + room_list.apply_input(Input::Viewport(vec![10..=15, 20..=25])).await?, + InputResult::Applied + ); + + // Re-changing the viewport has no effect. + assert_eq!( + room_list.apply_input(Input::Viewport(vec![10..=15, 20..=25])).await?, + InputResult::Ignored + ); // The `timeline_limit` is not repeated because it's sticky. sync_then_assert_request_and_fake_response! {