Skip to content

Commit

Permalink
fix(ui): Emit a RoomInfoNotableUpdateReasons::RECENCY_TIMESTAMP ony…
Browse files Browse the repository at this point in the history 
… for non-new rooms.

This patch avoids to emit a
`RoomInfoNotableUpdateReasons::RECENCY_TIMESTAMP` for rooms that are new.
Otherwise the entries in `matrix_sdk_ui::room_list_service::RoomList`
receive a `VectorDiff` because a new room is inserted, and then a
`VectorDiff` because the recency timestamp is updated. The second
`VectorDiff` is useless in this case.
  • Loading branch information
@Hywan
Hywan committed Jul 3, 2024
1 parent 6709a6b commit ebf3522
Show file tree
Hide file tree
Showing 4 changed files with 72 additions and 57 deletions.
22 changes: 15 additions & 7 deletions crates/matrix-sdk-base/src/sliding_sync.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@
// See the License for the specific language governing permissions and
// limitations under the License.

use std::collections::BTreeMap;
#[cfg(feature = "e2e-encryption")]
use std::ops::Deref;
use std::{collections::BTreeMap, ops::Not};

#[cfg(feature = "e2e-encryption")]
use matrix_sdk_common::deserialized_responses::SyncTimelineEvent;
Expand Down Expand Up @@ -341,6 +341,8 @@ impl BaseClient {
};

// Find or create the room in the store
let is_a_new_room = store.room_exists(room_id).not();

#[allow(unused_mut)] // Required for some feature flag combinations
let (mut room, mut room_info, invited_room) =
self.process_sliding_sync_room_membership(room_data, &state_events, store, room_id);
Expand Down Expand Up @@ -374,7 +376,7 @@ impl BaseClient {
.await?;
}

process_room_properties(room_id, room_data, &mut room_info, changes);
process_room_properties(room_id, room_data, &mut room_info, is_a_new_room, changes);

let timeline = self
.handle_timeline(
Expand Down Expand Up @@ -693,6 +695,7 @@ fn process_room_properties(
room_id: &RoomId,
room_data: &v4::SlidingSyncRoom,
room_info: &mut RoomInfo,
is_a_new_room: bool,
changes: &mut StateChanges,
) {
// Handle the room's avatar.
Expand Down Expand Up @@ -741,11 +744,16 @@ fn process_room_properties(
if let Some(recency_timestamp) = &room_data.timestamp {
room_info.update_recency_timestamp(*recency_timestamp);

changes
.room_info_notable_updates
.entry(room_id.to_owned())
.or_default()
.insert(RoomInfoNotableUpdateReasons::RECENCY_TIMESTAMP);
// If it's not a new room, let's emit a `RECENCY_TIMESTAMP` update.
// For a new room, the room will appear as new, so we don't care about this
// update.
if is_a_new_room.not() {
changes
.room_info_notable_updates
.entry(room_id.to_owned())
.or_default()
.insert(RoomInfoNotableUpdateReasons::RECENCY_TIMESTAMP);
}
}
}

Expand Down
5 changes: 5 additions & 0 deletions crates/matrix-sdk-base/src/store/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,11 @@ impl Store {
self.rooms.read().unwrap().get(room_id).cloned()
}

/// Check if a room exists.
pub(crate) fn room_exists(&self, room_id: &RoomId) -> bool {
self.rooms.read().unwrap().get(room_id).is_some()
}

/// Lookup the `Room` for the given `RoomId`, or create one, if it didn't
/// exist yet in the store
pub fn get_or_create_room(
Expand Down
63 changes: 40 additions & 23 deletions crates/matrix-sdk-ui/tests/integration/room_list_service.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1413,24 +1413,13 @@ async fn test_dynamic_entries_stream() -> Result<(), Error> {
};

// Assert the dynamic entries.
// It's pushed on the front because rooms are sorted by recency (based on their
// `origin_server_ts` of their latest event candidate).
// It's pushed on the front because rooms are sorted by recency.
assert_entries_batch! {
[dynamic_entries_stream]
push front [ "!r1:bar.org" ];
push front [ "!r4:bar.org" ];
end;
};
assert_entries_batch! {
[dynamic_entries_stream]
set [ 1 ] [ "!r1:bar.org" ];
end;
};
assert_entries_batch! {
[dynamic_entries_stream]
set [ 0 ] [ "!r4:bar.org" ];
end;
};
assert_pending!(dynamic_entries_stream);

sync_then_assert_request_and_fake_response! {
Expand Down Expand Up @@ -1510,16 +1499,6 @@ async fn test_dynamic_entries_stream() -> Result<(), Error> {
push front [ "!r7:bar.org" ];
end;
};
assert_entries_batch! {
[dynamic_entries_stream]
set [ 1 ] [ "!r5:bar.org" ];
end;
};
assert_entries_batch! {
[dynamic_entries_stream]
set [ 0 ] [ "!r7:bar.org" ];
end;
};
assert_pending!(dynamic_entries_stream);

// Now, let's change the dynamic entries!
Expand Down Expand Up @@ -1913,12 +1892,50 @@ async fn test_room_sorting() -> Result<(), Error> {
// | 4 | !r1 | 6 | Aaa |
// | 5 | !r4 | 5 | |

assert_pending!(stream);

sync_then_assert_request_and_fake_response! {
[server, room_list, sync]
states = Running => Running,
assert request >= {
"lists": {
ALL_ROOMS: {
"ranges": [[0, 5]],
},
},
},
respond with = {
"pos": "3",
"lists": {
ALL_ROOMS: {
"count": 6,
},
},
"rooms": {
"!r3:bar.org": {
"timestamp": 11,
},
},
},
};

assert_entries_batch! {
[stream]
set [ 2 ] [ "!r6:bar.org" ];
set [ 0 ] [ "!r3:bar.org" ];
end;
};

// Now we have:
//
// | index | room ID | recency | name |
// |-------|---------|---------|------|
// | 0 | !r3 | 11 | |
// | 1 | !r2 | 9 | |
// | 2 | !r6 | 8 | |
// | 3 | !r0 | 7 | Bbb |
// | 4 | !r1 | 6 | Aaa |
// | 5 | !r4 | 5 | |

assert_pending!(stream);

Ok(())
Expand Down
39 changes: 12 additions & 27 deletions testing/matrix-sdk-integration-testing/src/tests/sliding_sync/room.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -801,8 +801,8 @@ async fn test_delayed_decryption_latest_event() -> Result<()> {
assert_eq!(bob_room.state(), RoomState::Joined);

let alice_all_rooms = alice_sync_service.room_list_service().all_rooms().await.unwrap();
let (stream, entries) =
alice_all_rooms.entries_with_dynamic_adapters(10, alice.room_info_notable_update_receiver());
let (stream, entries) = alice_all_rooms
.entries_with_dynamic_adapters(10, alice.room_info_notable_update_receiver());
entries.set_filter(Box::new(new_filter_all(vec![])));
pin_mut!(stream);

Expand Down Expand Up @@ -848,18 +848,6 @@ async fn test_delayed_decryption_latest_event() -> Result<()> {
}
);

// Sometimes Synapse sends the same message twice. Let's consume useless `Set`…
// if they arrived before 3s.
if let Ok(Some(diffs)) = timeout(Duration::from_secs(3), stream.next()).await {
assert_eq!(diffs.len(), 1);
assert_matches!(
&diffs[0],
VectorDiff::Set { index: 0, value: room } => {
assert_eq!(room.room_id(), alice_room.room_id());
}
);
}

assert_pending!(stream);

Ok(())
Expand Down Expand Up @@ -927,8 +915,8 @@ async fn test_room_info_notable_update_deduplication() -> Result<()> {
alice_room.enable_encryption().await.unwrap();

let alice_all_rooms = alice_sync_service.room_list_service().all_rooms().await.unwrap();
let (stream, entries) =
alice_all_rooms.entries_with_dynamic_adapters(10, alice.room_info_notable_update_receiver());
let (stream, entries) = alice_all_rooms
.entries_with_dynamic_adapters(10, alice.room_info_notable_update_receiver());
entries.set_filter(Box::new(new_filter_all(vec![])));

pin_mut!(stream);
Expand Down Expand Up @@ -999,17 +987,14 @@ async fn test_room_info_notable_update_deduplication() -> Result<()> {
}
);

// Sometimes Synapse sends the same message twice. Let's consume useless `Set`…
// if they arrived before 3s.
if let Ok(Some(diffs)) = timeout(Duration::from_secs(3), stream.next()).await {
assert_eq!(diffs.len(), 1);
assert_matches!(
&diffs[0],
VectorDiff::Set { index: 0, value: room } => {
assert_eq!(room.room_id(), alice_room.room_id());
}
);
}
assert_let!(Some(diffs) = stream.next().await);
assert_eq!(diffs.len(), 1);
assert_matches!(
&diffs[0],
VectorDiff::Set { index: 0, value: room } => {
assert_eq!(room.room_id(), alice_room.room_id());
}
);

assert_pending!(stream);

Expand Down

0 comments on commit ebf3522

Please sign in to comment.