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.

Sign up for GitHub

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

Jump to bottom

Refactor and clean up our ambiguity handling #4218

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+145 −124
Open

Refactor and clean up our ambiguity handling #4218

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
13d236a
refactor(base)!: Rename DisplayName to RoomDisplayName
poljar Oct 29, 2024
4c73e55
refactor(base): Rename AmbiguityMap to DisplayNameUsers
poljar Nov 4, 2024
7158573
chore(base): Clean up the display name ambiguity calculation logic
poljar Nov 4, 2024
4c376a1
chore(base): Improve the docs for the DisplayNameUsers struct
poljar Nov 5, 2024
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
4 changes: 2 additions & 2 deletions crates/matrix-sdk-base/src/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1739,7 +1739,7 @@ mod tests {

use super::BaseClient;
use crate::{
store::StateStoreExt, test_utils::logged_in_base_client, DisplayName, RoomState,
store::StateStoreExt, test_utils::logged_in_base_client, RoomDisplayName, RoomState,
SessionMeta,
};

Expand Down Expand Up @@ -1871,7 +1871,7 @@ mod tests {
assert_eq!(room.state(), RoomState::Invited);
assert_eq!(
room.compute_display_name().await.expect("fetching display name failed"),
DisplayName::Calculated("Kyra".to_owned())
RoomDisplayName::Calculated("Kyra".to_owned())
);
}

Expand Down
2 changes: 1 addition & 1 deletion crates/matrix-sdk-base/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ pub use http;
pub use matrix_sdk_crypto as crypto;
pub use once_cell;
pub use rooms::{
DisplayName, Room, RoomCreateWithCreatorEventContent, RoomHero, RoomInfo,
Room, RoomCreateWithCreatorEventContent, RoomDisplayName, RoomHero, RoomInfo,
RoomInfoNotableUpdate, RoomInfoNotableUpdateReasons, RoomMember, RoomMemberships, RoomState,
RoomStateFilter,
};
Expand Down
12 changes: 7 additions & 5 deletions crates/matrix-sdk-base/src/rooms/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ use crate::MinimalStateEvent;
/// The name of the room, either from the metadata or calculated
/// according to [matrix specification](https://matrix.org/docs/spec/client_server/latest#calculating-the-display-name-for-a-room)
#[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)]
pub enum DisplayName {
pub enum RoomDisplayName {
/// The room has been named explicitly as
Named(String),
/// The room has a canonical alias that should be used
Expand All @@ -64,14 +64,16 @@ pub enum DisplayName {
Empty,
}

impl fmt::Display for DisplayName {
impl fmt::Display for RoomDisplayName {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
DisplayName::Named(s) | DisplayName::Calculated(s) | DisplayName::Aliased(s) => {
RoomDisplayName::Named(s)
| RoomDisplayName::Calculated(s)
| RoomDisplayName::Aliased(s) => {
write!(f, "{s}")
}
DisplayName::EmptyWas(s) => write!(f, "Empty Room (was {s})"),
DisplayName::Empty => write!(f, "Empty Room"),
RoomDisplayName::EmptyWas(s) => write!(f, "Empty Room (was {s})"),
RoomDisplayName::Empty => write!(f, "Empty Room"),
}
}
}
Expand Down
81 changes: 42 additions & 39 deletions crates/matrix-sdk-base/src/rooms/normal.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ use tokio::sync::broadcast;
use tracing::{debug, field::debug, info, instrument, warn};

use super::{
members::MemberRoomInfo, BaseRoomInfo, DisplayName, RoomCreateWithCreatorEventContent,
members::MemberRoomInfo, BaseRoomInfo, RoomCreateWithCreatorEventContent, RoomDisplayName,
RoomMember, RoomNotableTags,
};
#[cfg(feature = "experimental-sliding-sync")]
Expand Down Expand Up @@ -572,8 +572,8 @@ impl Room {
/// [`Self::cached_display_name`].
///
/// [spec]: <https://matrix.org/docs/spec/client_server/latest#calculating-the-display-name-for-a-room>
pub async fn compute_display_name(&self) -> StoreResult<DisplayName> {
let update_cache = |new_val: DisplayName| {
pub async fn compute_display_name(&self) -> StoreResult<RoomDisplayName> {
let update_cache = |new_val: RoomDisplayName| {
self.inner.update_if(|info| {
if info.cached_display_name.as_ref() != Some(&new_val) {
info.cached_display_name = Some(new_val.clone());
Expand All @@ -591,13 +591,13 @@ impl Room {
if let Some(name) = inner.name() {
let name = name.trim().to_owned();
drop(inner); // drop the lock on `self.inner` to avoid deadlocking in `update_cache`.
return Ok(update_cache(DisplayName::Named(name)));
return Ok(update_cache(RoomDisplayName::Named(name)));
}

if let Some(alias) = inner.canonical_alias() {
let alias = alias.alias().trim().to_owned();
drop(inner); // See above comment.
return Ok(update_cache(DisplayName::Aliased(alias)));
return Ok(update_cache(RoomDisplayName::Aliased(alias)));
}

inner.summary.clone()
Expand Down Expand Up @@ -703,7 +703,7 @@ impl Room {
///
/// This cache is refilled every time we call
/// [`Self::compute_display_name`].
pub fn cached_display_name(&self) -> Option<DisplayName> {
pub fn cached_display_name(&self) -> Option<RoomDisplayName> {
self.inner.read().cached_display_name.clone()
}

Expand Down Expand Up @@ -1103,7 +1103,7 @@ pub struct RoomInfo {
/// Filled by calling [`Room::compute_display_name`]. It's automatically
/// filled at start when creating a room, or on every successful sync.
#[serde(default, skip_serializing_if = "Option::is_none")]
pub(crate) cached_display_name: Option<DisplayName>,
pub(crate) cached_display_name: Option<RoomDisplayName>,

/// Cached user defined notification mode.
#[serde(default, skip_serializing_if = "Option::is_none")]
Expand Down Expand Up @@ -1767,7 +1767,10 @@ impl RoomStateFilter {
/// 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 compute_display_name_from_heroes(num_joined_invited: u64, mut heroes: Vec<&str>) -> DisplayName {
fn compute_display_name_from_heroes(
num_joined_invited: u64,
mut heroes: Vec<&str>,
) -> RoomDisplayName {
let num_heroes = heroes.len() as u64;
let num_joined_invited_except_self = num_joined_invited.saturating_sub(1);

Expand All @@ -1789,12 +1792,12 @@ fn compute_display_name_from_heroes(num_joined_invited: u64, mut heroes: Vec<&st
// User is alone.
if num_joined_invited <= 1 {
if names.is_empty() {
DisplayName::Empty
RoomDisplayName::Empty
} else {
DisplayName::EmptyWas(names)
RoomDisplayName::EmptyWas(names)
}
} else {
DisplayName::Calculated(names)
RoomDisplayName::Calculated(names)
}
}

Expand Down Expand Up @@ -1851,7 +1854,7 @@ mod tests {
use crate::{
rooms::RoomNotableTags,
store::{IntoStateStore, MemoryStore, StateChanges, StateStore},
BaseClient, DisplayName, MinimalStateEvent, OriginalMinimalStateEvent,
BaseClient, MinimalStateEvent, OriginalMinimalStateEvent, RoomDisplayName,
RoomInfoNotableUpdateReasons, SessionMeta,
};

Expand Down Expand Up @@ -2126,7 +2129,7 @@ mod tests {

assert_eq!(
info.cached_display_name.as_ref(),
Some(&DisplayName::Calculated("lol".to_owned())),
Some(&RoomDisplayName::Calculated("lol".to_owned())),
);
assert_eq!(
info.cached_user_defined_notification_mode.as_ref(),
Expand Down Expand Up @@ -2331,7 +2334,7 @@ mod tests {
#[async_test]
async fn test_display_name_for_joined_room_is_empty_if_no_info() {
let (_, room) = make_room_test_helper(RoomState::Joined);
assert_eq!(room.compute_display_name().await.unwrap(), DisplayName::Empty);
assert_eq!(room.compute_display_name().await.unwrap(), RoomDisplayName::Empty);
}

#[async_test]
Expand All @@ -2341,7 +2344,7 @@ mod tests {
.update(|info| info.base_info.canonical_alias = Some(make_canonical_alias_event()));
assert_eq!(
room.compute_display_name().await.unwrap(),
DisplayName::Aliased("test".to_owned())
RoomDisplayName::Aliased("test".to_owned())
);
}

Expand All @@ -2352,20 +2355,20 @@ mod tests {
.update(|info| info.base_info.canonical_alias = Some(make_canonical_alias_event()));
assert_eq!(
room.compute_display_name().await.unwrap(),
DisplayName::Aliased("test".to_owned())
RoomDisplayName::Aliased("test".to_owned())
);
room.inner.update(|info| info.base_info.name = Some(make_name_event()));
// Display name wasn't cached when we asked for it above, and name overrides
assert_eq!(
room.compute_display_name().await.unwrap(),
DisplayName::Named("Test Room".to_owned())
RoomDisplayName::Named("Test Room".to_owned())
);
}

#[async_test]
async fn test_display_name_for_invited_room_is_empty_if_no_info() {
let (_, room) = make_room_test_helper(RoomState::Invited);
assert_eq!(room.compute_display_name().await.unwrap(), DisplayName::Empty);
assert_eq!(room.compute_display_name().await.unwrap(), RoomDisplayName::Empty);
}

#[async_test]
Expand All @@ -2378,7 +2381,7 @@ mod tests {
});
room.inner.update(|info| info.base_info.name = Some(room_name));

assert_eq!(room.compute_display_name().await.unwrap(), DisplayName::Empty);
assert_eq!(room.compute_display_name().await.unwrap(), RoomDisplayName::Empty);
}

#[async_test]
Expand All @@ -2388,7 +2391,7 @@ mod tests {
.update(|info| info.base_info.canonical_alias = Some(make_canonical_alias_event()));
assert_eq!(
room.compute_display_name().await.unwrap(),
DisplayName::Aliased("test".to_owned())
RoomDisplayName::Aliased("test".to_owned())
);
}

Expand All @@ -2399,13 +2402,13 @@ mod tests {
.update(|info| info.base_info.canonical_alias = Some(make_canonical_alias_event()));
assert_eq!(
room.compute_display_name().await.unwrap(),
DisplayName::Aliased("test".to_owned())
RoomDisplayName::Aliased("test".to_owned())
);
room.inner.update(|info| info.base_info.name = Some(make_name_event()));
// Display name wasn't cached when we asked for it above, and name overrides
assert_eq!(
room.compute_display_name().await.unwrap(),
DisplayName::Named("Test Room".to_owned())
RoomDisplayName::Named("Test Room".to_owned())
);
}

Expand Down Expand Up @@ -2447,7 +2450,7 @@ mod tests {
room.inner.update_if(|info| info.update_from_ruma_summary(&summary));
assert_eq!(
room.compute_display_name().await.unwrap(),
DisplayName::Calculated("Matthew".to_owned())
RoomDisplayName::Calculated("Matthew".to_owned())
);
}

Expand All @@ -2469,7 +2472,7 @@ mod tests {

assert_eq!(
room.compute_display_name().await.unwrap(),
DisplayName::Calculated("Matthew".to_owned())
RoomDisplayName::Calculated("Matthew".to_owned())
);
}

Expand Down Expand Up @@ -2499,7 +2502,7 @@ mod tests {
room.inner.update_if(|info| info.update_from_ruma_summary(&summary));
assert_eq!(
room.compute_display_name().await.unwrap(),
DisplayName::Calculated("Matthew".to_owned())
RoomDisplayName::Calculated("Matthew".to_owned())
);
}

Expand All @@ -2524,7 +2527,7 @@ mod tests {

assert_eq!(
room.compute_display_name().await.unwrap(),
DisplayName::Calculated("Matthew".to_owned())
RoomDisplayName::Calculated("Matthew".to_owned())
);
}

Expand Down Expand Up @@ -2579,7 +2582,7 @@ mod tests {

assert_eq!(
room.compute_display_name().await.unwrap(),
DisplayName::Calculated("Bob, Carol, Denis, Erica, and 3 others".to_owned())
RoomDisplayName::Calculated("Bob, Carol, Denis, Erica, and 3 others".to_owned())
);
}

Expand Down Expand Up @@ -2628,7 +2631,7 @@ mod tests {

assert_eq!(
room.compute_display_name().await.unwrap(),
DisplayName::Calculated("Alice, Bob, Carol, Denis, Erica, and 2 others".to_owned())
RoomDisplayName::Calculated("Alice, Bob, Carol, Denis, Erica, and 2 others".to_owned())
);
}

Expand Down Expand Up @@ -2658,7 +2661,7 @@ mod tests {
room.inner.update_if(|info| info.update_from_ruma_summary(&summary));
assert_eq!(
room.compute_display_name().await.unwrap(),
DisplayName::EmptyWas("Matthew".to_owned())
RoomDisplayName::EmptyWas("Matthew".to_owned())
);
}

Expand Down Expand Up @@ -3054,34 +3057,34 @@ mod tests {
#[test]
fn test_calculate_room_name() {
let mut actual = compute_display_name_from_heroes(2, vec!["a"]);
assert_eq!(DisplayName::Calculated("a".to_owned()), actual);
assert_eq!(RoomDisplayName::Calculated("a".to_owned()), actual);

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

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

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

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

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

actual = compute_display_name_from_heroes(1, vec![]);
assert_eq!(DisplayName::Empty, actual);
assert_eq!(RoomDisplayName::Empty, actual);

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

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

actual = compute_display_name_from_heroes(1, vec!["a", "b", "c"]);
assert_eq!(DisplayName::EmptyWas("a, b, c".to_owned()), actual);
assert_eq!(RoomDisplayName::EmptyWas("a, b, c".to_owned()), actual);
}

#[test]
Expand Down
Loading
Loading