[Location sharing] - Message for live sharing in timeline (PSF-884) #5989

mnaturel · 2022-05-09T13:39:46Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Handling of the UI for the message associated to a live location share in the timeline. The message stays at the same position (at the timestamp of the start of the live). The message has 3 states:

Loading (live has been start but no location has been received yet)
Running (showing the last location of the user who is sharing)
Ended (the live has been stopped by the user or is timed out)

The UI will also change depending if the user is the creator of the live or not.

Known issues that will be fixed later:

Long waiting after pressing stop: a loader will be shown after pressing "Stop"
No refresh of the message if the user is viewing the timeline and the live goes from running state to timed out
Blinking of the map when location is updated

Motivation and context

Closes #5689

Screenshots / GIFs

Note: in the running state on emitter side, the live locatiion sharing top bar is not visible is some of the captures due to missing handling when restarting the app (like when toggling dark mode)

Running state (Emitter/default layout)	Running state (Emitter/bubble layout)

Running state (Watcher/default layout)	Running state (Watcher/bubble layout)

Ended state (default layout)	Ended state (bubble layout)

Tests

Ensure to enable location sharing in Settings -> Preferences
Ensure to enable developer mode in Settings -> Advanced settings
Ensure to enable the live location feature in the developer setting
Go to a room where you can send state event
Press the plus button on the new message layout
Press the location icon
Choose the "Live location" option
Check the message displays "Loading" state
Check the message displays the correct last location
Check the location is updated when moving (can be emulated by a fake route on emulator)
Press the stop button
Check the message displays the "Ended" 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()

…ut location

github-actions · 2022-05-09T15:46:07Z

Unit Test Results

122 files ±0 122 suites ±0 2m 10s ⏱️ -10s
205 tests ±0 205 ✔️ ±0 0 💤 ±0 0 ❌ ±0
690 runs ±0 690 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 4ebd1ea. ± Comparison against base commit 3674ae7.

♻️ This comment has been updated with latest results.

ganfra

Some remarks, otherwise good work!

ganfra · 2022-05-12T10:12:02Z

...dk/internal/session/room/aggregation/livelocation/DefaultLiveLocationAggregationProcessor.kt

@@ -60,22 +60,27 @@ internal class DefaultLiveLocationAggregationProcessor @Inject constructor() : L
        aggregatedSummary.isActive = content.isLive
    }

-    override fun handleBeaconLocationData(realm: Realm, event: Event, content: MessageBeaconLocationDataContent, roomId: String, isLocalEcho: Boolean) {
+    override fun handleBeaconLocationData(


Not sure we should have an interface for this

Removed, see 4a23109

ganfra · 2022-05-12T10:12:58Z

vector/src/main/java/im/vector/app/core/resources/DateProvider.kt

-        zoneId.rules.getOffset(now)
-    }
+    // recompute the zoneId each time we access it to handle change of timezones
+    private val defaultZoneId: ZoneId


Might be too costly?

I have done this change to handle the case of a change of timezone when the app is running. If we don't recompute the ZoneId, in some cases, we don't use the same as the one defined in the system and dates and times are not correct in the app. Do you have another solution in mind to fix this issue?

We should probably listen to android.intent.action.TIMEZONE_CHANGED intent if we really want this to be updated. But thats really not a common case, so I don't know?

I have created a dedicated issue to improve it: #6076

Maybe remove the change waiting for the improvement? What do you think @bmarty?

This can be handled later and this can stay like that for now I think.

ganfra · 2022-05-12T10:15:57Z

...rc/main/java/im/vector/app/features/home/room/detail/timeline/factory/TimelineItemFactory.kt

                    // Calls
                    EventType.CALL_INVITE,
                    EventType.CALL_HANGUP,
                    EventType.CALL_REJECT,
-                    EventType.CALL_ANSWER           -> callItemFactory.create(params)
+                    EventType.CALL_ANSWER             -> callItemFactory.create(params)
+                    in EventType.BEACON_LOCATION_DATA -> noticeItemFactory.create(params)


Should be grouped with other NoticeItem

Fixed see 2aeee79

ganfra · 2022-05-12T10:16:41Z

...ava/im/vector/app/features/home/room/detail/timeline/helper/MessageInformationDataFactory.kt

@@ -119,7 +119,8 @@ class MessageInformationDataFactory @Inject constructor(private val session: Ses
                isFirstFromThisSender = isFirstFromThisSender,
                isLastFromThisSender = isLastFromThisSender,
                e2eDecoration = e2eDecoration,
-                sendStateDecoration = sendStateDecoration
+                sendStateDecoration = sendStateDecoration,
+                liveLocationShareSummaryData = getLiveLocationShareSummaryData(event)


Not sure it should be part of this class

I tried to follow the same practice as for other extra information of events. Do you think I should extract it into its own LiveLocationShareSummaryData and associated LiveLocationShareSummaryDataFactory?

You could just add this as attribute of needed items. MessageInformationData should be used for data used by all items

I have removed the data class from MessageInformationData and encapsulated it into the live location item factory since this is the only area where it is used. See cbf8306

ganfra · 2022-05-12T10:18:59Z

...in/java/im/vector/app/features/home/room/detail/timeline/item/LiveLocationShareStatusItem.kt

+import android.widget.ImageView
+import im.vector.app.features.home.room.detail.timeline.style.TimelineMessageLayout
+
+interface LiveLocationShareStatusItem {


I don't see other implementations happening, so would remove the interface

The purpose of this interface was to be able to use delegation pattern to mutualize code in MessageLiveLocationInactiveItem and MessageLiveLocationStartItem. If I remove it, I will not be able to use it. Do you have another solution?

Ok, I hadn't seen the delegation, sorry

ganfra · 2022-05-12T10:19:46Z

vector/src/main/java/im/vector/app/features/location/live/LocationLiveMessageBannerView.kt

+import im.vector.app.features.themes.ThemeUtils
+import org.threeten.bp.Duration
+
+sealed class LocationLiveMessageBannerViewState(


Extract in a separate file?

Yes, done in b077351 and 3d13611

ganfra

LGTM now, thanks for the changes

bmarty

LGTM, just small remarks about the strings.

bmarty · 2022-05-17T15:24:55Z

vector/src/main/res/values/strings.xml

@@ -322,6 +322,14 @@
    <string name="start_chatting">Start Chatting</string>
    <string name="spaces">Spaces</string>

+    <!-- Time units -->
+    <plurals name="time_unit_hour_short">


Maybe add a comment for translator that if a short version exists in their language, it has to be used. For English there is no short version, so it can confuse the translators.

Done see e9d9319

bmarty · 2022-05-17T15:26:27Z

vector/src/main/res/values/strings.xml

+        <item quantity="other">hours</item>
+    </plurals>
+    <string name="time_unit_minute_short">min</string>
+    <string name="time_unit_second_short">sec</string>


It's weird to have a plural for hours, but not for minutes and seconds. It can be better to have only plurals to give more flexibility to the translators.
All plural items for English can be identical.

I agree. I decided to remove the plural and use "h" instead of "hour" to be consistent with other platforms. See e9d9319

bmarty · 2022-05-17T15:27:12Z

vector/src/main/res/values/strings.xml

@@ -3014,7 +3022,11 @@
    <string name="location_timeline_failed_to_load_map">Failed to load map</string>
    <string name="location_share_live_enabled">Live location enabled</string>
    <string name="location_share_live_started">Loading live location…</string>
+    <string name="location_share_live_ended">Live location ended</string>
+    <string name="location_share_live_view">View live location</string>
+    <string name="location_share_live_until">Live until %1$s</string>


Maybe add a comment to explain what will replace the template, with an example

Done in e9d9319.

bmarty · 2022-05-17T15:27:24Z

vector/src/main/res/values/strings.xml

    <string name="location_share_live_stop">Stop</string>
+    <string name="location_share_live_remaining_time">%1$s left</string>


Maybe add a comment to explain what will replace the template, with an example

Done in e9d9319.

bmarty

Thanks for the update!

github-actions · 2022-05-18T14:12:00Z

Matrix SDK

Integration Tests Results:

[org.matrix.android.sdk.session]
= passed=18 failures=2 errors=0 skipped=3
[org.matrix.android.sdk.account]
= passed=3 failures=0 errors=0 skipped=2
[org.matrix.android.sdk.internal]
= passed=5 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

Maxime NATUREL added 30 commits May 9, 2022 15:12

Adding live location summary data into MessageInformationData

90aad44

Using information data in message factory

0561fe5

Adding view state class

d89d6bc

Renaming message item factory

68a44c4

Fixes in DateProvider

d44a6c5

Building correct item depending on the state of the live

431d861

Custom view for the banner

df1ba8e

Rendering method

a412b21

Fix format of the duration

3acc139

Integration into location message item

bbec3a7

Creating a dedicated live location item

8c01214

Removing TODO

93634cd

Adding currentUserId attribute

11d9579

Adding countDownTimer for emitter view

c432985

Do not append 0 values when formatting duration

8b62822

Renaming userId into locationUserId in message location item

adbc430

Show running live state item

077977b

Fix display in Bubble mode

b577f6a

Fix some namings after rebase

3a36762

Format location event for debugging purpose

d341611

Binding stop button of emitter view

4a4ec47

Reducing period of location update to 2 seconds

b0c1ca8

Checking inactive state before loading to handle timed out live witho…

370b6a8

…ut location

Adding xml layout for inactive state

a37edb5

Show inactive message item when a live is inactive

c10b2a4

Adding changelog entry

889e09a

Fix pin position in static map

f2b66c5

Fix crash when mixing static and live location

0a21bd4

Fix send of the first location after start

6622651

Fix background color of bottom banner

82cbc35

Maxime NATUREL added 3 commits May 9, 2022 17:46

Moving the light "no location" background into default folder

738d486

Decreasing alpha of bottom banner in message view

cb5c6ec

Updating the no location map background

b2099b6

mnaturel marked this pull request as ready for review May 11, 2022 09:10

mnaturel requested review from a team, bmarty, ganfra and onurays and removed request for a team May 11, 2022 09:11

ganfra reviewed May 12, 2022

View reviewed changes

Maxime NATUREL added 4 commits May 16, 2022 10:11

Removing non necessary LiveLocationAggregationProcessor interface

4a23109

Using existing common When case for NoticeItem to handle location data

2aeee79

Extracting LocationLiveMessageBannerViewState into separated file

b077351

Fix filename

3d13611

mnaturel requested a review from ganfra May 16, 2022 08:40

Johennes changed the title ~~[Location sharing] - Message for live sharing in timeline~~ [Location sharing] - Message for live sharing in timeline (PSF-884) May 16, 2022

bmarty and others added 3 commits May 16, 2022 14:42

Merge branch 'develop' into feature/mna/PSF-884-location-view

afd2804

Fix missing dot at end of code documentation

1d6083b

Removing live location share data from MessageInformationData structure

cbf8306

mnaturel mentioned this pull request May 17, 2022

Listen for timezone changes #6076

Open

ganfra approved these changes May 17, 2022

View reviewed changes

bmarty requested changes May 17, 2022

View reviewed changes

Adding comments on some strings and removing non necessary plural

e9d9319

mnaturel requested a review from bmarty May 18, 2022 07:51

Fix parsing of location data in non encrypted room

4ebd1ea

bmarty approved these changes May 18, 2022

View reviewed changes

mnaturel merged commit 738ce18 into develop May 18, 2022

mnaturel deleted the feature/mna/PSF-884-location-view branch May 18, 2022 13:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Location sharing] - Message for live sharing in timeline (PSF-884) #5989

[Location sharing] - Message for live sharing in timeline (PSF-884) #5989

mnaturel commented May 9, 2022 •

edited

github-actions bot commented May 9, 2022 •

edited

ganfra left a comment

ganfra May 12, 2022

mnaturel May 16, 2022

ganfra May 12, 2022

mnaturel May 16, 2022

ganfra May 17, 2022

mnaturel May 17, 2022

ganfra May 17, 2022

bmarty May 17, 2022

ganfra May 12, 2022

mnaturel May 16, 2022

ganfra May 12, 2022

mnaturel May 16, 2022

ganfra May 17, 2022

mnaturel May 17, 2022

ganfra May 12, 2022

mnaturel May 16, 2022

ganfra May 17, 2022

ganfra May 12, 2022

mnaturel May 16, 2022 •

edited

ganfra left a comment

bmarty left a comment

bmarty May 17, 2022

mnaturel May 18, 2022

bmarty May 17, 2022

mnaturel May 18, 2022

bmarty May 17, 2022

mnaturel May 18, 2022

bmarty May 17, 2022

mnaturel May 18, 2022

bmarty left a comment

github-actions bot commented May 18, 2022

		<string name="location_share_live_stop">Stop</string>
		<string name="location_share_live_remaining_time">%1$s left</string>

[Location sharing] - Message for live sharing in timeline (PSF-884) #5989

[Location sharing] - Message for live sharing in timeline (PSF-884) #5989

Conversation

mnaturel commented May 9, 2022 • edited

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

github-actions bot commented May 9, 2022 • edited

Unit Test Results

ganfra 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

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

mnaturel May 16, 2022 • edited

Choose a reason for hiding this comment

ganfra left a comment

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

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

github-actions bot commented May 18, 2022

Matrix SDK

Integration Tests Results:

mnaturel commented May 9, 2022 •

edited

github-actions bot commented May 9, 2022 •

edited

mnaturel May 16, 2022 •

edited