Fix location crash #5084

bmarty · 2022-01-27T15:31:52Z

When the user want to share their location and then disable the location on their device, there was a crash on Android < R.

Now the user get a dialog and can safely close the screen.

Update

I use the same PR to fix more implementation issue:

Some lifecycle method was not called.
Add a ProgressBar when the user wait to see his location. It can take several seconds on my device
Move some logic to the ViewModel.
pin was not rendered if the user has no avatar
fix race condition between map ready / location update
NEW Give priority to GPS provider over the other ones

There is still some problems:

a regression from this PR: In a timeline, it there are several location events, the pin is not displayed on each item. I am pretty sure it's a stupid bug, I will investigate later.
In the timeline we do not cleanup properly the maps, I think it can lead to bug due to bad releasing of resources.
Same problem in the bottomsheet (message detail)

`LocationListener` does not have default implementation for some methods for Android versions below R

onurays

Surprised, approved, thanks a lot for testing.

… Activity.

github-actions · 2022-01-27T16:18:36Z

Unit Test Results

  76 files +4   76 suites +4 54s ⏱️ -1s
143 tests +2 143 ✔️ +2 0 💤 ±0 0 ❌ ±0
448 runs +8 448 ✔️ +8 0 💤 ±0 0 ❌ ±0

Results for commit 8ee23c1. ± Comparison against base commit a6ae709.

♻️ This comment has been updated with latest results.

github-actions · 2022-01-27T16:24:13Z

Matrix SDK

Integration Tests Results:

[org.matrix.android.sdk.session]
passed="21" failures="0" errors="0" skipped="2"
[org.matrix.android.sdk.account]
passed="5" failures="0" errors="0" skipped="2"
[org.matrix.android.sdk.internal]
passed="158" failures="1" errors="0" skipped="38"
[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"

bmarty · 2022-01-27T21:37:49Z

...src/main/java/im/vector/app/features/home/room/detail/timeline/helper/LocationPinProvider.kt

+ errorDrawable ?: return
+ val pinDrawable = createPinDrawable(errorDrawable)
+ cache[userId] = pinDrawable
+ callback(pinDrawable)


Fixes the issue when the user does not have an avatar

bmarty · 2022-01-27T21:40:53Z

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

@@ -18,15 +18,9 @@ package im.vector.app.features.location

 import android.graphics.drawable.Drawable

-interface VectorMapView {


This interface was implemented but not really used as a type

Yes, I tried to be flexible in case of changing the map library but has problems with epoxy.

bmarty · 2022-01-27T21:41:39Z

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

+ style
+ )
+ pendingState?.let { render(it) }
+ pendingState = null


Trying to fix race condition here

bmarty · 2022-01-27T21:42:33Z

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

- return map?.cameraPosition?.zoom
- }
-
- override fun onClick(callback: () -> Unit) {


this was not used

bmarty · 2022-01-27T21:43:11Z

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

+ android:layout_marginEnd="@dimen/layout_horizontal_margin"
+ app:layout_constraintBottom_toBottomOf="@id/shareLocationContainer"
+ app:layout_constraintEnd_toEndOf="@id/shareLocationContainer"
+ app:layout_constraintTop_toTopOf="@id/shareLocationContainer" />


bmarty · 2022-01-28T12:21:59Z

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

+ callback?.onLocationUpdate(lastKnownLocation.toLocationData())
+ }
+
+ locationManager.requestLocationUpdates(


Is it OK to call this multiple times this fun and call only once locationManager?.removeUpdates(this) in onStop? Maybe yes but I want to be sure.

Yes, you can see the location icon on Status Bar is disappearing when fragment is destroyed.

Ah didn't notice that. It does not disappear on my side

(I have to put the entire app into bg to make it disappear to be more precise.

ouchadam

working well in my local test

DoM1niC · 2022-01-28T20:38:08Z

for me the Marker is there but no Map Render in Timeline but the rest to seems work now :)

…uild the URLs in the controllers

bmarty · 2022-01-28T21:55:08Z

for me the Marker is there but no Map Render in Timeline but the rest to seems work now :)

Thanks for your feedback!

Loading images in the timeline can take a few seconds but is working here.

onurays · 2022-01-28T22:30:49Z

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

+ append(keyParam)
+ if (!resources.getBoolean(R.bool.is_rtl)) {
+ // On LTR languages we want the legal mentions to be displayed on the bottom left of the image
+ append("&attribution=bottomleft")


Nice catch.

onurays

Thank you for the great collaboration!

DoM1niC · 2022-01-29T00:24:25Z

OS: Oxygen OS 11 based on Android 11
Mhhh... I wait more then 30 Sec without luck here 🥺
Video
https://user-images.githubusercontent.com/1436958/151638693-96a96509-34dd-4470-b334-5a014820ff02.mp4

for me the Marker is there but no Map Render in Timeline but the rest to seems work now :)

Thanks for your feedback!

Loading images in the timeline can take a few seconds but is working here.

…d unit test it

bmarty · 2022-01-29T07:58:19Z

@DoM1niC any chance that you put a breakpoint here and get the URL location, then try to open it from the browser of your device? The image should be displayed.

onurays · 2022-01-31T08:37:57Z

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

+ }
+
+ @Test
+ fun invalidCases() {


Perfect, thanks for the test cases!

onurays · 2022-01-31T08:48:02Z

@DoM1niC any chance that you put a breakpoint here and get the URL location, then try to open it from the browser of your device? The image should be displayed.

I can reproduce it

…location. So do not ignore them. Not sure how if this is a universal rule...

ouchadam · 2022-01-31T13:00:29Z

...ava/im/vector/app/features/home/room/detail/timeline/action/MessageActionsEpoxyController.kt

+ val locationUrl = state.timelineEvent()?.root?.getClearContent()
+ ?.toModel<MessageLocationContent>(catchError = true)
+ ?.toLocationData()
+ ?.let { urlMapProvider.buildStaticMapUrl(it, INITIAL_MAP_ZOOM_IN_TIMELINE, 1200, 800) }


nit: is it worth extracting these to width/height constants or using named params?

Yes, and I think the values are not correct (since not related to the actual ImageView size). Anyway, could be improved later, this is not a big issue, Glide will crop the image anyway (I think)
Thanks for the review!

bmarty added 2 commits January 27, 2022 16:18

Fix a crash when disabling the location on the device

7242f1c

`LocationListener` does not have default implementation for some methods for Android versions below R

Inform the user when the location provider is disabled.

db3f60b

bmarty requested a review from onurays January 27, 2022 15:31

bmarty mentioned this pull request Jan 27, 2022

[Release] Element Android v1.3.16 #5058

Closed

56 tasks

onurays approved these changes Jan 27, 2022

View reviewed changes

Prevent the dialog from being cancellable, since OK button finish the…

e3242f0

… Activity.

ouchadam approved these changes Jan 27, 2022

View reviewed changes

bmarty added 4 commits January 27, 2022 21:11

Rework the location code - WIP

e9b9406

Rework the location code - WIP

1f53945

Rework the location code - WIP

55ed737

Add a loader waiting for the user location to be known

26c0fee

bmarty requested a review from onurays January 27, 2022 21:33

bmarty commented Jan 27, 2022

View reviewed changes

bmarty requested a review from ouchadam January 28, 2022 08:39

onurays added 2 commits January 28, 2022 14:17

Try to get location by using all available providers.

0f8c3bc

Fix multiple pin rendering.

4026ddb

bmarty commented Jan 28, 2022

View reviewed changes

Use static map image in timeline.

50279e3

ouchadam approved these changes Jan 28, 2022

View reviewed changes

onurays and others added 3 commits January 28, 2022 20:48

Use static map image in bottom sheet.

2dc52da

No need for an extra FrameLayout

e0ac8ee

Use a MaterialCarView

eff6942

Create a UrlMapProvider for a better handling of RTL languages, and b…

2ce3894

…uild the URLs in the controllers

bmarty requested a review from ouchadam January 28, 2022 21:55

onurays reviewed Jan 28, 2022

View reviewed changes

onurays approved these changes Jan 28, 2022

View reviewed changes

bmarty added 6 commits January 29, 2022 07:39

Use the existing item click mechanism

b14e557

Rename fun for clarity

83ed80e

Create an extension, improve the parsing algorithm, add robustness an…

303a858

…d unit test it

Avoid taking into account network location if we have gps location.

99f82d9

Avoid taking into account any provider location if we have gps location.

a8c251f

Format

2fbb434

onurays reviewed Jan 31, 2022

View reviewed changes

bmarty and others added 3 commits January 31, 2022 12:47

network "not live" lcoation can be more accurate than GPS "not live" …

ecd41d3

…location. So do not ignore them. Not sure how if this is a universal rule...

Changelog

4e3c730

Merge branch 'develop' into feature/bma/location_crash

8ee23c1

ouchadam reviewed Jan 31, 2022

View reviewed changes

ouchadam approved these changes Jan 31, 2022

View reviewed changes

bmarty merged commit 91e444c into develop Jan 31, 2022

bmarty deleted the feature/bma/location_crash branch January 31, 2022 13:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix location crash #5084

Fix location crash #5084

bmarty commented Jan 27, 2022 •

edited

Loading

onurays left a comment

github-actions bot commented Jan 27, 2022 •

edited

Loading

github-actions bot commented Jan 27, 2022 •

edited

Loading

bmarty Jan 27, 2022

bmarty Jan 27, 2022

onurays Jan 28, 2022

bmarty Jan 27, 2022

bmarty Jan 27, 2022

bmarty Jan 27, 2022

bmarty Jan 28, 2022

onurays Jan 28, 2022

bmarty Jan 28, 2022

bmarty Jan 28, 2022

ouchadam left a comment

DoM1niC commented Jan 28, 2022

bmarty commented Jan 28, 2022

onurays Jan 28, 2022

onurays left a comment

DoM1niC commented Jan 29, 2022 •

edited

Loading

bmarty commented Jan 29, 2022

onurays Jan 31, 2022

onurays commented Jan 31, 2022

ouchadam Jan 31, 2022

bmarty Jan 31, 2022

		@@ -18,15 +18,9 @@ package im.vector.app.features.location

		import android.graphics.drawable.Drawable

		interface VectorMapView {

Fix location crash #5084

Fix location crash #5084

Conversation

bmarty commented Jan 27, 2022 • edited Loading

onurays left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 27, 2022 • edited Loading

Unit Test Results

github-actions bot commented Jan 27, 2022 • edited Loading

Matrix SDK

Integration Tests Results:

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

ouchadam left a comment

Choose a reason for hiding this comment

DoM1niC commented Jan 28, 2022

bmarty commented Jan 28, 2022

Choose a reason for hiding this comment

onurays left a comment

Choose a reason for hiding this comment

DoM1niC commented Jan 29, 2022 • edited Loading

bmarty commented Jan 29, 2022

Choose a reason for hiding this comment

onurays commented Jan 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmarty commented Jan 27, 2022 •

edited

Loading

github-actions bot commented Jan 27, 2022 •

edited

Loading

github-actions bot commented Jan 27, 2022 •

edited

Loading

DoM1niC commented Jan 29, 2022 •

edited

Loading