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

feat(sdk,base,ui,ffi): Continue to remove dead code in SlidingSyncRoom #3084

Merged
merged 3 commits into from
Feb 1, 2024
Merged

feat(sdk,base,ui,ffi): Continue to remove dead code in SlidingSyncRoom #3084

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
75365a7
feat(sdk): Remove the `SlidingSyncRoom::is_initial_response` method.
Hywan Feb 1, 2024
6acd68c
feat(sdk): Remove the `SlidingSyncRoom::is_dm` method.
Hywan Feb 1, 2024
a802b73
feat(sdk): Remove the `SlidingSyncRoom::name` method.
Hywan Feb 1, 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
10 changes: 2 additions & 8 deletions crates/matrix-sdk-ui/src/room_list_service/room.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,15 +83,9 @@ impl Room {
self.inner.room.room_id()
}

/// Get the best possible name for the room.
///
/// If the sliding sync room has received a name from the server, then use
/// it, otherwise, let's calculate a name.
/// Get the name of the room if it exists.
pub async fn name(&self) -> Option<String> {
Some(match self.inner.sliding_sync_room.name() {
Some(name) => name,
None => self.inner.room.display_name().await.ok()?.to_string(),
})
Some(self.inner.room.display_name().await.ok()?.to_string())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we certain that this is not needed? The proxy calculates the room name for us even before we have all the members or events, i.e. it does what display_name() does for us.

I see that we're storing whatever the proxy sends to us as the m.room.name event in the RoomInfo since #2032.

But that PR seems to be questionable at best. Though the PR should mean that these two method do contain the same info.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, auto-merge strikes again 😨

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SS room name is synced with the room and room info during the sync process. So yeah, this is not needed to keep it in SS room anymore.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And to reply concretely to your mesasge, normally display_name returns early if RoomInfo contains a name,

matrix-rust-sdk/crates/matrix-sdk-base/src/rooms/normal.rs

Lines 568 to 575 in 40e0066

async fn calculate_name(&self) -> StoreResult<DisplayName> {
let summary = {
let inner = self.inner.read();
if let Some(name) = inner.name() {
let name = name.trim();
return Ok(DisplayName::Named(name.to_owned()));
}
.

Also, we have this test that ensures the behaviour at the sliding sync level,

matrix-rust-sdk/crates/matrix-sdk-base/src/sliding_sync.rs

Lines 1212 to 1232 in 40e0066

async fn display_name_from_sliding_sync_overrides_alias() {
// Given a logged-in client
let client = logged_in_client().await;
let room_id = room_id!("!r:e.uk");
let user_id = user_id!("@u:e.uk");
let room_alias_id = room_alias_id!("#myroom:e.uk");
// When the sliding sync response contains an explicit room name as well as an
// alias
let mut room = room_with_canonical_alias(room_alias_id, user_id);
room.name = Some("This came from the server".to_owned());
let response = response_with_room(room_id, room).await;
client.process_sliding_sync(&response, &()).await.expect("Failed to process sync");
// Then the room's name is just exactly what the server supplied
let client_room = client.get_room(room_id).expect("No room found");
assert_eq!(
client_room.display_name().await.unwrap().to_string(),
"This came from the server"
);
}
.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And to reply concretely to your mesasge, normally display_name returns early if RoomInfo contains a name,

I know what display_name() does, it follows the spec: https://spec.matrix.org/unstable/client-server-api/#calculating-the-display-name-for-a-room.

What I'm complaining about is that we put something that isn't a m.room.name event content into the field that always should contain a m.room.name event content.

The thing that the SS proxy returns is not a m.room.name event, it's the result of the calculation I linked previously done by the proxy.

Incidentally this thing works, because we created a fake m.room.name event and put it in the same field, the spec tells you to take this field verbatim. The problem is that this data, while also being a string, is semantically very different.

This might now contain the field "Tom, Dick, and Harry" for a three person room, but this string should be localized and we lost any way of doing and detecting this case because we pretended that it's actually an m.room.name event that contains "Tom, Dick, and Harry".

I should probably post this complaint on the PR instead or just open a new issue. I find such hacks that make things quickly work, but mangle with the correctness and make future features harder to implement, for example like the localization of room display names, the bane of our existence.

}

/// Get the underlying [`matrix_sdk::Room`].
Expand Down
3 changes: 1 addition & 2 deletions crates/matrix-sdk-ui/tests/integration/timeline/sliding_sync.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,8 +239,7 @@ async fn create_one_room(

assert!(update.rooms.contains(&room_id.to_owned()));

let room = sliding_sync.get_room(room_id).await.context("`get_room`")?;
assert_eq!(room.name(), Some(room_name.clone()));
let _room = sliding_sync.get_room(room_id).await.context("`get_room`")?;

Ok(())
}
Expand Down
100 changes: 0 additions & 100 deletions crates/matrix-sdk/src/sliding_sync/room.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,27 +61,6 @@ impl SlidingSyncRoom {
&self.inner.room_id
}

/// This rooms name as calculated by the server, if any
pub fn name(&self) -> Option<String> {
let inner = self.inner.inner.read().unwrap();

inner.name.to_owned()
}

/// Is this a direct message?
pub fn is_dm(&self) -> Option<bool> {
let inner = self.inner.inner.read().unwrap();

inner.is_dm
}

/// Was this an initial response?
pub fn is_initial_response(&self) -> Option<bool> {
let inner = self.inner.inner.read().unwrap();

inner.initial
}

/// Get the token for back-pagination.
pub fn prev_batch(&self) -> Option<String> {
self.inner.inner.read().unwrap().prev_batch.clone()
Expand Down Expand Up @@ -365,85 +344,6 @@ mod tests {
assert_eq!(room.room_id(), room_id);
}

macro_rules! test_getters {
(
$(
$test_name:ident {
$getter:ident () $( . $getter_field:ident )? = $default_value:expr;
receives $room_response:expr;
_ = $init_or_updated_value:expr;
receives nothing;
_ = $no_update_value:expr;
}
)+
) => {
$(
#[async_test]
async fn $test_name () {
// Default value.
{
let room = new_room(room_id!("!foo:bar.org"), room_response!({})).await;

assert_eq!(room.$getter() $( . $getter_field )?, $default_value, "default value");
}

// Some value when initializing.
{
let room = new_room(room_id!("!foo:bar.org"), $room_response).await;

assert_eq!(room.$getter() $( . $getter_field )?, $init_or_updated_value, "init value");
}

// Some value when updating.
{

let mut room = new_room(room_id!("!foo:bar.org"), room_response!({})).await;

// Value is set to the default value.
assert_eq!(room.$getter() $( . $getter_field )?, $default_value, "default value (bis)");

room.update($room_response, vec![]);

// Value has been updated.
assert_eq!(room.$getter() $( . $getter_field )?, $init_or_updated_value, "updated value");

room.update(room_response!({}), vec![]);

// Value is kept.
assert_eq!(room.$getter() $( . $getter_field )?, $no_update_value, "not updated value");
}

}
)+
};
}

test_getters! {
test_room_name {
name() = None;
receives room_response!({"name": "gordon"});
_ = Some("gordon".to_owned());
receives nothing;
_ = Some("gordon".to_owned());
}

test_room_is_dm {
is_dm() = None;
receives room_response!({"is_dm": true});
_ = Some(true);
receives nothing;
_ = Some(true);
}

test_room_is_initial_response {
is_initial_response() = None;
receives room_response!({"initial": true});
_ = Some(true);
receives nothing;
_ = Some(true);
}
}

#[async_test]
async fn test_prev_batch() {
// Default value.
Expand Down
Loading