From 8a1ff1967ff36b22214aa1c67cf1029df3c86425 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Tue, 5 Sep 2023 14:40:08 +0200 Subject: [PATCH 1/2] feat(ui): Implement `RoomListService::sync_indicator`. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch implements a new method: `RoomListService::sync_indicator`. It returns a `impl Stream` where `SyncIndicator` is a new enum with 2 variants: `Show` and `Hide`. `SyncIndicator` is the UI equivalent of a sync spinner/loader/toaster, that the app might want to show to the user to indicate when a _first_ request is sent and might be slow, i.e. the first response is taking a little bit of time to come. The term _first_ may be innapropriate as it covers the actual first sync request, but also the recovering sync request. It means that when a sync error happened, the sync indicator will be shown too, which is a pretty useful information for the user. It's not because a `SyncIndicator` should be shown that it must be send immediately onto the `Stream`. In case of a normal network conditions, without any delay, it can lead to a “blinking” visual effect. Some constants configure how long it takes to consider that a request is “slow”, and that the `SyncIndicator` is necessary to be shown (or hidden). --- .../src/room_list_service/mod.rs | 94 ++++++++- .../tests/integration/room_list_service.rs | 199 +++++++++++++++++- .../tests/integration/sliding_sync.rs | 4 + 3 files changed, 292 insertions(+), 5 deletions(-) diff --git a/crates/matrix-sdk-ui/src/room_list_service/mod.rs b/crates/matrix-sdk-ui/src/room_list_service/mod.rs index 2eaa69125..1887b68e7 100644 --- a/crates/matrix-sdk-ui/src/room_list_service/mod.rs +++ b/crates/matrix-sdk-ui/src/room_list_service/mod.rs @@ -67,7 +67,7 @@ mod room; mod room_list; mod state; -use std::{future::ready, sync::Arc}; +use std::{future::ready, sync::Arc, time::Duration}; use async_stream::stream; use eyeball::{SharedObservable, Subscriber}; @@ -91,7 +91,22 @@ use ruma::{ }; pub use state::*; use thiserror::Error; -use tokio::sync::{Mutex, RwLock}; +use tokio::{ + sync::{Mutex, RwLock}, + time::timeout, +}; + +/// Delay time before actually sending a [`SyncIndicator::Show`]. +/// +/// It's not because a `SyncIndicator` should be shown that it must be done +/// immediately. In case of a normal network conditions, without any delay, it +/// can lead to a “blinking” visual effect. This constant configures how long it +/// takes to consider that a request is “slow”, and that the `SyncIndicator` is +/// necessary to be shown. +pub const SYNC_INDICATOR_DELAY_BEFORE_SHOWING: Duration = Duration::from_millis(200); + +/// Delay time before actually sending a [`SyncIndicator::Hide`]. +pub const SYNC_INDICATOR_DELAY_BEFORE_HIDING: Duration = Duration::from_millis(0); /// The [`RoomListService`] type. See the module's documentation to learn more. #[derive(Debug)] @@ -323,6 +338,59 @@ impl RoomListService { } } + /// Get a [`Stream`] of [`SyncIndicator`]. + /// + /// Read the documentation of [`SyncIndicator`] to learn more about it. + pub fn sync_indicator(&self) -> impl Stream { + let mut state = self.state(); + + stream! { + // Ensure the `SyncIndicator` is always hidden to start with. + yield SyncIndicator::Hide; + + // Let's not wait for an update to happen. The `SyncIndicator` must be + // computed as fast as possible. + let mut current_state = state.next_now(); + + loop { + let (sync_indicator, yield_delay) = match current_state { + State::Init | State::Recovering | State::Error { .. } => { + (SyncIndicator::Show, SYNC_INDICATOR_DELAY_BEFORE_SHOWING) + } + + State::SettingUp | State::Running | State::Terminated { .. } => { + (SyncIndicator::Hide, SYNC_INDICATOR_DELAY_BEFORE_HIDING) + } + }; + + // `state.next().await` has a maximum of `yield_delay` time to execute… + let next_state = match timeout(yield_delay, state.next()).await { + // A new state has been received before `yield_delay` time. The new + // `sync_indicator` value won't be yielded. + Ok(next_state) => next_state, + + // No new state has been received before `yield_delay` time. The + // `sync_indicator` value can be yielded. + Err(_) => { + yield sync_indicator; + + // Now that `sync_indicator` has been yielded, let's wait on + // the next state again. + state.next().await + } + }; + + if let Some(next_state) = next_state { + // Update the `current_state`. + current_state = next_state; + } else { + // Something is broken with `self.state`. Let's stop this stream too. + break; + } + } + } + } + /// Get the [`Client`] that has been used to create [`Self`]. pub fn client(&self) -> &Client { &self.client @@ -474,6 +542,28 @@ pub enum InputResult { Ignored, } +/// An hint whether a _sync spinner/loader/toaster_ should be prompted to the +/// user, indicating that the [`RoomListService`] is syncing. +/// +/// This is entirely arbitrary and optinionated. Of course, once +/// [`RoomListService::sync`] has been called, it's going to be constantly +/// syncing, until [`RoomListService::stop_sync`] is called, or until an error +/// happened. But in some cases, it's better for the user experience to prompt +/// to the user that a sync is happening. It's usually the first sync, or the +/// recovering sync. However, the sync indicator must be prompted if the +/// aforementioned sync is “slow”, otherwise the indicator is likely to “blink” +/// pretty fast, which can be very confusing. It's also common to indicate to +/// the user that a syncing is happening in case of a network error, that +/// something is catching up etc. +#[derive(Debug, Eq, PartialEq)] +pub enum SyncIndicator { + /// Show the sync indicator. + Show, + + /// Hide the sync indicator. + Hide, +} + #[cfg(test)] mod tests { use std::future::ready; diff --git a/crates/matrix-sdk-ui/tests/integration/room_list_service.rs b/crates/matrix-sdk-ui/tests/integration/room_list_service.rs index 592090a2e..d95f27e01 100644 --- a/crates/matrix-sdk-ui/tests/integration/room_list_service.rs +++ b/crates/matrix-sdk-ui/tests/integration/room_list_service.rs @@ -1,4 +1,7 @@ -use std::ops::Not; +use std::{ + ops::Not, + time::{Duration, Instant}, +}; use assert_matches::assert_matches; use eyeball_im::VectorDiff; @@ -9,8 +12,9 @@ use matrix_sdk_test::async_test; use matrix_sdk_ui::{ room_list_service::{ filters::{new_filter_all, new_filter_fuzzy_match_room_name}, - Error, Input, InputResult, RoomListEntry, RoomListLoadingState, State, + Error, Input, InputResult, RoomListEntry, RoomListLoadingState, State, SyncIndicator, ALL_ROOMS_LIST_NAME as ALL_ROOMS, INVITES_LIST_NAME as INVITES, + SYNC_INDICATOR_DELAY_BEFORE_HIDING, SYNC_INDICATOR_DELAY_BEFORE_SHOWING, VISIBLE_ROOMS_LIST_NAME as VISIBLE_ROOMS, }, timeline::{TimelineItemKind, VirtualTimelineItem}, @@ -24,7 +28,7 @@ use ruma::{ }; use serde_json::json; use stream_assert::{assert_next_matches, assert_pending}; -use tokio::task::yield_now; +use tokio::{spawn, sync::mpsc::channel, task::yield_now}; use wiremock::MockServer; use crate::{ @@ -47,6 +51,7 @@ macro_rules! sync_then_assert_request_and_fake_response { $( states = $pre_state:pat => $post_state:pat, )? assert request $assert_request:tt { $( $request_json:tt )* }, respond with = $( ( code $code:expr ) )? { $( $response_json:tt )* } + $( , after delay = $response_delay:expr )? $(,)? ) => { sync_then_assert_request_and_fake_response! { @@ -55,6 +60,7 @@ macro_rules! sync_then_assert_request_and_fake_response { $( states = $pre_state => $post_state, )? assert request $assert_request { $( $request_json )* }, respond with = $( ( code $code ) )? { $( $response_json )* }, + $( after delay = $response_delay, )? } }; @@ -64,6 +70,7 @@ macro_rules! sync_then_assert_request_and_fake_response { $( states = $pre_state:pat => $post_state:pat, )? assert request $assert_request:tt { $( $request_json:tt )* }, respond with = $( ( code $code:expr ) )? { $( $response_json:tt )* } + $( , after delay = $response_delay:expr )? $(,)? ) => { { @@ -80,6 +87,7 @@ macro_rules! sync_then_assert_request_and_fake_response { sync matches $sync_result, assert request $assert_request { $( $request_json )* }, respond with = $( ( code $code ) )? { $( $response_json )* }, + $( after delay = $response_delay, )? }; $( assert_matches!(state.next().now_or_never(), Some(Some($post_state)), "post state"); )? @@ -2539,3 +2547,188 @@ async fn test_input_viewport() -> Result<(), Error> { Ok(()) } + +#[async_test] +async fn test_sync_indicator() -> Result<(), Error> { + let (_, server, room_list) = new_room_list_service().await?; + + let sync = room_list.sync(); + pin_mut!(sync); + + let sync_indicator = room_list.sync_indicator(); + + let request_margin = Duration::from_millis(20); + let request_1_delay = SYNC_INDICATOR_DELAY_BEFORE_SHOWING * 2; + let request_2_delay = SYNC_INDICATOR_DELAY_BEFORE_SHOWING * 3; + let request_4_delay = SYNC_INDICATOR_DELAY_BEFORE_SHOWING * 2; + let request_5_delay = SYNC_INDICATOR_DELAY_BEFORE_SHOWING * 2; + + let (in_between_requests_synchronizer_sender, mut in_between_requests_synchronizer) = + channel(1); + + macro_rules! assert_next_sync_indicator { + ($sync_indicator:ident, $pattern:pat, now $(,)?) => { + assert_matches!($sync_indicator.next().now_or_never(), Some(Some($pattern))); + }; + + ($sync_indicator:ident, $pattern:pat, under $time:expr $(,)?) => { + let now = Instant::now(); + assert_matches!($sync_indicator.next().await, Some($pattern)); + assert!(now.elapsed() < $time); + }; + } + + let sync_indicator_task = spawn(async move { + pin_mut!(sync_indicator); + + // `SyncIndicator` is forced to be hidden to begin with. + assert_next_sync_indicator!(sync_indicator, SyncIndicator::Hide, now); + + // Request 1. + { + // Sync has started, the `SyncIndicator` must be shown… but not immediately! + assert_next_sync_indicator!( + sync_indicator, + SyncIndicator::Show, + under SYNC_INDICATOR_DELAY_BEFORE_SHOWING + request_margin, + ); + + // Then, once the sync is done, the `SyncIndicator` must be hidden. + assert_next_sync_indicator!( + sync_indicator, + SyncIndicator::Hide, + under request_1_delay - SYNC_INDICATOR_DELAY_BEFORE_SHOWING + + SYNC_INDICATOR_DELAY_BEFORE_HIDING + + request_margin, + ); + } + + in_between_requests_synchronizer.recv().await.unwrap(); + assert_pending!(sync_indicator); + + // Request 2. + { + // Nothing happens, as the state transitions from `SettingUp` to + // `Running`, no `SyncIndicator` must be shown. + } + + in_between_requests_synchronizer.recv().await.unwrap(); + assert_pending!(sync_indicator); + + // Request 3. + { + // Sync has errored, the `SyncIndicator` should be show. Fortunately + // for us (fictional situation), the sync is restarted + // immediately, and `SyncIndicator` doesn't have time to + // be shown for this particular state update. + } + + in_between_requests_synchronizer.recv().await.unwrap(); + assert_pending!(sync_indicator); + + // Request 4. + { + // The system is recovering, It takes times (fictional situation)! + // `SyncIndicator` has time to show (off). + assert_next_sync_indicator!( + sync_indicator, + SyncIndicator::Show, + under SYNC_INDICATOR_DELAY_BEFORE_SHOWING + request_margin, + ); + } + + in_between_requests_synchronizer.recv().await.unwrap(); + assert_pending!(sync_indicator); + + // Request 5. + { + // It takes time for the system to recover… + assert_next_sync_indicator!( + sync_indicator, + SyncIndicator::Show, + under SYNC_INDICATOR_DELAY_BEFORE_SHOWING + request_margin, + ); + + // But finally, the system has recovered and is running. Time to hide the + // `SyncIndicator`. + assert_next_sync_indicator!( + sync_indicator, + SyncIndicator::Hide, + under request_5_delay - SYNC_INDICATOR_DELAY_BEFORE_SHOWING + + SYNC_INDICATOR_DELAY_BEFORE_HIDING + + request_margin, + ); + } + }); + + // Request 1. + sync_then_assert_request_and_fake_response! { + [server, room_list, sync] + states = Init => SettingUp, + assert request >= {}, + respond with = { + "pos": "0", + }, + after delay = request_1_delay, // Slow request! + }; + + in_between_requests_synchronizer_sender.send(()).await.unwrap(); + + // Request 2. + sync_then_assert_request_and_fake_response! { + [server, room_list, sync] + states = SettingUp => Running, + assert request >= {}, + respond with = { + "pos": "1", + }, + after delay = request_2_delay, // Slow request! + }; + + in_between_requests_synchronizer_sender.send(()).await.unwrap(); + + // Request 3. + sync_then_assert_request_and_fake_response! { + [server, room_list, sync] + sync matches Some(Err(_)), + states = Running => Error { .. }, + assert request >= {}, + respond with = (code 400) { + "error": "foo", + "errcode": "M_UNKNOWN", + }, + }; + + let sync = room_list.sync(); + pin_mut!(sync); + + in_between_requests_synchronizer_sender.send(()).await.unwrap(); + + // Request 4. + sync_then_assert_request_and_fake_response! { + [server, room_list, sync] + states = Error { .. } => Recovering, + assert request >= {}, + respond with = { + "pos": "2", + }, + after delay = request_4_delay, // Slow request! + }; + + in_between_requests_synchronizer_sender.send(()).await.unwrap(); + + // Request 5. + sync_then_assert_request_and_fake_response! { + [server, room_list, sync] + states = Recovering => Running, + assert request >= {}, + respond with = { + "pos": "3", + }, + after delay = request_5_delay, // Slow request! + }; + + sync_indicator_task.await.unwrap(); + + Ok(()) +} diff --git a/crates/matrix-sdk-ui/tests/integration/sliding_sync.rs b/crates/matrix-sdk-ui/tests/integration/sliding_sync.rs index 072f765fe..dac43c03c 100644 --- a/crates/matrix-sdk-ui/tests/integration/sliding_sync.rs +++ b/crates/matrix-sdk-ui/tests/integration/sliding_sync.rs @@ -60,6 +60,7 @@ macro_rules! sliding_sync_then_assert_request_and_fake_response { [$server:ident, $stream:ident] assert request $sign:tt { $( $request_json:tt )* }, respond with = $( ( code $code:expr ) )? { $( $response_json:tt )* } + $( , after delay = $response_delay:expr )? $(,)? ) => { sliding_sync_then_assert_request_and_fake_response! { @@ -67,6 +68,7 @@ macro_rules! sliding_sync_then_assert_request_and_fake_response { sync matches Some(Ok(_)), assert request $sign { $( $request_json )* }, respond with = $( ( code $code ) )? { $( $response_json )* }, + $( after delay = $response_delay, )? } }; @@ -75,6 +77,7 @@ macro_rules! sliding_sync_then_assert_request_and_fake_response { sync matches $sync_result:pat, assert request $sign:tt { $( $request_json:tt )* }, respond with = $( ( code $code:expr ) )? { $( $response_json:tt )* } + $( , after delay = $response_delay:expr )? $(,)? ) => { { @@ -96,6 +99,7 @@ macro_rules! sliding_sync_then_assert_request_and_fake_response { $( $response_json )* }) ) + $( .set_delay($response_delay) )? }) .mount_as_scoped(&$server) .await; From b8ed566d78c2b89f3a11397b0f50ac2146aa8f73 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 6 Sep 2023 14:10:35 +0200 Subject: [PATCH 2/2] feat(ffi): Add bindings for `RoomListService::sync_indicator`. This patch adds the `RoomListService::sync_indicator` method, along with the `RoomListServiceSyncIndicatorListener` callback interface, and `RoomListServiceSyncIndicator` enum. --- bindings/matrix-sdk-ffi/src/room_list.rs | 57 +++++++++++++++++++----- 1 file changed, 47 insertions(+), 10 deletions(-) diff --git a/bindings/matrix-sdk-ffi/src/room_list.rs b/bindings/matrix-sdk-ffi/src/room_list.rs index 143bf8225..504800686 100644 --- a/bindings/matrix-sdk-ffi/src/room_list.rs +++ b/bindings/matrix-sdk-ffi/src/room_list.rs @@ -115,6 +115,21 @@ impl RoomListService { async fn apply_input(&self, input: RoomListInput) -> Result<(), RoomListError> { self.inner.apply_input(input.into()).await.map(|_| ()).map_err(Into::into) } + + fn sync_indicator( + &self, + listener: Box, + ) -> Arc { + let sync_indicator_stream = self.inner.sync_indicator(); + + Arc::new(TaskHandle::new(RUNTIME.spawn(async move { + pin_mut!(sync_indicator_stream); + + while let Some(sync_indicator) = sync_indicator_stream.next().await { + listener.on_update(sync_indicator.into()); + } + }))) + } } #[derive(uniffi::Object)] @@ -216,15 +231,32 @@ pub enum RoomListServiceState { impl From for RoomListServiceState { fn from(value: matrix_sdk_ui::room_list_service::State) -> Self { - use matrix_sdk_ui::room_list_service::State::*; + use matrix_sdk_ui::room_list_service::State as S; match value { - Init => Self::Initial, - SettingUp => Self::SettingUp, - Recovering => Self::Recovering, - Running => Self::Running, - Error { .. } => Self::Error, - Terminated { .. } => Self::Terminated, + S::Init => Self::Initial, + S::SettingUp => Self::SettingUp, + S::Recovering => Self::Recovering, + S::Running => Self::Running, + S::Error { .. } => Self::Error, + S::Terminated { .. } => Self::Terminated, + } + } +} + +#[derive(uniffi::Enum)] +pub enum RoomListServiceSyncIndicator { + Show, + Hide, +} + +impl From for RoomListServiceSyncIndicator { + fn from(value: matrix_sdk_ui::room_list_service::SyncIndicator) -> Self { + use matrix_sdk_ui::room_list_service::SyncIndicator as SI; + + match value { + SI::Show => Self::Show, + SI::Hide => Self::Hide, } } } @@ -237,11 +269,11 @@ pub enum RoomListLoadingState { impl From for RoomListLoadingState { fn from(value: matrix_sdk_ui::room_list_service::RoomListLoadingState) -> Self { - use matrix_sdk_ui::room_list_service::RoomListLoadingState::*; + use matrix_sdk_ui::room_list_service::RoomListLoadingState as LS; match value { - NotLoaded => Self::NotLoaded, - Loaded { maximum_number_of_rooms } => Self::Loaded { maximum_number_of_rooms }, + LS::NotLoaded => Self::NotLoaded, + LS::Loaded { maximum_number_of_rooms } => Self::Loaded { maximum_number_of_rooms }, } } } @@ -256,6 +288,11 @@ pub trait RoomListLoadingStateListener: Send + Sync + Debug { fn on_update(&self, state: RoomListLoadingState); } +#[uniffi::export(callback_interface)] +pub trait RoomListServiceSyncIndicatorListener: Send + Sync + Debug { + fn on_update(&self, sync_indicator: RoomListServiceSyncIndicator); +} + #[derive(uniffi::Enum)] pub enum RoomListEntriesUpdate { Append { values: Vec },