[Location sharing] - Navigation to Map view from live location message (PSF-888) #6092

mnaturel · 2022-05-18T13:57:40Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Add possibility to navigate to a map screen from a running live location share message.
User pins on map and other UI will be implemented in another pull request.

Motivation and context

Part of #6012

Screenshots / GIFs

Tests

Ensure to enable location sharing in Settings -> Preferences
Ensure to enable developer mode in Settings -> Advanced
Ensure to enable the feature flaf for live location sharing
Start a live location share in a room
Press the running live message
Check a new screen is opened with a map of the world
Press back
Stop the live
Check pressing on the ended live message does nothing

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-18T14:38:29Z

Unit Test Results

124 files +  2 124 suites +2 2m 10s ⏱️ +8s
217 tests +12 217 ✔️ +12 0 💤 ±0 0 ❌ ±0
714 runs +24 714 ✔️ +24 0 💤 ±0 0 ❌ ±0

Results for commit 7bb73ff. ± Comparison against base commit 738ce18.

♻️ This comment has been updated with latest results.

ganfra · 2022-05-19T09:51:42Z

...vector/app/features/home/room/detail/timeline/factory/LiveLocationShareMessageItemFactory.kt

@@ -75,7 +75,8 @@ class LiveLocationShareMessageItemFactory @Inject constructor(
        val height = dimensionConverter.dpToPx(MessageItemFactory.MESSAGE_LOCATION_ITEM_HEIGHT_IN_DP)

        return MessageLiveLocationInactiveItem_()
-                .attributes(attributes)
+                // disable the click on this state item
+                .attributes(attributes.copy(itemClickListener = null))


Maybe we can avoid to add the clickListener before or add a new boolean to disable instead?

I am not sure I can do better and more simple for that. Deciding whether the item should be clickable depends on the status of the live which is computed inside this factory using some fields of the summary data. Unless you have a solution in mind?

I agree it's a bit strange to do like this. Also I am pretty sure that in the future, if we want the item to be clickable, we may hardly understand why the item is not clickable because of this.

Not a blocker anyway.

ganfra · 2022-05-19T09:53:32Z

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

+/**
+ * Screen showing a map with all the current users sharing their live location in room.
+ */
+class LocationLiveMapViewFragment @Inject constructor(


Maybe start using @androidentrypoint for fragment with field injection?

Yes, I will try to make the change.

See c46aaa2

ganfra

Some small remarks, otherwise LGTM

bmarty · 2022-05-23T08:47:48Z

vector/src/main/java/im/vector/app/core/di/FragmentModule.kt

+    /*@Binds
+    @IntoMap
+    @FragmentKey(LocationLiveMapViewFragment::class)
+    fun bindLocationLiveMapViewFragment(fragment: LocationLiveMapViewFragment): Fragment*/


Please remove commented out code, or will it be uncommented in a future PR?

Oups not intended, sorry.

I am wondering if we could have this type of auto verification (no commented code) in Danger?

Sonar is checking that

Removed in 721d7cb

bmarty · 2022-05-23T08:51:53Z

...vector/app/features/home/room/detail/timeline/factory/LiveLocationShareMessageItemFactory.kt

@@ -75,7 +75,8 @@ class LiveLocationShareMessageItemFactory @Inject constructor(
        val height = dimensionConverter.dpToPx(MessageItemFactory.MESSAGE_LOCATION_ITEM_HEIGHT_IN_DP)

        return MessageLiveLocationInactiveItem_()
-                .attributes(attributes)
+                // disable the click on this state item
+                .attributes(attributes.copy(itemClickListener = null))


I agree it's a bit strange to do like this. Also I am pretty sure that in the future, if we want the item to be clickable, we may hardly understand why the item is not clickable because of this.

Not a blocker anyway.

bmarty · 2022-05-23T08:52:55Z

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

+
+/**
+ * Screen showing a map with all the current users sharing their live location in room.
+ */


Thanks for the comment!

bmarty · 2022-05-23T08:58:00Z

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

+                            val fragment = SupportMapFragment.newInstance(options)
+                            addFragment(R.id.liveLocationMapContainer, fragment, tag = MAP_FRAGMENT_TAG)
+                            fragment
+                        }


Maybe reduce to

?: run { val options = MapboxMapOptions.createFromAttributes(requireContext(), null) SupportMapFragment.newInstance(options) .also { addFragment(R.id.liveLocationMapContainer, it, tag = MAP_FRAGMENT_TAG) } }

Also maybe extract to a private fun getOrCreateSupportMapFragment() for code clarity.

Thanks for the suggestion. Done in b331521

bmarty · 2022-05-23T08:58:49Z

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

+
+    private fun setupMap() {
+        val mapFragment: SupportMapFragment =
+                parentFragmentManager.findFragmentByTag(MAP_FRAGMENT_TAG) as? SupportMapFragment


I think you should use the childFramentManager here

bmarty · 2022-05-23T08:59:20Z

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

+                        ?: run {
+                            val options = MapboxMapOptions.createFromAttributes(requireContext(), null)
+                            val fragment = SupportMapFragment.newInstance(options)
+                            addFragment(R.id.liveLocationMapContainer, fragment, tag = MAP_FRAGMENT_TAG)


I think you should use addChildFragment here.

bmarty · 2022-05-23T09:02:19Z

vector/src/main/res/layout/fragment_live_location_map.xml

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


Maybe rename the file to something more generic and reusable like fragment_simple_container.xml. I have check and a similar fragment layout does not exist yet.

Done in 7bb73ff

bmarty

Thanks for the update!
As discussed, OK for the click listener management.

github-actions · 2022-05-23T12:37:58Z

Matrix SDK

Integration Tests Results:

[org.matrix.android.sdk.session]
= passed=19 failures=1 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

Maxime NATUREL added 7 commits May 18, 2022 14:44

Adding changelog entry

b2765e4

Creation of map view screen and basic navigation

52c0fa4

Updating the title of the map view screen

54d7d47

Use SupportMapFragment instead of custom fragment

26cddd2

Rollback to Fragment to be able to use Maverick capabilities

23e8cad

Enable navigation only from running state item

4864980

Remove unused imports

5265856

mnaturel requested review from a team, bmarty and ganfra and removed request for a team May 18, 2022 13:58

ganfra reviewed May 19, 2022

View reviewed changes

ganfra approved these changes May 19, 2022

View reviewed changes

Using @androidentrypoint in Fragment

c46aaa2

bmarty requested changes May 23, 2022

View reviewed changes

Maxime NATUREL added 3 commits May 23, 2022 11:30

Remove commented code

721d7cb

Improving code in Fragment

b331521

Making the layout file more generic

7bb73ff

mnaturel requested a review from bmarty May 23, 2022 09:45

bmarty approved these changes May 23, 2022

View reviewed changes

mnaturel merged commit 9a38d59 into develop May 23, 2022

mnaturel deleted the feature/mna/PSF-888-navigation branch May 23, 2022 11:54

mnaturel mentioned this pull request May 30, 2022

[Location sharing] - Show user live location pins in map view (PSF-1019) #6129

Merged

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Location sharing] - Navigation to Map view from live location message (PSF-888) #6092

[Location sharing] - Navigation to Map view from live location message (PSF-888) #6092

mnaturel commented May 18, 2022

github-actions bot commented May 18, 2022 •

edited

Loading

ganfra May 19, 2022

mnaturel May 23, 2022 •

edited

Loading

bmarty May 23, 2022

ganfra May 19, 2022

mnaturel May 23, 2022

mnaturel May 23, 2022

ganfra left a comment

bmarty May 23, 2022

mnaturel May 23, 2022

mnaturel May 23, 2022

bmarty May 23, 2022

mnaturel May 23, 2022

bmarty May 23, 2022

bmarty May 23, 2022

bmarty May 23, 2022

mnaturel May 23, 2022

bmarty May 23, 2022

bmarty May 23, 2022

bmarty May 23, 2022

mnaturel May 23, 2022

bmarty left a comment

github-actions bot commented May 23, 2022

[Location sharing] - Navigation to Map view from live location message (PSF-888) #6092

[Location sharing] - Navigation to Map view from live location message (PSF-888) #6092

Conversation

mnaturel commented May 18, 2022

Type of change

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

github-actions bot commented May 18, 2022 • edited Loading

Unit Test Results

Choose a reason for hiding this comment

mnaturel May 23, 2022 • edited Loading

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

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

bmarty left a comment

Choose a reason for hiding this comment

github-actions bot commented May 23, 2022

Matrix SDK

Integration Tests Results:

github-actions bot commented May 18, 2022 •

edited

Loading

mnaturel May 23, 2022 •

edited

Loading