Fix image posters randomly loading

Multiple things going on here: - TmdbImageUrlProvider was not a data class, so didn't implement equals() - BindingAdapter uses doOnLayout, which is not cleared if the item changes before the layout pass. Closes #275
chrisbanes · Apr 18, 2019 · 432e882 · 432e882
1 parent 101c8e7
commit 432e882
Show file tree

Hide file tree

Showing 5 changed files with 81 additions and 43 deletions.
diff --git a/app/src/main/java/app/tivi/ui/databinding/TiviBindingAdapters.kt b/app/src/main/java/app/tivi/ui/databinding/TiviBindingAdapters.kt
@@ -27,6 +27,7 @@ import android.widget.TextView
 import androidx.core.view.doOnLayout
 import androidx.core.view.isVisible
 import androidx.databinding.BindingAdapter
+import app.tivi.R
 import app.tivi.extensions.doOnApplyWindowInsets
 import app.tivi.extensions.resolveThemeReferenceResId
 import app.tivi.tmdb.TmdbImageUrlProvider
@@ -35,45 +36,83 @@ import app.tivi.ui.glide.GlideApp
 import app.tivi.util.ScrimUtil
 import com.google.android.material.shape.CornerFamily
 import com.google.android.material.shape.MaterialShapeDrawable
+import java.util.Objects
 import kotlin.math.roundToInt
 
-@BindingAdapter("tmdbPosterPath", "tmdbImageUrlProvider", "imageSaturateOnLoad")
-fun loadPoster(view: ImageView, path: String?, urlProvider: TmdbImageUrlProvider?, saturateOnLoad: Boolean?) {
-    if (path != null && urlProvider != null) {
-        view.doOnLayout {
-            GlideApp.with(view)
-                    .let { r -> if (saturateOnLoad == true) r.saturateOnLoad() else r.asDrawable() }
-                    .load(urlProvider.getPosterUrl(path, it.width))
-                    .thumbnail(GlideApp.with(view).load(urlProvider.getPosterUrl(path, 0)))
-                    .into(view)
-        }
-    } else {
-        GlideApp.with(view).clear(view)
-    }
-}
-
-@BindingAdapter("tmdbPosterPath", "tmdbImageUrlProvider")
-fun loadPoster(view: ImageView, path: String?, urlProvider: TmdbImageUrlProvider?) {
-    loadPoster(view, path, urlProvider, true)
-}
-
-@BindingAdapter("tmdbBackdropPath", "tmdbImageUrlProvider")
-fun loadBackdrop(view: ImageView, path: String?, urlProvider: TmdbImageUrlProvider?) {
-    loadBackdrop(view, path, urlProvider, true)
+@BindingAdapter(
+        "tmdbPosterPath",
+        "tmdbImageUrlProvider",
+        "imageSaturateOnLoad",
+        requireAll = false
+)
+fun loadPoster(
+    view: ImageView,
+    path: String?,
+    urlProvider: TmdbImageUrlProvider?,
+    saturateOnLoad: Boolean?
+) {
+    loadImage(view, path, urlProvider, saturateOnLoad, "poster", TmdbImageUrlProvider::getPosterUrl)
 }
 
-@BindingAdapter("tmdbBackdropPath", "tmdbImageUrlProvider", "imageSaturateOnLoad")
-fun loadBackdrop(view: ImageView, path: String?, urlProvider: TmdbImageUrlProvider?, saturateOnLoad: Boolean?) {
+@BindingAdapter(
+        "tmdbBackdropPath",
+        "tmdbImageUrlProvider",
+        "imageSaturateOnLoad",
+        requireAll = false
+)
+fun loadBackdrop(
+    view: ImageView,
+    path: String?,
+    urlProvider: TmdbImageUrlProvider?,
+    saturateOnLoad: Boolean?
+) = loadImage(view, path, urlProvider, saturateOnLoad, "backdrop", TmdbImageUrlProvider::getBackdropUrl)
+
+private inline fun loadImage(
+    view: ImageView,
+    path: String?,
+    urlProvider: TmdbImageUrlProvider?,
+    saturateOnLoad: Boolean?,
+    type: String,
+    crossinline urlEr: (TmdbImageUrlProvider, String, Int) -> String
+) {
     if (path != null && urlProvider != null) {
+        val requestKey = Objects.hash(path, urlProvider, type)
+        view.setTag(R.id.loading, requestKey)
+
         view.doOnLayout {
-            GlideApp.with(view)
-                    .let { r -> if (saturateOnLoad == true) r.saturateOnLoad() else r.asDrawable() }
-                    .load(urlProvider.getBackdropUrl(path, it.width))
-                    .thumbnail(GlideApp.with(view).load(urlProvider.getBackdropUrl(path, 0)))
+            if (it.getTag(R.id.loading) != requestKey) {
+                // The request key is different, exit now since there's we've probably be rebound to a different
+                // item
+                return@doOnLayout
+            }
+
+            GlideApp.with(it)
+                    .let { r ->
+                        if (saturateOnLoad == null || saturateOnLoad) {
+                            // If we don't have a value, or we're explicitly set the yes, saturate on load
+                            r.saturateOnLoad()
+                        } else {
+                            r.asDrawable()
+                        }
+                    }
+                    .load(urlEr(urlProvider, path, view.width))
+                    .thumbnail(
+                            GlideApp.with(view)
+                                    .let { tr ->
+                                        if (saturateOnLoad == null || saturateOnLoad) {
+                                            // If we don't have a value, or we're explicitly set the yes, saturate on load
+                                            tr.saturateOnLoad()
+                                        } else {
+                                            tr.asDrawable()
+                                        }
+                                    }
+                                    .load(urlEr(urlProvider, path, 0))
+                    )
                     .into(view)
         }
     } else {
         GlideApp.with(view).clear(view)
+        view.setImageDrawable(null)
     }
 }
 
@@ -154,13 +193,12 @@ fun applySystemWindows(
     systemWindowRight: Boolean,
     systemWindowBottom: Boolean
 ) {
-    view.doOnApplyWindowInsets { view, insets, paddingState ->
+    view.doOnApplyWindowInsets { v, insets, paddingState ->
         val left = if (systemWindowLeft) insets.systemWindowInsetLeft else 0
         val top = if (systemWindowTop) insets.systemWindowInsetTop else 0
         val right = if (systemWindowRight) insets.systemWindowInsetRight else 0
         val bottom = if (systemWindowBottom) insets.systemWindowInsetBottom else 0
-
-        view.setPadding(
+        v.setPadding(
                 paddingState.left + left,
                 paddingState.top + top,
                 paddingState.right + right,

diff --git a/data/src/main/java/app/tivi/data/resultentities/EntryWithShow.kt b/data/src/main/java/app/tivi/data/resultentities/EntryWithShow.kt
@@ -31,6 +31,6 @@ interface EntryWithShow<ET : Entry> {
         }
 
     fun generateStableId(): Long {
-        return Objects.hash(entry!!::class, show.id).toLong()
+        return Objects.hash(entry!!::class.java.name, show.id).toLong()
     }
 }
diff --git a/tmdb/src/main/java/app/tivi/tmdb/TmdbImageSizes.kt b/tmdb/src/main/java/app/tivi/tmdb/TmdbImageSizes.kt
@@ -18,9 +18,9 @@ package app.tivi.tmdb
 
 object TmdbImageSizes {
 
-    val baseImageUrl = "https://image.tmdb.org/t/p/"
+    const val baseImageUrl = "https://image.tmdb.org/t/p/"
 
-    val posterSizes = arrayOf(
+    val posterSizes = listOf(
             "w92",
             "w154",
             "w185",
@@ -30,7 +30,7 @@ object TmdbImageSizes {
             "original"
     )
 
-    val backdropSizes = arrayOf(
+    val backdropSizes = listOf(
             "w300",
             "w780",
             "w1280",

diff --git a/tmdb/src/main/java/app/tivi/tmdb/TmdbImageUrlProvider.kt b/tmdb/src/main/java/app/tivi/tmdb/TmdbImageUrlProvider.kt
@@ -18,10 +18,10 @@ package app.tivi.tmdb
 
 private val IMAGE_SIZE_PATTERN = "w(\\d+)$".toRegex()
 
-class TmdbImageUrlProvider(
+data class TmdbImageUrlProvider(
     private val baseImageUrl: String = TmdbImageSizes.baseImageUrl,
-    private val posterSizes: Array<String> = TmdbImageSizes.posterSizes,
-    private val backdropSizes: Array<String> = TmdbImageSizes.backdropSizes
+    private val posterSizes: List<String> = TmdbImageSizes.posterSizes,
+    private val backdropSizes: List<String> = TmdbImageSizes.backdropSizes
 ) {
     fun getPosterUrl(path: String, imageWidth: Int): String {
         return "$baseImageUrl${selectSize(posterSizes, imageWidth)}$path"
@@ -31,7 +31,7 @@ class TmdbImageUrlProvider(
         return "$baseImageUrl${selectSize(backdropSizes, imageWidth)}$path"
     }
 
-    private fun selectSize(sizes: Array<String>, imageWidth: Int): String {
+    private fun selectSize(sizes: List<String>, imageWidth: Int): String {
         var previousSize: String? = null
         var previousWidth = 0
 

diff --git a/tmdb/src/main/java/app/tivi/tmdb/TmdbManager.kt b/tmdb/src/main/java/app/tivi/tmdb/TmdbManager.kt
@@ -59,9 +59,9 @@ class TmdbManager @Inject constructor(
     private fun onConfigurationLoaded(configuration: Configuration) {
         configuration.images?.let {
             val newProvider = TmdbImageUrlProvider(
-                    it.secure_base_url,
-                    it.poster_sizes.toTypedArray(),
-                    it.backdrop_sizes.toTypedArray())
+                    it.secure_base_url!!,
+                    it.poster_sizes ?: emptyList(),
+                    it.backdrop_sizes ?: emptyList())
             imageProviderSubject.onNext(newProvider)
         }
     }