From 079881f34ab0fd5ed8332676e1b505b64b4c5dfa Mon Sep 17 00:00:00 2001 From: Ondrej Ruttkay Date: Tue, 6 Aug 2024 00:55:45 +0200 Subject: [PATCH 1/5] Add a check for invalid index before deletion --- .../java/com/duckduckgo/app/tabs/ui/TabSwitcherActivity.kt | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) 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 27d97d34057e..b050186b502b 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 @@ -297,8 +297,10 @@ class TabSwitcherActivity : DuckDuckGoActivity(), TabSwitcherListener, Coroutine } override fun onTabDeleted(position: Int, deletedBySwipe: Boolean) { - val tab = tabsAdapter.getTab(position) - launch { viewModel.onMarkTabAsDeletable(tab, deletedBySwipe) } + if (position >= 0 && position < tabsAdapter.itemCount) { + val tab = tabsAdapter.getTab(position) + launch { viewModel.onMarkTabAsDeletable(tab, deletedBySwipe) } + } } override fun onTabMoved(from: Int, to: Int) { From bb60e26488d7adde44ce75e64dd6638911f7c3ff Mon Sep 17 00:00:00 2001 From: Ondrej Ruttkay Date: Tue, 6 Aug 2024 11:33:43 +0200 Subject: [PATCH 2/5] Add a check before scrolling --- .../java/com/duckduckgo/app/tabs/ui/TabSwitcherActivity.kt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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 b050186b502b..c1ebbd97eaf3 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 @@ -220,7 +220,9 @@ class TabSwitcherActivity : DuckDuckGoActivity(), TabSwitcherListener, Coroutine private fun scrollToShowCurrentTab() { val index = tabsAdapter.adapterPositionForTab(selectedTabId) - tabsRecycler.post { tabsRecycler.scrollToPosition(index) } + if (index != -1) { + tabsRecycler.post { tabsRecycler.scrollToPosition(index) } + } } private fun processCommand(command: Command) { From 8bfcfbddca920ce5e8250e6987179f992c8c82f1 Mon Sep 17 00:00:00 2001 From: Ondrej Ruttkay Date: Tue, 6 Aug 2024 11:46:07 +0200 Subject: [PATCH 3/5] Possibly avoid a race condition by always creating a new diff callback class --- .../app/browser/tabpreview/TabEntityDiffCallback.kt | 2 +- .../java/com/duckduckgo/app/tabs/ui/TabSwitcherAdapter.kt | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/app/src/main/java/com/duckduckgo/app/browser/tabpreview/TabEntityDiffCallback.kt b/app/src/main/java/com/duckduckgo/app/browser/tabpreview/TabEntityDiffCallback.kt index 3a711d373ce0..d55e692dafd7 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/tabpreview/TabEntityDiffCallback.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/tabpreview/TabEntityDiffCallback.kt @@ -22,7 +22,7 @@ import com.duckduckgo.app.tabs.model.TabEntity class TabEntityDiffCallback( private val oldList: List, - var newList: List, + private val newList: List, ) : DiffUtil.Callback() { private fun areItemsTheSame( 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 bc78018226ce..37b2b9b62637 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 @@ -54,7 +54,6 @@ class TabSwitcherAdapter( ) : Adapter() { private val list = mutableListOf() - private val diffCallback = TabEntityDiffCallback(list, listOf()) private var isDragging: Boolean = false @@ -201,8 +200,7 @@ class TabSwitcherAdapter( } private fun submitList(updatedList: List) { - diffCallback.newList = updatedList - val diffResult = DiffUtil.calculateDiff(diffCallback) + val diffResult = DiffUtil.calculateDiff(TabEntityDiffCallback(list, updatedList)) list.clear() list.addAll(updatedList) From d1588f5c6a3c4f99fc8dfa8db23b9e5714333b29 Mon Sep 17 00:00:00 2001 From: Ondrej Ruttkay Date: Tue, 6 Aug 2024 12:06:50 +0200 Subject: [PATCH 4/5] Make the getTab() call safer --- .../duckduckgo/app/tabs/ui/TabGridItemDecorator.kt | 12 ++++-------- .../duckduckgo/app/tabs/ui/TabSwitcherActivity.kt | 3 +-- .../com/duckduckgo/app/tabs/ui/TabSwitcherAdapter.kt | 2 +- 3 files changed, 6 insertions(+), 11 deletions(-) diff --git a/app/src/main/java/com/duckduckgo/app/tabs/ui/TabGridItemDecorator.kt b/app/src/main/java/com/duckduckgo/app/tabs/ui/TabGridItemDecorator.kt index b636479a1a39..332f647a47f4 100644 --- a/app/src/main/java/com/duckduckgo/app/tabs/ui/TabGridItemDecorator.kt +++ b/app/src/main/java/com/duckduckgo/app/tabs/ui/TabGridItemDecorator.kt @@ -53,14 +53,10 @@ class TabGridItemDecorator( val child = recyclerView.getChildAt(i) val positionInAdapter = recyclerView.getChildAdapterPosition(child) - if (positionInAdapter < 0) { - continue - } - - val tab = adapter.getTab(positionInAdapter) - - if (tab.tabId == selectedTabId) { - drawSelectedTabDecoration(child, canvas) + adapter.getTab(positionInAdapter)?.let { tab -> + if (tab.tabId == selectedTabId) { + drawSelectedTabDecoration(child, canvas) + } } } 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 c1ebbd97eaf3..16bce4bb3723 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 @@ -299,8 +299,7 @@ class TabSwitcherActivity : DuckDuckGoActivity(), TabSwitcherListener, Coroutine } override fun onTabDeleted(position: Int, deletedBySwipe: Boolean) { - if (position >= 0 && position < tabsAdapter.itemCount) { - val tab = tabsAdapter.getTab(position) + tabsAdapter.getTab(position)?.let { tab -> launch { viewModel.onMarkTabAsDeletable(tab, deletedBySwipe) } } } 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 37b2b9b62637..fdca4510edcb 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 @@ -207,7 +207,7 @@ class TabSwitcherAdapter( diffResult.dispatchUpdatesTo(this) } - fun getTab(position: Int): TabEntity = list[position] + fun getTab(position: Int): TabEntity? = list.getOrNull(position) fun adapterPositionForTab(tabId: String?): Int { if (tabId == null) return -1 From df36fcb30ab07f93094c53f82f9030f570762837 Mon Sep 17 00:00:00 2001 From: Ondrej Ruttkay Date: Tue, 6 Aug 2024 12:11:24 +0200 Subject: [PATCH 5/5] Make the VM properties immutable --- .../java/com/duckduckgo/app/tabs/ui/TabSwitcherViewModel.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/com/duckduckgo/app/tabs/ui/TabSwitcherViewModel.kt b/app/src/main/java/com/duckduckgo/app/tabs/ui/TabSwitcherViewModel.kt index 1ba0146656f8..dd74718b393b 100644 --- a/app/src/main/java/com/duckduckgo/app/tabs/ui/TabSwitcherViewModel.kt +++ b/app/src/main/java/com/duckduckgo/app/tabs/ui/TabSwitcherViewModel.kt @@ -55,9 +55,9 @@ class TabSwitcherViewModel @Inject constructor( const val REINSTALL_VARIANT = "ru" } - var tabs: LiveData> = tabRepository.liveTabs + val tabs: LiveData> = tabRepository.liveTabs val activeTab = tabRepository.liveSelectedTab - var deletableTabs: LiveData> = tabRepository.flowDeletableTabs.asLiveData( + val deletableTabs: LiveData> = tabRepository.flowDeletableTabs.asLiveData( context = viewModelScope.coroutineContext, )