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

sdk&ffi: server unstable features support for MSC4028 #3192

Merged

Conversation

hanadi92
Copy link
Contributor

@hanadi92 hanadi92 commented Mar 6, 2024

Fixes #3191

Allows support for fetching the unstable_features from /_matrix/clients/versions.
Specifically, to be used for checking the state of org.matrix.msc4028 through ffi to the clients.

P.S. ^ I can split them into two PRs if necessary.

  • Public API changes documented in changelogs (optional)

Signed-off-by: @hanadi92

@hanadi92 hanadi92 requested a review from a team as a code owner March 6, 2024 20:16
@hanadi92 hanadi92 requested review from poljar and removed request for a team March 6, 2024 20:16
@hanadi92 hanadi92 marked this pull request as draft March 6, 2024 20:21
Copy link

codecov bot commented Mar 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.83%. Comparing base (a204b29) to head (d8ffdd5).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3192      +/-   ##
==========================================
+ Coverage   83.81%   83.83%   +0.01%     
==========================================
  Files         232      232              
  Lines       23965    23984      +19     
==========================================
+ Hits        20087    20106      +19     
  Misses       3878     3878              

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

@hanadi92 hanadi92 force-pushed the feat-server-unstable-features-support branch from f78838c to 175fae5 Compare March 6, 2024 20:40
@hanadi92 hanadi92 marked this pull request as ready for review March 6, 2024 20:54
Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

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

Sweet, thanks for writing a patch! Small comments here, I could take care of addressing those if you don't have the time :-)

bindings/matrix-sdk-ffi/src/notification_settings.rs Outdated Show resolved Hide resolved
/// Check whether [MSC 4028 push rule][rule] is enabled on the homeserver.
///
/// [rule]: https://github.com/matrix-org/matrix-spec-proposals/blob/giomfo/push_encrypted_events/proposals/4028-push-all-encrypted-events-except-for-muted-rooms.md
pub async fn can_homeserver_push_encrypted_event_to_device(&self) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

There's a bit of logic here, and I think that this could be useful for other clients. Could this method go to the SDK, next to unstable_features, please? (and add a test for it too 🙏)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

of course! good hint. let me know if what I pushed goes in the direction of ur suggestion 👍

@@ -90,6 +90,7 @@ pub struct ClientBuilder {
request_config: RequestConfig,
respect_login_well_known: bool,
server_versions: Option<Box<[MatrixVersion]>>,
unstable_features: Option<BTreeMap<String, bool>>,
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 unused in ClientBuilder, and can be removed. :-)

@hanadi92
Copy link
Contributor Author

hanadi92 commented Mar 8, 2024

Thanks for the review! Changes are made accordingly. Let me know what you think about them :) I think the Code Coverage is breaking on a non-related issue.

@hanadi92 hanadi92 requested a review from bnjbvr March 8, 2024 08:37
Copy link
Member

@bnjbvr bnjbvr left a comment

Choose a reason for hiding this comment

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

Looks good! I'll try to apply my suggestions or address the latest comments myself. Thank you!

crates/matrix-sdk/src/client/mod.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/client/mod.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/client/mod.rs Outdated Show resolved Hide resolved
Signed-off-by: Benjamin Bouvier <[email protected]>
@bnjbvr bnjbvr merged commit 724d133 into matrix-org:main Mar 8, 2024
34 checks passed
@hanadi92
Copy link
Contributor Author

hanadi92 commented Mar 8, 2024

Looks good! I'll try to apply my suggestions or address the latest comments myself. Thank you!

@bnjbvr Thanks a lot for taking the time in general and doing those changes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing support for fetching homeserver unstable features (ffi MSC4028)
2 participants