Tab manager ANRs fix attempt (#5113)

Task/Issue URL: https://app.asana.com/0/1207418217763355/1208338167068031/f ### Description This PR attempts to fix a couple of ANRs that happen in the tab manager using the following attempts: - [Scale the bitmap to minimum to reduce the memory footprint](d1b7b77) - [Enable fixed grid size to optimize recycler view perf](3b01bb3) - [Set stable IDs for tab switcher adapter and simplify the code](6112898) - [Optimize the tab switcher layout](dbbe15e) ### Steps to test this PR The ANRs are not easily reproducible. Smoke testing the tab manager will suffice.
duckduckgo · Oct 9, 2024 · 785a80f · 785a80f
1 parent d818a4e
commit 785a80f
Show file tree

Hide file tree

Showing 7 changed files with 85 additions and 196 deletions.
diff --git a/app/src/main/java/com/duckduckgo/app/browser/tabpreview/WebViewPreviewGenerator.kt b/app/src/main/java/com/duckduckgo/app/browser/tabpreview/WebViewPreviewGenerator.kt
@@ -20,7 +20,10 @@ import android.annotation.SuppressLint
 import android.graphics.Bitmap
 import android.webkit.WebView
 import androidx.core.view.drawToBitmap
+import com.duckduckgo.common.ui.view.toPx
 import com.duckduckgo.common.utils.DispatcherProvider
+import com.duckduckgo.mobile.android.R
+import kotlin.math.roundToInt
 import kotlinx.coroutines.withContext
 
 interface WebViewPreviewGenerator {
@@ -32,7 +35,10 @@ class FileBasedWebViewPreviewGenerator(private val dispatchers: DispatcherProvid
     override suspend fun generatePreview(webView: WebView): Bitmap {
         disableScrollbars(webView)
         val fullSizeBitmap = createBitmap(webView)
-        val scaledBitmap = scaleBitmap(fullSizeBitmap)
+
+        val scaledHeight = webView.context.resources.getDimension(R.dimen.gridItemPreviewHeight).toPx()
+        val scaledWidth = scaledHeight / fullSizeBitmap.height * fullSizeBitmap.width
+        val scaledBitmap = scaleBitmap(fullSizeBitmap, scaledHeight.roundToInt(), scaledWidth.roundToInt())
         enableScrollbars(webView)
         return scaledBitmap
     }
@@ -59,18 +65,14 @@ class FileBasedWebViewPreviewGenerator(private val dispatchers: DispatcherProvid
     }
 
     @SuppressLint("AvoidComputationUsage")
-    private suspend fun scaleBitmap(bitmap: Bitmap): Bitmap {
+    private suspend fun scaleBitmap(bitmap: Bitmap, scaledHeight: Int, scaledWidth: Int): Bitmap {
         return withContext(dispatchers.computation()) {
             return@withContext Bitmap.createScaledBitmap(
                 bitmap,
-                (bitmap.width * COMPRESSION_RATIO).toInt(),
-                (bitmap.height * COMPRESSION_RATIO).toInt(),
+                scaledWidth,
+                scaledHeight,
                 false,
             )
         }
     }
-
-    companion object {
-        private const val COMPRESSION_RATIO = 0.5
-    }
 }
diff --git a/app/src/main/java/com/duckduckgo/app/tabs/ui/TabSwitcherActivity.kt b/app/src/main/java/com/duckduckgo/app/tabs/ui/TabSwitcherActivity.kt
@@ -177,6 +177,7 @@ class TabSwitcherActivity : DuckDuckGoActivity(), TabSwitcherListener, Coroutine
 
         tabItemDecorator = TabItemDecorator(this, selectedTabId)
         tabsRecycler.addItemDecoration(tabItemDecorator)
+        tabsRecycler.setHasFixedSize(true)
     }
 
     private fun configureObservers() {

diff --git a/app/src/main/java/com/duckduckgo/app/tabs/ui/TabSwitcherAdapter.kt b/app/src/main/java/com/duckduckgo/app/tabs/ui/TabSwitcherAdapter.kt
@@ -46,10 +46,10 @@ import com.duckduckgo.app.tabs.model.TabSwitcherData.LayoutType
 import com.duckduckgo.app.tabs.ui.TabSwitcherAdapter.TabViewHolder
 import com.duckduckgo.app.tabs.ui.TabSwitcherAdapter.TabViewHolder.GridTabViewHolder
 import com.duckduckgo.app.tabs.ui.TabSwitcherAdapter.TabViewHolder.ListTabViewHolder
-import com.duckduckgo.common.ui.view.gone
 import com.duckduckgo.common.ui.view.show
 import com.duckduckgo.common.utils.swap
 import java.io.File
+import java.util.Collections.swap
 import kotlinx.coroutines.launch
 import timber.log.Timber
 
@@ -61,131 +61,83 @@ class TabSwitcherAdapter(
 ) : Adapter<TabViewHolder>() {
 
     private val list = mutableListOf<TabEntity>()
-
     private var isDragging: Boolean = false
     private var layoutType: LayoutType = LayoutType.GRID
 
-    override fun onCreateViewHolder(
-        parent: ViewGroup,
-        viewType: Int,
-    ): TabViewHolder {
+    init {
+        setHasStableIds(true)
+    }
+
+    override fun getItemId(position: Int): Long {
+        return list[position].tabId.hashCode().toLong()
+    }
+
+    override fun onCreateViewHolder(parent: ViewGroup, viewType: Int): TabViewHolder {
         val inflater = LayoutInflater.from(parent.context)
-        when (layoutType) {
+        return when (layoutType) {
             LayoutType.GRID -> {
                 val binding = ItemTabGridBinding.inflate(inflater, parent, false)
-                return GridTabViewHolder(
-                    binding = binding,
-                    favicon = binding.favicon,
-                    tabPreview = binding.tabPreview,
-                    title = binding.title,
-                    close = binding.close,
-                    cardContentsContainer = binding.cardContentsContainer,
-                    tabUnread = binding.tabUnread,
-                )
+                GridTabViewHolder(binding)
             }
             LayoutType.LIST -> {
                 val binding = ItemTabListBinding.inflate(inflater, parent, false)
-
-                return ListTabViewHolder(
-                    binding = binding,
-                    favicon = binding.favicon,
-                    title = binding.title,
-                    url = binding.url,
-                    close = binding.close,
-                    cardContentsContainer = binding.cardContentsContainer,
-                    tabUnread = binding.tabUnread,
-                )
+                ListTabViewHolder(binding)
             }
         }
     }
 
-    override fun getItemViewType(position: Int): Int {
-        return layoutType.ordinal
-    }
+    override fun getItemViewType(position: Int): Int = layoutType.ordinal
 
-    override fun getItemCount(): Int {
-        return list.size
-    }
+    override fun getItemCount(): Int = list.size
 
-    override fun onBindViewHolder(
-        holder: TabViewHolder,
-        position: Int,
-    ) {
+    override fun onBindViewHolder(holder: TabViewHolder, position: Int) {
+        val tab = list[position]
         when (holder) {
-            is GridTabViewHolder -> bindGridTab(holder, position)
-            is ListTabViewHolder -> bindListTab(holder, position)
+            is GridTabViewHolder -> bindGridTab(holder, tab)
+            is ListTabViewHolder -> bindListTab(holder, tab)
         }
     }
 
-    private fun bindListTab(
-        holder: ListTabViewHolder,
-        position: Int,
-    ) {
+    private fun bindListTab(holder: ListTabViewHolder, tab: TabEntity) {
         val context = holder.binding.root.context
-        val tab = list[position]
-
         holder.title.text = extractTabTitle(tab, context)
-
-        if (tab.url.isNullOrEmpty()) {
-            holder.url.gone()
-        } else {
-            holder.url.text = tab.url
-            holder.url.show()
-        }
-
+        holder.url.text = tab.url ?: ""
+        holder.url.visibility = if (tab.url.isNullOrEmpty()) View.GONE else View.VISIBLE
         updateUnreadIndicator(holder, tab)
         loadFavicon(tab, holder.favicon)
         attachClickListeners(holder, tab)
     }
 
-    private fun bindGridTab(
-        holder: GridTabViewHolder,
-        position: Int,
-    ) {
+    private fun bindGridTab(holder: GridTabViewHolder, tab: TabEntity) {
         val context = holder.binding.root.context
-        val tab = list[position]
         val glide = Glide.with(context)
-
         holder.title.text = extractTabTitle(tab, context)
-
         updateUnreadIndicator(holder, tab)
         loadFavicon(tab, holder.favicon)
         loadTabPreviewImage(tab, glide, holder)
         attachClickListeners(holder, tab)
     }
 
-    private fun extractTabTitle(
-        tab: TabEntity,
-        context: Context,
-    ): String {
+    private fun extractTabTitle(tab: TabEntity, context: Context): String {
         var title = tab.displayTitle(context)
         title = title.removeSuffix(DUCKDUCKGO_TITLE_SUFFIX)
         return title
     }
 
-    private fun updateUnreadIndicator(
-        holder: TabViewHolder,
-        tab: TabEntity,
-    ) {
+    private fun updateUnreadIndicator(holder: TabViewHolder, tab: TabEntity) {
         holder.tabUnread.visibility = if (tab.viewed) View.INVISIBLE else View.VISIBLE
     }
 
-    override fun onBindViewHolder(
-        holder: TabViewHolder,
-        position: Int,
-        payloads: MutableList<Any>,
-    ) {
+    override fun onBindViewHolder(holder: TabViewHolder, position: Int, payloads: MutableList<Any>) {
         if (payloads.isEmpty()) {
             onBindViewHolder(holder, position)
             return
         }
 
         val tab = list[position]
-
         for (payload in payloads) {
             val bundle = payload as Bundle
-
-            for (key: String in bundle.keySet()) {
+            for (key in bundle.keySet()) {
                 Timber.v("$key changed - Need an update for $tab")
             }
 
@@ -213,32 +165,17 @@ class TabSwitcherAdapter(
         }
     }
 
-    private fun loadFavicon(
-        tab: TabEntity,
-        view: ImageView,
-    ) {
+    private fun loadFavicon(tab: TabEntity, view: ImageView) {
         val url = tab.url ?: return
         lifecycleOwner.lifecycleScope.launch {
             faviconManager.loadToViewFromLocalWithPlaceholder(tab.tabId, url, view)
         }
     }
 
-    private fun loadTabPreviewImage(
-        tab: TabEntity,
-        glide: RequestManager,
-        holder: GridTabViewHolder,
-    ) {
-        val previewFile = tab.tabPreviewFile
-        if (previewFile == null) {
-            glide.clear(holder.tabPreview)
-            return
-        }
-
+    private fun loadTabPreviewImage(tab: TabEntity, glide: RequestManager, holder: GridTabViewHolder) {
+        val previewFile = tab.tabPreviewFile ?: return glide.clear(holder.tabPreview)
         val cachedWebViewPreview = File(webViewPreviewPersister.fullPathForFile(tab.tabId, previewFile))
-        if (!cachedWebViewPreview.exists()) {
-            glide.clear(holder.tabPreview)
-            return
-        }
+        if (!cachedWebViewPreview.exists()) return glide.clear(holder.tabPreview)
 
         glide.load(cachedWebViewPreview)
             .transition(DrawableTransitionOptions.withCrossFade())
@@ -247,10 +184,7 @@ class TabSwitcherAdapter(
         holder.tabPreview.show()
     }
 
-    private fun attachClickListeners(
-        holder: TabViewHolder,
-        tab: TabEntity,
-    ) {
+    private fun attachClickListeners(holder: TabViewHolder, tab: TabEntity) {
         holder.rootView.setOnClickListener {
             if (!isDragging) {
                 itemClickListener.onTabSelected(tab)
@@ -263,18 +197,14 @@ class TabSwitcherAdapter(
 
     fun updateData(updatedList: List<TabEntity>) {
         val diffResult = DiffUtil.calculateDiff(TabEntityDiffCallback(list, updatedList))
-
         list.clear()
         list.addAll(updatedList)
         diffResult.dispatchUpdatesTo(this)
     }
 
     fun getTab(position: Int): TabEntity? = list.getOrNull(position)
 
-    fun adapterPositionForTab(tabId: String?): Int {
-        if (tabId == null) return -1
-        return list.indexOfFirst { it.tabId == tabId }
-    }
+    fun adapterPositionForTab(tabId: String?): Int = list.indexOfFirst { it.tabId == tabId }
 
     fun onDraggingStarted() {
         isDragging = true
@@ -301,30 +231,21 @@ class TabSwitcherAdapter(
 
     sealed class TabViewHolder(
         val rootView: View,
-        open val favicon: ImageView,
-        open val title: TextView,
-        open val close: ImageView,
-        open val tabUnread: ImageView,
-        open val cardContentsContainer: ViewGroup,
+        val favicon: ImageView,
+        val title: TextView,
+        val close: ImageView,
+        val tabUnread: ImageView,
     ) : ViewHolder(rootView) {
         data class GridTabViewHolder(
             val binding: ItemTabGridBinding,
-            override val favicon: ImageView,
-            val tabPreview: ImageView,
-            override val title: TextView,
-            override val close: ImageView,
-            override val tabUnread: ImageView,
-            override val cardContentsContainer: ViewGroup,
-        ) : TabViewHolder(binding.root, favicon, title, close, tabUnread, cardContentsContainer)
+        ) : TabViewHolder(binding.root, binding.favicon, binding.title, binding.close, binding.tabUnread) {
+            val tabPreview: ImageView = binding.tabPreview
+        }
 
         data class ListTabViewHolder(
             val binding: ItemTabListBinding,
-            override val favicon: ImageView,
-            override val title: TextView,
-            val url: TextView,
-            override val close: ImageView,
-            override val tabUnread: ImageView,
-            override val cardContentsContainer: ViewGroup,
-        ) : TabViewHolder(binding.root, favicon, title, close, tabUnread, cardContentsContainer)
+        ) : TabViewHolder(binding.root, binding.favicon, binding.title, binding.close, binding.tabUnread) {
+            val url: TextView = binding.url
+        }
     }
 }
diff --git a/app/src/main/res/layout/activity_tab_switcher.xml b/app/src/main/res/layout/activity_tab_switcher.xml
@@ -14,15 +14,27 @@
   ~ limitations under the License.
   -->
 
-<androidx.constraintlayout.widget.ConstraintLayout xmlns:android="http://schemas.android.com/apk/res/android"
+<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
     xmlns:tools="http://schemas.android.com/tools"
+    xmlns:app="http://schemas.android.com/apk/res-auto"
     android:layout_width="match_parent"
     android:layout_height="match_parent"
     android:background="?attr/daxColorBackground"
+    android:orientation="vertical"
     tools:context="com.duckduckgo.app.tabs.ui.TabSwitcherActivity">
 
     <include layout="@layout/include_default_toolbar" />
 
-    <include layout="@layout/content_tab_switcher" />
+    <androidx.recyclerview.widget.RecyclerView
+        android:id="@+id/tabsRecycler"
+        android:layout_width="match_parent"
+        android:layout_height="match_parent"
+        android:clipToPadding="false"
+        android:padding="@dimen/keyline_2"
+        app:layoutManager="androidx.recyclerview.widget.GridLayoutManager"
+        tools:itemCount="3"
+        tools:listitem="@layout/item_tab_grid"
+        tools:showIn="@layout/activity_tab_switcher"
+        tools:spanCount="2" />
 
-</androidx.constraintlayout.widget.ConstraintLayout>
+</LinearLayout>