sdk: implement MSC3489 live location sharing #3621

torrybr · 2024-06-28T02:46:14Z

Add support for MSC3489 live location sharing. Requesting an initial review to clarify some technical questions and help get this over the finish line.

Building on this Ruma PR

Public API changes documented in changelogs (optional)

Signed-off-by:

crates/matrix-sdk/src/room/mod.rs

crates/matrix-sdk-ui/src/timeline/beacons.rs

bindings/matrix-sdk-ffi/src/timeline/mod.rs

crates/matrix-sdk-ui/src/timeline/event_handler.rs

codecov · 2024-07-01T15:46:33Z

Codecov Report

Attention: Patch coverage is 73.21429% with 15 lines in your changes missing coverage. Please review.

Project coverage is 84.23%. Comparing base (d8b2b74) to head (df93412).
Report is 164 commits behind head on main.

Files	Patch %	Lines
crates/matrix-sdk-ui/src/timeline/beacons.rs	66.66%	8 Missing ⚠️
crates/matrix-sdk-ui/src/timeline/event_handler.rs	88.00%	3 Missing ⚠️
crates/matrix-sdk-base/src/rooms/mod.rs	33.33%	2 Missing ⚠️
crates/matrix-sdk/src/room/mod.rs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3621      +/-   ##
==========================================
- Coverage   84.25%   84.23%   -0.03%     
==========================================
  Files         259      260       +1     
  Lines       26592    26648      +56     
==========================================
+ Hits        22406    22446      +40     
- Misses       4186     4202      +16

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

bnjbvr

Thanks for the PR! supporting live location sharing in the SDK will be grand!

So, I appreciate the amount of code you've written here, and that it might be sufficient to have the basic feature working. However, I think the work here could be split (to make reviews faster) and layered in different ways:

support for the base crate and SDK could go in a separate PR
sending the beacon events themselves could happen in the SDK main crate too (I haven't seen any code for this, I think?)
better handling of beacon events coming before their associated beacon info event could be another PR.
then support for the timeline / FFI bindings

Each PR would also have tests at its own level, to make sure that it's behaving as intended. We're requiring tests for major functionality like this, otherwise it's too big a risk to discover later that it may be broken in subtle ways.

In fact, I think there's also potential for a more ergonomic, higher level API for beacons:

provide a stream to listen to new live location shares (client wide)
for each live location share, a stream to listen to new beacon events, and updates about the beacon info itself (stopped/redacted/etc.)

Then, the timeline could spawn a task, and provide UI updates based on both stream updates.

How does that sound?

crates/matrix-sdk-base/src/rooms/mod.rs

crates/matrix-sdk/src/room/mod.rs

bnjbvr · 2024-07-11T13:06:01Z

crates/matrix-sdk-ui/src/timeline/event_handler.rs

+ user_id: OwnedUserId,
+ ) {
+ let mut beacon_state = BeaconState::new(c, user_id.clone());
+ if let Flow::Remote { event_id, .. } = self.ctx.flow.clone() {


nit: use &self.ctx.flow instead of cloning

bnjbvr · 2024-07-11T13:06:55Z

crates/matrix-sdk-ui/src/timeline/event_handler.rs

+ });
+
+ if !found {
+ warn!("Did not find beacon_info event in timeline items. Adding to pending list");


warn should be kept for non-fatal errors.

Suggested change

warn!("Did not find beacon_info event in timeline items. Adding to pending list");

debug!("Did not find beacon_info event in timeline items. Adding to pending list");

bnjbvr · 2024-07-11T13:17:08Z

bindings/matrix-sdk-ffi/src/timeline/content.rs

+ let Some(location) = beacon_state.last_location() else {
+ return TimelineItemContentKind::FailedToParseMessageLike {
+ event_type: "org.matrix.msc3672.beacon".to_string(),
+ error: "Could not find beacon last location content".to_string(),
+ };
+ };


If the last location was not set, then I think the TimelineItemContent should not exist in the first place.

bnjbvr · 2024-07-11T13:17:50Z

bindings/matrix-sdk-ffi/src/timeline/mod.rs

+ pub async fn send_user_location_beacon(
+ self: Arc<Self>,
+ geo_uri: String,
+ ) -> Result<(), ClientError> {


There's way too much logic in this function; it should not live at the FFI layer, otherwise it's ~impossible to test.

bnjbvr · 2024-07-11T13:18:45Z

crates/matrix-sdk-ui/src/timeline/beacons.rs

+//! This module handles rendering of MSC3489 live location sharing in the
+//! timeline.


I don't think having this in the timeline is the proper cutting point. Instead, this would be more useful to have at the SDK layer, and maybe even with a slightly different API.

bnjbvr · 2024-07-11T13:26:02Z

Also, we'd need some way to implement redaction of all the beacon events related to a beacon info event, as indicated by the MSC.

torrybr · 2024-07-12T01:02:28Z

@bnjbvr Thanks for taking time to provide such detailed feedback! I agree with all your points and can work on splitting this up into simpler PRs and layer it as described. The stream API is good approach and would make life a lot easier 😃

torrybr · 2024-07-18T18:27:00Z

In fact, I think there's also potential for a more ergonomic, higher level API for beacons:

provide a stream to listen to new live location shares (client wide)

for each live location share, a stream to listen to new beacon events, and updates about the beacon info itself
(stopped/redacted/etc.)

@bnjbvr Do you have an example of streams being used in the codebase? This should help me get some more direction once I get to this.

bnjbvr · 2024-07-22T08:32:36Z

@bnjbvr Do you have an example of streams being used in the codebase? This should help me get some more direction once I get to this.

You can look for functions which names end up with _stream :)

torrybr · 2024-08-06T21:28:10Z

@bnjbvr I'm working on handling beacon events and associating them with their beacon info events, as mentioned in

better handling of beacon events coming before their associated beacon info event could be another PR

Any guidance on where to place this logic specifically where I should be grabbing the beacon events from?

bnjbvr · 2024-08-14T08:48:28Z

@bnjbvr I'm working on handling beacon events and associating them with their beacon info events, as mentioned in

better handling of beacon events coming before their associated beacon info event could be another PR

Any guidance on where to place this logic specifically where I should be grabbing the beacon events from?

Sweet! I suppose that could go in a subsequent step, when it's time to display/render the beacons themselves, in the timeline. As far as I understand, the support should be pretty minimal/straightforward in the SDK, and ordering is only important as soon as you want to display something, right? Happy to chat about this in the Matrix SDK dev room, if needs be.

torrybr commented Jun 28, 2024

View reviewed changes

crates/matrix-sdk/src/room/mod.rs Show resolved Hide resolved

crates/matrix-sdk/src/room/mod.rs Show resolved Hide resolved

crates/matrix-sdk-ui/src/timeline/beacons.rs Show resolved Hide resolved

torrybr commented Jun 28, 2024

View reviewed changes

bindings/matrix-sdk-ffi/src/timeline/mod.rs Show resolved Hide resolved

torrybr commented Jun 28, 2024

View reviewed changes

crates/matrix-sdk-ui/src/timeline/event_handler.rs Show resolved Hide resolved

torrybr marked this pull request as ready for review July 1, 2024 12:55

torrybr requested a review from a team as a code owner July 1, 2024 12:55

torrybr requested review from poljar and removed request for a team July 1, 2024 12:55

torrybr changed the title ~~Draft: Support for MSC3489 live location sharing~~ Support for MSC3489 live location sharing Jul 1, 2024

torrybr marked this pull request as draft July 1, 2024 13:07

torrybr changed the title ~~Support for MSC3489 live location sharing~~ Draft: Support for MSC3489 live location sharing Jul 1, 2024

torrybr marked this pull request as ready for review July 1, 2024 15:31

torrybr changed the title ~~Draft: Support for MSC3489 live location sharing~~ Support for MSC3489 live location sharing Jul 1, 2024

torrybr force-pushed the feat/live-location branch from 5785d01 to 33816d2 Compare July 2, 2024 01:18

torrybr added 3 commits July 3, 2024 10:48

feat(beacons): unstable support for msc3489

218a951

tests(clippy): fix clippy test errors

48eab18

tests(beacons): clean up tests file logic and format

df93412

torrybr force-pushed the feat/live-location branch from 2a702bb to df93412 Compare July 3, 2024 14:50

poljar requested review from bnjbvr and removed request for poljar July 9, 2024 10:38

bnjbvr requested changes Jul 11, 2024

View reviewed changes

torrybr marked this pull request as draft July 18, 2024 20:22

torrybr mentioned this pull request Jul 19, 2024

sdk: basic support for sending and stopping live location shares #3741

Merged

torrybr mentioned this pull request Jul 26, 2024

sdk: basic support for sending location beacons #3771

Merged

bnjbvr changed the title ~~Support for MSC3489 live location sharing~~ sdk: implement MSC3489 live location sharing Sep 3, 2024

torrybr mentioned this pull request Sep 4, 2024

MSC3489 Live Location Sharing #3945

Open

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sdk: implement MSC3489 live location sharing #3621

sdk: implement MSC3489 live location sharing #3621

torrybr commented Jun 28, 2024 •

edited

Loading

codecov bot commented Jul 1, 2024 •

edited

Loading

bnjbvr left a comment

bnjbvr Jul 11, 2024

bnjbvr Jul 11, 2024

bnjbvr Jul 11, 2024

bnjbvr Jul 11, 2024

bnjbvr Jul 11, 2024

bnjbvr commented Jul 11, 2024

torrybr commented Jul 12, 2024

torrybr commented Jul 18, 2024

bnjbvr commented Jul 22, 2024

torrybr commented Aug 6, 2024

bnjbvr commented Aug 14, 2024

	warn!("Did not find beacon_info event in timeline items. Adding to pending list");
	debug!("Did not find beacon_info event in timeline items. Adding to pending list");

		//! This module handles rendering of MSC3489 live location sharing in the
		//! timeline.

sdk: implement MSC3489 live location sharing #3621

Are you sure you want to change the base?

sdk: implement MSC3489 live location sharing #3621

Conversation

torrybr commented Jun 28, 2024 • edited Loading

codecov bot commented Jul 1, 2024 • edited Loading

Codecov Report

bnjbvr left a comment

Choose a reason for hiding this comment

bnjbvr Jul 11, 2024

Choose a reason for hiding this comment

bnjbvr Jul 11, 2024

Choose a reason for hiding this comment

bnjbvr Jul 11, 2024

Choose a reason for hiding this comment

bnjbvr Jul 11, 2024

Choose a reason for hiding this comment

bnjbvr Jul 11, 2024

Choose a reason for hiding this comment

bnjbvr commented Jul 11, 2024

torrybr commented Jul 12, 2024

torrybr commented Jul 18, 2024

bnjbvr commented Jul 22, 2024

torrybr commented Aug 6, 2024

bnjbvr commented Aug 14, 2024

torrybr commented Jun 28, 2024 •

edited

Loading

codecov bot commented Jul 1, 2024 •

edited

Loading