search

matrix-org / matrix-rust-sdk

Matrix Client-Server SDK for Rust
Apache License 2.0
1.26k stars 252 forks source link

sdk: add support for listening to stream of live location updates #4025

Open torrybr opened 1 month ago

torrybr commented 1 month ago

Conversation related to this issue can be found here

This merge request adds subscribe_to_live_location_shares to the matrix_sdk::Room that allows clients to listen for beacon updates using a background task. The method provides an easy way to subscribe to live location sharing events within a room, handling event processing internally.

Follow-up tasks will include adding support for the event cache.

stefanceriu commented 1 month ago

Seems that the test aren't particularly happy with this PR. Can you please have a look?

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 90.90909% with 2 lines in your changes missing coverage. Please review.

Project coverage is 85.02%. Comparing base (cefd5a2) to head (05c1172).

Files with missing lines Patch % Lines
crates/matrix-sdk/src/room/mod.rs 90.90% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #4025 +/- ## ========================================== + Coverage 85.00% 85.02% +0.01% ========================================== Files 274 274 Lines 29945 29964 +19 ========================================== + Hits 25456 25477 +21 + Misses 4489 4487 -2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

torrybr commented 5 days ago

Updated with more comprehensive tests and ready for a re-review thanks

Hywan commented 3 days ago

I've just merged https://github.com/matrix-org/matrix-rust-sdk/pull/4253, and I believe it might interest you, or may your code a bit simpler. What do you think?

torrybr commented 1 day ago

I've just merged #4253, and I believe it might interest you, or may your code a bit simpler. What do you think?

Would you be open to merging this as-is using the existing .add_event_listener approach? I’d like to revisit it next week to refactor based on your new implementation, but it may take me some time to fully get up to speed with the new process. Given that this has been open for a while, merging it now would provide me a solid foundation for completing the rest of the location-sharing feature!

Recent commit makes use of the EventHandlerDropGuard as suggested and keeps it a little simpler without breaking it out.