Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tags : introduce set_is_favorite and set_is_low_priority #3075

Merged
merged 4 commits into from
Feb 8, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions bindings/matrix-sdk-ffi/src/room.rs
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,14 @@ impl Room {
})))
}

pub async fn update_notable_tags(
&self,
notable_tags: RoomNotableTags,
) -> Result<(), ClientError> {
self.inner.update_notable_tags(notable_tags).await?;
Ok(())
}

/// Redacts an event from the room.
///
/// # Arguments
Expand Down
7 changes: 6 additions & 1 deletion crates/matrix-sdk-base/src/rooms/normal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -655,11 +655,16 @@ impl Room {

/// Returns the current RoomNotableTags and subscribe to changes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, use:

- RoomNotableTags
+ [`RoomNotableTags`]

to first put RoomNotableTags in a <code>, and then to create a link to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Hywan I let you handle those changes on your own. There is tons of places where [ ] notation is not used (almost everywhere actually).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is your code. Can you fix it please? I'll handle the others, but please handle it in your code.

pub async fn notable_tags_stream(&self) -> (RoomNotableTags, Subscriber<RoomNotableTags>) {
(self.current_notable_tags().await, self.notable_tags.subscribe())
}

/// Return the current RoomNotableTags extracted from store.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Return the current RoomNotableTags extracted from store.
/// Return the current [`RoomNotableTags`] extracted from store.

pub async fn current_notable_tags(&self) -> RoomNotableTags {
let current_tags = self.tags().await.unwrap_or_else(|e| {
warn!("Failed to get tags from store: {}", e);
None
});
(RoomNotableTags::new(current_tags), self.notable_tags.subscribe())
RoomNotableTags::new(current_tags)
}

/// Get the `RoomMember` with the given `user_id`.
Expand Down
16 changes: 15 additions & 1 deletion crates/matrix-sdk/src/room/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use matrix_sdk_base::{
},
instant::Instant,
store::StateStoreExt,
RoomMemberships, StateChanges,
RoomMemberships, RoomNotableTags, StateChanges,
};
use matrix_sdk_common::timeout::timeout;
use mime::Mime;
Expand Down Expand Up @@ -937,6 +937,20 @@ impl Room {
self.client.send(request, None).await
}

/// Update the tags from the room by calling set_tag or remove_tag method if
/// needed.
pub async fn update_notable_tags(&self, notable_tags: RoomNotableTags) -> Result<()> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is best to do like this for the future, or offer method for each tags like
set_is_favorite(bool)
set_is_low_priority(bool)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Me neither. @bnjbvr do you have an opinion?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't find it likely to change multiple ones at the same time, this shouldn't happen quite often so performance isn't critical, and having a single method for each notable tag makes for a better API IMO, so I'd in favor of having set_is_... methods 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it's worth checking the local value tags before sending the request?
I guess we can have some race condition (especially with this issue) which could prevent the request to happen when it should.

let current_notable_tags = self.inner.current_notable_tags().await;
if current_notable_tags.is_favorite != notable_tags.is_favorite {
if notable_tags.is_favorite {
self.set_tag(TagName::Favorite, TagInfo::default()).await?;
} else {
self.remove_tag(TagName::Favorite).await?;
}
}
Ok(())
}

/// Sets whether this room is a DM.
///
/// When setting this room as DM, it will be marked as DM for all active
Expand Down
84 changes: 81 additions & 3 deletions crates/matrix-sdk/tests/integration/room/joined.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,16 @@ use matrix_sdk::{
config::SyncSettings,
room::{Receipts, ReportedContentScore},
};
use matrix_sdk_base::RoomState;
use matrix_sdk_test::{async_test, test_json, DEFAULT_TEST_ROOM_ID};
use matrix_sdk_base::{RoomNotableTags, RoomState};
use matrix_sdk_test::{
async_test, test_json, JoinedRoomBuilder, RoomAccountDataTestEvent, SyncResponseBuilder,
DEFAULT_TEST_ROOM_ID,
};
use ruma::{
api::client::{membership::Invite3pidInit, receipt::create_receipt::v3::ReceiptType},
assign, event_id,
events::{receipt::ReceiptThread, room::message::RoomMessageEventContent},
int, mxc_uri, owned_event_id, thirdparty, uint, user_id, TransactionId,
int, mxc_uri, owned_event_id, room_id, thirdparty, uint, user_id, TransactionId,
};
use serde_json::json;
use wiremock::{
Expand Down Expand Up @@ -642,3 +645,78 @@ async fn report_content() {

room.report_content(event_id, Some(score), Some(reason.to_owned())).await.unwrap();
}

#[async_test]
async fn update_notable_tags() {
let (client, server) = logged_in_client().await;
let room_id = room_id!("!test:example.org");
let sync_settings = SyncSettings::new().timeout(Duration::from_millis(3000));

let mut ev_builder = SyncResponseBuilder::new();
ev_builder.add_joined_room(JoinedRoomBuilder::new(room_id));
mock_sync(&server, ev_builder.build_json_sync_response(), None).await;
client.sync_once(sync_settings.clone()).await.unwrap();
server.reset().await;

let room = client.get_room(room_id).unwrap();
let (initial_notable_tags, mut notable_tags_stream) = room.notable_tags_stream().await;
// No tags set up in the sync response, so no favorite.
assert!(!initial_notable_tags.is_favorite);

// Makes sure that the right endpoint is called when updating the notable tags
Mock::given(method("PUT"))
.and(path_regex(r"^/_matrix/client/r0/user/.*/rooms/.*/tags/m\.favourite"))
.and(header("authorization", "Bearer 1234"))
.respond_with(ResponseTemplate::new(200).set_body_json(&*test_json::EMPTY))
.expect(1)
.mount(&server)
.await;

let notable_tags = RoomNotableTags { is_favorite: true };
room.update_notable_tags(notable_tags).await.unwrap();

ev_builder.add_joined_room(
JoinedRoomBuilder::new(room_id).add_account_data(RoomAccountDataTestEvent::Tags),
);
mock_sync(&server, ev_builder.build_json_sync_response(), None).await;
client.sync_once(sync_settings.clone()).await.unwrap();
server.reset().await;
// ensure the notable tags stream is updated when the sync response is received
assert!(notable_tags_stream.next().await.unwrap().is_favorite);

Mock::given(method("DELETE"))
.and(path_regex(r"^/_matrix/client/r0/user/.*/rooms/.*/tags/m\.favourite"))
.and(header("authorization", "Bearer 1234"))
.respond_with(ResponseTemplate::new(200).set_body_json(&*test_json::EMPTY))
.expect(1)
.mount(&server)
.await;

let notable_tags = RoomNotableTags { is_favorite: false };
room.update_notable_tags(notable_tags).await.unwrap();

ev_builder.add_joined_room(JoinedRoomBuilder::new(room_id).add_account_data(
RoomAccountDataTestEvent::Custom(json!({
"content": {
"tags": {}
},
"type": "m.tag"
})),
));
mock_sync(&server, ev_builder.build_json_sync_response(), None).await;
client.sync_once(sync_settings.clone()).await.unwrap();
server.reset().await;
assert!(!notable_tags_stream.next().await.unwrap().is_favorite);

// Ensure the endpoint is not called when the value doesn't change
Mock::given(method("DELETE"))
.and(path_regex(r"^/_matrix/client/r0/user/.*/rooms/.*/tags/m\.favourite"))
.and(header("authorization", "Bearer 1234"))
.respond_with(ResponseTemplate::new(200).set_body_json(&*test_json::EMPTY))
.expect(0)
.mount(&server)
.await;

let notable_tags = RoomNotableTags { is_favorite: false };
room.update_notable_tags(notable_tags).await.unwrap();
}
Loading