Live Location Sharing - Beacon Info #5651

onurays · 2022-03-28T13:27:51Z

A beacon info state event needs to be sent when live location sharing started.

github-actions · 2022-03-28T14:05:37Z

Unit Test Results

110 files +  4 110 suites +4 1m 21s ⏱️ +15s
195 tests +  7 195 ✔️ +  7 0 💤 ±0 0 ❌ ±0
650 runs +28 650 ✔️ +28 0 💤 ±0 0 ❌ ±0

Results for commit 273b481. ± Comparison against base commit 3cf7765.

This pull request removes 2 and adds 9 tests. Note that renamed tests count towards both.

im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given registration has started and has dummy step to do, when handling action, then ignores other steps and executes dummy
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ when registering account, then updates state and emits account created event

im.vector.app.features.onboarding.DirectLoginUseCaseTest ‑ given direct authentication throws, when logging in directly, then returns failure result with original cause
im.vector.app.features.onboarding.DirectLoginUseCaseTest ‑ given wellknown fails with content, when logging in directly, then returns success with direct session result
im.vector.app.features.onboarding.DirectLoginUseCaseTest ‑ given wellknown fails without content, when logging in directly, then returns well known error
im.vector.app.features.onboarding.DirectLoginUseCaseTest ‑ given wellknown throws, when logging in directly, then returns failure result with original cause
im.vector.app.features.onboarding.DirectLoginUseCaseTest ‑ when logging in directly, then returns success with direct session result
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given has sign in with matrix id sign mode, when handling login or register action fails, then emits error
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given has sign in with matrix id sign mode, when handling login or register action, then logs in directly
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given personalisation enabled and registration has started and has dummy step to do, when handling action, then ignores other steps and executes dummy
im.vector.app.features.onboarding.OnboardingViewModelTest ‑ given personalisation enabled, when registering account, then updates state and emits account created event

♻️ This comment has been updated with latest results.

ouchadam · 2022-03-28T15:48:58Z

...oid/src/main/java/org/matrix/android/sdk/api/session/room/model/LiveLocationBeaconContent.kt

+         */
+        @Json(name = "m.beacon_info") val beaconInfo: BeaconInfo? = null,
+        /**
+         * Beacon creation timestamp.


I'm not sure if we do it in other places, but it might be handy to know the timestamp format, for example epoch seconds or milliseconds

Renamed and used clock class.

ouchadam · 2022-03-28T15:50:36Z

vector/src/main/java/im/vector/app/features/location/LocationSharingService.kt

@@ -40,6 +47,7 @@ class LocationSharingService : VectorService(), LocationTracker.Callback {

    @Inject lateinit var notificationUtils: NotificationUtils
    @Inject lateinit var locationTracker: LocationTracker
+    @Inject lateinit var session: Session


is it safe to inject the session directly or should we use the ActiveSessionHolder? 🤔

Right, not safe here. Done.

ouchadam

thanks for the updates! might be worth a second pair of eyes from @mnaturel

bmarty

Some remarks

bmarty · 2022-03-30T08:12:18Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/events/model/EventType.kt

@@ -49,6 +49,7 @@ object EventType {
    const val STATE_ROOM_JOIN_RULES = "m.room.join_rules"
    const val STATE_ROOM_GUEST_ACCESS = "m.room.guest_access"
    const val STATE_ROOM_POWER_LEVELS = "m.room.power_levels"
+    const val STATE_ROOM_BEACON_INFO = "m.beacon_info"


There is no unstable prefix?

(see https://github.com/matrix-org/matrix-spec-proposals/blob/stefan/ephemeral-location-streaming/proposals/3672-ephemeral-location-streaming.md#unstable-prefix)

Oh no, of course there is. Thanks for catching.

bmarty · 2022-03-30T08:18:53Z

vector/src/main/java/im/vector/app/features/location/LocationSharingService.kt

+                        session.coroutineScope.launch {
+                            sendBeaconInfo(session, roomArgs)
+                        }
+                    }


Just to double check: this code does not send multiple beacon events during a period of time, but just one event, right?

Yes, location events will be sent later.

bmarty · 2022-03-30T08:21:39Z

vector/src/main/java/im/vector/app/features/location/LocationSharingService.kt

+        session
+                .getRoom(roomArgs.roomId)
+                ?.sendStateEvent(
+                        eventType = EventType.STATE_ROOM_BEACON_INFO,


see the MCS: type should have the form "m.beacon_info.@stefan:matrix.org"
https://github.com/matrix-org/matrix-spec-proposals/blob/stefan/ephemeral-location-streaming/proposals/3672-ephemeral-location-streaming.md#proposal

dynamic keys are quite painful for json parsers, do we have other examples of them in the spec? would be great to avoid and replace with a content value if possible

We also have https://github.com/matrix-org/matrix-spec-proposals/blob/andybalaam/owner-state-events/proposals/3757-restricting-who-can-overwrite-a-state-event.md but let me clarify this with the team.

mnaturel

I need to test to confirm. But seems good to me. I just have small remarks.
Is it also possible to update the PR description with the correct links to the MSCs we eventually use?

mnaturel · 2022-04-01T13:01:12Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/model/BeaconInfo.kt

@@ -0,0 +1,33 @@
+/*
+ * Copyright 2020 The Matrix.org Foundation C.I.C.


Can you update the date to 2022?

Nice catch :) Done.

mnaturel · 2022-04-01T13:01:37Z

...oid/src/main/java/org/matrix/android/sdk/api/session/room/model/LiveLocationBeaconContent.kt

@@ -0,0 +1,47 @@
+/*
+ * Copyright 2020 The Matrix.org Foundation C.I.C.


Same comment about the date.

mnaturel · 2022-04-01T13:08:18Z

...oid/src/main/java/org/matrix/android/sdk/api/session/room/model/LiveLocationBeaconContent.kt

+ * limitations under the License.
+ */
+
+package org.matrix.android.sdk.api.session.room.model


Maybe we should create a dedicated subpackage for models concerning the live location feature? For instance livelocation. Or do you think it is in the right package?

I was thinking about this, done.

mnaturel · 2022-04-01T13:11:30Z

...oid/src/main/java/org/matrix/android/sdk/api/session/room/model/LiveLocationBeaconContent.kt

+         * Beacon creation timestamp.
+         */
+        @Json(name = "org.matrix.msc3488.ts") val unstableTimestampAsMilliseconds: Long? = null,
+        @Json(name = "m.ts") val timestampAsMillisecond: Long? = null,


Is it possible to unify the naming? Indeed, the unstable field has an ending "s" in its name and the stable one has no ending "s".

Nice catch, done.

mnaturel · 2022-04-01T13:13:34Z

...oid/src/main/java/org/matrix/android/sdk/api/session/room/model/LiveLocationBeaconContent.kt

+        /**
+         * Live location asset type.
+         */
+        @Json(name = "org.matrix.msc3488.asset") val unstableLocationAsset: LocationAsset = LocationAsset("m.self"),


Is it possible to use the defined const value LocationAssetType.SELF instead of "m.self"?

Right, it has been changed. Done.

mnaturel · 2022-04-01T13:24:26Z

vector/src/main/java/im/vector/app/features/location/LocationSharingService.kt

+            activeSessionHolder
+                    .getSafeActiveSession()
+                    ?.let { session ->
+                        session.coroutineScope.launch {


Maybe it would be better to precise the IO Dispatcher here like this:

session.coroutineScope.launch(session.coroutineDispatchers.io) { sendBeaconInfo(session, roomArgs) }

I am not sure but reading the implementation of SessionCoroutineScopes, it seems the default Dispatcher is the Main one. @ouchadam or @bmarty could you confirm? Or am I missing something?

I am also not so confident, but IO seems better. @bmarty?

Session.getRoom() is a blocking non suspend call, ideally the api would be suspending and internally use the correct dispatcher where needed, until then wrapping with an IO context would be better!

I thought this would have triggered a network on main thread warning 🤔

Onuray Sahin added 3 commits March 28, 2022 14:45

Create beacon content class.

32cf3fe

Send beacon info.

152c921

Changelog added.

6952552

onurays requested review from ouchadam and mnaturel March 28, 2022 13:29

ouchadam reviewed Mar 28, 2022

View reviewed changes

Code review fixes.

2a4182e

ouchadam approved these changes Mar 29, 2022

View reviewed changes

bmarty reviewed Mar 30, 2022

View reviewed changes

Code review fixes, use unstable prefixes as MSCs suggest.

922d68c

mnaturel reviewed Apr 1, 2022

View reviewed changes

Code review fixes.

97de6de

mnaturel approved these changes Apr 1, 2022

View reviewed changes

Fix lint error.

273b481

onurays merged commit 48d40d8 into develop Apr 4, 2022

onurays deleted the feature/ons/live_location_beacon_info branch April 4, 2022 09:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Live Location Sharing - Beacon Info #5651

Live Location Sharing - Beacon Info #5651

onurays commented Mar 28, 2022

github-actions bot commented Mar 28, 2022 •

edited

ouchadam Mar 28, 2022

onurays Mar 29, 2022

ouchadam Mar 28, 2022

onurays Mar 29, 2022

ouchadam left a comment •

edited

bmarty left a comment

bmarty Mar 30, 2022

bmarty Mar 30, 2022

onurays Mar 30, 2022

onurays Apr 1, 2022

bmarty Mar 30, 2022

onurays Mar 30, 2022

bmarty Mar 30, 2022

ouchadam Mar 30, 2022 •

edited

onurays Mar 30, 2022

onurays Apr 1, 2022

mnaturel left a comment •

edited

mnaturel Apr 1, 2022

onurays Apr 1, 2022

mnaturel Apr 1, 2022

onurays Apr 1, 2022

mnaturel Apr 1, 2022

onurays Apr 1, 2022

mnaturel Apr 1, 2022

onurays Apr 1, 2022

mnaturel Apr 1, 2022

onurays Apr 1, 2022

mnaturel Apr 1, 2022

onurays Apr 1, 2022

ouchadam Apr 1, 2022

		@@ -0,0 +1,33 @@
		/*
		* Copyright 2020 The Matrix.org Foundation C.I.C.

		@@ -0,0 +1,47 @@
		/*
		* Copyright 2020 The Matrix.org Foundation C.I.C.

Live Location Sharing - Beacon Info #5651

Live Location Sharing - Beacon Info #5651

Conversation

onurays commented Mar 28, 2022

github-actions bot commented Mar 28, 2022 • edited

Unit Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ouchadam left a comment • edited

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ouchadam Mar 30, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnaturel left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 28, 2022 •

edited

ouchadam left a comment •

edited

ouchadam Mar 30, 2022 •

edited

mnaturel left a comment •

edited