chore(room_list): allow knock state event as `latest_event` #4170

jmartinesp · 2024-10-25T14:02:36Z

This allows clients to display pending knocking requests in the room list items (needed by EXI and EXA).

Public API changes documented in changelogs (optional)

Signed-off-by:

codecov · 2024-10-25T14:17:19Z

Codecov Report

Attention: Patch coverage is 90.56604% with 5 lines in your changes missing coverage. Please review.

Project coverage is 84.85%. Comparing base (6752cf7) to head (61f48ba).
Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/matrix-sdk-base/src/client.rs	66.66%	1 Missing ⚠️
crates/matrix-sdk-base/src/latest_event.rs	91.66%	1 Missing ⚠️
crates/matrix-sdk-base/src/rooms/normal.rs	83.33%	1 Missing ⚠️
crates/matrix-sdk-base/src/sliding_sync/mod.rs	90.90%	1 Missing ⚠️
...trix-sdk-ui/src/timeline/event_item/content/mod.rs	91.66%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4170   +/-   ##
=======================================
  Coverage   84.84%   84.85%           
=======================================
  Files         270      270           
  Lines       28991    29031   +40     
=======================================
+ Hits        24597    24633   +36     
- Misses       4394     4398    +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

stefanceriu

Very nice, lgtm 👌

jmartinesp · 2024-10-29T08:31:22Z

@stefanceriu I added some logic changes in c79eaf0 in case you want to re-review. @bnjbvr let me know if you want to take a look too.

jmartinesp · 2024-10-29T08:36:24Z

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

+    /// Gets this Room's power levels.
+    pub async fn power_levels(&self) -> Option<RoomPowerLevels> {
+        self.store
+            .get_state_event_static::<RoomPowerLevelsEventContent>(&self.room_id)
+            .await
+            .ok()
+            .flatten()
+            .and_then(|raw| raw.deserialize().ok())
+            .map(|ev| ev.power_levels())
+    }
+


We already have matrix_sdk::Room::room_power_levels, but we needed something at the matrix_sdk_base crate too. Since sdk::Room already has access to the fns in sdk_base::Room, should I just remove the one in sdk and use this one instead?

Indeed, they do the same thing. Can we have this function here return a Result instead, to not silently swallow errors, please? (and we can keep the shorter power_levels name, I like it better 👍)

stefanceriu · 2024-10-29T08:39:38Z

crates/matrix-sdk-base/src/latest_event.rs

+            // can either accept or decline them
+            if let AnySyncStateEvent::RoomMember(member) = state {
+                if matches!(member.membership(), MembershipState::Knock) {
+                    let can_accept_or_decline_knocks = match power_levels_info {


Would it make sense to move this logic to within the RoomPowerLevels, similar to the other can_foos?

I'm not sure it's worth it, since it'll only be used here (I hope).

stefanceriu · 2024-10-29T08:49:20Z

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

@@ -698,14 +698,34 @@ async fn cache_latest_events(
    let mut encrypted_events =
        Vec::with_capacity(room.latest_encrypted_events.read().unwrap().capacity());

+    // Try to get room power levels from the current changes


This feels like a lot of work to do just in case you get a knocked state event, which I presume doesn't happen very often.

I agree, but I didn't find any alternatives other than not implementing this part of the functionality 🫤 . We'd have to change the sync process so we can process these latest events after the client has processed the sync request and saved all the data to the store and I think that would be quite complex.

stefanceriu · 2024-10-29T08:58:16Z

crates/matrix-sdk-ui/src/timeline/event_item/mod.rs

@@ -158,9 +158,21 @@ impl EventTimelineItem {
        let event_id = event.event_id().to_owned();
        let is_own = client.user_id().map(|uid| uid == sender).unwrap_or(false);

+        // Get the room's power levels for calculating the latest event


Same here I guess but I'm not sure how heavy it actually is. Does fetching a room and power levels have any potential of introducing slowdowns?

Passing the client into PossibleLatestEvent would be weird and it wouldn't improve the sliding sync part 🤔

I think getting both the room and the state event should be quite lightweight, but I'm not sure how much it could affect performance. The alternative could be to only load them if we reach a knock state event, maybe, although I'd have to change some data structures.

bnjbvr · 2024-10-29T09:00:56Z

I'll do a lightweight review (I promise 😇)

bnjbvr

LGTM, thanks!

bnjbvr · 2024-10-29T11:02:47Z

crates/matrix-sdk-base/src/latest_event.rs

@@ -37,6 +47,9 @@ pub enum PossibleLatestEvent<'a> {
    /// This message is suitable - it's a call notification
    YesCallNotify(&'a SyncCallNotifyEvent),

+    /// This state event is suitable - it's a knock membership change


Suggested change

/// This state event is suitable - it's a knock membership change

/// This state event is suitable - it's a knock membership change

/// that can be handled by the current user.

bnjbvr · 2024-10-29T11:05:33Z

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

+    /// Gets this Room's power levels.
+    pub async fn power_levels(&self) -> Option<RoomPowerLevels> {
+        self.store
+            .get_state_event_static::<RoomPowerLevelsEventContent>(&self.room_id)
+            .await
+            .ok()
+            .flatten()
+            .and_then(|raw| raw.deserialize().ok())
+            .map(|ev| ev.power_levels())
+    }
+


Indeed, they do the same thing. Can we have this function here return a Result instead, to not silently swallow errors, please? (and we can keep the shorter power_levels name, I like it better 👍)

bnjbvr · 2024-10-29T11:06:35Z

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

+    let power_levels_from_changes = || {
+        let state_changes = changes?.state.get(room_info.room_id())?;
+        let room_power_levels_state =
+            state_changes.get(&StateEventType::RoomPowerLevels)?.values().next()?;
+        match room_power_levels_state.deserialize().ok()? {
+            AnySyncStateEvent::RoomPowerLevels(ev) => Some(ev.power_levels()),
+            _ => None,
+        }
+    };
+
+    // If we didn't get any info, try getting it from local data
+    let power_levels = match power_levels_from_changes() {
+        Some(power_levels) => Some(power_levels),
+        None => room.power_levels().await,
+    };


Beautiful 🤩

bnjbvr · 2024-10-29T11:08:26Z

crates/matrix-sdk-ui/src/timeline/event_item/mod.rs

+        let room_power_levels_info = match (client.user_id(), power_levels.as_ref()) {
+            (Some(user_id), Some(power_levels)) => Some((user_id, power_levels)),
+            _ => None,
+        };


GOOD NEWS EVERYONE! There's Option::zip() that does exactly the same thing :)

Suggested change

let room_power_levels_info = match (client.user_id(), power_levels.as_ref()) {

(Some(user_id), Some(power_levels)) => Some((user_id, power_levels)),

_ => None,

};

let room_power_levels_info = client.user_id().zip(power_levels.as_ref()));

This allows clients to display pending knocking requests in the room list items.

…nt user can act on them That is, if their power level allows them to either invite or kick users.

This has been replaced by `sdk_base::Room::power_levels`, which can also be used from `sdk::Room`

jmartinesp requested a review from a team as a code owner October 25, 2024 14:02

jmartinesp requested review from stefanceriu and removed request for a team October 25, 2024 14:02

stefanceriu approved these changes Oct 28, 2024

View reviewed changes

jmartinesp force-pushed the misc/add-knock-to-latest-events branch from 7df1d4a to c79eaf0 Compare October 29, 2024 08:30

jmartinesp requested a review from stefanceriu October 29, 2024 08:31

jmartinesp commented Oct 29, 2024

View reviewed changes

stefanceriu reviewed Oct 29, 2024

View reviewed changes

bnjbvr self-requested a review October 29, 2024 09:00

bnjbvr approved these changes Oct 29, 2024

View reviewed changes

jmartinesp force-pushed the misc/add-knock-to-latest-events branch from c79eaf0 to c97b36a Compare October 29, 2024 11:38

jmartinesp added 3 commits October 29, 2024 12:40

feat(room_list): allow knock state event as latest_event

985d175

This allows clients to display pending knocking requests in the room list items.

refactor(room_list): only display the knock state events if the curre…

682a373

…nt user can act on them That is, if their power level allows them to either invite or kick users.

refactor(room): remove sdk::Room::room_power_levels function

61f48ba

This has been replaced by `sdk_base::Room::power_levels`, which can also be used from `sdk::Room`

jmartinesp force-pushed the misc/add-knock-to-latest-events branch from c97b36a to 61f48ba Compare October 29, 2024 11:40

jmartinesp enabled auto-merge (rebase) October 29, 2024 11:47

jmartinesp merged commit 0353583 into main Oct 29, 2024
39 checks passed

jmartinesp deleted the misc/add-knock-to-latest-events branch October 29, 2024 11:55

jmartinesp mentioned this pull request Oct 30, 2024

[Story] Receiving requests to join element-hq/element-meta#2579

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(room_list): allow knock state event as `latest_event` #4170

chore(room_list): allow knock state event as `latest_event` #4170

jmartinesp commented Oct 25, 2024

codecov bot commented Oct 25, 2024 •

edited

Loading

stefanceriu left a comment

jmartinesp commented Oct 29, 2024

jmartinesp Oct 29, 2024

bnjbvr Oct 29, 2024

stefanceriu Oct 29, 2024

jmartinesp Oct 29, 2024

stefanceriu Oct 29, 2024

jmartinesp Oct 29, 2024

stefanceriu Oct 29, 2024

jmartinesp Oct 29, 2024

bnjbvr commented Oct 29, 2024

bnjbvr left a comment

bnjbvr Oct 29, 2024

bnjbvr Oct 29, 2024

bnjbvr Oct 29, 2024

bnjbvr Oct 29, 2024

	/// This state event is suitable - it's a knock membership change
	/// This state event is suitable - it's a knock membership change
	/// that can be handled by the current user.

chore(room_list): allow knock state event as latest_event #4170

chore(room_list): allow knock state event as latest_event #4170

Conversation

jmartinesp commented Oct 25, 2024

codecov bot commented Oct 25, 2024 • edited Loading

Codecov Report

stefanceriu left a comment

Choose a reason for hiding this comment

jmartinesp commented Oct 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnjbvr commented Oct 29, 2024

bnjbvr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chore(room_list): allow knock state event as `latest_event` #4170

chore(room_list): allow knock state event as `latest_event` #4170

codecov bot commented Oct 25, 2024 •

edited

Loading