Skip to content

Commit

Permalink
Close mozilla-mobile#7313: Use storage/loader to get thumbnails
Browse files Browse the repository at this point in the history
We were seeing odd bugs and performance issues from trying to map the
disk cache into the TabsTrayPresenter.

A better solution, would be to load the thumbnails straight from the
cache, and incremental updates via the store.
  • Loading branch information
jonalmeida committed Jun 12, 2020
1 parent eecfd92 commit 68d5518
Show file tree
Hide file tree
Showing 16 changed files with 285 additions and 36 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions components/browser/tabstray/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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

/**
Expand All @@ -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)
Expand Down Expand Up @@ -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)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
Expand All @@ -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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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`() {
Expand Down Expand Up @@ -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()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
/* 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 deferredIcon = storage.loadThumbnail(id)

view.get()?.setTag(R.id.mozac_browser_thumbnails_tag_job, deferredIcon)
val onAttachStateChangeListener = CancelOnDetach(deferredIcon).also {
view.get()?.addOnAttachStateChangeListener(it)
}

try {
val icon = deferredIcon.await()
view.get()?.setImageBitmap(icon)
} catch (e: CancellationException) {
view.get()?.setImageDrawable(error)
} finally {
view.get()?.removeOnAttachStateChangeListener(onAttachStateChangeListener)
view.get()?.setTag(R.id.mozac_browser_thumbnails_tag_job, null)
}
}
}
8 changes: 8 additions & 0 deletions components/browser/thumbnails/src/main/res/values/tags.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?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>
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
/* 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 icon into image view`() {
val mockedBitmap: Bitmap = mock()
val result = CompletableDeferred<Bitmap>()
val view: ImageView = mock()
val storage: ThumbnailStorage = mock()
val icons = spy(ThumbnailLoader(storage))

doReturn(result).`when`(storage).loadThumbnail("123")

icons.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 icons = spy(ThumbnailLoader(storage))

doReturn(result).`when`(storage).loadThumbnail("123")

icons.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 icons = spy(ThumbnailLoader(storage))

doReturn(previousJob).`when`(view).getTag(R.id.mozac_browser_thumbnails_tag_job)
doReturn(result).`when`(storage).loadThumbnail("123")

icons.loadIntoView(view, "123")

verify(previousJob).cancel()

result.cancel()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
)
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Loading

0 comments on commit 68d5518

Please sign in to comment.