-
Notifications
You must be signed in to change notification settings - Fork 252
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: add support for listening to stream of live location updates #4025
base: main
Are you sure you want to change the base?
Conversation
Seems that the test aren't particularly happy with this PR. Can you please have a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. I've left some feedback that can impact the global design of your patch.
3b574dd
to
7066fb3
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ 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 ☔ View full report in Codecov by Sentry. |
29a2552
to
4cc5b45
Compare
Updated with more comprehensive tests and ready for a re-review thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we are almost done! Can you rebase your git
history please?
I've just merged #4253, and I believe it might interest you, or may your code a bit simpler. What do you think? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is still issues with the code (see comments). However, I suggest you to use #4253, where you can return your own subscriber, in which the produced stream can map the stream from EventHandlerSubscriber
to generate a LiveLocationShare
. See StreamExt::map
.
#[derive(Debug)] | ||
pub struct LiveLocationSubscription { | ||
/// Manages the event handler lifecycle. | ||
pub event_handler_handle: EventHandlerHandle, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The caller has to take care of removing the event handler. With #4253, it becomes automatic/invisible for the caller.
} | ||
|
||
impl Drop for LiveLocationSubscription { | ||
fn drop(&mut self) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's where you would have to call Client::remove_event_handler
. You can use a EventHandlerDropGuard
though, that would be this thing automatic too. If you use #4253, this is no longer required as you don't need to store a receiver
and the event_handler_handle
.
Would you be open to merging this as-is using the existing Recent commit makes use of the |
c2e3cf4
to
e21d9fb
Compare
e21d9fb
to
05c1172
Compare
Conversation related to this issue can be found here
This merge request adds
subscribe_to_live_location_shares
to thematrix_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.