Commit ee34a972 authored by MozLando's avatar MozLando
Browse files

Merge #7356



7356: Issue #7313: Use ThumbnailLoader for the TabViewHolder r=gabrielluong,boek a=jonalmeida

Instead of trying to inline the thumbnail images from disk into the TabsTrayPresenter, we can load them from the `ThumbnailStorage` via the `ThumbnailLoader` and rely on the `TabsTrayPresenter` to consume new thumbnail updates only from the store.

This fixes some perf issues, inconsistencies, and duplicate updates to the tabs tray.
Co-authored-by: default avatarJonathan Almeida <jalmeida@mozilla.com>
parents d6e32af3 1f143e1f
......@@ -40,7 +40,7 @@ import mozilla.components.browser.icons.preparer.TippyTopIconPreparer
import mozilla.components.browser.icons.processor.DiskIconProcessor
import mozilla.components.browser.icons.processor.IconProcessor
import mozilla.components.browser.icons.processor.MemoryIconProcessor
import mozilla.components.browser.icons.utils.CancelOnDetach
import mozilla.components.support.images.CancelOnDetach
import mozilla.components.browser.icons.utils.IconDiskCache
import mozilla.components.browser.icons.utils.IconMemoryCache
import mozilla.components.browser.state.state.BrowserState
......
......@@ -32,6 +32,7 @@ dependencies {
implementation project(':ui-icons')
implementation project(':ui-colors')
implementation project(':support-base')
implementation project(':support-images')
implementation project(':support-ktx')
implementation Dependencies.androidx_appcompat
......
......@@ -14,6 +14,7 @@ import mozilla.components.browser.tabstray.thumbnail.TabThumbnailView
import mozilla.components.concept.tabstray.Tab
import mozilla.components.concept.tabstray.TabsTray
import mozilla.components.support.base.observer.Observable
import mozilla.components.support.images.loader.ImageLoader
import mozilla.components.support.ktx.kotlin.tryGetHostFromUrl
/**
......@@ -36,7 +37,8 @@ abstract class TabViewHolder(view: View) : RecyclerView.ViewHolder(view) {
*/
class DefaultTabViewHolder(
itemView: View,
private val tabsTray: BrowserTabsTray
private val tabsTray: BrowserTabsTray,
private val thumbnailLoader: ImageLoader? = null
) : TabViewHolder(itemView) {
private val iconView: ImageView? = itemView.findViewById(R.id.mozac_browser_tabstray_icon)
private val titleView: TextView = itemView.findViewById(R.id.mozac_browser_tabstray_title)
......@@ -79,7 +81,12 @@ class DefaultTabViewHolder(
closeView.imageTintList = ColorStateList.valueOf(tabsTray.styling.itemTextColor)
}
thumbnailView.setImageBitmap(tab.thumbnail)
// In the final else case, we have no cache or fresh screenshot; do nothing instead of clearing the image.
if (thumbnailLoader != null && tab.thumbnail == null) {
thumbnailLoader.loadIntoView(thumbnailView, tab.id)
} else if (tab.thumbnail != null) {
thumbnailView.setImageBitmap(tab.thumbnail)
}
iconView?.setImageBitmap(tab.icon)
}
......
......@@ -11,6 +11,7 @@ import mozilla.components.concept.tabstray.Tabs
import mozilla.components.concept.tabstray.TabsTray
import mozilla.components.support.base.observer.Observable
import mozilla.components.support.base.observer.ObserverRegistry
import mozilla.components.support.images.loader.ImageLoader
/**
* Function responsible for creating a `TabViewHolder` in the `TabsAdapter`.
......@@ -24,16 +25,18 @@ typealias ViewHolderProvider = (ViewGroup, BrowserTabsTray) -> TabViewHolder
*/
@Suppress("TooManyFunctions")
open class TabsAdapter(
delegate: Observable<TabsTray.Observer> = ObserverRegistry(),
thumbnailLoader: ImageLoader? = null,
private val viewHolderProvider: ViewHolderProvider = { parent, tabsTray ->
DefaultTabViewHolder(
LayoutInflater.from(parent.context).inflate(
R.layout.mozac_browser_tabstray_item,
parent,
false),
tabsTray
tabsTray,
thumbnailLoader
)
}
},
delegate: Observable<TabsTray.Observer> = ObserverRegistry()
) : RecyclerView.Adapter<TabViewHolder>(),
TabsTray,
Observable<TabsTray.Observer> by delegate {
......
......@@ -17,13 +17,17 @@ class TabThumbnailView(context: Context, attrs: AttributeSet) : AppCompatImageVi
@VisibleForTesting
public override fun setFrame(l: Int, t: Int, r: Int, b: Int): Boolean {
val changed = super.setFrame(l, t, r, b)
if (changed) {
val matrix = imageMatrix
val scaleFactor = width / drawable.intrinsicWidth.toFloat()
matrix.setScale(scaleFactor, scaleFactor, 0f, 0f)
imageMatrix = matrix
val result = super.setFrame(l, t, r, b)
val matrix = imageMatrix
val scaleFactor = if (drawable != null) {
width / drawable.intrinsicWidth.toFloat()
} else {
1F
}
return changed
matrix.setScale(scaleFactor, scaleFactor, 0f, 0f)
imageMatrix = matrix
return result
}
}
......@@ -13,17 +13,22 @@ import androidx.test.ext.junit.runners.AndroidJUnit4
import mozilla.components.concept.tabstray.Tab
import mozilla.components.concept.tabstray.TabsTray
import mozilla.components.support.base.observer.ObserverRegistry
import mozilla.components.support.images.loader.ImageLoader
import mozilla.components.support.test.any
import mozilla.components.support.test.eq
import mozilla.components.support.test.mock
import mozilla.components.support.test.nullable
import mozilla.components.support.test.robolectric.testContext
import org.junit.Assert.assertEquals
import org.junit.Assert.assertTrue
import org.junit.Test
import org.junit.runner.RunWith
import org.mockito.Mockito.doReturn
import org.mockito.Mockito.never
import org.mockito.Mockito.verify
@RunWith(AndroidJUnit4::class)
class TabViewHolderTest {
class DefaultTabViewHolderTest {
@Test
fun `URL from session is assigned to view`() {
......@@ -146,6 +151,38 @@ class TabViewHolderTest {
assertTrue(thumbnailView.drawable != null)
}
@Test
fun `thumbnail is set from loader`() {
val view = LayoutInflater.from(testContext).inflate(R.layout.mozac_browser_tabstray_item, null)
val loader: ImageLoader = mock()
val viewHolder = DefaultTabViewHolder(view, mockTabsTrayWithStyles(), loader)
val tabWithThumbnail = Tab("123", "https://example.com", thumbnail = mock())
val tab = Tab("123", "https://example.com")
viewHolder.bind(tabWithThumbnail, false, mock())
verify(loader, never()).loadIntoView(any(), eq("123"), nullable(), nullable())
viewHolder.bind(tab, false, mock())
verify(loader).loadIntoView(any(), eq("123"), nullable(), nullable())
}
@Test
fun `thumbnailView does not change when there is no cache or new thumbnail`() {
val view = LayoutInflater.from(testContext).inflate(R.layout.mozac_browser_tabstray_item, null)
val viewHolder = DefaultTabViewHolder(view, mockTabsTrayWithStyles())
val tab = Tab("123", "https://example.com")
val thumbnailView = view.findViewById<ImageView>(R.id.mozac_browser_tabstray_thumbnail)
thumbnailView.setImageBitmap(mock())
val drawable = thumbnailView.drawable
viewHolder.bind(tab, false, mock())
assertEquals(drawable, thumbnailView.drawable)
}
companion object {
fun mockTabsTrayWithStyles(): BrowserTabsTray {
val styles: TabsTrayStyling = mock()
......
......@@ -7,6 +7,7 @@ package mozilla.components.browser.tabstray
import android.view.LayoutInflater
import androidx.recyclerview.widget.ItemTouchHelper
import androidx.test.ext.junit.runners.AndroidJUnit4
import mozilla.components.browser.tabstray.DefaultTabViewHolderTest.Companion.mockTabsTrayWithStyles
import mozilla.components.concept.tabstray.TabsTray
import mozilla.components.support.base.observer.Observable
import mozilla.components.support.base.observer.ObserverRegistry
......@@ -48,7 +49,7 @@ class TabTouchCallbackTest {
@Test
fun `onChildDraw alters alpha of ViewHolder on swipe gesture`() {
val view = LayoutInflater.from(testContext).inflate(R.layout.mozac_browser_tabstray_item, null)
val holder = DefaultTabViewHolder(view, TabViewHolderTest.mockTabsTrayWithStyles())
val holder = DefaultTabViewHolder(view, mockTabsTrayWithStyles())
val callback = TabTouchCallback(mock())
holder.itemView.alpha = 0f
......
......@@ -40,7 +40,7 @@ class TabsAdapterTest {
@Test
fun `onCreateViewHolder will create whatever TabViewHolder is provided`() {
val adapter = TabsAdapter { _, _ -> TestTabViewHolder(View(testContext)) }
val adapter = TabsAdapter(viewHolderProvider = { _, _ -> TestTabViewHolder(View(testContext)) })
adapter.tabsTray = mock()
val type = adapter.onCreateViewHolder(FrameLayout(testContext), 0)
......
......@@ -4,6 +4,7 @@
package mozilla.components.browser.tabstray.thumbnail
import android.graphics.Matrix
import android.graphics.drawable.Drawable
import android.widget.ImageView
import androidx.test.ext.junit.runners.AndroidJUnit4
......@@ -14,6 +15,8 @@ import org.junit.Assert.assertNotEquals
import org.junit.Test
import org.junit.runner.RunWith
import org.mockito.Mockito.`when`
import org.mockito.Mockito.spy
import org.mockito.Mockito.verify
import org.robolectric.Robolectric.buildAttributeSet
@RunWith(AndroidJUnit4::class)
......@@ -61,6 +64,18 @@ class TabThumbnailViewTest {
assertEquals(matrix, matrix2)
}
@Test
fun `view scaleFactor does not change if there is no drawable`() {
val view = spy(TabThumbnailView(testContext, emptyAttributeSet()))
val matrix: Matrix = spy(Matrix())
`when`(view.imageMatrix).thenReturn(matrix)
view.setFrame(5, 5, 5, 5)
verify(matrix).setScale(1f, 1f, 0f, 0f)
}
}
private fun emptyAttributeSet() = buildAttributeSet().build()
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
package mozilla.components.browser.thumbnails.loader
import android.graphics.drawable.Drawable
import android.widget.ImageView
import androidx.annotation.MainThread
import kotlinx.coroutines.CancellationException
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.Job
import kotlinx.coroutines.launch
import mozilla.components.browser.thumbnails.R
import mozilla.components.browser.thumbnails.storage.ThumbnailStorage
import mozilla.components.support.images.CancelOnDetach
import mozilla.components.support.images.loader.ImageLoader
import java.lang.ref.WeakReference
/**
* An implementation of [ImageLoader] for loading thumbnails into a [ImageView].
*/
class ThumbnailLoader(private val storage: ThumbnailStorage) : ImageLoader {
override fun loadIntoView(
view: ImageView,
id: String,
placeholder: Drawable?,
error: Drawable?
) {
CoroutineScope(Dispatchers.Main).launch {
loadIntoViewInternal(WeakReference(view), id, placeholder, error)
}
}
@MainThread
private suspend fun loadIntoViewInternal(
view: WeakReference<ImageView>,
id: String,
placeholder: Drawable?,
error: Drawable?
) {
// If we previously started loading into the view, cancel the job.
val existingJob = view.get()?.getTag(R.id.mozac_browser_thumbnails_tag_job) as? Job
existingJob?.cancel()
view.get()?.setImageDrawable(placeholder)
// Create a loading job
val deferredThumbnail = storage.loadThumbnail(id)
view.get()?.setTag(R.id.mozac_browser_thumbnails_tag_job, deferredThumbnail)
val onAttachStateChangeListener = CancelOnDetach(deferredThumbnail).also {
view.get()?.addOnAttachStateChangeListener(it)
}
try {
val thumbnail = deferredThumbnail.await()
view.get()?.setImageBitmap(thumbnail)
} catch (e: CancellationException) {
view.get()?.setImageDrawable(error)
} finally {
view.get()?.removeOnAttachStateChangeListener(onAttachStateChangeListener)
view.get()?.setTag(R.id.mozac_browser_thumbnails_tag_job, null)
}
}
}
<?xml version="1.0" encoding="utf-8"?>
<!-- This Source Code Form is subject to the terms of the Mozilla Public
- License, v. 2.0. If a copy of the MPL was not distributed with this
- file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
<resources>
<item name="mozac_browser_thumbnails_tag_job" type="id" />
</resources>
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
package mozilla.components.browser.thumbnails.loader
import android.graphics.Bitmap
import android.graphics.drawable.Drawable
import android.widget.ImageView
import androidx.test.ext.junit.runners.AndroidJUnit4
import kotlinx.coroutines.CompletableDeferred
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.Job
import kotlinx.coroutines.test.TestCoroutineDispatcher
import kotlinx.coroutines.test.resetMain
import kotlinx.coroutines.test.setMain
import mozilla.components.browser.thumbnails.R
import mozilla.components.browser.thumbnails.storage.ThumbnailStorage
import mozilla.components.support.test.any
import mozilla.components.support.test.eq
import mozilla.components.support.test.mock
import mozilla.components.support.test.rule.MainCoroutineRule
import org.junit.After
import org.junit.Before
import org.junit.Rule
import org.junit.Test
import org.junit.runner.RunWith
import org.mockito.Mockito.doReturn
import org.mockito.Mockito.never
import org.mockito.Mockito.spy
import org.mockito.Mockito.verify
@RunWith(AndroidJUnit4::class)
class ThumbnailLoaderTest {
private val testDispatcher = TestCoroutineDispatcher()
@get:Rule
val coroutinesTestRule = MainCoroutineRule(testDispatcher)
@Before
fun setup() {
Dispatchers.setMain(testDispatcher)
}
@After
fun teardown() {
Dispatchers.resetMain()
testDispatcher.cleanupTestCoroutines()
}
@Test
fun `automatically load thumbnails into image view`() {
val mockedBitmap: Bitmap = mock()
val result = CompletableDeferred<Bitmap>()
val view: ImageView = mock()
val storage: ThumbnailStorage = mock()
val loader = spy(ThumbnailLoader(storage))
doReturn(result).`when`(storage).loadThumbnail("123")
loader.loadIntoView(view, "123")
verify(view).setImageDrawable(null)
verify(view).addOnAttachStateChangeListener(any())
verify(view).setTag(eq(R.id.mozac_browser_thumbnails_tag_job), any())
verify(view, never()).setImageBitmap(any())
result.complete(mockedBitmap)
verify(view).setImageBitmap(mockedBitmap)
verify(view).removeOnAttachStateChangeListener(any())
verify(view).setTag(R.id.mozac_browser_thumbnails_tag_job, null)
}
@Test
fun `loadIntoView sets drawable to error if cancelled`() {
val result = CompletableDeferred<Bitmap>()
val view: ImageView = mock()
val placeholder: Drawable = mock()
val error: Drawable = mock()
val storage: ThumbnailStorage = mock()
val loader = spy(ThumbnailLoader(storage))
doReturn(result).`when`(storage).loadThumbnail("123")
loader.loadIntoView(view, "123", placeholder = placeholder, error = error)
verify(view).setImageDrawable(placeholder)
result.cancel()
verify(view).setImageDrawable(error)
verify(view).removeOnAttachStateChangeListener(any())
verify(view).setTag(R.id.mozac_browser_thumbnails_tag_job, null)
}
@Test
fun `loadIntoView cancels previous jobs`() {
val result = CompletableDeferred<Bitmap>()
val view: ImageView = mock()
val previousJob: Job = mock()
val storage: ThumbnailStorage = mock()
val loader = spy(ThumbnailLoader(storage))
doReturn(previousJob).`when`(view).getTag(R.id.mozac_browser_thumbnails_tag_job)
doReturn(result).`when`(storage).loadThumbnail("123")
loader.loadIntoView(view, "123")
verify(previousJob).cancel()
result.cancel()
}
}
......@@ -24,17 +24,4 @@ data class Tab(
val icon: Bitmap? = null,
val thumbnail: Bitmap? = null,
val mediaState: Media.State? = null
) {
override fun equals(other: Any?): Boolean {
if (javaClass != other?.javaClass) return false
other as Tab
return id == other.id &&
url == other.url &&
title == other.title &&
icon?.generationId == other.icon?.generationId &&
thumbnail?.generationId == other.thumbnail?.generationId &&
mediaState == other.mediaState
}
}
)
......@@ -45,16 +45,6 @@ class TabsTrayPresenter(
private suspend fun collect(flow: Flow<BrowserState>) {
flow.map { it.toTabs(tabsFilter) }
.map { tabs ->
if (thumbnailsUseCases != null) {
// Load the tab thumbnail from the memory or disk caches.
tabs.copy(list = tabs.list.map { tab ->
tab.copy(thumbnail = thumbnailsUseCases.loadThumbnail(tab.id))
})
} else {
tabs
}
}
.ifChanged()
.collect { tabs ->
// Do not invoke the callback on start if this is the initial state.
......
......@@ -39,6 +39,7 @@ dependencies {
implementation Dependencies.androidx_annotation
implementation Dependencies.kotlin_stdlib
implementation Dependencies.kotlin_coroutines
testImplementation project(':support-test')
......
......@@ -2,7 +2,7 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
package mozilla.components.browser.icons.utils
package mozilla.components.support.images
import android.view.View
import kotlinx.coroutines.Job
......@@ -10,7 +10,7 @@ import kotlinx.coroutines.Job
/**
* Cancels the provided job when a view is detached from the window
*/
internal class CancelOnDetach(private val job: Job) : View.OnAttachStateChangeListener {
class CancelOnDetach(private val job: Job) : View.OnAttachStateChangeListener {
override fun onViewAttachedToWindow(v: View?) = Unit
......
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
package mozilla.components.support.images.loader
import android.graphics.drawable.Drawable
import android.widget.ImageView
import androidx.annotation.MainThread
/**
* A loader that can load an image from an ID directly into an [ImageView].
*/
interface ImageLoader {
/**
* Loads an image asynchronously and then displays it in the [ImageView].
* If the view is detached from the window before loading is completed, then loading is cancelled.
*
* @param view [ImageView] to load the image into.
* @param id Load image for this given ID.
* @param placeholder [Drawable] to display while image is loading.
* @param error [Drawable] to display if loading fails.
*/
@MainThread
fun loadIntoView(
view: ImageView,
id: String,
placeholder: Drawable? = null,
error: Drawable? = null
)
}
......@@ -2,7 +2,7 @@
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
package mozilla.components.browser.icons.utils
package mozilla.components.support.images
import android.view.View
import kotlinx.coroutines.Job
......
......@@ -69,6 +69,13 @@ permalink: /changelog/
* Smaller binary library after a big dependency was dropped
* Limit the number of upload retries in all implementations
* **support-images**:
* Added `ImageLoader` API for loading images directly into an `ImageView`.
* **browser-tabstray**:
* ⚠️ **This is a breaking change**: `TabsAdapter` and `DefaultTabViewHolder` take an optional `ImageLoader` for loading browser thumbnails.
* Fixed a bug in `TabsThumbnailView` where the `scaleFactor` was not applied all the time when expected.
# 44.0.0
* [Commits](https://github.com/mozilla-mobile/android-components/compare/v43.0.0...v44.0.0)
......
......@@ -112,6 +112,7 @@ dependencies {
implementation project(':support-utils')
implementation project(':feature-downloads')
implementation project(':support-images')
implementation project(':support-ktx')