#5417: Pinned location sharing #5479

mnaturel · 2022-03-09T14:54:00Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Implementation of the pinned location sharing. User can now select a location on the map different from its current location. User can also zoom to its current location using a dedicated button placed above the compass icon.

Motivation and context

Closes #5417

Screenshots / GIFs

User location sharing

Pinned location sharing

Tests

User location case

Ensure location sharing option is enabled in "Settings" -> "Preferences"
Go to a room or a direct message
Click on the "add" icon
Select the location icon
Check you see the map with your current location
Check you see the confirmation view with your avatar and color
Click on the confirmation view
Check the location is correctly shared in the timeline of the room/message

Pinned location case

Ensure location sharing option is enabled in "Settings" -> "Preferences"
Go to a room or a direct message
Click on the "add" icon
Select the location icon
Check you see the map with your current location
Move the map on another location
Check the confirmation view has changed and corresponds to pin location option
Check the pin on the map is now a generic pin instead of the avatar
Click on the confirmation view
Check the location is correctly shared in the timeline of the room/message

Compatibility with previous versions

Share user location or a pinned location in a room
Go to the room using a previous version of the app
Check the events are represented as user location (Generic pin would not be displayed for pinned location due to the current enum model for asset type and the change of the naming for the asset field)
Share user location with a previous version in a room
Go to the room using the new app version
Check we correctly see the shared user location

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-03-09T16:41:28Z

Unit Test Results

102 files +4 102 suites +4 1m 9s ⏱️ +4s
182 tests +2 182 ✔️ +2 0 💤 ±0 0 ❌ ±0
598 runs +8 598 ✔️ +8 0 💤 ±0 0 ❌ ±0

Results for commit b72c87d. ± Comparison against base commit db0cfd4.

♻️ This comment has been updated with latest results.

mnaturel · 2022-03-09T16:45:43Z

Hello @gaelledel, this PR is ready for design review. Can you check everything is okay?

gaelledel

Awesome work :D

onurays · 2022-03-14T15:05:44Z

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

+    object CurrentUserLocationSharingAction : LocationSharingAction()
+    data class PinnedLocationSharingAction(val locationData: LocationData?) : LocationSharingAction()
+    data class LocationTargetChangeAction(val locationData: LocationData) : LocationSharingAction()
+    object ZoomToUserLocationAction : LocationSharingAction()


Can you remove Action suffix like ZoomToUserLocation to continue the naming convention of other actions?

Yes in fact it is not necessary.

onurays · 2022-03-14T15:25:16Z

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

@@ -130,21 +143,52 @@ class LocationSharingFragment @Inject constructor(
                .show()
    }

+    private fun initLocateBtn() {


I would prefer renaming to initLocateButton()

onurays · 2022-03-15T11:24:34Z

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

-                        pinDrawable = it
-                )
+    private fun updatePin(isUserPin: Boolean? = true) {
+        if (isUserPin == true) {


We generally prefer if (isUserPin.orFalse()).

Thanks, I was not aware of this extension.

onurays · 2022-03-15T11:28:37Z

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

        pinId = DEFAULT_PIN_ID,
-        pinDrawable = pinDrawable
+        pinDrawable = null,
+        showPin = areTargetAndUserLocationEqual == false


.orFalse can be used here too.

Actually I replaced it by areTargetAndUserLocationEqual.orTrue().not() to be equivalent to areTargetAndUserLocationEqual == false.

onurays · 2022-03-15T11:30:44Z

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

    private var mapRefs: MapRefs? = null
    private var initZoomDone = false
+    private var showLocationBtn = false


Could you please rename locateBtn, createLocateBtn, showLocationBtn, initLocateBtn and adjustCompassBtn?

Yes no problem, done.

onurays

Nice work! Tested on my device. I just have small remarks.

mnaturel · 2022-03-15T14:49:21Z

Nice work! Tested on my device. I just have small remarks.

Thanks! I fixed all comments and rebased the branch on develop. Can you have a look again?

mnaturel · 2022-03-15T14:59:11Z

When testing again with previous version, I now have a "Malformed event" message for pinned location events sent with a version built from this branch. This is due to the enum type LocationAssetType which is not flexible. I do not have any solution for this...

mnaturel force-pushed the feature/mna/PSF-735-pinned-location branch from 2cc7751 to fce6c66 Compare March 9, 2022 16:05

mnaturel assigned gaelledel Mar 9, 2022

gaelledel approved these changes Mar 10, 2022

View reviewed changes

mnaturel unassigned gaelledel Mar 11, 2022

mnaturel requested a review from onurays March 11, 2022 08:47

onurays reviewed Mar 14, 2022

View reviewed changes

mnaturel marked this pull request as ready for review March 14, 2022 15:31

onurays reviewed Mar 15, 2022

View reviewed changes

onurays suggested changes Mar 15, 2022

View reviewed changes

mnaturel force-pushed the feature/mna/PSF-735-pinned-location branch from 70a6d2b to 19e7ec2 Compare March 15, 2022 14:39

onurays self-requested a review March 15, 2022 15:28

onurays approved these changes Mar 15, 2022

View reviewed changes

mnaturel enabled auto-merge March 15, 2022 15:33

Maxime Naturel added 10 commits March 15, 2022 17:09

Show options menu on top of the map

9256b56

Adding static pin at the center of the map

a1d155d

Handling press on pinned location option

dec075f

Comparing target and user location using Flow in ViewModel

01879e2

Compare locations use case implementation

6fc6bf1

Updating location target drawable dynamically

93c397d

Show dot pin for user location

57fcfeb

Show icon to reset map to user location

40e9284

Using style attibute for locate button visibility

01aff36

Zoom to user location action

125b8d2

Maxime Naturel added 9 commits March 15, 2022 17:09

Recompute location comparison on new user location

8d1822d

Distinguish user location and pinned location sharing

04405c7

Adding unit tests for use case

094e62c

Adding changelog entry

9864e51

Fixing asset type representation

f495150

Change naming for initLocateButton() method

6779fa1

Removing Action suffix in ViewAction types

abd25c2

Using extensions for Boolean? type

9ecf12a

Renaming "btn" into "button"

b72c87d

mnaturel force-pushed the feature/mna/PSF-735-pinned-location branch from 19e7ec2 to b72c87d Compare March 15, 2022 16:09

mnaturel merged commit 9ef235f into develop Mar 15, 2022

mnaturel deleted the feature/mna/PSF-735-pinned-location branch March 15, 2022 16:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#5417: Pinned location sharing #5479

#5417: Pinned location sharing #5479

mnaturel commented Mar 9, 2022 •

edited

Loading

github-actions bot commented Mar 9, 2022 •

edited

Loading

mnaturel commented Mar 9, 2022

gaelledel left a comment

onurays Mar 14, 2022

mnaturel Mar 14, 2022

onurays Mar 14, 2022

onurays Mar 15, 2022

mnaturel Mar 15, 2022

onurays Mar 15, 2022

mnaturel Mar 15, 2022

onurays Mar 15, 2022 •

edited

Loading

mnaturel Mar 15, 2022

onurays left a comment •

edited

Loading

mnaturel commented Mar 15, 2022

mnaturel commented Mar 15, 2022

#5417: Pinned location sharing #5479

#5417: Pinned location sharing #5479

Conversation

mnaturel commented Mar 9, 2022 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

User location sharing

Pinned location sharing

Tests

Tested devices

Checklist

github-actions bot commented Mar 9, 2022 • edited Loading

Unit Test Results

mnaturel commented Mar 9, 2022

gaelledel 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

onurays Mar 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onurays left a comment • edited Loading

Choose a reason for hiding this comment

mnaturel commented Mar 15, 2022

mnaturel commented Mar 15, 2022

mnaturel commented Mar 9, 2022 •

edited

Loading

github-actions bot commented Mar 9, 2022 •

edited

Loading

onurays Mar 15, 2022 •

edited

Loading

onurays left a comment •

edited

Loading