#5395: Location sharing options view #5411

mnaturel · 2022-03-03T10:00:21Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Adding a new custom view named LocationSharingOptionPickerView to be able to show different options of location sharing.
The only visible changes we can see now in the app is the icon and text displayed to confirm sharing and the avatar display in timeline and map (see captures below).
In fact, we only have user current location option available for now. Icon is now the user avatar with background color corresponding to the user color both in the pins of the map and in the confirmation view. Ripple effect has also been added when the confirmation view is pressed.

Motivation and context

Closes #5395.

Screenshots / GIFs

Current visible changes in the app

Changes not currently visible in the app

For UI validation, only the option picker view is relevant, the map view has not been updated for now)

Tests

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
Check the avatar background color is yours and consistent both in the map and in the confirmation view
Click on the confirmation view
Check the location is correctly shared in the timeline of the room/message
Check the avatar background color is yours and consistent in the timeline

To test the other options, go to LocationSharingFragment at initOptionsPicker() method and set the options as you want. For example, views.shareLocationOptionsPicker.render(LocationSharingOption.USER_CURRENT, LocationSharingOption.USER_LIVE) or views.shareLocationOptionsPicker.render(LocationSharingOption.PINNED)

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-03T10:33:00Z

Unit Test Results

  96 files ±0   96 suites ±0 1m 7s ⏱️ -1s
172 tests ±0 172 ✔️ ±0 0 💤 ±0 0 ❌ ±0
564 runs ±0 564 ✔️ ±0 0 💤 ±0 0 ❌ ±0

Results for commit 2bdafde. ± Comparison against base commit 2048b85.

♻️ This comment has been updated with latest results.

mnaturel · 2022-03-03T10:35:07Z

Hi @gaelledel , can you please review the design? I have added some relevant screenshots of the added modifications.

gaelledel

Ace! Looking forward to see future developments

ariskotsomitopoulos

Good Job with the custom views, it seems fine to me

ariskotsomitopoulos · 2022-03-03T11:18:37Z

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

+            // TODO
+        }
+        views.shareLocationOptionsPicker.optionUserLive.debouncedClicks {
+            // TODO


What do we plan to do here?

For the live location sharing, the app will send location of the user periodically during a certain amount of time until the user stops sharing its location. And in timeline we will also be able to see locations of different several users currently live sharing their location.

bmarty

Nice work!
A few (small) remarks

bmarty · 2022-03-03T13:27:41Z

library/ui-styles/src/main/res/values/theme_light.xml

@@ -142,6 +142,9 @@
        <item name="vctr_keyword_style">@style/Widget.Vector.Keyword</item>

        <item name="vctr_toast_background">@color/vctr_toast_background_light</item>
+
+        <!-- Location sharing -->
+        <item name="vctr_live_location">@color/vctr_live_location_dark</item>


I think you mean vctr_live_location_light (even if the actual value is the same)

Yes exactly, thanks! I missed that, I will fix this.

bmarty · 2022-03-03T13:30:23Z

vector/src/main/java/im/vector/app/features/location/option/LocationSharingOption.kt

+enum class LocationSharingOption {
+    USER_CURRENT, // current user's location
+    USER_LIVE,    // user's location during a certain amount of time
+    PINNED        // static location pinned by the user


On the form I prefer to have comment above the value, using KDoc for better integration with IDE. So like

/** * static location pinned by the user */ PINNED

Okay I will update it.

bmarty · 2022-03-03T13:31:58Z

vector/src/main/java/im/vector/app/features/location/option/LocationSharingOptionPickerView.kt

+        applyBackground()
+    }
+
+    fun setOptions(vararg options: LocationSharingOption) {


Generally those methods are called render in the codebase.
Also maybe passing a List as parameter could be better than vararg, but I do not have a strong opinion on it.

Okay for the naming. I was wondering about a Set instead of vararg but I have chosen vararg because it easier to use and read (no extra setOf()).

bmarty · 2022-03-03T13:34:39Z

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

@@ -0,0 +1,11 @@
+<?xml version="1.0" encoding="utf-8"?>


Can you move and rename this file to the same location and spirit than https://github.com/vector-im/element-android/blob/main/library/ui-styles/src/main/res/values/stylable_badge_floating_action_button.xml please?

Okay I see. We declare one file per view. I will do the update.

bmarty · 2022-03-03T13:35:19Z

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

@@ -2924,9 +2924,17 @@
    <!-- Location -->
    <string name="location_activity_title_static_sharing">Share location</string>
    <string name="location_activity_title_preview">Location</string>
+    <!-- TODO delete -->


Thanks. Related lint issue about unused resourced could be ignore. Maybe add a lint ignore in the tag to please lint and not break develop once merged. I will update the recipe to delete strings if it works (or you can do it if you want to!).

mnaturel · 2022-03-04T12:59:06Z

Nice work! A few (small) remarks

Thanks for the review! I have fixed all discussions, can you check again? I will need this work to move on #5417.

onurays · 2022-03-08T15:36:14Z

We may have a quick win here to share the location as a pin by extending LocationAssetType like:

@JsonClass(generateAdapter = false)
enum class LocationAssetType {
    /**
     *
     **/
    @Json(name = "m.self")
    SELF,

    /**
     *
     **/
    @Json(name = "m.pin")
    PIN
}

Timeline already handles the asset type and renders as a pin if it is different than m.self.

mnaturel · 2022-03-09T08:40:22Z

We may have a quick win here to share the location as a pin by extending LocationAssetType like:
@JsonClass(generateAdapter = false)
enum class LocationAssetType {
    /**
     *
     **/
    @Json(name = "m.self")
    SELF,

    /**
     *
     **/
    @Json(name = "m.pin")
    PIN
}
Timeline already handles the asset type and renders as a pin if it is different than m.self.

Thanks for the info! I will try to use it on my next PR to handle static pinned location sharing.

bmarty · 2022-03-09T14:34:12Z

library/ui-styles/src/main/res/values/stylable_location_sharing_option_picker_view.xml

+        <attr name="locShareIconBackgroundTint" format="color" />
+        <attr name="locShareIconPadding" format="dimension" />
+        <attr name="locShareIconDescription" format="string" />
+        <attr name="locShareTitle" format="string" />


Thanks for using a prefix, it's far better when attributes are used in the XML!

bmarty

Thanks for the update!
(static review only)

mnaturel assigned gaelledel Mar 3, 2022

gaelledel approved these changes Mar 3, 2022

View reviewed changes

mnaturel unassigned gaelledel Mar 3, 2022

mnaturel marked this pull request as ready for review March 3, 2022 10:58

ariskotsomitopoulos approved these changes Mar 3, 2022

View reviewed changes

bmarty requested changes Mar 3, 2022

View reviewed changes

Maxime Naturel added 19 commits March 9, 2022 09:44

Adding custom view for options picker

a98f502

Adding changelog entry

1c6b310

Set options method

55c6383

Adding other options

4586426

Adding icon for live location

3bbb716

Using correct string resources for options

0707877

Adding dividers

fb76402

Changing color of the live location icon

8bcc594

Fixing ripple effect

42fca9b

Adding user avatar and color for current location option

b11f7f2

Renaming package to option

d36409d

Setting colorSurface for background

e4c9acd

Fixing code syntax

edc77b0

Set option to user current location

8f362d8

Use user color for pin background

7cad30e

View extension methods to tint current background

9b6811c

Remove usage of strings to be deleted

82515cf

Rollback wrong sharing options

3aa1032

Fix live location color on light theme

f4b5353

Maxime Naturel added 6 commits March 9, 2022 09:46

Fix comment form in enum

442d722

Renaming picker view method

9adb87b

Moving view attributes into uikit dedicated file

046d906

Ignore unused strings to be deleted

6515e45

Fix attributes style id conflicts

89db867

Fix wrong click listener on the current available option

2bdafde

mnaturel force-pushed the feature/mna/PSF-735-sharing-options-view branch from 4718c00 to 2bdafde Compare March 9, 2022 09:16

bmarty reviewed Mar 9, 2022

View reviewed changes

bmarty approved these changes Mar 9, 2022

View reviewed changes

mnaturel merged commit f12afe0 into develop Mar 9, 2022

mnaturel deleted the feature/mna/PSF-735-sharing-options-view branch March 9, 2022 15:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#5395: Location sharing options view #5411

#5395: Location sharing options view #5411

mnaturel commented Mar 3, 2022 •

edited

Loading

github-actions bot commented Mar 3, 2022 •

edited

Loading

mnaturel commented Mar 3, 2022

gaelledel left a comment

ariskotsomitopoulos left a comment

ariskotsomitopoulos Mar 3, 2022

mnaturel Mar 3, 2022

bmarty left a comment

bmarty Mar 3, 2022

mnaturel Mar 3, 2022

bmarty Mar 3, 2022

mnaturel Mar 3, 2022

bmarty Mar 3, 2022

mnaturel Mar 3, 2022

bmarty Mar 3, 2022

mnaturel Mar 3, 2022

bmarty Mar 3, 2022

mnaturel commented Mar 4, 2022

onurays commented Mar 8, 2022 •

edited

Loading

mnaturel commented Mar 9, 2022

bmarty Mar 9, 2022

bmarty left a comment

#5395: Location sharing options view #5411

#5395: Location sharing options view #5411

Conversation

mnaturel commented Mar 3, 2022 • edited Loading

Type of change

Content

Motivation and context

Screenshots / GIFs

Current visible changes in the app

Changes not currently visible in the app

Tests

Tested devices

Checklist

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

Unit Test Results

mnaturel commented Mar 3, 2022

gaelledel left a comment

Choose a reason for hiding this comment

ariskotsomitopoulos left a 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

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 commented Mar 4, 2022

onurays commented Mar 8, 2022 • edited Loading

mnaturel commented Mar 9, 2022

Choose a reason for hiding this comment

bmarty left a comment

Choose a reason for hiding this comment

mnaturel commented Mar 3, 2022 •

edited

Loading

github-actions bot commented Mar 3, 2022 •

edited

Loading

onurays commented Mar 8, 2022 •

edited

Loading