Refactored location sharing timeline items to use the mapserver configured by the wellknown file #7852

artkoenig · 2022-12-27T15:49:09Z

Type of change

Feature
Bugfix
Technical
Other :

Content

The map server configuration (style.json), provided by the wellknown file, should be used for all use-cases, showing locations on a map.

Motivation and context

The map configuration (style.json), provided by the wellknown is not used for displaying the location in the timeline. Currently the Maptiler static map API is used for this, which is loading map data from the other source. This is problematic, due to copyright and/or network limitations, which may prohibit the use of map sources other than specified in the wellknown file.

This PR also removes the static attribution string, hard-coded in the app. The attribution should be defined in the style.json file to match the used data source.

Tested devices

Physical
Emulator
OS version(s):
API 21, API 33

Signed-off-by: Artjom König artjom.koenig.ext@bwi.de

…known file

mnaturel

Thanks for raising this PR! I have added some comments I think we should address.
Also, when testing the build in "Matrix.org", I could not test the changes since the location icon was no more visible in the composer. It seems to confirm one of my comments about hiding the feature based on the "Well Known" server config.

In fact, Matrix.org does not have a map tiler config so we should have a fallback in a certain way.
Could you please test again using Matrix.org?

Also, I don't know if you have done it but could you perform a Leak Canary analysis when scrolling the timeline to see if there is any leak? It can be done by enabling the developer mode in Advanced settings. Then, on the home screen there will be a small gear icon on top right corner. Pressing it will open the debug screen with a "Memory leaks" entry.

vector/src/main/java/im/vector/app/features/attachments/AttachmentTypeSelectorViewModel.kt

vector/src/main/java/im/vector/app/features/home/HomeActivityViewModel.kt

...or/src/main/java/im/vector/app/features/home/room/detail/composer/MessageComposerFragment.kt

mnaturel · 2022-12-28T11:02:17Z

vector/src/main/java/im/vector/app/core/epoxy/bottomsheet/BottomSheetMessagePreviewItem.kt

+            holder.staticMapView.getMapAsync { mapbox ->
+                mapbox.setStyle(safeLocationUiData.locationUrl)
+                safeLocationUiData.locationData?.let {
+                    mapbox.zoomToLocation(it, animate = false)


Here I think the zoom level may not be correct. What we want in the timeline is INITIAL_MAP_ZOOM_IN_TIMELINE. And zoomToLocation is not using this value but another one. We may have to change the extension method to pass the required zoom level.

mnaturel · 2022-12-28T11:09:49Z

...rc/main/java/im/vector/app/features/home/room/detail/timeline/item/AbsMessageLocationItem.kt

-                })
-                .transform(imageCornerTransformation)
-                .into(holder.staticMapImageView)
+        holder.staticMapView.addOnDidFailLoadingMapListener {


Some clean up about the map should be added in the unbind(holder: H) method. All listeners that can be removed should be removed.

vector/src/test/java/im/vector/app/features/attachments/AttachmentTypeSelectorViewModelTest.kt

mnaturel · 2022-12-28T13:14:15Z

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

-        private val rawService: RawService,
-        locationSharingConfig: LocationSharingConfig,
-) {
-    private val keyParam = "?key=${locationSharingConfig.mapTilerKey}"


I wonder if we should also remove the LocationSharingConfig class and the MapTiler API key.
@onurays What do you think? I am asking you since it appears you have added this key in the project and you may have more context about it.

After some tests, it seems a fallback is still necessary on Matrix.org for example.

I added the fallback implementation to the maptiler API, it is configurable via LocationSharingConfig

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

Johennes · 2023-01-02T08:51:07Z

My understanding was that we previously decided not to use the dynamic maps because of poor performance in the timeline. Have we falsified this concern?

artkoenig · 2023-01-03T12:36:23Z

@Johennes to favor performance over functional correctness is IMO problematic. The new solution is slower and more memory consuming (here some tests results for scrolling a timeline full with location events):

Old:

New:

And also network performance (for one location event in the timeline):
Old:

New:

But

The Web-Client and the iOS-Client already use the dynamic maps in the timeline - for the sake of consistency, the Android-App should have the same behavior
The solution has to respect the customer's mapstyle settings. As I stated above - in the enterprise setup, there are copyright- and network access limitations (due to security considerations) which prevent using other than specified map servers
Even with the worst case scenario (timeline is full with location events), the performance impact is not so bad that you could not use the solution

mnaturel

Thanks for the updates. I have added some new remarks mostly about code styling/naming I think we should handle for clarity.

mnaturel · 2023-01-03T13:23:34Z

...rc/main/java/im/vector/app/features/home/room/detail/timeline/item/AbsMessageLocationItem.kt

@@ -1,5 +1,6 @@
 /*
 * Copyright (c) 2021 New Vector Ltd
+ * Copyright (c) 2022 BWI GmbH


Why are there these changes of copyrights? I am not entirely sure is is allowed to be changed to something else than New Vector Ltd.

It looks like @artkoenig has added here the copyright notice to show that copyright goes to BWI GmbH.
It is fine to have the joint copyright here.

I'm not an expert in this area but I know there has been irritation about this topic recently. So just to double check that we're doing the correct thing here: Has this particular question been confirmed with legal?

mnaturel · 2023-01-03T13:27:45Z

...rc/main/java/im/vector/app/features/home/room/detail/timeline/item/AbsMessageLocationItem.kt

-            width = mapWidth
-            height = mapHeight
+
+        holder.staticMapView.apply {


As a general practice in the project, we try to avoid using apply as much as possible as it leads sometimes to strange bugs since the scope of this is changed inside.

mnaturel · 2023-01-03T13:31:33Z

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

@@ -18,4 +18,5 @@ package im.vector.app.features.location

 data class LocationSharingConfig(
        val mapTilerKey: String,
+        val isMapTilerFallbackEnabled: Boolean


Could you add a trailing comma here?

mnaturel · 2023-01-03T13:32:54Z

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

+fun MapboxMap?.zoomToLocation(
+        locationData: LocationData,
+        animate: Boolean = true,
+        zoomLevel: Double = INITIAL_MAP_ZOOM_IN_PREVIEW) {


I think there is a missing breaking line after the last parameter and also a missing trailing comma.

mnaturel · 2023-01-03T13:48:52Z

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

@@ -332,7 +332,7 @@ class LiveLocationMapViewFragment :
                .find { it.matrixItem.id == userId }
                ?.locationData
                ?.let { locationData ->
-                    mapboxMap?.get()?.zoomToLocation(locationData, preserveCurrentZoomLevel = false)
+                    mapboxMap?.get()?.zoomToLocation(locationData, true)


It think we should name the argument when assigning the value to make it clear about the purpose of the boolean..

Suggested change

mapboxMap?.get()?.zoomToLocation(locationData, true)

mapboxMap?.get()?.zoomToLocation(locationData, animate = true)

mnaturel · 2023-01-03T13:55:38Z

vector/src/test/java/im/vector/app/features/attachments/AttachmentTypeSelectorViewModelTest.kt

@@ -136,7 +135,7 @@ internal class AttachmentTypeSelectorViewModelTest {
        return AttachmentTypeSelectorViewModel(
                initialState,
                vectorFeatures = fakeVectorFeatures,
-                vectorPreferences = fakeVectorPreferences.instance,
+                vectorPreferences = fakeVectorPreferences.instance


Trailing comma should be kept accross the project.

mnaturel · 2023-01-03T13:57:07Z

vector/src/test/java/im/vector/app/features/location/UrlMapProviderTest.kt

@@ -0,0 +1,56 @@
+/*
+ * Copyright (c) 2022 BWI GmbH


Same comment as before about copyright of new files. I guess it should be New Vector Ltd.

mnaturel · 2023-01-03T13:59:31Z

vector/src/test/java/im/vector/app/features/location/UrlMapProviderTest.kt

+
+    @Test
+    fun `given enabled fallback to maptiler API, when map configuration is not set, then the fallback url should be returned`() {
+        val wellknownService = FakeWellknownService()


Is it possible to separate by a new line the different Given, When, Then blocks of code? This is a pattern we try to follow to ease readability of tests and also separate well what is part of each block.

mnaturel · 2023-01-03T14:01:19Z

vector/src/test/java/im/vector/app/features/location/UrlMapProviderTest.kt

+    fun `given enabled fallback to maptiler API, when map configuration is not set, then the fallback url should be returned`() {
+        val wellknownService = FakeWellknownService()
+        wellknownService.givenMissingMapConfiguration()
+        val urlMapProvider = UrlMapProvider(wellknownService.instance, LocationSharingConfig("", true))


I think we should use named parameter in these tests as well so that it is clear of the boolean purpose.

Suggested change

val urlMapProvider = UrlMapProvider(wellknownService.instance, LocationSharingConfig("", true))

val urlMapProvider = UrlMapProvider(wellknownService.instance, LocationSharingConfig("", isMapTilerFallbackEnabled = true))

mnaturel · 2023-01-03T14:03:01Z

vector/src/main/java/im/vector/app/features/attachments/AttachmentTypeSelectorViewModel.kt

@@ -27,6 +27,7 @@ import im.vector.app.core.platform.EmptyViewEvents
 import im.vector.app.core.platform.VectorViewModel
 import im.vector.app.core.platform.VectorViewModelAction
 import im.vector.app.features.VectorFeatures
+import im.vector.app.features.location.UrlMapProvider


This import should be removed.

Johennes · 2023-01-03T14:31:45Z

Thanks for compiling the performance data @artkoenig. The CPU activity does look quite a bit inflated, sadly. Seems like a factor of 5 in the peaks. 😞

I think it's reasonable to allow forks to use the vector maps with style.json if they can accept the performance overhead. I don't think we should force this onto all FOSS users though, especially since with the default style.json there seem to be little to no differences between the static and the vector maps.

What seems better to me is to add support for both variants to the well-known file and let clients pick (which coincidentally is what we already had planned here).

WDYT?

bmarty · 2023-01-03T16:16:46Z

Thanks for the PR. If I am correct it is addressing this comment?

artkoenig · 2023-01-04T12:11:46Z

@bmarty actually not

@Johennes after an internal discussion, we decided to implement this internally due to time constraints. I will adjust the implementation to keep the differences to FOSS as minimal as possible to avoid merge conflicts. We will wait until you provide an official solution for configuring the static map API via the well-known. Feel free to close the PR or postpone it for later use.

Johennes · 2023-01-05T13:40:29Z

Here's a screen recording of scrolling through a room with multiple location tiles on a Nokia 5.4 with a build from this PR:

vector.mp4

And here is a recording of the same action on the current store version of the app:

static.mp4

Unfortunately, the scrolling experience is quite poor with the vector maps compared to the static images.

artkoenig added 2 commits December 27, 2022 12:42

refactored location sharing to use the mapserver provided by the well…

b7a68ed

…known file

removed fixed attribution, added changelog

0f3e1cd

artkoenig changed the title ~~Refactored location sharing to use the mapserver provided by the wellknown file~~ Refactored location sharing timeline items to use the mapserver configured by the wellknown file Dec 27, 2022

fixed unit tests

2bb6cc8

mnaturel suggested changes Dec 28, 2022

View reviewed changes

artkoenig added 3 commits December 28, 2022 19:51

changes, based on feedback

dd976f4

refactored to KISS

8f2aa1e

fixed copyright info

96780ec

artkoenig requested a review from mnaturel December 29, 2022 11:57

mnaturel reviewed Jan 3, 2023

View reviewed changes

bmarty added the Z-Community-PR Issue is solved by a community member's PR label Jan 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored location sharing timeline items to use the mapserver configured by the wellknown file #7852

Refactored location sharing timeline items to use the mapserver configured by the wellknown file #7852

artkoenig commented Dec 27, 2022 •

edited

mnaturel left a comment

mnaturel Dec 28, 2022

artkoenig Dec 28, 2022

mnaturel Dec 28, 2022

artkoenig Dec 28, 2022

mnaturel Dec 28, 2022

mnaturel Dec 28, 2022

artkoenig Dec 28, 2022

Johennes commented Jan 2, 2023

artkoenig commented Jan 3, 2023

mnaturel left a comment

mnaturel Jan 3, 2023

yncyrydybyl Feb 15, 2023

Johennes Feb 15, 2023

mnaturel Jan 3, 2023

mnaturel Jan 3, 2023

mnaturel Jan 3, 2023

mnaturel Jan 3, 2023

mnaturel Jan 3, 2023

mnaturel Jan 3, 2023

mnaturel Jan 3, 2023

mnaturel Jan 3, 2023

mnaturel Jan 3, 2023

Johennes commented Jan 3, 2023

bmarty commented Jan 3, 2023

artkoenig commented Jan 4, 2023

Johennes commented Jan 5, 2023

	mapboxMap?.get()?.zoomToLocation(locationData, true)
	mapboxMap?.get()?.zoomToLocation(locationData, animate = true)

	val urlMapProvider = UrlMapProvider(wellknownService.instance, LocationSharingConfig("", true))
	val urlMapProvider = UrlMapProvider(wellknownService.instance, LocationSharingConfig("", isMapTilerFallbackEnabled = true))

Refactored location sharing timeline items to use the mapserver configured by the wellknown file #7852

Are you sure you want to change the base?

Refactored location sharing timeline items to use the mapserver configured by the wellknown file #7852

Conversation

artkoenig commented Dec 27, 2022 • edited

Type of change

Content

Motivation and context

Tested devices

mnaturel 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

Johennes commented Jan 2, 2023

artkoenig commented Jan 3, 2023

mnaturel 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

Johennes commented Jan 3, 2023

bmarty commented Jan 3, 2023

artkoenig commented Jan 4, 2023

Johennes commented Jan 5, 2023

artkoenig commented Dec 27, 2022 •

edited