From 6bd17fcd4f3ed82f14c576e5528af61fa09e2b5a Mon Sep 17 00:00:00 2001 From: 0nko Date: Thu, 16 Jan 2025 18:42:08 +0100 Subject: [PATCH 01/16] Do not repeat injection and pulse animation initialization on each attachment --- .../app/browser/omnibar/OmnibarLayout.kt | 74 ++++++++----------- 1 file changed, 32 insertions(+), 42 deletions(-) diff --git a/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt b/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt index b77db976257f..4f4113cdd115 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt @@ -32,14 +32,15 @@ import android.widget.ProgressBar import android.widget.TextView import androidx.appcompat.widget.Toolbar import androidx.coordinatorlayout.widget.CoordinatorLayout -import androidx.core.view.ViewCompat.isAttachedToWindow import androidx.core.view.doOnLayout import androidx.core.view.isInvisible import androidx.core.view.isVisible +import androidx.lifecycle.LifecycleOwner import androidx.lifecycle.ViewModelProvider import androidx.lifecycle.findViewTreeLifecycleOwner import androidx.lifecycle.findViewTreeViewModelStoreOwner -import androidx.lifecycle.viewmodel.viewModelFactory +import androidx.lifecycle.flowWithLifecycle +import androidx.lifecycle.lifecycleScope import com.airbnb.lottie.LottieAnimationView import com.duckduckgo.anvil.annotations.InjectWith import com.duckduckgo.app.browser.PulseAnimation @@ -84,13 +85,9 @@ import com.duckduckgo.di.scopes.FragmentScope import com.google.android.material.appbar.AppBarLayout import dagger.android.support.AndroidSupportInjection import javax.inject.Inject -import kotlinx.coroutines.CoroutineScope -import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.SupervisorJob -import kotlinx.coroutines.coroutineScope -import kotlinx.coroutines.flow.launchIn +import kotlinx.coroutines.flow.collectLatest import kotlinx.coroutines.flow.map -import kotlinx.coroutines.flow.onEach +import kotlinx.coroutines.launch import timber.log.Timber @InjectWith(FragmentScope::class) @@ -140,7 +137,13 @@ class OmnibarLayout @JvmOverloads constructor( @Inject lateinit var pixel: Pixel - private lateinit var pulseAnimation: PulseAnimation + private val lifecycleOwner: LifecycleOwner by lazy { + requireNotNull(findViewTreeLifecycleOwner()) + } + + private val pulseAnimation: PulseAnimation by lazy { + PulseAnimation(lifecycleOwner) + } private var omnibarTextListener: Omnibar.TextListener? = null private var omnibarItemPressedListener: Omnibar.ItemPressedListener? = null @@ -190,6 +193,8 @@ class OmnibarLayout @JvmOverloads constructor( R.layout.view_new_omnibar } inflate(context, layout, this) + + AndroidSupportInjection.inject(this) } private fun omnibarViews(): List = listOf( @@ -227,8 +232,6 @@ class OmnibarLayout @JvmOverloads constructor( } } - private var coroutineScope: CoroutineScope? = null - private val smoothProgressAnimator by lazy { SmoothProgressAnimator(pageLoadingIndicator) } private val viewModel: OmnibarLayoutViewModel by lazy { @@ -239,21 +242,19 @@ class OmnibarLayout @JvmOverloads constructor( } override fun onAttachedToWindow() { - AndroidSupportInjection.inject(this) super.onAttachedToWindow() - pulseAnimation = PulseAnimation(findViewTreeLifecycleOwner()!!) - - @SuppressLint("NoHardcodedCoroutineDispatcher") - coroutineScope = CoroutineScope(SupervisorJob() + Dispatchers.Main) - - viewModel.viewState - .onEach { render(it) } - .launchIn(coroutineScope!!) + lifecycleOwner.lifecycleScope.launch { + viewModel.viewState.flowWithLifecycle(lifecycleOwner.lifecycle).collectLatest { + render(it) + } + } - viewModel.commands() - .onEach { processCommand(it) } - .launchIn(coroutineScope!!) + lifecycleOwner.lifecycleScope.launch { + viewModel.commands().flowWithLifecycle(lifecycleOwner.lifecycle).collectLatest { + processCommand(it) + } + } viewModel.onAttachedToWindow() @@ -388,6 +389,7 @@ class OmnibarLayout @JvmOverloads constructor( } private fun render(viewState: ViewState) { + Timber.d("$$$ Omnibar: render $this") when (viewState.viewMode) { is CustomTab -> { renderCustomTabMode(viewState, viewState.viewMode) @@ -614,31 +616,19 @@ class OmnibarLayout @JvmOverloads constructor( // omnibar only scrollable when browser showing and the fire button is not promoted if (targetView != null) { - if (this::pulseAnimation.isInitialized) { - if (pulseAnimation.isActive) { - pulseAnimation.stop() - } - doOnLayout { - if (this::pulseAnimation.isInitialized) { - pulseAnimation.playOn(targetView) - } - } - } - } else { - if (this::pulseAnimation.isInitialized) { + if (pulseAnimation.isActive) { pulseAnimation.stop() } - } - } - - fun isPulseAnimationPlaying(): Boolean { - return if (this::pulseAnimation.isInitialized) { - pulseAnimation.isActive + doOnLayout { + pulseAnimation.playOn(targetView) + } } else { - false + pulseAnimation.stop() } } + fun isPulseAnimationPlaying() = pulseAnimation.isActive + private fun createCookiesAnimation(isCosmetic: Boolean) { if (this::animatorHelper.isInitialized) { animatorHelper.createCookiesAnimation( From 9d20d24cc2655e519367dde9aec44fda4ef65c1d Mon Sep 17 00:00:00 2001 From: 0nko Date: Thu, 16 Jan 2025 18:43:32 +0100 Subject: [PATCH 02/16] Remove the onboarding restriction for tab swiping --- .../com/duckduckgo/app/browser/BrowserActivity.kt | 15 ++++----------- .../duckduckgo/app/browser/BrowserViewModel.kt | 7 ------- 2 files changed, 4 insertions(+), 18 deletions(-) diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserActivity.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserActivity.kt index 886137e2394e..b6a832e24553 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserActivity.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserActivity.kt @@ -96,7 +96,6 @@ import javax.inject.Inject import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Job import kotlinx.coroutines.flow.collectLatest -import kotlinx.coroutines.flow.combine import kotlinx.coroutines.launch import kotlinx.coroutines.runBlocking import timber.log.Timber @@ -168,13 +167,7 @@ open class BrowserActivity : DuckDuckGoActivity() { _currentTab = value } - private val isInEditMode = SwitcherFlow() - - private val isSwipingEnabled by lazy { - combine(viewModel.isOnboardingCompleted, isInEditMode) { isOnboardingCompleted, isInEditMode -> - isOnboardingCompleted && !isInEditMode - } - } + private val isOmnibarInEditMode = SwitcherFlow() private val viewModel: BrowserViewModel by bindViewModel() @@ -533,8 +526,8 @@ open class BrowserActivity : DuckDuckGoActivity() { // enable/disable swiping based on the edit mode and onboarding state lifecycleScope.launch { - isSwipingEnabled.flowWithLifecycle(lifecycle).collectLatest { - tabPager.isUserInputEnabled = it + isOmnibarInEditMode.flowWithLifecycle(lifecycle).collectLatest { isInEditMode -> + tabPager.isUserInputEnabled = !isInEditMode } } } else { @@ -557,7 +550,7 @@ open class BrowserActivity : DuckDuckGoActivity() { tabPager.postDelayed(TAB_SWIPING_OBSERVER_DELAY) { currentTab?.isInEditMode?.let { lifecycleScope.launch { - isInEditMode.switch(it) + isOmnibarInEditMode.switch(it) } } } diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserViewModel.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserViewModel.kt index 9cb3077d8b8a..2ddb7d33cb60 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserViewModel.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserViewModel.kt @@ -33,8 +33,6 @@ import com.duckduckgo.app.global.rating.AppEnjoymentPromptEmitter import com.duckduckgo.app.global.rating.AppEnjoymentPromptOptions import com.duckduckgo.app.global.rating.AppEnjoymentUserEventRecorder import com.duckduckgo.app.global.rating.PromptCount -import com.duckduckgo.app.onboarding.store.AppStage -import com.duckduckgo.app.onboarding.store.UserStageStore import com.duckduckgo.app.pixels.AppPixelName import com.duckduckgo.app.pixels.AppPixelName.APP_ENJOYMENT_DIALOG_SHOWN import com.duckduckgo.app.pixels.AppPixelName.APP_ENJOYMENT_DIALOG_USER_CANCELLED @@ -88,7 +86,6 @@ class BrowserViewModel @Inject constructor( private val showOnAppLaunchFeature: ShowOnAppLaunchFeature, private val showOnAppLaunchOptionHandler: ShowOnAppLaunchOptionHandler, private val swipingTabsFeature: SwipingTabsFeatureProvider, - userStageStore: UserStageStore, ) : ViewModel(), CoroutineScope { @@ -136,10 +133,6 @@ class BrowserViewModel @Inject constructor( tabs.indexOf(selectedTab) }.filterNot { it == -1 } - val isOnboardingCompleted: Flow = userStageStore.currentAppStage - .distinctUntilChanged() - .map { it != AppStage.DAX_ONBOARDING } - private var dataClearingObserver = Observer { state -> when (state) { ApplicationClearDataState.INITIALIZING -> { From 33862a317c166ef0a7049ce059d1da6df73600ea Mon Sep 17 00:00:00 2001 From: 0nko Date: Thu, 16 Jan 2025 19:44:15 +0100 Subject: [PATCH 03/16] Update the unit tests --- .../duckduckgo/app/browser/BrowserActivity.kt | 16 +++++++++---- .../app/browser/BrowserViewModel.kt | 5 ++++ .../app/browser/BrowserViewModelTest.kt | 23 +++++++++++++++---- 3 files changed, 34 insertions(+), 10 deletions(-) diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserActivity.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserActivity.kt index b6a832e24553..5fb35bba22c0 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserActivity.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserActivity.kt @@ -96,6 +96,9 @@ import javax.inject.Inject import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Job import kotlinx.coroutines.flow.collectLatest +import kotlinx.coroutines.flow.distinctUntilChanged +import kotlinx.coroutines.flow.launchIn +import kotlinx.coroutines.flow.onEach import kotlinx.coroutines.launch import kotlinx.coroutines.runBlocking import timber.log.Timber @@ -524,12 +527,11 @@ open class BrowserActivity : DuckDuckGoActivity() { } } - // enable/disable swiping based on the edit mode and onboarding state - lifecycleScope.launch { - isOmnibarInEditMode.flowWithLifecycle(lifecycle).collectLatest { isInEditMode -> - tabPager.isUserInputEnabled = !isInEditMode + isOmnibarInEditMode.distinctUntilChanged() + .onEach { + viewModel.onOmnibarEditModeChanged(it) } - } + .launchIn(lifecycleScope) } else { viewModel.selectedTab.observe(this) { if (it != null) { @@ -764,6 +766,10 @@ open class BrowserActivity : DuckDuckGoActivity() { } else { showWebContent() } + + if (swipingTabsFeature.isEnabled) { + tabPager.isUserInputEnabled = viewState.isTabSwipingEnabled + } } } diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserViewModel.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserViewModel.kt index 2ddb7d33cb60..4fe1be3cc44f 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserViewModel.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserViewModel.kt @@ -94,6 +94,7 @@ class BrowserViewModel @Inject constructor( data class ViewState( val hideWebContent: Boolean = true, + val isTabSwipingEnabled: Boolean = false, ) sealed class Command { @@ -352,6 +353,10 @@ class BrowserViewModel @Inject constructor( pixel.fire(AppPixelName.SWIPE_TABS_USED) pixel.fire(pixel = AppPixelName.SWIPE_TABS_USED_DAILY, type = Daily()) } + + fun onOmnibarEditModeChanged(isInEditMode: Boolean) { + viewState.value = currentViewState.copy(isTabSwipingEnabled = !isInEditMode) + } } /** diff --git a/app/src/test/java/com/duckduckgo/app/browser/BrowserViewModelTest.kt b/app/src/test/java/com/duckduckgo/app/browser/BrowserViewModelTest.kt index 1d74917360fa..234610690b7b 100644 --- a/app/src/test/java/com/duckduckgo/app/browser/BrowserViewModelTest.kt +++ b/app/src/test/java/com/duckduckgo/app/browser/BrowserViewModelTest.kt @@ -29,7 +29,6 @@ import com.duckduckgo.app.global.rating.AppEnjoymentPromptEmitter import com.duckduckgo.app.global.rating.AppEnjoymentPromptOptions import com.duckduckgo.app.global.rating.AppEnjoymentUserEventRecorder import com.duckduckgo.app.global.rating.PromptCount -import com.duckduckgo.app.onboarding.store.UserStageStore import com.duckduckgo.app.pixels.AppPixelName import com.duckduckgo.app.statistics.pixels.Pixel import com.duckduckgo.app.statistics.pixels.Pixel.PixelParameter @@ -39,7 +38,6 @@ import com.duckduckgo.app.tabs.model.TabRepository import com.duckduckgo.common.test.CoroutineTestRule import com.duckduckgo.feature.toggles.api.FakeFeatureToggleFactory import com.duckduckgo.feature.toggles.api.Toggle.State -import com.duckduckgo.tabs.model.TabDataRepositoryTest.Companion.TAB_ID import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.test.runTest import org.junit.After @@ -85,8 +83,6 @@ class BrowserViewModelTest { @Mock private lateinit var showOnAppLaunchOptionHandler: ShowOnAppLaunchOptionHandler - @Mock private lateinit var userStageStore: UserStageStore - private val fakeShowOnAppLaunchFeatureToggle = FakeFeatureToggleFactory.create(ShowOnAppLaunchFeature::class.java) private lateinit var testee: BrowserViewModel @@ -324,6 +320,24 @@ class BrowserViewModelTest { verify(showOnAppLaunchOptionHandler).handleAppLaunchOption() } + @Test + fun whenOmnibarIsInEditModeTabSwipingIsDisabled() { + swipingTabsFeature.self().setRawStoredState(State(enable = true)) + + val isInEditMode = true + testee.onOmnibarEditModeChanged(isInEditMode) + assertEquals(!isInEditMode, testee.viewState.value!!.isTabSwipingEnabled) + } + + @Test + fun whenOmnibarIsInNotEditModeTabSwipingIsEnabled() { + swipingTabsFeature.self().setRawStoredState(State(enable = true)) + + val isInEditMode = false + testee.onOmnibarEditModeChanged(isInEditMode) + assertEquals(!isInEditMode, testee.viewState.value!!.isTabSwipingEnabled) + } + private fun initTestee() { testee = BrowserViewModel( tabRepository = mockTabRepository, @@ -337,7 +351,6 @@ class BrowserViewModelTest { skipUrlConversionOnNewTabFeature = skipUrlConversionOnNewTabFeature, showOnAppLaunchFeature = fakeShowOnAppLaunchFeatureToggle, showOnAppLaunchOptionHandler = showOnAppLaunchOptionHandler, - userStageStore = userStageStore, swipingTabsFeature = swipingTabsFeatureProvider, ) } From 0f639049c291ddcc9384f8f4c2a535157c52ceba Mon Sep 17 00:00:00 2001 From: 0nko Date: Thu, 16 Jan 2025 19:48:54 +0100 Subject: [PATCH 04/16] Remove unrelated comment --- .../java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt | 1 - 1 file changed, 1 deletion(-) diff --git a/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt b/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt index 4f4113cdd115..37052c24aac8 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt @@ -614,7 +614,6 @@ class OmnibarLayout @JvmOverloads constructor( null } - // omnibar only scrollable when browser showing and the fire button is not promoted if (targetView != null) { if (pulseAnimation.isActive) { pulseAnimation.stop() From 2910eccc0a1802acb3e7e0f27a164ce06de5437a Mon Sep 17 00:00:00 2001 From: 0nko Date: Fri, 17 Jan 2025 12:04:27 +0100 Subject: [PATCH 05/16] Optimize ViewModel flow subscriptions to avoid duplicates --- .../app/browser/omnibar/OmnibarLayout.kt | 29 ++++++++++--------- .../browser/omnibar/OmnibarLayoutViewModel.kt | 10 +++---- 2 files changed, 20 insertions(+), 19 deletions(-) diff --git a/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt b/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt index 37052c24aac8..95e4843ca231 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt @@ -151,6 +151,8 @@ class OmnibarLayout @JvmOverloads constructor( private var decoration: Decoration? = null private var stateBuffer: MutableList = mutableListOf() + private var viewModelSubscribed = false + internal val findInPage by lazy { IncludeFindInPageBinding.bind(findViewById(R.id.findInPage)) } internal val omnibarTextInput: KeyboardAwareEditText by lazy { findViewById(R.id.omnibarTextInput) } internal val tabsMenu: TabSwitcherButton by lazy { findViewById(R.id.tabsMenu) } @@ -244,19 +246,21 @@ class OmnibarLayout @JvmOverloads constructor( override fun onAttachedToWindow() { super.onAttachedToWindow() - lifecycleOwner.lifecycleScope.launch { - viewModel.viewState.flowWithLifecycle(lifecycleOwner.lifecycle).collectLatest { - render(it) + if (!viewModelSubscribed) { + lifecycleOwner.lifecycleScope.launch { + viewModel.viewState.flowWithLifecycle(lifecycleOwner.lifecycle).collectLatest { + render(it) + } } - } - lifecycleOwner.lifecycleScope.launch { - viewModel.commands().flowWithLifecycle(lifecycleOwner.lifecycle).collectLatest { - processCommand(it) + lifecycleOwner.lifecycleScope.launch { + viewModel.commands().flowWithLifecycle(lifecycleOwner.lifecycle).collectLatest { + processCommand(it) + } } - } - viewModel.onAttachedToWindow() + viewModelSubscribed = true + } if (decoration != null) { decorateDeferred(decoration!!) @@ -389,7 +393,6 @@ class OmnibarLayout @JvmOverloads constructor( } private fun render(viewState: ViewState) { - Timber.d("$$$ Omnibar: render $this") when (viewState.viewMode) { is CustomTab -> { renderCustomTabMode(viewState, viewState.viewMode) @@ -417,10 +420,8 @@ class OmnibarLayout @JvmOverloads constructor( } private fun renderTabIcon(viewState: ViewState) { - if (viewState.shouldUpdateTabsCount) { - tabsMenu.count = viewState.tabs.count() - tabsMenu.hasUnread = viewState.tabs.firstOrNull { !it.viewed } != null - } + tabsMenu.count = viewState.tabCount + tabsMenu.hasUnread = viewState.hasUnreadTabs } private fun renderLeadingIconState(iconState: OmnibarLayoutViewModel.LeadingIconState) { diff --git a/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayoutViewModel.kt b/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayoutViewModel.kt index 364bd611eea8..bf874e2c9977 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayoutViewModel.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayoutViewModel.kt @@ -44,7 +44,6 @@ import com.duckduckgo.app.pixels.AppPixelName import com.duckduckgo.app.statistics.pixels.Pixel import com.duckduckgo.app.statistics.pixels.Pixel.PixelParameter.FIRE_BUTTON_STATE import com.duckduckgo.app.statistics.pixels.Pixel.PixelType.Unique -import com.duckduckgo.app.tabs.model.TabEntity import com.duckduckgo.app.tabs.model.TabRepository import com.duckduckgo.app.trackerdetection.model.Entity import com.duckduckgo.browser.api.UserBrowserProperties @@ -99,7 +98,8 @@ class OmnibarLayoutViewModel @Inject constructor( val updateOmnibarText: Boolean = false, val shouldMoveCaretToEnd: Boolean = false, val shouldMoveCaretToStart: Boolean = false, - val tabs: List = emptyList(), + val tabCount: Int = 0, + val hasUnreadTabs: Boolean = false, val shouldUpdateTabsCount: Boolean = false, val showVoiceSearch: Boolean = false, val showClearButton: Boolean = false, @@ -126,13 +126,13 @@ class OmnibarLayoutViewModel @Inject constructor( GLOBE, } - fun onAttachedToWindow() { + init { tabRepository.flowTabs .onEach { tabs -> _viewState.update { it.copy( - shouldUpdateTabsCount = tabs.count() != it.tabs.count() || tabs.isNotEmpty(), - tabs = tabs, + tabCount = tabs.size, + hasUnreadTabs = tabs.firstOrNull { !it.viewed } != null, ) } }.flowOn(dispatcherProvider.io()) From 79fcdcadfbaf3bfa5773b4a9379d18656d6ba1a6 Mon Sep 17 00:00:00 2001 From: 0nko Date: Fri, 17 Jan 2025 12:55:09 +0100 Subject: [PATCH 06/16] Cancel flow subscriptions if omnibar is removed --- .../duckduckgo/app/browser/omnibar/Omnibar.kt | 2 ++ .../app/browser/omnibar/OmnibarLayout.kt | 25 +++++++++++++------ .../browser/omnibar/OmnibarLayoutViewModel.kt | 1 + 3 files changed, 21 insertions(+), 7 deletions(-) diff --git a/app/src/main/java/com/duckduckgo/app/browser/omnibar/Omnibar.kt b/app/src/main/java/com/duckduckgo/app/browser/omnibar/Omnibar.kt index 8059d453616d..abc81dfcb458 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/omnibar/Omnibar.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/omnibar/Omnibar.kt @@ -68,10 +68,12 @@ class Omnibar( when (omnibarPosition) { OmnibarPosition.TOP -> { binding.rootView.removeView(binding.newOmnibarBottom) + binding.newOmnibarBottom.onRemoved() } OmnibarPosition.BOTTOM -> { binding.rootView.removeView(binding.newOmnibar) + binding.newOmnibar.onRemoved() // remove the default top abb bar behavior removeAppBarBehavior(binding.autoCompleteSuggestionsList) diff --git a/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt b/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt index 95e4843ca231..dcbb8c7d21b9 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt @@ -85,6 +85,7 @@ import com.duckduckgo.di.scopes.FragmentScope import com.google.android.material.appbar.AppBarLayout import dagger.android.support.AndroidSupportInjection import javax.inject.Inject +import kotlinx.coroutines.Job import kotlinx.coroutines.flow.collectLatest import kotlinx.coroutines.flow.map import kotlinx.coroutines.launch @@ -151,7 +152,9 @@ class OmnibarLayout @JvmOverloads constructor( private var decoration: Decoration? = null private var stateBuffer: MutableList = mutableListOf() - private var viewModelSubscribed = false + private var isViewModelSubscribed = false + private var viewStateJob: Job? = null + private var commandJob: Job? = null internal val findInPage by lazy { IncludeFindInPageBinding.bind(findViewById(R.id.findInPage)) } internal val omnibarTextInput: KeyboardAwareEditText by lazy { findViewById(R.id.omnibarTextInput) } @@ -243,23 +246,29 @@ class OmnibarLayout @JvmOverloads constructor( )[OmnibarLayoutViewModel::class.java] } + // One OmnibarLayout is always removed from the view hierarchy, we should cancel any flow subscriptions + fun onRemoved() { + // viewStateJob?.cancel() + // commandJob?.cancel() + } + override fun onAttachedToWindow() { super.onAttachedToWindow() - if (!viewModelSubscribed) { - lifecycleOwner.lifecycleScope.launch { + if (!isViewModelSubscribed) { + viewStateJob = lifecycleOwner.lifecycleScope.launch { viewModel.viewState.flowWithLifecycle(lifecycleOwner.lifecycle).collectLatest { render(it) } } - lifecycleOwner.lifecycleScope.launch { + commandJob = lifecycleOwner.lifecycleScope.launch { viewModel.commands().flowWithLifecycle(lifecycleOwner.lifecycle).collectLatest { processCommand(it) } } - viewModelSubscribed = true + isViewModelSubscribed = true } if (decoration != null) { @@ -420,8 +429,10 @@ class OmnibarLayout @JvmOverloads constructor( } private fun renderTabIcon(viewState: ViewState) { - tabsMenu.count = viewState.tabCount - tabsMenu.hasUnread = viewState.hasUnreadTabs + if (viewState.shouldUpdateTabsCount) { + tabsMenu.count = viewState.tabCount + tabsMenu.hasUnread = viewState.hasUnreadTabs + } } private fun renderLeadingIconState(iconState: OmnibarLayoutViewModel.LeadingIconState) { diff --git a/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayoutViewModel.kt b/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayoutViewModel.kt index bf874e2c9977..6054d106548c 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayoutViewModel.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayoutViewModel.kt @@ -131,6 +131,7 @@ class OmnibarLayoutViewModel @Inject constructor( .onEach { tabs -> _viewState.update { it.copy( + shouldUpdateTabsCount = tabs.size != it.tabCount && tabs.isNotEmpty(), tabCount = tabs.size, hasUnreadTabs = tabs.firstOrNull { !it.viewed } != null, ) From 87601908abf84d4d756e39dff9e1764fb0499de8 Mon Sep 17 00:00:00 2001 From: 0nko Date: Fri, 17 Jan 2025 13:17:50 +0100 Subject: [PATCH 07/16] Fix the unit tests --- .../omnibar/OmnibarLayoutViewModelTest.kt | 38 +++++++++---------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/app/src/test/java/com/duckduckgo/app/browser/omnibar/OmnibarLayoutViewModelTest.kt b/app/src/test/java/com/duckduckgo/app/browser/omnibar/OmnibarLayoutViewModelTest.kt index 906e4e78e1d8..c3be5a0a49ea 100644 --- a/app/src/test/java/com/duckduckgo/app/browser/omnibar/OmnibarLayoutViewModelTest.kt +++ b/app/src/test/java/com/duckduckgo/app/browser/omnibar/OmnibarLayoutViewModelTest.kt @@ -68,29 +68,18 @@ class OmnibarLayoutViewModelTest { @Before fun before() { - testee = OmnibarLayoutViewModel( - tabRepository = tabRepository, - voiceSearchAvailability = voiceSearchAvailability, - voiceSearchPixelLogger = voiceSearchPixelLogger, - duckDuckGoUrlDetector = duckDuckGoUrlDetector, - duckPlayer = duckPlayer, - pixel = pixel, - userBrowserProperties = userBrowserProperties, - dispatcherProvider = coroutineTestRule.testDispatcherProvider, - ) - whenever(tabRepository.flowTabs).thenReturn(flowOf(emptyList())) whenever(voiceSearchAvailability.shouldShowVoiceSearch(any(), any(), any(), any())).thenReturn(true) whenever(duckPlayer.isDuckPlayerUri(DUCK_PLAYER_URL)).thenReturn(true) + + initializeViewModel() } @Test fun whenViewModelAttachedAndNoTabsOpenThenTabsRetrieved() = runTest { - testee.onAttachedToWindow() - testee.viewState.test { val viewState = awaitItem() - assertTrue(viewState.tabs.isEmpty()) + assertTrue(viewState.tabCount == 0) } } @@ -98,20 +87,33 @@ class OmnibarLayoutViewModelTest { fun whenViewModelAttachedAndTabsOpenedThenTabsRetrieved() = runTest { whenever(tabRepository.flowTabs).thenReturn(flowOf(listOf(TabEntity(tabId = "0", position = 0)))) - testee.onAttachedToWindow() + initializeViewModel() testee.viewState.test { val viewState = awaitItem() - assertTrue(viewState.tabs.size == 1) + assertTrue(viewState.tabCount == 1) assertTrue(viewState.shouldUpdateTabsCount) } } + private fun initializeViewModel() { + testee = OmnibarLayoutViewModel( + tabRepository = tabRepository, + voiceSearchAvailability = voiceSearchAvailability, + voiceSearchPixelLogger = voiceSearchPixelLogger, + duckDuckGoUrlDetector = duckDuckGoUrlDetector, + duckPlayer = duckPlayer, + pixel = pixel, + userBrowserProperties = userBrowserProperties, + dispatcherProvider = coroutineTestRule.testDispatcherProvider, + ) + } + @Test fun whenViewModelAttachedAndVoiceSearchSupportedThenPixelLogged() = runTest { whenever(voiceSearchAvailability.isVoiceSearchSupported).thenReturn(true) - testee.onAttachedToWindow() + initializeViewModel() verify(voiceSearchPixelLogger).log() } @@ -120,8 +122,6 @@ class OmnibarLayoutViewModelTest { fun whenViewModelAttachedAndVoiceSearchNotSupportedThenPixelLogged() = runTest { whenever(voiceSearchAvailability.isVoiceSearchSupported).thenReturn(false) - testee.onAttachedToWindow() - verifyNoInteractions(voiceSearchPixelLogger) } From cc5528bb25590caed60d67e14c3b84a17dab701c Mon Sep 17 00:00:00 2001 From: 0nko Date: Fri, 17 Jan 2025 15:45:20 +0100 Subject: [PATCH 08/16] Uncomment the code --- .../java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt b/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt index dcbb8c7d21b9..8a7a7b1516d6 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt @@ -248,8 +248,8 @@ class OmnibarLayout @JvmOverloads constructor( // One OmnibarLayout is always removed from the view hierarchy, we should cancel any flow subscriptions fun onRemoved() { - // viewStateJob?.cancel() - // commandJob?.cancel() + viewStateJob?.cancel() + commandJob?.cancel() } override fun onAttachedToWindow() { From 3114d366faab0a28caa0a69692d16be24455eaa6 Mon Sep 17 00:00:00 2001 From: 0nko Date: Fri, 17 Jan 2025 16:14:44 +0100 Subject: [PATCH 09/16] Simplify and fix the edit mode detection --- .../duckduckgo/app/browser/BrowserActivity.kt | 29 ++----------- .../app/browser/BrowserTabFragment.kt | 8 +++- .../duckduckgo/common/utils/SwitcherFlow.kt | 41 ------------------- 3 files changed, 10 insertions(+), 68 deletions(-) delete mode 100644 common/common-utils/src/main/java/com/duckduckgo/common/utils/SwitcherFlow.kt diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserActivity.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserActivity.kt index 5fb35bba22c0..7423af096169 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserActivity.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserActivity.kt @@ -83,7 +83,6 @@ import com.duckduckgo.common.ui.view.show import com.duckduckgo.common.ui.view.toPx import com.duckduckgo.common.ui.viewbinding.viewBinding import com.duckduckgo.common.utils.DispatcherProvider -import com.duckduckgo.common.utils.SwitcherFlow import com.duckduckgo.common.utils.playstore.PlayStoreUtils import com.duckduckgo.di.scopes.ActivityScope import com.duckduckgo.navigation.api.GlobalActivityStarter @@ -170,8 +169,6 @@ open class BrowserActivity : DuckDuckGoActivity() { _currentTab = value } - private val isOmnibarInEditMode = SwitcherFlow() - private val viewModel: BrowserViewModel by bindViewModel() private var instanceStateBundles: CombinedInstanceState? = null @@ -515,9 +512,6 @@ open class BrowserActivity : DuckDuckGoActivity() { lifecycleScope.launch { viewModel.selectedTabFlow.flowWithLifecycle(lifecycle).collectLatest { tabManager.onSelectedTabChanged(it) - - // update the observed edit mode flow observer to the current tab - switchEditModeObserver() } } @@ -526,12 +520,6 @@ open class BrowserActivity : DuckDuckGoActivity() { onMoveToTabRequested(it) } } - - isOmnibarInEditMode.distinctUntilChanged() - .onEach { - viewModel.onOmnibarEditModeChanged(it) - } - .launchIn(lifecycleScope) } else { viewModel.selectedTab.observe(this) { if (it != null) { @@ -547,17 +535,6 @@ open class BrowserActivity : DuckDuckGoActivity() { } } - private fun switchEditModeObserver() { - // switch the edit mode flow to the current tab - tabPager.postDelayed(TAB_SWIPING_OBSERVER_DELAY) { - currentTab?.isInEditMode?.let { - lifecycleScope.launch { - isOmnibarInEditMode.switch(it) - } - } - } - } - private fun removeObservers() { viewModel.command.removeObservers(this) @@ -748,8 +725,6 @@ open class BrowserActivity : DuckDuckGoActivity() { private const val LAUNCH_FROM_CLEAR_DATA_ACTION = "LAUNCH_FROM_CLEAR_DATA_ACTION" private const val MAX_ACTIVE_TABS = 40 - - private const val TAB_SWIPING_OBSERVER_DELAY = 500L } inner class BrowserStateRenderer { @@ -975,6 +950,10 @@ open class BrowserActivity : DuckDuckGoActivity() { } } + fun onEditModeChanged(isInEditMode: Boolean) { + viewModel.onOmnibarEditModeChanged(isInEditMode) + } + private data class CombinedInstanceState( val originalInstanceState: Bundle?, val newInstanceState: Bundle?, diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt index 3689162c79f1..7fe35dcce5ef 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt @@ -627,8 +627,6 @@ class BrowserTabFragment : } } - val isInEditMode by lazy { omnibar.isInEditMode } - private val errorSnackbar: Snackbar by lazy { binding.browserLayout.makeSnackbarWithNoBottomInset(R.string.crashedWebViewErrorMessage, Snackbar.LENGTH_INDEFINITE) .setBehavior(NonDismissibleBehavior()) @@ -899,6 +897,12 @@ class BrowserTabFragment : configureOmnibarTextInput() configureItemPressedListener() configureCustomTab() + + omnibar.isInEditMode.onEach { isInEditMode -> + if (isActiveTab) { + browserActivity?.onEditModeChanged(isInEditMode) + } + }.launchIn(lifecycleScope) } private fun onOmnibarTabsButtonPressed() { diff --git a/common/common-utils/src/main/java/com/duckduckgo/common/utils/SwitcherFlow.kt b/common/common-utils/src/main/java/com/duckduckgo/common/utils/SwitcherFlow.kt deleted file mode 100644 index b8472868b7c5..000000000000 --- a/common/common-utils/src/main/java/com/duckduckgo/common/utils/SwitcherFlow.kt +++ /dev/null @@ -1,41 +0,0 @@ -/* - * Copyright (c) 2025 DuckDuckGo - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -package com.duckduckgo.common.utils - -import kotlinx.coroutines.ExperimentalCoroutinesApi -import kotlinx.coroutines.flow.Flow -import kotlinx.coroutines.flow.MutableSharedFlow -import kotlinx.coroutines.flow.distinctUntilChanged -import kotlinx.coroutines.flow.flatMapLatest - -/** - * SwitcherFlow is a utility class that allows switching between multiple upstream flows. - * It uses a MutableSharedFlow to emit the latest upstream flow and ensures that only distinct - * values are emitted by using distinctUntilChanged. - * - * @param T the type of elements emitted by the upstream flows - * @property flowOfSources a MutableSharedFlow that holds the current upstream flow - * - */ -@OptIn(ExperimentalCoroutinesApi::class) -class SwitcherFlow( - private val flowOfSources: MutableSharedFlow> = MutableSharedFlow>(), -) : Flowby flowOfSources.flatMapLatest({ it }).distinctUntilChanged() { - suspend fun switch(upstream: Flow) { - flowOfSources.emit(upstream) - } -} From 6355e1b6197c1fe9338faad6233b17072de0d689 Mon Sep 17 00:00:00 2001 From: 0nko Date: Fri, 17 Jan 2025 16:20:11 +0100 Subject: [PATCH 10/16] Optimize imports --- .../main/java/com/duckduckgo/app/browser/BrowserActivity.kt | 3 --- 1 file changed, 3 deletions(-) diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserActivity.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserActivity.kt index 7423af096169..d8d36ebe24ec 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserActivity.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserActivity.kt @@ -95,9 +95,6 @@ import javax.inject.Inject import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Job import kotlinx.coroutines.flow.collectLatest -import kotlinx.coroutines.flow.distinctUntilChanged -import kotlinx.coroutines.flow.launchIn -import kotlinx.coroutines.flow.onEach import kotlinx.coroutines.launch import kotlinx.coroutines.runBlocking import timber.log.Timber From 81dac775d05595873ea202a1a2fc9a24554cdf19 Mon Sep 17 00:00:00 2001 From: 0nko Date: Fri, 17 Jan 2025 16:25:43 +0100 Subject: [PATCH 11/16] Add a missing feature flag check --- .../duckduckgo/app/browser/BrowserTabFragment.kt | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt index 7fe35dcce5ef..897bce257744 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt @@ -897,12 +897,17 @@ class BrowserTabFragment : configureOmnibarTextInput() configureItemPressedListener() configureCustomTab() + configureEditModeChangeDetection() + } - omnibar.isInEditMode.onEach { isInEditMode -> - if (isActiveTab) { - browserActivity?.onEditModeChanged(isInEditMode) - } - }.launchIn(lifecycleScope) + private fun configureEditModeChangeDetection() { + if (swipingTabsFeature.isEnabled) { + omnibar.isInEditMode.onEach { isInEditMode -> + if (isActiveTab) { + browserActivity?.onEditModeChanged(isInEditMode) + } + }.launchIn(lifecycleScope) + } } private fun onOmnibarTabsButtonPressed() { From 8b2945fd5deacac6cc34fa8f29ebbdc729ad8f17 Mon Sep 17 00:00:00 2001 From: 0nko Date: Sun, 26 Jan 2025 18:57:57 +0100 Subject: [PATCH 12/16] Use a coroutine scope that is automatically cancelled when a view is detached --- .../duckduckgo/app/browser/omnibar/Omnibar.kt | 2 - .../app/browser/omnibar/OmnibarLayout.kt | 44 +++++++++---------- 2 files changed, 21 insertions(+), 25 deletions(-) diff --git a/app/src/main/java/com/duckduckgo/app/browser/omnibar/Omnibar.kt b/app/src/main/java/com/duckduckgo/app/browser/omnibar/Omnibar.kt index abc81dfcb458..8059d453616d 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/omnibar/Omnibar.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/omnibar/Omnibar.kt @@ -68,12 +68,10 @@ class Omnibar( when (omnibarPosition) { OmnibarPosition.TOP -> { binding.rootView.removeView(binding.newOmnibarBottom) - binding.newOmnibarBottom.onRemoved() } OmnibarPosition.BOTTOM -> { binding.rootView.removeView(binding.newOmnibar) - binding.newOmnibar.onRemoved() // remove the default top abb bar behavior removeAppBarBehavior(binding.autoCompleteSuggestionsList) diff --git a/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt b/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt index 8a7a7b1516d6..e689bc1f552b 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt @@ -40,7 +40,6 @@ import androidx.lifecycle.ViewModelProvider import androidx.lifecycle.findViewTreeLifecycleOwner import androidx.lifecycle.findViewTreeViewModelStoreOwner import androidx.lifecycle.flowWithLifecycle -import androidx.lifecycle.lifecycleScope import com.airbnb.lottie.LottieAnimationView import com.duckduckgo.anvil.annotations.InjectWith import com.duckduckgo.app.browser.PulseAnimation @@ -85,7 +84,10 @@ import com.duckduckgo.di.scopes.FragmentScope import com.google.android.material.appbar.AppBarLayout import dagger.android.support.AndroidSupportInjection import javax.inject.Inject -import kotlinx.coroutines.Job +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.cancel import kotlinx.coroutines.flow.collectLatest import kotlinx.coroutines.flow.map import kotlinx.coroutines.launch @@ -142,6 +144,8 @@ class OmnibarLayout @JvmOverloads constructor( requireNotNull(findViewTreeLifecycleOwner()) } + private lateinit var coroutineScope: CoroutineScope + private val pulseAnimation: PulseAnimation by lazy { PulseAnimation(lifecycleOwner) } @@ -152,10 +156,6 @@ class OmnibarLayout @JvmOverloads constructor( private var decoration: Decoration? = null private var stateBuffer: MutableList = mutableListOf() - private var isViewModelSubscribed = false - private var viewStateJob: Job? = null - private var commandJob: Job? = null - internal val findInPage by lazy { IncludeFindInPageBinding.bind(findViewById(R.id.findInPage)) } internal val omnibarTextInput: KeyboardAwareEditText by lazy { findViewById(R.id.omnibarTextInput) } internal val tabsMenu: TabSwitcherButton by lazy { findViewById(R.id.tabsMenu) } @@ -246,29 +246,22 @@ class OmnibarLayout @JvmOverloads constructor( )[OmnibarLayoutViewModel::class.java] } - // One OmnibarLayout is always removed from the view hierarchy, we should cancel any flow subscriptions - fun onRemoved() { - viewStateJob?.cancel() - commandJob?.cancel() - } - override fun onAttachedToWindow() { super.onAttachedToWindow() - if (!isViewModelSubscribed) { - viewStateJob = lifecycleOwner.lifecycleScope.launch { - viewModel.viewState.flowWithLifecycle(lifecycleOwner.lifecycle).collectLatest { - render(it) - } - } + @SuppressLint("NoHardcodedCoroutineDispatcher") + coroutineScope = CoroutineScope(SupervisorJob() + Dispatchers.Main) - commandJob = lifecycleOwner.lifecycleScope.launch { - viewModel.commands().flowWithLifecycle(lifecycleOwner.lifecycle).collectLatest { - processCommand(it) - } + coroutineScope.launch { + viewModel.viewState.flowWithLifecycle(lifecycleOwner.lifecycle).collectLatest { + render(it) } + } - isViewModelSubscribed = true + coroutineScope.launch { + viewModel.commands().flowWithLifecycle(lifecycleOwner.lifecycle).collectLatest { + processCommand(it) + } } if (decoration != null) { @@ -284,6 +277,11 @@ class OmnibarLayout @JvmOverloads constructor( } } + override fun onDetachedFromWindow() { + super.onDetachedFromWindow() + coroutineScope.cancel() + } + @SuppressLint("ClickableViewAccessibility") fun setOmnibarTextListener(textListener: Omnibar.TextListener) { omnibarTextListener = textListener From 2b344c77adde4f52ef5cd89176997e0ab1c6051e Mon Sep 17 00:00:00 2001 From: 0nko Date: Sun, 26 Jan 2025 18:59:31 +0100 Subject: [PATCH 13/16] Fix an issue when tabs flow emissions may get lost while tab is not active --- .../browser/omnibar/OmnibarLayoutViewModel.kt | 26 +++++++------------ 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayoutViewModel.kt b/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayoutViewModel.kt index 6054d106548c..0b5865ea7b3a 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayoutViewModel.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayoutViewModel.kt @@ -58,11 +58,11 @@ import kotlinx.coroutines.channels.BufferOverflow.DROP_OLDEST import kotlinx.coroutines.channels.Channel import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow -import kotlinx.coroutines.flow.asStateFlow +import kotlinx.coroutines.flow.SharingStarted +import kotlinx.coroutines.flow.combine import kotlinx.coroutines.flow.flowOn -import kotlinx.coroutines.flow.launchIn -import kotlinx.coroutines.flow.onEach import kotlinx.coroutines.flow.receiveAsFlow +import kotlinx.coroutines.flow.stateIn import kotlinx.coroutines.flow.update import kotlinx.coroutines.launch import timber.log.Timber @@ -80,7 +80,13 @@ class OmnibarLayoutViewModel @Inject constructor( ) : ViewModel() { private val _viewState = MutableStateFlow(ViewState()) - val viewState = _viewState.asStateFlow() + val viewState = combine(_viewState, tabRepository.flowTabs) { state, tabs -> + state.copy( + shouldUpdateTabsCount = tabs.size != state.tabCount && tabs.isNotEmpty(), + tabCount = tabs.size, + hasUnreadTabs = tabs.firstOrNull { !it.viewed } != null, + ) + }.flowOn(dispatcherProvider.io()).stateIn(viewModelScope, SharingStarted.WhileSubscribed(5000L), ViewState()) private val command = Channel(1, DROP_OLDEST) fun commands(): Flow = command.receiveAsFlow() @@ -127,18 +133,6 @@ class OmnibarLayoutViewModel @Inject constructor( } init { - tabRepository.flowTabs - .onEach { tabs -> - _viewState.update { - it.copy( - shouldUpdateTabsCount = tabs.size != it.tabCount && tabs.isNotEmpty(), - tabCount = tabs.size, - hasUnreadTabs = tabs.firstOrNull { !it.viewed } != null, - ) - } - }.flowOn(dispatcherProvider.io()) - .launchIn(viewModelScope) - logVoiceSearchAvailability() } From 4d6ee30e5422a67cbe7e6d7ce78937472b094fc2 Mon Sep 17 00:00:00 2001 From: 0nko Date: Sun, 26 Jan 2025 19:00:45 +0100 Subject: [PATCH 14/16] Fix an issue when the long tap on tabs would not work sometimes --- .../com/duckduckgo/app/browser/BrowserTabViewModel.kt | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt index daf44478f129..4fe720909466 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt @@ -357,6 +357,7 @@ import kotlinx.coroutines.flow.combine import kotlinx.coroutines.flow.debounce import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.drop +import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.flatMapLatest import kotlinx.coroutines.flow.flowOn import kotlinx.coroutines.flow.launchIn @@ -2547,13 +2548,13 @@ class BrowserTabViewModel @Inject constructor( command.value = GenerateWebViewPreviewImage if (swipingTabsFeature.isEnabled) { - val emptyTab = tabs.value?.firstOrNull { it.url.isNullOrBlank() }?.tabId - if (emptyTab != null) { - viewModelScope.launch { + viewModelScope.launch { + val emptyTab = tabRepository.flowTabs.first().firstOrNull { it.url.isNullOrBlank() }?.tabId + if (emptyTab != null) { tabRepository.select(tabId = emptyTab) + } else { + command.value = LaunchNewTab } - } else { - command.value = LaunchNewTab } } else { command.value = LaunchNewTab From dc215fb4effd0863c15f16c807a7056be31d64a2 Mon Sep 17 00:00:00 2001 From: 0nko Date: Mon, 27 Jan 2025 10:24:25 +0100 Subject: [PATCH 15/16] Fix a unit test --- .../java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt b/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt index b3f2136eb5d4..6dd00d19962b 100644 --- a/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt +++ b/app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt @@ -2319,7 +2319,7 @@ class BrowserTabViewModelTest { @Test fun whenUserRequestedToOpenNewTabAndEmptyTabExistsThenSelectTheEmptyTab() = runTest { val emptyTabId = "EMPTY_TAB" - tabsLiveData.value = listOf(TabEntity(emptyTabId)) + whenever(mockTabRepository.flowTabs).thenReturn(flowOf(listOf(TabEntity(emptyTabId)))) testee.userRequestedOpeningNewTab() verify(mockCommandObserver, atLeastOnce()).onChanged(commandCaptor.capture()) From 8f5f80eb6ac25e39c6ab4c3be51dba2489f9264b Mon Sep 17 00:00:00 2001 From: 0nko Date: Mon, 27 Jan 2025 22:09:26 +0100 Subject: [PATCH 16/16] Optimize imports --- .../java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt b/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt index 2ef57491bab9..ddf72292e2a5 100644 --- a/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt +++ b/app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt @@ -87,10 +87,8 @@ import javax.inject.Inject import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.SupervisorJob -import kotlinx.coroutines.flow.collectLatest -import kotlinx.coroutines.coroutineScope import kotlinx.coroutines.cancel -import kotlinx.coroutines.flow.launchIn +import kotlinx.coroutines.flow.collectLatest import kotlinx.coroutines.flow.map import kotlinx.coroutines.launch import timber.log.Timber