Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix location crash #5084

Merged
merged 23 commits into from
Jan 31, 2022
Merged
Changes from 14 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
7242f1c
Fix a crash when disabling the location on the device
bmarty Jan 27, 2022
db3f60b
Inform the user when the location provider is disabled.
bmarty Jan 27, 2022
e3242f0
Prevent the dialog from being cancellable, since OK button finish the…
bmarty Jan 27, 2022
e9b9406
Rework the location code - WIP
bmarty Jan 27, 2022
1f53945
Rework the location code - WIP
bmarty Jan 27, 2022
55ed737
Rework the location code - WIP
bmarty Jan 27, 2022
26c0fee
Add a loader waiting for the user location to be known
bmarty Jan 27, 2022
0f8c3bc
Try to get location by using all available providers.
onurays Jan 28, 2022
4026ddb
Fix multiple pin rendering.
onurays Jan 28, 2022
50279e3
Use static map image in timeline.
onurays Jan 28, 2022
2dc52da
Use static map image in bottom sheet.
onurays Jan 28, 2022
e0ac8ee
No need for an extra FrameLayout
bmarty Jan 28, 2022
eff6942
Use a MaterialCarView
bmarty Jan 28, 2022
2ce3894
Create a UrlMapProvider for a better handling of RTL languages, and b…
bmarty Jan 28, 2022
b14e557
Use the existing item click mechanism
bmarty Jan 29, 2022
83ed80e
Rename fun for clarity
bmarty Jan 29, 2022
303a858
Create an extension, improve the parsing algorithm, add robustness an…
bmarty Jan 29, 2022
99f82d9
Avoid taking into account network location if we have gps location.
bmarty Jan 29, 2022
a8c251f
Avoid taking into account any provider location if we have gps location.
bmarty Jan 29, 2022
2fbb434
Format
bmarty Jan 29, 2022
ecd41d3
network "not live" lcoation can be more accurate than GPS "not live" …
bmarty Jan 31, 2022
4e3c730
Changelog
bmarty Jan 31, 2022
8ee23c1
Merge branch 'develop' into feature/bma/location_crash
bmarty Jan 31, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions library/ui-styles/src/main/res/values-ldrtl/bools.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<?xml version="1.0" encoding="utf-8"?>
<resources>

<bool name="is_rtl">true</bool>

</resources>
2 changes: 2 additions & 0 deletions library/ui-styles/src/main/res/values/bools.xml
Original file line number Diff line number Diff line change
@@ -4,4 +4,6 @@
<!-- Created to detect what has to be implemented (especially in the settings) -->
<bool name="false_not_implemented">false</bool>

<bool name="is_rtl">false</bool>

</resources>
Original file line number Diff line number Diff line change
@@ -17,24 +17,25 @@
package im.vector.app.core.epoxy.bottomsheet

import android.text.method.MovementMethod
import android.widget.FrameLayout
import android.widget.ImageView
import android.widget.TextView
import androidx.core.view.isVisible
import com.airbnb.epoxy.EpoxyAttribute
import com.airbnb.epoxy.EpoxyModelClass
import com.bumptech.glide.request.RequestOptions
import im.vector.app.R
import im.vector.app.core.epoxy.ClickListener
import im.vector.app.core.epoxy.VectorEpoxyHolder
import im.vector.app.core.epoxy.VectorEpoxyModel
import im.vector.app.core.epoxy.onClick
import im.vector.app.core.extensions.setTextOrHide
import im.vector.app.core.glide.GlideApp
import im.vector.app.features.displayname.getBestName
import im.vector.app.features.home.AvatarRenderer
import im.vector.app.features.home.room.detail.timeline.helper.LocationPinProvider
import im.vector.app.features.home.room.detail.timeline.item.BindingOptions
import im.vector.app.features.home.room.detail.timeline.tools.findPillsAndProcess
import im.vector.app.features.location.LocationData
import im.vector.app.features.location.MapTilerMapView
import im.vector.app.features.media.ImageContentRenderer
import im.vector.lib.core.utils.epoxy.charsequence.EpoxyCharSequence
import org.matrix.android.sdk.api.util.MatrixItem
@@ -70,7 +71,7 @@ abstract class BottomSheetMessagePreviewItem : VectorEpoxyModel<BottomSheetMessa
var time: String? = null

@EpoxyAttribute
var locationData: LocationData? = null
var locationUrl: String? = null

@EpoxyAttribute
var locationPinProvider: LocationPinProvider? = null
@@ -97,17 +98,21 @@ abstract class BottomSheetMessagePreviewItem : VectorEpoxyModel<BottomSheetMessa
body.charSequence.findPillsAndProcess(coroutineScope) { it.bind(holder.body) }
holder.timestamp.setTextOrHide(time)

holder.mapView.isVisible = locationData != null
holder.body.isVisible = locationData == null
locationData?.let { location ->
holder.mapView.initialize {
if (holder.view.isAttachedToWindow) {
holder.mapView.zoomToLocation(location.latitude, location.longitude, 15.0)
locationPinProvider?.create(matrixItem.id) { pinDrawable ->
holder.mapView.addPinToMap(matrixItem.id, pinDrawable)
holder.mapView.updatePinLocation(matrixItem.id, location.latitude, location.longitude)
}
}
if (locationUrl == null) {
holder.body.isVisible = true
holder.mapViewContainer.isVisible = false
} else {
holder.body.isVisible = false
holder.mapViewContainer.isVisible = true
GlideApp.with(holder.staticMapImageView)
.load(locationUrl)
.apply(RequestOptions.centerCropTransform())
.into(holder.staticMapImageView)

locationPinProvider?.create(matrixItem.id) { pinDrawable ->
GlideApp.with(holder.staticMapPinImageView)
.load(pinDrawable)
.into(holder.staticMapPinImageView)
}
}
}
@@ -124,6 +129,8 @@ abstract class BottomSheetMessagePreviewItem : VectorEpoxyModel<BottomSheetMessa
val bodyDetails by bind<TextView>(R.id.bottom_sheet_message_preview_body_details)
val timestamp by bind<TextView>(R.id.bottom_sheet_message_preview_timestamp)
val imagePreview by bind<ImageView>(R.id.bottom_sheet_message_preview_image)
val mapView by bind<MapTilerMapView>(R.id.bottom_sheet_message_preview_location)
val mapViewContainer by bind<FrameLayout>(R.id.mapViewContainer)
val staticMapImageView by bind<ImageView>(R.id.staticMapImageView)
val staticMapPinImageView by bind<ImageView>(R.id.staticMapPinImageView)
}
}
Original file line number Diff line number Diff line change
@@ -39,7 +39,9 @@ import im.vector.app.features.home.room.detail.timeline.item.E2EDecoration
import im.vector.app.features.home.room.detail.timeline.tools.createLinkMovementMethod
import im.vector.app.features.home.room.detail.timeline.tools.linkify
import im.vector.app.features.html.SpanUtils
import im.vector.app.features.location.INITIAL_MAP_ZOOM_IN_TIMELINE
import im.vector.app.features.location.LocationData
import im.vector.app.features.location.UrlMapProvider
import im.vector.app.features.media.ImageContentRenderer
import im.vector.lib.core.utils.epoxy.charsequence.toEpoxyCharSequence
import org.matrix.android.sdk.api.extensions.orFalse
@@ -62,6 +64,7 @@ class MessageActionsEpoxyController @Inject constructor(
private val spanUtils: SpanUtils,
private val eventDetailsFormatter: EventDetailsFormatter,
private val dateFormatter: VectorDateFormatter,
private val urlMapProvider: UrlMapProvider,
private val locationPinProvider: LocationPinProvider
) : TypedEpoxyController<MessageActionState>() {

@@ -74,9 +77,11 @@ class MessageActionsEpoxyController @Inject constructor(
val formattedDate = dateFormatter.format(date, DateFormatKind.MESSAGE_DETAIL)
val body = state.messageBody.linkify(host.listener)
val bindingOptions = spanUtils.getBindingOptions(body)
val locationData = state.timelineEvent()?.root?.getClearContent()?.toModel<MessageLocationContent>(catchError = true)?.let {
LocationData.create(it.getUri())
}
val locationUrl = state.timelineEvent()?.root?.getClearContent()
?.toModel<MessageLocationContent>(catchError = true)
?.let { LocationData.create(it.getUri()) }
?.let { urlMapProvider.buildStaticMapUrl(it, INITIAL_MAP_ZOOM_IN_TIMELINE, 1200, 800) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!


bottomSheetMessagePreviewItem {
id("preview")
avatarRenderer(host.avatarRenderer)
@@ -89,7 +94,7 @@ class MessageActionsEpoxyController @Inject constructor(
body(body.toEpoxyCharSequence())
bodyDetails(host.eventDetailsFormatter.format(state.timelineEvent()?.root)?.toEpoxyCharSequence())
time(formattedDate)
locationData(locationData)
locationUrl(locationUrl)
locationPinProvider(host.locationPinProvider)
}

Original file line number Diff line number Diff line change
@@ -16,6 +16,7 @@

package im.vector.app.features.home.room.detail.timeline.factory

import android.content.res.Resources
import android.text.Spannable
import android.text.SpannableStringBuilder
import android.text.Spanned
@@ -71,7 +72,9 @@ import im.vector.app.features.html.EventHtmlRenderer
import im.vector.app.features.html.PillsPostProcessor
import im.vector.app.features.html.SpanUtils
import im.vector.app.features.html.VectorHtmlCompressor
import im.vector.app.features.location.INITIAL_MAP_ZOOM_IN_TIMELINE
import im.vector.app.features.location.LocationData
import im.vector.app.features.location.UrlMapProvider
import im.vector.app.features.media.ImageContentRenderer
import im.vector.app.features.media.VideoContentRenderer
import im.vector.app.features.settings.VectorPreferences
@@ -127,7 +130,10 @@ class MessageItemFactory @Inject constructor(
private val session: Session,
private val voiceMessagePlaybackTracker: VoiceMessagePlaybackTracker,
private val locationPinProvider: LocationPinProvider,
private val vectorPreferences: VectorPreferences) {
private val vectorPreferences: VectorPreferences,
private val urlMapProvider: UrlMapProvider,
private val resources: Resources
) {

// TODO inject this properly?
private var roomId: String = ""
@@ -207,9 +213,16 @@ class MessageItemFactory @Inject constructor(
}
}

val width = resources.displayMetrics.widthPixels - dimensionConverter.dpToPx(60)
val height = dimensionConverter.dpToPx(200)

val locationUrl = locationData?.let {
urlMapProvider.buildStaticMapUrl(it, INITIAL_MAP_ZOOM_IN_TIMELINE, width, height)
}

return MessageLocationItem_()
.attributes(attributes)
.locationData(locationData)
.locationUrl(locationUrl)
.userId(informationData.senderId)
.locationPinProvider(locationPinProvider)
.highlighted(highlight)
Original file line number Diff line number Diff line change
@@ -28,6 +28,7 @@ import im.vector.app.core.glide.GlideApp
import im.vector.app.core.utils.DimensionConverter
import im.vector.app.features.home.AvatarRenderer
import org.matrix.android.sdk.api.util.toMatrixItem
import timber.log.Timber
import javax.inject.Inject
import javax.inject.Singleton

@@ -54,22 +55,36 @@ class LocationPinProvider @Inject constructor(
val size = dimensionConverter.dpToPx(44)
avatarRenderer.render(glideRequests, it, object : CustomTarget<Drawable>(size, size) {
override fun onResourceReady(resource: Drawable, transition: Transition<in Drawable>?) {
val bgUserPin = ContextCompat.getDrawable(context, R.drawable.bg_map_user_pin)!!
val layerDrawable = LayerDrawable(arrayOf(bgUserPin, resource))
val horizontalInset = dimensionConverter.dpToPx(4)
val topInset = dimensionConverter.dpToPx(4)
val bottomInset = dimensionConverter.dpToPx(8)
layerDrawable.setLayerInset(1, horizontalInset, topInset, horizontalInset, bottomInset)

cache[userId] = layerDrawable

callback(layerDrawable)
Timber.d("## Location: onResourceReady")
val pinDrawable = createPinDrawable(resource)
cache[userId] = pinDrawable
callback(pinDrawable)
}

override fun onLoadCleared(placeholder: Drawable?) {
// Is it possible? Put placeholder instead?
// FIXME The doc says it has to be implemented and should free resources
Timber.d("## Location: onLoadCleared")
}

override fun onLoadFailed(errorDrawable: Drawable?) {
Timber.w("## Location: onLoadFailed")
errorDrawable ?: return
val pinDrawable = createPinDrawable(errorDrawable)
cache[userId] = pinDrawable
callback(pinDrawable)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes the issue when the user does not have an avatar

}
})
}
}

private fun createPinDrawable(drawable: Drawable): Drawable {
val bgUserPin = ContextCompat.getDrawable(context, R.drawable.bg_map_user_pin)!!
val layerDrawable = LayerDrawable(arrayOf(bgUserPin, drawable))
val horizontalInset = dimensionConverter.dpToPx(4)
val topInset = dimensionConverter.dpToPx(4)
val bottomInset = dimensionConverter.dpToPx(8)
layerDrawable.setLayerInset(1, horizontalInset, topInset, horizontalInset, bottomInset)
return layerDrawable
}
}
Original file line number Diff line number Diff line change
@@ -16,15 +16,14 @@

package im.vector.app.features.home.room.detail.timeline.item

import android.widget.FrameLayout
import androidx.constraintlayout.widget.ConstraintLayout
import android.widget.ImageView
import com.airbnb.epoxy.EpoxyAttribute
import com.airbnb.epoxy.EpoxyModelClass
import com.bumptech.glide.request.RequestOptions
import im.vector.app.R
import im.vector.app.core.epoxy.onClick
import im.vector.app.core.glide.GlideApp
import im.vector.app.features.home.room.detail.timeline.helper.LocationPinProvider
import im.vector.app.features.location.LocationData
import im.vector.app.features.location.MapTilerMapView

@EpoxyModelClass(layout = R.layout.item_timeline_event_base)
abstract class MessageLocationItem : AbsMessageItem<MessageLocationItem.Holder>() {
@@ -37,7 +36,7 @@ abstract class MessageLocationItem : AbsMessageItem<MessageLocationItem.Holder>(
var callback: Callback? = null

@EpoxyAttribute
var locationData: LocationData? = null
var locationUrl: String? = null

@EpoxyAttribute
var userId: String? = null
@@ -47,37 +46,35 @@ abstract class MessageLocationItem : AbsMessageItem<MessageLocationItem.Holder>(

override fun bind(holder: Holder) {
super.bind(holder)
renderSendState(holder.mapViewContainer, null)
renderSendState(holder.view, null)

val location = locationData ?: return
val location = locationUrl ?: return
val locationOwnerId = userId ?: return

holder.clickableMapArea.onClick {
holder.view.onClick {
callback?.onMapClicked()
}

holder.mapView.apply {
initialize {
zoomToLocation(location.latitude, location.longitude, INITIAL_ZOOM)
GlideApp.with(holder.staticMapImageView)
.load(location)
.apply(RequestOptions.centerCropTransform())
.into(holder.staticMapImageView)

locationPinProvider?.create(locationOwnerId) { pinDrawable ->
addPinToMap(locationOwnerId, pinDrawable)
updatePinLocation(locationOwnerId, location.latitude, location.longitude)
}
}
locationPinProvider?.create(locationOwnerId) { pinDrawable ->
GlideApp.with(holder.staticMapPinImageView)
.load(pinDrawable)
.into(holder.staticMapPinImageView)
}
}

override fun getViewType() = STUB_ID

class Holder : AbsMessageItem.Holder(STUB_ID) {
val mapViewContainer by bind<ConstraintLayout>(R.id.mapViewContainer)
val mapView by bind<MapTilerMapView>(R.id.mapView)
val clickableMapArea by bind<FrameLayout>(R.id.clickableMapArea)
val staticMapImageView by bind<ImageView>(R.id.staticMapImageView)
val staticMapPinImageView by bind<ImageView>(R.id.staticMapPinImageView)
}

companion object {
private const val STUB_ID = R.id.messageContentLocationStub
private const val INITIAL_ZOOM = 15.0
}
}
10 changes: 7 additions & 3 deletions vector/src/main/java/im/vector/app/features/location/Config.kt
Original file line number Diff line number Diff line change
@@ -16,6 +16,10 @@

package im.vector.app.features.location

const val INITIAL_MAP_ZOOM = 15.0
const val MIN_TIME_MILLIS_TO_UPDATE_LOCATION = 1 * 60 * 1000L // every 1 minute
const val MIN_DISTANCE_METERS_TO_UPDATE_LOCATION = 10f
const val MAP_BASE_URL = "https://api.maptiler.com/maps/streets/style.json"
const val STATIC_MAP_BASE_URL = "https://api.maptiler.com/maps/basic/static/"

const val INITIAL_MAP_ZOOM_IN_PREVIEW = 15.0
const val INITIAL_MAP_ZOOM_IN_TIMELINE = 17.0
const val MIN_TIME_TO_UPDATE_LOCATION_MILLIS = 5 * 1_000L // every 5 seconds
const val MIN_DISTANCE_TO_UPDATE_LOCATION_METERS = 10f
Loading