Skip to content

Commit

Permalink
feat(ui): The recency sorter now uses recency_timestamp.
Browse files Browse the repository at this point in the history 
This patch changes the `recency` sorter to use `Room::recency_timestamp`
instead of `LatestEvent::event_origin_server_ts` to sort rooms.
  • Loading branch information
@Hywan
Hywan committed Jul 3, 2024
1 parent 9744ad4 commit 2229489
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 278 deletions.
215 changes: 45 additions & 170 deletions crates/matrix-sdk-ui/src/room_list_service/sorters/recency.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<F>
where
F: Fn(&Room, &Room) -> (Option<LatestEvent>, Option<LatestEvent>),
F: Fn(&Room, &Room) -> (Option<MilliSecondsSinceUnixEpoch>, Option<MilliSecondsSinceUnixEpoch>),
{
latest_events: F,
timestamps: F,
}

impl<F> RecencyMatcher<F>
where
F: Fn(&Room, &Room) -> (Option<LatestEvent>, Option<LatestEvent>),
F: Fn(&Room, &Room) -> (Option<MilliSecondsSinceUnixEpoch>, Option<MilliSecondsSinceUnixEpoch>),
{
fn matches(&self, left: &Room, right: &Room) -> Ordering {
if left.id() == right.id() {
Expand All @@ -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,

Expand All @@ -67,222 +66,98 @@ 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) }
}

#[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);
}
Expand Down
Loading

0 comments on commit 2229489

Please sign in to comment.