sdk-base: add `Room::pinned_events(&self)` #3747

jmartinesp · 2024-07-22T10:22:08Z

This can be used to subscribe to pinned event ids in a room.

Implements the first part of #3726.

Public API changes documented in changelogs (optional)

Signed-off-by:

codecov · 2024-07-22T12:25:51Z

Codecov Report

Attention: Patch coverage is 62.50000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 84.02%. Comparing base (99da0ff) to head (3e78ee7).

Files	Patch %	Lines
crates/matrix-sdk-base/src/rooms/mod.rs	40.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3747      +/-   ##
==========================================
- Coverage   84.02%   84.02%   -0.01%     
==========================================
  Files         260      260              
  Lines       26729    26737       +8     
==========================================
+ Hits        22459    22465       +6     
- Misses       4270     4272       +2

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

bnjbvr

Sounds like you're on the right track! Thanks for writing a test.

bnjbvr · 2024-07-22T13:07:24Z

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

@@ -340,6 +340,9 @@ pub struct StateChanges {
    /// A map from room id to a map of a display name and a set of user ids that
    /// share that display name in the given room.
    pub ambiguity_maps: BTreeMap<OwnedRoomId, BTreeMap<String, BTreeSet<OwnedUserId>>>,
+
+    /// A map of `RoomId` to pinned `OwnedEventId`s for each room.
+    pub pinned_events: BTreeMap<OwnedRoomId, Vec<OwnedEventId>>,


I don't think this should be here: this is not stored, and this struct is meant to include things that are to be stored. Instead, you probably want a local variable in the one function that makes use of this pinned_events field.

Removed this in eafed32

bnjbvr · 2024-07-22T13:07:44Z

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

+
+    /// The list of pinned event ids in this room in an observable format so
+    /// they can be subscribed to.
+    pub pinned_event_ids: SharedObservable<Vec<OwnedEventId>>,


nit: not pub, since there's a getter for a subscriber

Also, would it make sense to use ObservableVector here? If we did that, and rejigger the API so that we only push new pinned event / remove pinned event, we could have a finer-grained API, but maybe it's not worth it?

I can try 🤞 .

ObservableVector doesn't implement Clone so it can't be used in Room, which does.

bnjbvr · 2024-07-22T13:25:07Z

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

+            if let Some(room) = self.store.room(&room_id) {
+                room.pinned_event_ids.update(|ids| {
+                    ids.clear();
+                    ids.append(&mut pinned_event_ids.clone());


🦀 🧑‍🏫

Vec::extend can take an iterator of the element type (that's ids)

and a Vec also has an IntoIter implementation, so it can be used as the parameter for extend (here I'm talking about pinned_event_ids).

Combining the two, that gives you a way to avoid the cloning:

Suggested change

ids.append(&mut pinned_event_ids.clone());

ids.extend(pinned_event_ids);

I had to use Vector::extend_from_slice in some places instead, but it's done in 8c43c75

bnjbvr · 2024-07-22T13:28:22Z

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

@@ -1173,6 +1180,15 @@ impl BaseClient {
            }
        }

+        for (room_id, pinned_event_ids) in changes.pinned_events.clone() {


nit: you shouldn't need the clone() here, and as said before changes.pinned_events should be a local variable

Removed in 8c43c75

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

jmartinesp · 2024-07-22T15:06:58Z

I had to add an extra check so the pinned event ids are only emitted if they are different from the previous list.

bnjbvr · 2024-07-22T15:21:11Z

I had to add an extra check so the pinned event ids are only emitted if they are different from the previous list.

Sweet, did you add a test for that too? 🥺

jmartinesp · 2024-07-22T15:30:43Z

I had to add an extra check so the pinned event ids are only emitted if they are different from the previous list.

Sweet, did you add a test for that too? 🥺

The test for this is the previously added one not failing anymore 😅 .

jmartinesp · 2024-07-23T06:32:21Z

I actually ended up adding an extra test check and improving how the new values are set.

bnjbvr · 2024-07-23T09:05:35Z

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

+    /// The list of pinned event ids in this room in an observable format so
+    /// they can be subscribed to.
+    pub(crate) pinned_event_ids: SharedObservable<Vec<OwnedEventId>>,


So the reason why we usually don't put things in this struct is that they're not persisted after a reload; if you reload a room, you'd have to wait for the next sync to arrive to give you the list of events, and if you happen to be offline, that means that now you don't have access to pinned events.

From here, we have a few possibilities:

include the Vec<OwnedEventId> in RoomInfo,

include the pinned event in the BaseRoomInfo, and have a getter to read the pinned event ids from there

have a new (de)serializable field in Room that is filled at startup, and updated as the events come. You've done half of the work here by having it be updated as the events come, and we'd need some way to serialize/deserialize it too later.

My preference would likely go to the first or second option (second option might be faster to ser/de), to benefit from persistence. Can you also add a test that would've failed without this change, please?

Ok, I tried fixing this in 4aa51ca using the 2nd strategy 🤞 .

jmartinesp · 2024-07-23T11:24:25Z

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

+            AnyStrippedStateEvent::RoomPinnedEvents(p) => {
+                if let Some(pinned) = p.content.pinned.clone() {
+                    self.pinned_events = Some(RoomPinnedEventsEventContent::new(pinned));
+                }
+            }


I'm not entirely sure about this part.

Stripped events = for a room in which we're invited. I think your code is fine, we could also decide to store None there, but this shouldn't matter much 👍 I don't expect one to be able to look at pinned events in an invited room, at least for a first pass.

bnjbvr

Thanks!

bnjbvr · 2024-07-23T14:24:44Z

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

+            AnyStrippedStateEvent::RoomPinnedEvents(p) => {
+                if let Some(pinned) = p.content.pinned.clone() {
+                    self.pinned_events = Some(RoomPinnedEventsEventContent::new(pinned));
+                }
+            }


Stripped events = for a room in which we're invited. I think your code is fine, we could also decide to store None there, but this shouldn't matter much 👍 I don't expect one to be able to look at pinned events in an invited room, at least for a first pass.

bnjbvr · 2024-07-23T14:27:12Z

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

@@ -117,6 +118,8 @@ pub struct BaseRoomInfo {
    /// others, and this field collects them.
    #[serde(skip_serializing_if = "RoomNotableTags::is_empty", default)]
    pub(crate) notable_tags: RoomNotableTags,
+    /// The `m.room.pinned_events` of this room.
+    pub(crate) pinned_events: Option<RoomPinnedEventsEventContent>,


Note to self: i think we could have used MinimalStateEvent here, but it's unclear what's the value of it, so no need to change anything here.

I can try changing it so it matches the other fields if it's easy to do (I'm not sure how I missed it 🫤 ).

Actually, it does make everything way more complex just to store a possible event id which isn't used anywhere, so it's probably not worth to add it.

Also add `Room::pinned_events(&self)` to get the current pinned events at any time.

jmartinesp requested a review from a team as a code owner July 22, 2024 10:22

jmartinesp requested review from bnjbvr and removed request for a team July 22, 2024 10:22

jmartinesp force-pushed the jme/feat/reactive-pinned-events branch from b08d07f to 1750b00 Compare July 22, 2024 12:11

bnjbvr requested changes Jul 22, 2024

View reviewed changes

jmartinesp force-pushed the jme/feat/reactive-pinned-events branch 2 times, most recently from 8c43c75 to 0d76899 Compare July 22, 2024 14:56

jmartinesp requested a review from bnjbvr July 22, 2024 14:56

jmartinesp mentioned this pull request Jul 23, 2024

sdk & ffi: add pin_event and unpin_event fns, as well as power level checks #3750

Merged

1 task

bnjbvr requested changes Jul 23, 2024

View reviewed changes

jmartinesp commented Jul 23, 2024

View reviewed changes

jmartinesp requested a review from bnjbvr July 23, 2024 12:53

bnjbvr approved these changes Jul 23, 2024

View reviewed changes

sdk-base: add pinned events to BaseRoomInfo to keep track of them.

3e78ee7

Also add `Room::pinned_events(&self)` to get the current pinned events at any time.

jmartinesp force-pushed the jme/feat/reactive-pinned-events branch from 4aa51ca to 3e78ee7 Compare July 23, 2024 15:03

jmartinesp enabled auto-merge (rebase) July 23, 2024 15:07

jmartinesp merged commit 79010af into main Jul 23, 2024
40 checks passed

jmartinesp deleted the jme/feat/reactive-pinned-events branch July 23, 2024 15:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sdk-base: add `Room::pinned_events(&self)` #3747

sdk-base: add `Room::pinned_events(&self)` #3747

jmartinesp commented Jul 22, 2024 •

edited

Loading

codecov bot commented Jul 22, 2024 •

edited

Loading

bnjbvr left a comment

bnjbvr Jul 22, 2024

jmartinesp Jul 22, 2024

bnjbvr Jul 22, 2024

jmartinesp Jul 22, 2024

jmartinesp Jul 22, 2024

bnjbvr Jul 22, 2024

jmartinesp Jul 22, 2024

bnjbvr Jul 22, 2024

jmartinesp Jul 22, 2024

jmartinesp commented Jul 22, 2024

bnjbvr commented Jul 22, 2024

jmartinesp commented Jul 22, 2024

jmartinesp commented Jul 23, 2024

bnjbvr Jul 23, 2024

jmartinesp Jul 23, 2024

jmartinesp Jul 23, 2024

bnjbvr Jul 23, 2024

bnjbvr left a comment

bnjbvr Jul 23, 2024

bnjbvr Jul 23, 2024

jmartinesp Jul 23, 2024

jmartinesp Jul 23, 2024

	ids.append(&mut pinned_event_ids.clone());
	ids.extend(pinned_event_ids);

sdk-base: add Room::pinned_events(&self) #3747

sdk-base: add Room::pinned_events(&self) #3747

Conversation

jmartinesp commented Jul 22, 2024 • edited Loading

codecov bot commented Jul 22, 2024 • edited Loading

Codecov Report

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

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

jmartinesp commented Jul 22, 2024

bnjbvr commented Jul 22, 2024

jmartinesp commented Jul 22, 2024

jmartinesp commented Jul 23, 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

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

sdk-base: add `Room::pinned_events(&self)` #3747

sdk-base: add `Room::pinned_events(&self)` #3747

jmartinesp commented Jul 22, 2024 •

edited

Loading

codecov bot commented Jul 22, 2024 •

edited

Loading