Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6385 +/- ##
==========================================
- Coverage 89.87% 89.85% -0.03%
==========================================
Files 378 378
Lines 103445 103552 +107
Branches 103445 103552 +107
==========================================
+ Hits 92973 93047 +74
- Misses 6902 6922 +20
- Partials 3570 3583 +13 ☔ View full report in Codecov by Sentry. |
fb8bd4d to
2cf6859
Compare
| const DEFAULT_ROOM_SUBSCRIPTION_EXTRA_REQUIRED_STATE: &[(StateEventType, &str)] = | ||
| &[(StateEventType::RoomPinnedEvents, "")]; | ||
| const DEFAULT_ROOM_SUBSCRIPTION_EXTRA_REQUIRED_STATE: &[(StateEventType, &str)] = &[ | ||
| (StateEventType::RoomPinnedEvents, ""), | ||
| ]; |
There was a problem hiding this comment.
This feels unnecessary, ai? 🤔
There was a problem hiding this comment.
Yeah I think I had first added the beacon_info to this list instead
| pub struct LiveLocationShares; | ||
|
|
||
| impl LiveLocationShares { |
There was a problem hiding this comment.
I think I would've preferred it if this service would work on top of the ObservableVector and setup its observers in the constructor, similar to all the other list based services we're building, for consistency.
A lot of these services live on the UI crate level but room_directory_search makes use of it on the sdk crate level.
There was a problem hiding this comment.
I thought we said we didn't want ObservableVector for such api? But happy to change if you think it's better.
There was a problem hiding this comment.
Ah, I see what caused the confusing: I though you were asking if we should bother tracking atomic updates and I thought it's fine, we can just reload the whole list every time. All while still using ObservableVector e.g. space filters
matrix-rust-sdk/crates/matrix-sdk-ui/src/spaces/mod.rs
Lines 488 to 489 in eb51c86
I think it would be great if you can change it so the handlers, the helpers and everything else is contained withing this LiveLocationShares service 🙏
There was a problem hiding this comment.
Ok I see! I've been using Stream api as it's also used in other places in the sdk.
crates/matrix-sdk/src/room/mod.rs
Outdated
| /// Get all currently active live location shares in the room. | ||
| /// | ||
| /// Returns a list of all users who are currently sharing their live | ||
| /// location (excluding the current user), along with their latest known | ||
| /// location if available in the event cache. | ||
| pub async fn get_active_live_location_shares( | ||
| &self, | ||
| ) -> Result<Vec<LiveLocationShare>> { | ||
| crate::live_location_share::get_active_live_location_shares(self).await | ||
| } | ||
|
|
There was a problem hiding this comment.
I would expected all usages to go through LiveLocationShares, not bypass that whole layer. (See my other comment about ObservableVector)
# Conflicts: # bindings/matrix-sdk-ffi/CHANGELOG.md
7baa551 to
1ab091a
Compare
|
Ok @stefanceriu the PR is ready to re-review I think :D |
stefanceriu
left a comment
There was a problem hiding this comment.
This is looking great, just a couple of small comments!
I'll be out for a bit so I'm going to let the review gods choose somebody else to help you out with a merge (squash 🙏 ) next week 🚀.
| /// The listener is called immediately with the current list of shares as a | ||
| /// `Reset` update, then called again with incremental updates whenever the | ||
| /// list changes (location updates, shares starting/stopping). | ||
| pub async fn subscribe_to_live_location_shares( |
There was a problem hiding this comment.
Should this just return the LiveLocationShares "service" so the final user can do whatever they need with it? (similar to the ThreadListService, SpaceService etc.?
| user_id: event.user_id.to_string(), | ||
| }]) | ||
| // Keep live_location_shares alive to preserve event handlers. | ||
| let _live_location_shares = live_location_shares; |
There was a problem hiding this comment.
It's probably best we let the final user worry about lifetimes.
| pub location: LocationContent, | ||
| /// A timestamp in milliseconds since Unix Epoch on that day in local | ||
| /// time. | ||
| /// A timestamp in milliseconds since Unix Epoch on that day in local time. |
There was a problem hiding this comment.
This is actually origin_server_ts as far as I can tell so not sure what local times means.
| /// | ||
| /// Returns an error if the event is redacted, stripped, not found or could | ||
| /// not be deserialized. | ||
| pub(crate) async fn get_user_beacon_info( |
There was a problem hiding this comment.
I wonder if we should just fold this into LiveLocationShares now 🤔
| pub beacon_info: Option<BeaconInfoEventContent>, | ||
| /// The user ID of the person sharing their live location. | ||
| pub user_id: OwnedUserId, | ||
| /// The user's last known location, if any beacon has been received. |
There was a problem hiding this comment.
Techincally the "asset"'s location
Motivation
The previous observe_live_location_shares API was limited: it emitted individual beacon events one at a time, excluded the own user's shares, provided beacon_info only as an Option (requiring a state fetch per
event), and had no awareness of share start/stop lifecycle.
What changed
New stream semantics (matrix-sdk)
Room::observe_live_location_shares is replaced by Room::subscribe_to_live_location_shares. The new method returns a Stream<Item = Vec> that always emits the full snapshot of currently active
shares:
FFI bindings (matrix-sdk-ffi)
Room::observe_live_location_shares is similarly replaced by Room::subscribe_to_live_location_shares, which takes a LiveLocationShareListener callback (new interface) called with the full updated
Vec on every change.
CHANGELOG.mdfiles.Signed-off-by: