matrix-rust-sdk icon indicating copy to clipboard operation
matrix-rust-sdk copied to clipboard

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

Open torrybr opened this issue 1 year ago • 2 comments

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.

torrybr avatar Sep 22 '24 15:09 torrybr

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

stefanceriu avatar Sep 24 '24 08:09 stefanceriu

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 85.01%. Comparing base (cefd5a2) to head (e10d8eb). Report is 175 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4025   +/-   ##
=======================================
  Coverage   85.00%   85.01%           
=======================================
  Files         274      275    +1     
  Lines       29945    29951    +6     
=======================================
+ Hits        25456    25463    +7     
+ Misses       4489     4488    -1     

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

codecov[bot] avatar Sep 24 '24 14:09 codecov[bot]

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

torrybr avatar Nov 10 '24 20:11 torrybr

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?

Hywan avatar Nov 13 '24 10:11 Hywan

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.

torrybr avatar Nov 14 '24 22:11 torrybr

See https://github.com/torrybr/matrix-rust-sdk/pull/1, which does most of the job.

Hywan avatar Nov 19 '24 08:11 Hywan

Updated and rebased with the newest approach for using an observable stream. Appreciate the help and discussion around this PR, thanks!

torrybr avatar Nov 20 '24 21:11 torrybr