Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

base: cache the computed display name and reuse that in the search filters #3516

Merged
merged 14 commits into from
Jun 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions benchmarks/benches/room_bench.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ pub fn receive_all_members_benchmark(c: &mut Criterion) {
.expect("initial filling of sqlite failed");

let base_client = BaseClient::with_store_config(StoreConfig::new().state_store(sqlite_store));

runtime
.block_on(base_client.set_session_meta(
SessionMeta {
Expand All @@ -72,6 +73,7 @@ pub fn receive_all_members_benchmark(c: &mut Criterion) {
None,
))
.expect("Could not set session meta");

base_client.get_or_create_room(&room_id, RoomState::Joined);

let request = get_member_events::v3::Request::new(room_id.clone());
Expand Down
4 changes: 2 additions & 2 deletions bindings/matrix-sdk-ffi/src/room.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,8 @@ impl Room {
/// Returns the room's name from the state event if available, otherwise
/// compute a room name based on the room's nature (DM or not) and number of
/// members.
pub fn display_name(&self) -> Result<String, ClientError> {
Ok(RUNTIME.block_on(self.inner.computed_display_name())?.to_string())
pub fn display_name(&self) -> Option<String> {
Some(self.inner.cached_display_name()?.to_string())
}

/// The raw name as present in the room state event.
Expand Down
2 changes: 1 addition & 1 deletion bindings/matrix-sdk-ffi/src/room_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ impl RoomInfo {

Ok(Self {
id: room.room_id().to_string(),
display_name: room.computed_display_name().await.ok().map(|name| name.to_string()),
display_name: room.cached_display_name().map(|name| name.to_string()),
raw_name: room.name(),
topic: room.topic(),
avatar_url: room.avatar_url().map(Into::into),
Expand Down
2 changes: 1 addition & 1 deletion bindings/matrix-sdk-ffi/src/room_list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -489,7 +489,7 @@ impl RoomListItem {
/// compute a room name based on the room's nature (DM or not) and number of
/// members.
fn display_name(&self) -> Option<String> {
RUNTIME.block_on(self.inner.computed_display_name())
self.inner.cached_display_name()
}

fn avatar_url(&self) -> Option<String> {
Expand Down
14 changes: 13 additions & 1 deletion crates/matrix-sdk-base/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -751,6 +751,7 @@ impl BaseClient {
RoomState::Joined,
self.roominfo_update_sender.clone(),
);

if room.state() != RoomState::Joined {
let _sync_lock = self.sync_lock().lock().await;

Expand All @@ -777,6 +778,7 @@ impl BaseClient {
RoomState::Left,
self.roominfo_update_sender.clone(),
);

if room.state() != RoomState::Left {
let _sync_lock = self.sync_lock().lock().await;

Expand Down Expand Up @@ -852,6 +854,7 @@ impl BaseClient {
RoomState::Joined,
self.roominfo_update_sender.clone(),
);

let mut room_info = room.clone_info();

room_info.mark_as_joined();
Expand Down Expand Up @@ -964,6 +967,7 @@ impl BaseClient {
RoomState::Left,
self.roominfo_update_sender.clone(),
);

let mut room_info = room.clone_info();
room_info.mark_as_left();
room_info.mark_state_partially_synced();
Expand Down Expand Up @@ -1022,6 +1026,7 @@ impl BaseClient {
RoomState::Invited,
self.roominfo_update_sender.clone(),
);

let mut room_info = room.clone_info();
room_info.mark_as_invited();
room_info.mark_state_fully_synced();
Expand Down Expand Up @@ -1066,6 +1071,13 @@ impl BaseClient {
self.apply_changes(&changes, false);
}

// Now that all the rooms information have been saved, update the display name
// cache (which relies on information stored in the database). This will
// live in memory, until the next sync which will saves the room info to
// disk; we do this to avoid saving that would be redundant with the
// above. Oh well.
new_rooms.update_in_memory_caches(&self.store).await;

info!("Processed a sync response in {:?}", now.elapsed());

let response = SyncResponse {
Expand Down Expand Up @@ -1655,7 +1667,7 @@ mod tests {
let room = client.get_room(room_id).expect("Room not found");
assert_eq!(room.state(), RoomState::Invited);
assert_eq!(
room.computed_display_name().await.expect("fetching display name failed"),
room.compute_display_name().await.expect("fetching display name failed"),
DisplayName::Calculated("Kyra".to_owned())
);
}
Expand Down
86 changes: 1 addition & 85 deletions crates/matrix-sdk-base/src/rooms/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,15 +122,6 @@ impl BaseRoomInfo {
Self::default()
}

pub(crate) fn calculate_room_name(
&self,
joined_member_count: u64,
invited_member_count: u64,
heroes: Vec<&str>,
) -> DisplayName {
calculate_room_name(joined_member_count, invited_member_count, heroes)
}

/// Get the room version of this room.
///
/// For room versions earlier than room version 11, if the event is
Expand Down Expand Up @@ -359,45 +350,6 @@ impl Default for BaseRoomInfo {
}
}

/// Calculate room name according to step 3 of the [naming algorithm.]
///
/// [naming algorithm]: https://spec.matrix.org/latest/client-server-api/#calculating-the-display-name-for-a-room
fn calculate_room_name(
joined_member_count: u64,
invited_member_count: u64,
mut heroes: Vec<&str>,
) -> DisplayName {
let num_heroes = heroes.len() as u64;
let invited_joined = invited_member_count + joined_member_count;
let invited_joined_minus_one = invited_joined.saturating_sub(1);

// Stabilize ordering.
heroes.sort_unstable();

let names = if num_heroes == 0 && invited_joined > 1 {
format!("{} people", invited_joined)
} else if num_heroes >= invited_joined_minus_one {
heroes.join(", ")
} else if num_heroes < invited_joined_minus_one && invited_joined > 1 {
// TODO: What length does the spec want us to use here and in
// the `else`?
format!("{}, and {} others", heroes.join(", "), (invited_joined - num_heroes))
} else {
"".to_owned()
};

// User is alone.
if invited_joined <= 1 {
if names.is_empty() {
DisplayName::Empty
} else {
DisplayName::EmptyWas(names)
}
} else {
DisplayName::Calculated(names)
}
}

/// The content of an `m.room.create` event, with a required `creator` field.
///
/// Starting with room version 11, the `creator` field should be removed and the
Expand Down Expand Up @@ -563,43 +515,7 @@ mod tests {

use ruma::events::tag::{TagInfo, TagName, Tags};

use super::{calculate_room_name, BaseRoomInfo, DisplayName, RoomNotableTags};

#[test]
fn test_calculate_room_name() {
let mut actual = calculate_room_name(2, 0, vec!["a"]);
assert_eq!(DisplayName::Calculated("a".to_owned()), actual);

actual = calculate_room_name(3, 0, vec!["a", "b"]);
assert_eq!(DisplayName::Calculated("a, b".to_owned()), actual);

actual = calculate_room_name(4, 0, vec!["a", "b", "c"]);
assert_eq!(DisplayName::Calculated("a, b, c".to_owned()), actual);

actual = calculate_room_name(5, 0, vec!["a", "b", "c"]);
assert_eq!(DisplayName::Calculated("a, b, c, and 2 others".to_owned()), actual);

actual = calculate_room_name(5, 0, vec![]);
assert_eq!(DisplayName::Calculated("5 people".to_owned()), actual);

actual = calculate_room_name(0, 0, vec![]);
assert_eq!(DisplayName::Empty, actual);

actual = calculate_room_name(1, 0, vec![]);
assert_eq!(DisplayName::Empty, actual);

actual = calculate_room_name(0, 1, vec![]);
assert_eq!(DisplayName::Empty, actual);

actual = calculate_room_name(1, 0, vec!["a"]);
assert_eq!(DisplayName::EmptyWas("a".to_owned()), actual);

actual = calculate_room_name(1, 0, vec!["a", "b"]);
assert_eq!(DisplayName::EmptyWas("a, b".to_owned()), actual);

actual = calculate_room_name(1, 0, vec!["a", "b", "c"]);
assert_eq!(DisplayName::EmptyWas("a, b, c".to_owned()), actual);
}
use super::{BaseRoomInfo, RoomNotableTags};

#[test]
fn test_handle_notable_tags_favourite() {
Expand Down
Loading
Loading