Tags: introduce RoomNotableTags #3071
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3071 +/- ##
==========================================
- Coverage 83.70% 83.69% -0.01%
==========================================
Files 222 222
Lines 23357 23385 +28
==========================================
+ Hits 19550 19573 +23
- Misses 3807 3812 +5 ☔ View full report in Codecov by Sentry. |
bindings/matrix-sdk-ffi/src/room.rs
Outdated
| #[derive(uniffi::Record)] | ||
| pub struct RoomNotableTags { | ||
| is_favorite: bool, | ||
| } | ||
| impl From<SdkNotableTags> for RoomNotableTags { | ||
| fn from(value: SdkNotableTags) -> Self { | ||
| RoomNotableTags { is_favorite: value.is_favorite } | ||
| } | ||
| } |
There was a problem hiding this comment.
This seems like it could be replaced by a #[cfg_attr(feature = "uniffi", derive(uniffi::Record)] on the SDK type.
There was a problem hiding this comment.
Oh, I didn't know we're able to expose directly sdk type! It's not done anywhere else, so not sure we want that? @bnjbvr what do you think?
There was a problem hiding this comment.
Yeah it's only since recently that this is possible. We do it for two or three enum types, one here.
There was a problem hiding this comment.
@ganfra looks fine to me. (This is the reason why the namespacing on the Kotlin-side is different for those structs, btw :-))
bindings/matrix-sdk-ffi/src/room.rs
Outdated
| } | ||
| impl From<SdkNotableTags> for RoomNotableTags { |
There was a problem hiding this comment.
nit: let the code breathe please :-)
| } | |
| impl From<SdkNotableTags> for RoomNotableTags { | |
| } | |
| impl From<SdkNotableTags> for RoomNotableTags { |
crates/matrix-sdk-base/src/client.rs
Outdated
| if let Some(room) = self.store.get_room(room_id) { | ||
| let tags = if let Some(AnyRoomAccountDataEvent::Tag(event)) = room_account_data | ||
| .get(&RoomAccountDataEventType::Tag) | ||
| .and_then(|r| r.deserialize().ok()) |
There was a problem hiding this comment.
A failure to deserialize will be silent. Can you expand this code a bit so that we log failures to deserialize, please?
| } | ||
| /// Returns the current RoomNotableTags and subscribe to changes. |
There was a problem hiding this comment.
| } | |
| /// Returns the current RoomNotableTags and subscribe to changes. | |
| } | |
| /// Returns the current RoomNotableTags and subscribe to changes. |
| } | ||
| /// Returns the current RoomNotableTags and subscribe to changes. | ||
| pub async fn notable_tags_stream(&self) -> (RoomNotableTags, Subscriber<RoomNotableTags>) { | ||
| let current_tags = self.tags().await.unwrap_or(None); |
There was a problem hiding this comment.
nit: Result::unwrap_or(self, None) is equivalent to Result::ok(self), but it would be nice to either log the error here, or let the caller know that we failed by returning a Result<> ourselves.
| /// Computes the provided tags to create a `RoomNotableTags` instance. | ||
| pub fn new(tags: Option<Tags>) -> Self { | ||
| RoomNotableTags { | ||
| is_favorite: tags.map(|tag| tag.contains_key(&TagName::Favorite)).unwrap_or(false), |
There was a problem hiding this comment.
nit: map(...).unwrap_or(x) can be rewritten as map_or(x, ...)
There was a problem hiding this comment.
Here it could even be: .is_some_and(...).
| let (_, mut notable_tags_subscriber) = room.notable_tags_stream().await; | ||
| stream_assert::assert_pending!(notable_tags_subscriber); | ||
| let notable_tags = RoomNotableTags::new(None); | ||
| room.set_notable_tags(notable_tags); | ||
| use futures_util::FutureExt as _; | ||
| assert!(notable_tags_subscriber.next().now_or_never().is_some()); | ||
| stream_assert::assert_pending!(notable_tags_subscriber); |
There was a problem hiding this comment.
here come the blank lines... 😁
| let (_, mut notable_tags_subscriber) = room.notable_tags_stream().await; | |
| stream_assert::assert_pending!(notable_tags_subscriber); | |
| let notable_tags = RoomNotableTags::new(None); | |
| room.set_notable_tags(notable_tags); | |
| use futures_util::FutureExt as _; | |
| assert!(notable_tags_subscriber.next().now_or_never().is_some()); | |
| stream_assert::assert_pending!(notable_tags_subscriber); | |
| let (_, mut notable_tags_subscriber) = room.notable_tags_stream().await; | |
| stream_assert::assert_pending!(notable_tags_subscriber); | |
| let notable_tags = RoomNotableTags::new(None); | |
| room.set_notable_tags(notable_tags); | |
| use futures_util::FutureExt as _; | |
| assert!(notable_tags_subscriber.next().now_or_never().is_some()); | |
| stream_assert::assert_pending!(notable_tags_subscriber); |
| let mut changes = StateChanges::new("".to_owned()); | ||
| let tag_json: &Value = &test_json::TAG; | ||
| let tag_raw = Raw::new(tag_json).unwrap().cast(); | ||
| let tag_event = tag_raw.deserialize().unwrap(); | ||
| changes.add_room_account_data(room.room_id(), tag_event, tag_raw); | ||
| store.save_changes(&changes).await.unwrap(); | ||
| let (initial_notable_tags, _) = room.notable_tags_stream().await; | ||
| assert!(initial_notable_tags.is_favorite); |
There was a problem hiding this comment.
| let mut changes = StateChanges::new("".to_owned()); | |
| let tag_json: &Value = &test_json::TAG; | |
| let tag_raw = Raw::new(tag_json).unwrap().cast(); | |
| let tag_event = tag_raw.deserialize().unwrap(); | |
| changes.add_room_account_data(room.room_id(), tag_event, tag_raw); | |
| store.save_changes(&changes).await.unwrap(); | |
| let (initial_notable_tags, _) = room.notable_tags_stream().await; | |
| assert!(initial_notable_tags.is_favorite); | |
| let mut changes = StateChanges::new("".to_owned()); | |
| let tag_json: &Value = &test_json::TAG; | |
| let tag_raw = Raw::new(tag_json).unwrap().cast(); | |
| let tag_event = tag_raw.deserialize().unwrap(); | |
| changes.add_room_account_data(room.room_id(), tag_event, tag_raw); | |
| store.save_changes(&changes).await.unwrap(); | |
| let (initial_notable_tags, _) = room.notable_tags_stream().await; | |
| assert!(initial_notable_tags.is_favorite); |
|
2 new commits :
|
|
(Note: clippy isn't happy) |
ganfra
force-pushed
the
fga/room_notable_tags
branch
from
fd7aea7
to
6669dab
Compare
Handles "Expose a way to have is_favorite: bool live in the bindings" from #3005