From 785a80f6bf12d9b504f304c6243c0df33cad0a8b Mon Sep 17 00:00:00 2001 From: Ondrej Ruttkay <0nko@users.noreply.github.com> Date: Wed, 9 Oct 2024 14:14:32 +0200 Subject: [PATCH] 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](https://github.com/duckduckgo/Android/commit/d1b7b772f28708dd2028376b3f6c190723cb009d) - [Enable fixed grid size to optimize recycler view perf](https://github.com/duckduckgo/Android/commit/3b01bb3d04a8f36ee790f3cd4b42a3453ce2f839) - [Set stable IDs for tab switcher adapter and simplify the code](https://github.com/duckduckgo/Android/commit/6112898aabb023e8d094a6f224ca774408fc98eb) - [Optimize the tab switcher layout](https://github.com/duckduckgo/Android/commit/dbbe15e4e3956368d48c5324c2934aa1807a70aa) ### Steps to test this PR The ANRs are not easily reproducible. Smoke testing the tab manager will suffice. --- .../tabpreview/WebViewPreviewGenerator.kt | 18 +- .../app/tabs/ui/TabSwitcherActivity.kt | 1 + .../app/tabs/ui/TabSwitcherAdapter.kt | 165 +++++------------- .../main/res/layout/activity_tab_switcher.xml | 18 +- .../main/res/layout/content_tab_switcher.xml | 42 ----- app/src/main/res/layout/item_tab_grid.xml | 35 ++-- .../res/values/design-system-dimensions.xml | 2 +- 7 files changed, 85 insertions(+), 196 deletions(-) delete mode 100644 app/src/main/res/layout/content_tab_switcher.xml 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 index 3b5636cec947..4554cee1acab 100644 --- 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 index 88b1e74ef9ac..caa44ade280b 100644 --- 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 index 4f145968a410..b33be228fc2a 100644 --- 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() { private val list = mutableListOf() - 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, - ) { + override fun onBindViewHolder(holder: TabViewHolder, position: Int, payloads: MutableList) { 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,7 +197,6 @@ class TabSwitcherAdapter( fun updateData(updatedList: List) { val diffResult = DiffUtil.calculateDiff(TabEntityDiffCallback(list, updatedList)) - list.clear() list.addAll(updatedList) diffResult.dispatchUpdatesTo(this) @@ -271,10 +204,7 @@ class TabSwitcherAdapter( 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 index 70e483662624..b52eb44aecc4 100644 --- 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. --> - - + - \ No newline at end of file + \ No newline at end of file diff --git a/app/src/main/res/layout/content_tab_switcher.xml b/app/src/main/res/layout/content_tab_switcher.xml deleted file mode 100644 index 433a0ed1b9e5..000000000000 --- a/app/src/main/res/layout/content_tab_switcher.xml +++ /dev/null @@ -1,42 +0,0 @@ - - - - - - diff --git a/app/src/main/res/layout/item_tab_grid.xml b/app/src/main/res/layout/item_tab_grid.xml index 0df4f54f2cbd..ffbc5863ea8c 100644 --- a/app/src/main/res/layout/item_tab_grid.xml +++ b/app/src/main/res/layout/item_tab_grid.xml @@ -18,11 +18,8 @@ xmlns:tools="http://schemas.android.com/tools" android:layout_width="match_parent" android:layout_height="wrap_content" - app:cardCornerRadius="10dp" - android:layout_marginStart="7dp" - android:layout_marginTop="6dp" - android:layout_marginEnd="7dp" - android:layout_marginBottom="10dp"> + android:layout_margin="@dimen/keyline_2" + app:cardCornerRadius="10dp"> + tools:ignore="MissingConstraints" /> - \ No newline at end of file diff --git a/common/common-ui/src/main/res/values/design-system-dimensions.xml b/common/common-ui/src/main/res/values/design-system-dimensions.xml index 419f676c1bce..2f0e76e65e13 100644 --- a/common/common-ui/src/main/res/values/design-system-dimensions.xml +++ b/common/common-ui/src/main/res/values/design-system-dimensions.xml @@ -40,7 +40,6 @@ 3dp - 36dp 60dp 48dp 16dp @@ -71,6 +70,7 @@ 56dp 32dp 72dp + 170dp 280dp