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

feat(base): New RoomInfoNotableUpdateReasons::READ_RECEIPT #3680

Merged
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: 0 additions & 2 deletions crates/matrix-sdk-base/src/read_receipts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -449,8 +449,6 @@ fn events_intersects<'a>(
/// that has been just received for an event that came in a previous sync.
///
/// See this module's documentation for more information.
///
/// Returns a boolean indicating if a field changed value in the read receipts.
#[instrument(skip_all, fields(room_id = %room_id))]
pub(crate) fn compute_unread_counts(
user_id: &UserId,
Expand Down
3 changes: 3 additions & 0 deletions crates/matrix-sdk-base/src/rooms/normal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,9 @@ bitflags! {

/// The latest event of the `Room` has changed.
const LATEST_EVENT = 0b0000_0010;

/// A read receipt has changed.
const READ_RECEIPT = 0b0000_0100;
}
}

Expand Down
121 changes: 107 additions & 14 deletions crates/matrix-sdk-base/src/sliding_sync.rs
Copy link
Member

Choose a reason for hiding this comment

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

Please fold 0e73620 into the commits it belongs to (consider git-absorb next time!)

Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,11 @@ impl BaseClient {
);

if prev_read_receipts != room_info.read_receipts {
room_info_notable_updates
.entry(room_id.clone())
.or_default()
.insert(RoomInfoNotableUpdateReasons::READ_RECEIPT);

changes.add_room(room_info);
}
}
Expand Down Expand Up @@ -806,7 +811,7 @@ mod tests {
rooms::normal::{RoomHero, RoomInfoNotableUpdateReasons},
store::MemoryStore,
test_utils::logged_in_base_client,
BaseClient, Room, RoomState,
BaseClient, Room, RoomInfoNotableUpdate, RoomState,
};

#[async_test]
Expand Down Expand Up @@ -1847,6 +1852,92 @@ mod tests {
}
}

#[async_test]
async fn test_recency_timestamp_can_trigger_a_notable_update_reason() {
// Given a logged-in client
let client = logged_in_base_client(None).await;
let mut room_info_notable_update_stream = client.room_info_notable_update_receiver();
let room_id = room_id!("!r:e.uk");

// When I send sliding sync response containing a room with a recency timestamp.
let room = assign!(v4::SlidingSyncRoom::new(), {
timestamp: Some(MilliSecondsSinceUnixEpoch(42u32.into())),
});
let response = response_with_room(room_id, room);
client.process_sliding_sync(&response, &()).await.expect("Failed to process sync");

// Then a room info notable update is NOT received, because it's the first time
// the room is seen.
assert_matches!(
room_info_notable_update_stream.recv().await,
Ok(RoomInfoNotableUpdate { room_id: received_room_id, reasons: received_reasons }) => {
assert_eq!(received_room_id, room_id);
assert!(!received_reasons.contains(RoomInfoNotableUpdateReasons::RECENCY_TIMESTAMP));
Copy link
Member

Choose a reason for hiding this comment

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

Does this pass?

assert!(received_reasons.is_empty());

Copy link
Member Author

Choose a reason for hiding this comment

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

I prefer to test the reason is not present, in case other reasons may exist. We don't care about the other reasons at this step. Tests are less likely to break in the future if these kind of assertions.

}
);

// When I send sliding sync response containing a room with a recency timestamp.
let room = assign!(v4::SlidingSyncRoom::new(), {
timestamp: Some(MilliSecondsSinceUnixEpoch(43u32.into())),
});
let response = response_with_room(room_id, room);
client.process_sliding_sync(&response, &()).await.expect("Failed to process sync");

// Then a room info notable update is received.
assert_matches!(
room_info_notable_update_stream.recv().await,
Ok(RoomInfoNotableUpdate { room_id: received_room_id, reasons: received_reasons }) => {
assert_eq!(received_room_id, room_id);
assert!(received_reasons.contains(RoomInfoNotableUpdateReasons::RECENCY_TIMESTAMP));
}
);
}

#[async_test]
async fn test_read_receipt_can_trigger_a_notable_update_reason() {
// Given a logged-in client
let client = logged_in_base_client(None).await;
let mut room_info_notable_update_stream = client.room_info_notable_update_receiver();

// When I send sliding sync response containing a new room.
let room_id = room_id!("!r:e.uk");
let room = v4::SlidingSyncRoom::new();
let response = response_with_room(room_id, room);
client.process_sliding_sync(&response, &()).await.expect("Failed to process sync");

// Then a room info notable update is NOT received.
assert_matches!(
room_info_notable_update_stream.recv().await,
Ok(RoomInfoNotableUpdate { room_id: received_room_id, reasons: received_reasons }) => {
assert_eq!(received_room_id, room_id);
assert!(!received_reasons.contains(RoomInfoNotableUpdateReasons::READ_RECEIPT));
Copy link
Member

Choose a reason for hiding this comment

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

Same as before, can this be a check that reasons is empty?

}
);

// When I send sliding sync response containing a couple of events with no read
// receipt.
let room_id = room_id!("!r:e.uk");
let events = vec![
make_raw_event("m.room.message", "$3"),
Copy link
Member

Choose a reason for hiding this comment

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

make_raw_event and make_event should be replaced with the EventFactory 😇

Copy link
Member Author

Choose a reason for hiding this comment

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

This entire test suite should be revisited. Later. For another time. But I agree!

make_raw_event("m.room.message", "$4"),
make_raw_event("m.read", "$5"),
Copy link
Member

Choose a reason for hiding this comment

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

Technically this is wrong, as this will create a m.read event with the content of a m.room.message event; it seems compute_unread_counts() still does something with it (store it, maybe?), but this could be a bit better…

Copy link
Member Author

@Hywan Hywan Jul 10, 2024

Choose a reason for hiding this comment

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

There is no RoomReadReceipt before, and now there is one, so the previous and the new one are different, which triggers something. Do you have a better minimal test in mind?

Copy link
Member

Choose a reason for hiding this comment

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

All I'm saying is that the m.read event should be created in a better way (with a proper content property), because it doesn't make sense now :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Since you've approved the PR, I understand it's fine for this particular test. The read receipt mechanism is already pretty well test. Here we only want to test the notable update reason is triggered, which is fine for the moment.

];
let room = assign!(v4::SlidingSyncRoom::new(), {
timeline: events,
});
let response = response_with_room(room_id, room);
client.process_sliding_sync(&response, &()).await.expect("Failed to process sync");

// Then a room info notable update is received.
assert_matches!(
room_info_notable_update_stream.recv().await,
Ok(RoomInfoNotableUpdate { room_id: received_room_id, reasons: received_reasons }) => {
assert_eq!(received_room_id, room_id);
assert!(received_reasons.contains(RoomInfoNotableUpdateReasons::READ_RECEIPT));
}
);
}

async fn choose_event_to_cache(events: &[SyncTimelineEvent]) -> Option<SyncTimelineEvent> {
let room = make_room();
let mut room_info = room.clone_info();
Expand Down Expand Up @@ -1883,20 +1974,22 @@ mod tests {
)
}

fn make_event(typ: &str, id: &str) -> SyncTimelineEvent {
SyncTimelineEvent::new(
Raw::from_json_string(
json!({
"type": typ,
"event_id": id,
"content": { "msgtype": "m.text", "body": "my msg" },
"sender": "@u:h.uk",
"origin_server_ts": 12344445,
})
.to_string(),
)
.unwrap(),
fn make_raw_event(typ: &str, id: &str) -> Raw<AnySyncTimelineEvent> {
Raw::from_json_string(
json!({
"type": typ,
"event_id": id,
"content": { "msgtype": "m.text", "body": "my msg" },
"sender": "@u:h.uk",
"origin_server_ts": 12344445,
})
.to_string(),
)
.unwrap()
}

fn make_event(typ: &str, id: &str) -> SyncTimelineEvent {
SyncTimelineEvent::new(make_raw_event(typ, id))
}

fn make_encrypted_event(id: &str) -> SyncTimelineEvent {
Expand Down
7 changes: 5 additions & 2 deletions crates/matrix-sdk-ui/src/room_list_service/room_list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,8 @@ fn merge_stream_and_receiver(
raw_stream: impl Stream<Item = Vec<VectorDiff<Room>>>,
mut room_info_notable_update_receiver: broadcast::Receiver<RoomInfoNotableUpdate>,
) -> impl Stream<Item = Vec<VectorDiff<Room>>> {
use RoomInfoNotableUpdateReasons as NotableUpdate;

stream! {
pin_mut!(raw_stream);

Expand All @@ -215,8 +217,9 @@ fn merge_stream_and_receiver(
let reasons = &update.reasons;

// We are interested by these _reasons_.
if reasons.contains(RoomInfoNotableUpdateReasons::LATEST_EVENT) ||
reasons.contains(RoomInfoNotableUpdateReasons::RECENCY_TIMESTAMP) {
if reasons.contains(NotableUpdate::LATEST_EVENT) ||
reasons.contains(NotableUpdate::RECENCY_TIMESTAMP) ||
reasons.contains(NotableUpdate::READ_RECEIPT) {
// Emit a `VectorDiff::Set` for the specific rooms.
if let Some(index) = raw_current_values.iter().position(|room| room.room_id() == update.room_id) {
let update = VectorDiff::Set { index, value: raw_current_values[index].clone() };
Expand Down
Loading