Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Issue #7313: Use ThumbnailLoader for the TabViewHolder #7356

Merged
merged 4 commits into from
Jun 12, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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()
Copy link
Contributor

Choose a reason for hiding this comment

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

would putting this parameter after the closure prevent us from using the trailing closures when initializing the TabsAdapter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@boek I explicitly wanted to avoid that since we've had issues (mozilla-mobile/fenix#10495) where the scope of the trailing closure did not make it obvious it was being invoked during the onCreateViewHolder call of the adapter and that could cause crashes depending on what work was done in that closure.

I thought it was better to have an explicit named param in that case to avoid regressions.

) : RecyclerView.Adapter<TabViewHolder>(),
TabsTray,
Observable<TabsTray.Observer> by delegate {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
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
Expand Up @@ -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
Expand All @@ -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)
Expand Down Expand Up @@ -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()
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 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)
}
}
}
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>
Loading