[Location sharing] - Update DB entity when a live is timed out (PSF-999) #6128

mnaturel · 2022-05-23T14:22:20Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Introducing DeactivateLiveLocationShareWorker to set the isActive field to false after a live location share is timed out.

Motivation and context

Closes #6123
It enables also auto-refresh of the message tile when on timeline view when a live has just timed out.
It will ease the request on database to find all currently active lives in a room.

Screenshots / GIFs

Tests

Ensure the location sharing in enabled in Settings -> Preferences
Ensure the live location feature is enabled in Settings -> Labs
Go to a room
Start sharing your live location for 15 minutes
Take a coffee and wait for 15 minutes
Check the timeline the message tile is refreshed from running state to stopped state

Tested devices

Physical
Emulator
OS version(s): Android 11

Checklist

Changes has been tested on an Android device or Android emulator with API 21
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request includes a new file under ./changelog.d. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off
You've made a self review of your PR
If you have modified the screen flow, or added new screens to the application, you have updated the test UiAllScreensSanityTest.allScreensTest()

github-actions · 2022-05-23T14:59:56Z

Unit Test Results

124 files ±0 124 suites ±0 2m 11s ⏱️ -8s
220 tests ±0 220 ✔️ ±0 0 💤 ±0 0 ❌ ±0
726 runs ±0 726 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 791d4fb. ± Comparison against base commit 9a38d59.

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

im.vector.app.features.onboarding.ftueauth.FtueMissingRegistrationStagesComparatorTest ‑ when ordering stages, then prioritizes email

im.vector.app.features.onboarding.ftueauth.MatrixOrgRegistrationStagesComparatorTest ‑ when ordering stages, then prioritizes email

♻️ This comment has been updated with latest results.

onurays

Nice improvement. This is required to mark other users' timeout, in case of they cannot update state event because of network problems etc., right?

mnaturel · 2022-05-24T15:30:14Z

Nice improvement. This is required to mark other users' timeout, in case of they cannot update state event because of network problems etc., right?

In fact, this was done mainly for 2 reasons:

auto-refresh of the message tile when on timeline view when a live has just timed out.
ease the request on database to find all currently active lives in a room (we just need to rely on 1 field).

bmarty

Nice improvement, thanks!
Only some minor comments.

bmarty · 2022-05-25T08:13:33Z

...roid/sdk/internal/session/room/aggregation/livelocation/DeactivateLiveLocationShareWorker.kt

+
+    private suspend fun deactivateLiveLocationShare(params: Params) {
+        awaitTransaction(realmConfiguration) { realm ->
+            val aggregatedSummary = LiveLocationShareAggregatedSummaryEntity.getOrCreate(


Do we need a getOrCreate here? I mean if the instance is not found, does it worth creating it? I guess it will be always found, except after a clear cache maybe.

You are right, I changed it in b081429

bmarty · 2022-05-25T08:18:07Z

...droid/sdk/internal/session/room/aggregation/livelocation/LiveLocationAggregationProcessor.kt

+    }
+
+    private fun scheduleDeactivationAfterTimeout(eventId: String, roomId: String, endOfLiveTimestampMillis: Long?) {
+        endOfLiveTimestampMillis?.let { endOfLiveMillis ->


Nit: personnally I prefer to have this first line:

endOfLiveTimestampMillis?: return

It has the advantage to avoid to "rename" endOfLiveTimestampMillis to endOfLiveMillis and also reduce the indentation level.

Yes I agree it is more readable. I changed it in cfdc18d

bmarty · 2022-05-25T08:19:54Z

...droid/sdk/internal/session/room/aggregation/livelocation/LiveLocationAggregationProcessor.kt

+        endOfLiveTimestampMillis?.let { endOfLiveMillis ->
+            val workParams = DeactivateLiveLocationShareWorker.Params(sessionId = sessionId, eventId = eventId, roomId = roomId)
+            val workData = WorkerParamsFactory.toData(workParams)
+            val workName = DeactivateLiveLocationShareWorker.getWorkName(eventId = eventId, roomId = roomId)


Thanks for using named parameters!

bmarty · 2022-05-25T08:21:27Z

...droid/sdk/internal/session/room/aggregation/livelocation/LiveLocationAggregationProcessor.kt

+
+            workManagerProvider.workManager.enqueueUniqueWork(
+                    workName,
+                    ExistingWorkPolicy.KEEP,


I think you should use ExistingWorkPolicy.REPLACE here.

I have done the change in 8864a3c. But I am not sure if it will make a big difference.

bmarty · 2022-05-25T08:29:49Z

...roid/sdk/internal/session/room/aggregation/livelocation/DeactivateLiveLocationShareWorker.kt

+    companion object {
+        private const val WORK_NAME_PREFIX = "DeactivateLiveLocationWork-"
+
+        fun getWorkName(eventId: String, roomId: String) = "${WORK_NAME_PREFIX}$eventId-$roomId"


I do not know if the length of the string passed for uniqueWorkName can be limited, but maybe better to be safe here:
Maybe call .md5(), so replace by:

fun getWorkName(eventId: String, roomId: String): String { val hash = "$eventId$roomId".md5() return "DeactivateLiveLocationWork-$hash" }

(you can keep the const WORK_NAME_PREFIX if you prefer)

Thanks for the suggestion. I updated the code, see 791d4fb

sonarqubecloud · 2022-05-25T09:28:57Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

bmarty · 2022-05-25T12:51:40Z

.../matrix/android/sdk/internal/database/query/LiveLocationShareAggregatedSummaryEntityQuery.kt

+        eventId: String,
+): LiveLocationShareAggregatedSummaryEntity? {
+    return LiveLocationShareAggregatedSummaryEntity.where(realm, roomId, eventId).findFirst()
+}


Not for this PR, but now the line 55 could be replaced by a call to this new method.

bmarty

Thanks for the update!
can be merged from my POV.

github-actions · 2022-05-25T13:26:49Z

Matrix SDK

Integration Tests Results:

[org.matrix.android.sdk.session]
= passed=20 failures=0 errors=0 skipped=3
[org.matrix.android.sdk.account]
= passed=3 failures=0 errors=0 skipped=2
[org.matrix.android.sdk.internal]
= passed=6 failures=1 errors=0 skipped=0
[org.matrix.android.sdk.ordering]
= passed=16 failures=0 errors=0 skipped=0
[org.matrix.android.sdk.PermalinkParserTest]
= passed=2 failures=0 errors=0 skipped=0

mnaturel changed the title ~~[Location sharing] - Update DB entity when a live is timed out~~ [Location sharing] - Update DB entity when a live is timed out (PSF-999) May 23, 2022

mnaturel requested review from a team, bmarty and ericdecanini and removed request for a team May 23, 2022 14:30

mnaturel marked this pull request as ready for review May 23, 2022 14:31

Maxime NATUREL added 5 commits May 23, 2022 17:34

Adding changelog entry

695c234

Adding comment on isActive field

d76b93c

Creation of a worker to deactivate a live after timeout

47eb717

Schedule work during aggregation

683a9cd

Rely only on isActive field on UI side

b05fc76

mnaturel force-pushed the feature/mna/PSF-999-auto-refresh-db branch from a8a9c7f to b05fc76 Compare May 23, 2022 15:40

mnaturel requested a review from onurays May 24, 2022 08:36

onurays approved these changes May 24, 2022

View reviewed changes

bmarty requested changes May 25, 2022

View reviewed changes

Maxime NATUREL added 4 commits May 25, 2022 10:59

Replace getOrCreate() by get() in worker

b081429

Improve code readability in aggregation processor

cfdc18d

Using replace instead of keep strategy for the worker

8864a3c

Improve computation of the work name to limit its length

791d4fb

mnaturel requested review from bmarty and removed request for ericdecanini May 25, 2022 09:33

bmarty reviewed May 25, 2022

View reviewed changes

bmarty approved these changes May 25, 2022

View reviewed changes

mnaturel merged commit c8fb034 into develop May 25, 2022

mnaturel deleted the feature/mna/PSF-999-auto-refresh-db branch May 25, 2022 12:59

[Location sharing] - Update DB entity when a live is timed out (PSF-999) #6128

[Location sharing] - Update DB entity when a live is timed out (PSF-999) #6128

Uh oh!

Conversation

mnaturel commented May 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

Uh oh!

github-actions bot commented May 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

Uh oh!

onurays left a comment

Choose a reason for hiding this comment

Uh oh!

mnaturel commented May 24, 2022

Uh oh!

bmarty left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mnaturel May 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented May 25, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmarty left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented May 25, 2022

Matrix SDK

Integration Tests Results:

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mnaturel commented May 23, 2022 •

edited

Loading

github-actions bot commented May 23, 2022 •

edited

Loading

mnaturel May 25, 2022 •

edited

Loading