From ff439546c51a85ab0ba02cd265b667a15801f573 Mon Sep 17 00:00:00 2001
From: Benoit Marty <benoit@matrix.org>
Date: Fri, 2 Feb 2024 15:34:51 +0100
Subject: [PATCH] Improve cache of drawables used for rendering location pin.
 In particular, use the Glide cache, and ensure that if an error occurs and
 later the avatar can be retrieved, the cache will be replaced. Also limit
 cache size to 32. Also use UserItem as a key, instead of just the userId, so
 that if displayName or avatarUrl change, there will be not cache hit.

---
 .../timeline/helper/LocationPinProvider.kt    | 38 ++++++++++++-------
 1 file changed, 25 insertions(+), 13 deletions(-)

diff --git a/vector/src/main/java/im/vector/app/features/home/room/detail/timeline/helper/LocationPinProvider.kt b/vector/src/main/java/im/vector/app/features/home/room/detail/timeline/helper/LocationPinProvider.kt
index 7f276f2f73..fb4169568c 100644
--- a/vector/src/main/java/im/vector/app/features/home/room/detail/timeline/helper/LocationPinProvider.kt
+++ b/vector/src/main/java/im/vector/app/features/home/room/detail/timeline/helper/LocationPinProvider.kt
@@ -19,7 +19,7 @@ package im.vector.app.features.home.room.detail.timeline.helper
 import android.content.Context
 import android.graphics.drawable.Drawable
 import android.graphics.drawable.LayerDrawable
-import androidx.annotation.ColorInt
+import android.util.LruCache
 import androidx.core.content.ContextCompat
 import androidx.core.graphics.drawable.DrawableCompat
 import com.bumptech.glide.request.target.CustomTarget
@@ -30,11 +30,17 @@ 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.session.getUserOrDefault
+import org.matrix.android.sdk.api.util.MatrixItem
 import org.matrix.android.sdk.api.util.toMatrixItem
 import timber.log.Timber
 import javax.inject.Inject
 import javax.inject.Singleton
 
+private data class CachedDrawable(
+        val drawable: Drawable,
+        val isError: Boolean,
+)
+
 @Singleton
 class LocationPinProvider @Inject constructor(
         private val context: Context,
@@ -43,7 +49,7 @@ class LocationPinProvider @Inject constructor(
         private val avatarRenderer: AvatarRenderer,
         private val matrixItemColorProvider: MatrixItemColorProvider
 ) {
-    private val cache = mutableMapOf<String, Drawable>()
+    private val cache = LruCache<MatrixItem.UserItem, CachedDrawable>(32)
 
     private val glideRequests by lazy {
         GlideApp.with(context)
@@ -60,23 +66,16 @@ class LocationPinProvider @Inject constructor(
             return
         }
 
-        if (cache.contains(userId)) {
-            callback(cache[userId]!!)
-            return
-        }
-
         activeSessionHolder
                 .getActiveSession()
                 .getUserOrDefault(userId)
                 .toMatrixItem()
                 .let { userItem ->
                     val size = dimensionConverter.dpToPx(44)
-                    val bgTintColor = matrixItemColorProvider.getColor(userItem)
                     avatarRenderer.render(glideRequests, userItem, object : CustomTarget<Drawable>(size, size) {
                         override fun onResourceReady(resource: Drawable, transition: Transition<in Drawable>?) {
                             Timber.d("## Location: onResourceReady")
-                            val pinDrawable = createPinDrawable(resource, bgTintColor)
-                            cache[userId] = pinDrawable
+                            val pinDrawable = createPinDrawable(userItem, resource, isError = false)
                             callback(pinDrawable)
                         }
 
@@ -87,17 +86,29 @@ class LocationPinProvider @Inject constructor(
                         }
 
                         override fun onLoadFailed(errorDrawable: Drawable?) {
+                            // Note: `onLoadFailed` is also called when the user has no avatarUrl
+                            // and the errorDrawable is actually the placeholder.
                             Timber.w("## Location: onLoadFailed")
                             errorDrawable ?: return
-                            val pinDrawable = createPinDrawable(errorDrawable, bgTintColor)
-                            cache[userId] = pinDrawable
+                            val pinDrawable = createPinDrawable(userItem, errorDrawable, isError = true)
                             callback(pinDrawable)
                         }
                     })
                 }
     }
 
-    private fun createPinDrawable(drawable: Drawable, @ColorInt bgTintColor: Int): Drawable {
+    private fun createPinDrawable(
+            userItem: MatrixItem.UserItem,
+            drawable: Drawable,
+            isError: Boolean,
+    ): Drawable {
+        val fromCache = cache.get(userItem)
+        // Return the cached drawable only if it is valid, or the new drawable is again an error
+        if (fromCache != null && (!fromCache.isError || isError)) {
+            return fromCache.drawable
+        }
+
+        val bgTintColor = matrixItemColorProvider.getColor(userItem)
         val bgUserPin = ContextCompat.getDrawable(context, R.drawable.bg_map_user_pin)!!
         // use mutate on drawable to avoid sharing the color when we have multiple different user pins
         DrawableCompat.setTint(bgUserPin.mutate(), bgTintColor)
@@ -106,6 +117,7 @@ class LocationPinProvider @Inject constructor(
         val topInset = dimensionConverter.dpToPx(4)
         val bottomInset = dimensionConverter.dpToPx(8)
         layerDrawable.setLayerInset(1, horizontalInset, topInset, horizontalInset, bottomInset)
+        cache.put(userItem, CachedDrawable(layerDrawable, isError))
         return layerDrawable
     }
 }