From 22294896901f0e6385802a66719d206257a09500 Mon Sep 17 00:00:00 2001 From: Ivan Enderlin Date: Wed, 3 Jul 2024 11:15:35 +0200 Subject: [PATCH] feat(ui): The `recency` sorter now uses `recency_timestamp`. This patch changes the `recency` sorter to use `Room::recency_timestamp` instead of `LatestEvent::event_origin_server_ts` to sort rooms. --- .../src/room_list_service/sorters/recency.rs | 215 ++++-------------- .../tests/integration/room_list_service.rs | 117 +--------- 2 files changed, 54 insertions(+), 278 deletions(-) diff --git a/crates/matrix-sdk-ui/src/room_list_service/sorters/recency.rs b/crates/matrix-sdk-ui/src/room_list_service/sorters/recency.rs index 0a65685b8f3..e9d774daeb0 100644 --- a/crates/matrix-sdk-ui/src/room_list_service/sorters/recency.rs +++ b/crates/matrix-sdk-ui/src/room_list_service/sorters/recency.rs @@ -12,22 +12,22 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::{cmp::Ordering, ops::Deref}; +use std::cmp::Ordering; -use matrix_sdk_base::latest_event::LatestEvent; +use ruma::MilliSecondsSinceUnixEpoch; use super::{Room, Sorter}; struct RecencyMatcher where - F: Fn(&Room, &Room) -> (Option, Option), + F: Fn(&Room, &Room) -> (Option, Option), { - latest_events: F, + timestamps: F, } impl RecencyMatcher where - F: Fn(&Room, &Room) -> (Option, Option), + F: Fn(&Room, &Room) -> (Option, Option), { fn matches(&self, left: &Room, right: &Room) -> Ordering { if left.id() == right.id() { @@ -38,24 +38,23 @@ where // same data, because they are all built from the same store. They can be seen // as shallow clones of each others. In practice it's really great: a `Room` can // never be outdated. However, for the case of sorting rooms, it breaks the - // search algorithm. `left` and `right` will have the exact same `LatestEvent`, - // so `left` and `right` will always be `Ordering::Equal`. This is wrong: if - // `left` is compared with `right` and if they are both the same room, it means - // that one of them (either `left`, or `right`, it's not important) has received - // an update. The room position is very likely to change. But if they compare to - // `Equal`, the position may not change. It actually depends of the search - // algorithm used by [`eyeball_im_util::SortBy`]. + // search algorithm. `left` and `right` will have the exact same recency + // timestamp, so `left` and `right` will always be `Ordering::Equal`. This is + // wrong: if `left` is compared with `right` and if they are both the same room, + // it means that one of them (either `left`, or `right`, it's not important) has + // received an update. The room position is very likely to change. But if they + // compare to `Equal`, the position may not change. It actually depends of the + // search algorithm used by [`eyeball_im_util::SortBy`]. // // Since this room received an update, it is more recent than the previous one // we matched against, so return `Ordering::Greater`. return Ordering::Greater; } - match (self.latest_events)(left, right) { - (Some(left_latest_event), Some(right_latest_event)) => left_latest_event - .event_origin_server_ts() - .cmp(&right_latest_event.event_origin_server_ts()) - .reverse(), + match (self.timestamps)(left, right) { + (Some(left_timestamp), Some(right_timestamp)) => { + left_timestamp.cmp(&right_timestamp).reverse() + } (Some(_), None) => Ordering::Less, @@ -67,13 +66,11 @@ where } /// Create a new sorter that will sort two [`Room`] by recency, i.e. by -/// comparing their `origin_server_ts` value. The `Room` with the newest -/// `origin_server_ts` comes first, i.e. newest < oldest. +/// comparing their [`matrix_sdk_base::RoomInfo::recency_timestamp`] value. The +/// `Room` with the newest recency timestamp comes first, i.e. newest < oldest. pub fn new_sorter() -> impl Sorter { let matcher = RecencyMatcher { - latest_events: move |left, right| { - (left.deref().latest_event(), right.deref().latest_event()) - }, + timestamps: move |left, right| (left.recency_timestamp(), right.recency_timestamp()), }; move |left, right| -> Ordering { matcher.matches(left, right) } @@ -81,208 +78,86 @@ pub fn new_sorter() -> impl Sorter { #[cfg(test)] mod tests { - use matrix_sdk_test::{async_test, sync_timeline_event, ALICE, BOB}; - use ruma::room_id; + use matrix_sdk_test::async_test; + use ruma::{room_id, MilliSecondsSinceUnixEpoch, UInt}; use super::{ super::super::filters::{client_and_server_prelude, new_rooms}, *, }; + macro_rules! ms { + ($value:literal) => { + MilliSecondsSinceUnixEpoch(UInt::new_wrapping($value as u64)) + }; + } + #[async_test] - async fn test_with_two_latest_events() { + async fn test_with_two_recency_timestamps() { let (client, server, sliding_sync) = client_and_server_prelude().await; let [room_a, room_b] = new_rooms([room_id!("!a:b.c"), room_id!("!d:e.f")], &client, &server, &sliding_sync) .await; - // `room_a` has an older latest event than `room_b`. + // `room_a` has an older recency timestamp than `room_b`. { - let matcher = RecencyMatcher { - latest_events: |_left, _right| { - ( - Some(LatestEvent::new( - sync_timeline_event!({ - "content": { - "body": "foo", - "msgtype": "m.text", - }, - "sender": &*ALICE, - "event_id": "$foo", - "origin_server_ts": 1, - "type": "m.room.message", - }) - .into(), - )), - Some(LatestEvent::new( - sync_timeline_event!({ - "content": { - "body": "bar", - "msgtype": "m.text", - }, - "sender": &*BOB, - "event_id": "$bar", - "origin_server_ts": 2, - "type": "m.room.message", - }) - .into(), - )), - ) - }, - }; + let matcher = + RecencyMatcher { timestamps: |_left, _right| (Some(ms!(1)), Some(ms!(2))) }; // `room_a` is greater than `room_b`, i.e. it must come after `room_b`. assert_eq!(matcher.matches(&room_a, &room_b), Ordering::Greater); } - // `room_b` has an older latest event than `room_a`. + // `room_b` has an older recency timestamp than `room_a`. { - let matcher = RecencyMatcher { - latest_events: |_left, _right| { - ( - Some(LatestEvent::new( - sync_timeline_event!({ - "content": { - "body": "foo", - "msgtype": "m.text", - }, - "sender": &*ALICE, - "event_id": "$foo", - "origin_server_ts": 2, - "type": "m.room.message", - }) - .into(), - )), - Some(LatestEvent::new( - sync_timeline_event!({ - "content": { - "body": "bar", - "msgtype": "m.text", - }, - "sender": &*BOB, - "event_id": "$bar", - "origin_server_ts": 1, - "type": "m.room.message", - }) - .into(), - )), - ) - }, - }; + let matcher = + RecencyMatcher { timestamps: |_left, _right| (Some(ms!(2)), Some(ms!(1))) }; // `room_a` is less than `room_b`, i.e. it must come before `room_b`. assert_eq!(matcher.matches(&room_a, &room_b), Ordering::Less); } - // `room_a` has an equally old latest event than `room_b`. + // `room_a` has an equally old recency timestamp than `room_b`. { - let matcher = RecencyMatcher { - latest_events: |_left, _right| { - ( - Some(LatestEvent::new( - sync_timeline_event!({ - "content": { - "body": "foo", - "msgtype": "m.text", - }, - "sender": &*ALICE, - "event_id": "$foo", - "origin_server_ts": 1, - "type": "m.room.message", - }) - .into(), - )), - Some(LatestEvent::new( - sync_timeline_event!({ - "content": { - "body": "bar", - "msgtype": "m.text", - }, - "sender": &*BOB, - "event_id": "$bar", - "origin_server_ts": 1, - "type": "m.room.message", - }) - .into(), - )), - ) - }, - }; + let matcher = + RecencyMatcher { timestamps: |_left, _right| (Some(ms!(1)), Some(ms!(1))) }; assert_eq!(matcher.matches(&room_a, &room_b), Ordering::Equal); } } #[async_test] - async fn test_with_one_latest_event() { + async fn test_with_one_recency_timestamp() { let (client, server, sliding_sync) = client_and_server_prelude().await; let [room_a, room_b] = new_rooms([room_id!("!a:b.c"), room_id!("!d:e.f")], &client, &server, &sliding_sync) .await; - // `room_a` has a latest event, `room_b` has no latest event. + // `room_a` has a recency timestamp, `room_b` has no recency timestamp. { - let matcher = RecencyMatcher { - latest_events: |_left, _right| { - ( - Some(LatestEvent::new( - sync_timeline_event!({ - "content": { - "body": "foo", - "msgtype": "m.text", - }, - "sender": &*ALICE, - "event_id": "$foo", - "origin_server_ts": 1, - "type": "m.room.message", - }) - .into(), - )), - None, - ) - }, - }; + let matcher = RecencyMatcher { timestamps: |_left, _right| (Some(ms!(1)), None) }; assert_eq!(matcher.matches(&room_a, &room_b), Ordering::Less); } - // `room_a` has no latest event, `room_b` has a latest event. + // `room_a` has no recency timestamp, `room_b` has a recency timestamp. { - let matcher = RecencyMatcher { - latest_events: |_left, _right| { - ( - None, - Some(LatestEvent::new( - sync_timeline_event!({ - "content": { - "body": "bar", - "msgtype": "m.text", - }, - "sender": &*BOB, - "event_id": "$bar", - "origin_server_ts": 1, - "type": "m.room.message", - }) - .into(), - )), - ) - }, - }; + let matcher = RecencyMatcher { timestamps: |_left, _right| (None, Some(ms!(1))) }; assert_eq!(matcher.matches(&room_a, &room_b), Ordering::Greater); } } #[async_test] - async fn test_with_zero_latest_event() { + async fn test_with_zero_recency_timestamp() { let (client, server, sliding_sync) = client_and_server_prelude().await; let [room_a, room_b] = new_rooms([room_id!("!a:b.c"), room_id!("!d:e.f")], &client, &server, &sliding_sync) .await; - // `room_a` and `room_b` has no latest event. + // `room_a` and `room_b` has no recency timestamp. { - let matcher = RecencyMatcher { latest_events: |_left, _right| (None, None) }; + let matcher = RecencyMatcher { timestamps: |_left, _right| (None, None) }; assert_eq!(matcher.matches(&room_a, &room_b), Ordering::Equal); } diff --git a/crates/matrix-sdk-ui/tests/integration/room_list_service.rs b/crates/matrix-sdk-ui/tests/integration/room_list_service.rs index 2309885499b..47fddfad322 100644 --- a/crates/matrix-sdk-ui/tests/integration/room_list_service.rs +++ b/crates/matrix-sdk-ui/tests/integration/room_list_service.rs @@ -1292,18 +1292,7 @@ async fn test_dynamic_entries_stream() -> Result<(), Error> { "rooms": { "!r0:bar.org": { "initial": true, - "timeline": [ - { - "content": { - "body": "foo", - "msgtype": "m.text", - }, - "sender": "@a:b.c", - "event_id": "$t0", - "origin_server_ts": 1, - "type": "m.room.message", - }, - ], + "timestamp": 1, "required_state": [ { "content": { @@ -1357,18 +1346,7 @@ async fn test_dynamic_entries_stream() -> Result<(), Error> { "rooms": { "!r1:bar.org": { "initial": true, - "timeline": [ - { - "content": { - "body": "foo", - "msgtype": "m.text", - }, - "sender": "@a:b.c", - "event_id": "$t1", - "origin_server_ts": 2, - "type": "m.room.message", - }, - ], + "timestamp": 2, "required_state": [ { "content": { @@ -1384,18 +1362,7 @@ async fn test_dynamic_entries_stream() -> Result<(), Error> { }, "!r2:bar.org": { "initial": true, - "timeline": [ - { - "content": { - "body": "foo", - "msgtype": "m.text", - }, - "sender": "@a:b.c", - "event_id": "$t2", - "origin_server_ts": 3, - "type": "m.room.message", - }, - ], + "timestamp": 3, "required_state": [ { "content": { @@ -1411,18 +1378,7 @@ async fn test_dynamic_entries_stream() -> Result<(), Error> { }, "!r3:bar.org": { "initial": true, - "timeline": [ - { - "content": { - "body": "foo", - "msgtype": "m.text", - }, - "sender": "@a:b.c", - "event_id": "$t3", - "origin_server_ts": 4, - "type": "m.room.message", - }, - ], + "timestamp": 4, "required_state": [ { "content": { @@ -1438,18 +1394,7 @@ async fn test_dynamic_entries_stream() -> Result<(), Error> { }, "!r4:bar.org": { "initial": true, - "timeline": [ - { - "content": { - "body": "foo", - "msgtype": "m.text", - }, - "sender": "@a:b.c", - "event_id": "$t4", - "origin_server_ts": 5, - "type": "m.room.message", - }, - ], + "timestamp": 5, "required_state": [ { "content": { @@ -1508,18 +1453,7 @@ async fn test_dynamic_entries_stream() -> Result<(), Error> { "rooms": { "!r5:bar.org": { "initial": true, - "timeline": [ - { - "content": { - "body": "foo", - "msgtype": "m.text", - }, - "sender": "@a:b.c", - "event_id": "$t5", - "origin_server_ts": 6, - "type": "m.room.message", - }, - ], + "timestamp": 6, "required_state": [ { "content": { @@ -1535,18 +1469,7 @@ async fn test_dynamic_entries_stream() -> Result<(), Error> { }, "!r6:bar.org": { "initial": true, - "timeline": [ - { - "content": { - "body": "foo", - "msgtype": "m.text", - }, - "sender": "@a:b.c", - "event_id": "$t6", - "origin_server_ts": 7, - "type": "m.room.message", - }, - ], + "timestamp": 7, "required_state": [ { "content": { @@ -1562,18 +1485,7 @@ async fn test_dynamic_entries_stream() -> Result<(), Error> { }, "!r7:bar.org": { "initial": true, - "timeline": [ - { - "content": { - "body": "foo", - "msgtype": "m.text", - }, - "sender": "@a:b.c", - "event_id": "$t7", - "origin_server_ts": 8, - "type": "m.room.message", - }, - ], + "timestamp": 8, "required_state": [ { "content": { @@ -1704,18 +1616,7 @@ async fn test_dynamic_entries_stream() -> Result<(), Error> { "rooms": { "!r0:bar.org": { "initial": true, - "timeline": [ - { - "content": { - "body": "foo", - "msgtype": "m.text", - }, - "sender": "@a:b.c", - "event_id": "$t8", - "origin_server_ts": 9, - "type": "m.room.message", - }, - ], + "timestamp": 9, "required_state": [], }, },