Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Swiping tabs: Fix duplicate animations & edit mode detection #5481

Open
wants to merge 11 commits into
base: feature/ondrej/swiping-tabs
Choose a base branch
from
Open

Swiping tabs: Fix duplicate animations & edit mode detection #5481

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
6bd17fc
Do not repeat injection and pulse animation initialization on each at…
0nko Jan 16, 2025
9d20d24
Remove the onboarding restriction for tab swiping
0nko Jan 16, 2025
33862a3
Update the unit tests
0nko Jan 16, 2025
0f63904
Remove unrelated comment
0nko Jan 16, 2025
2910ecc
Optimize ViewModel flow subscriptions to avoid duplicates
0nko Jan 17, 2025
79fcdca
Cancel flow subscriptions if omnibar is removed
0nko Jan 17, 2025
8760190
Fix the unit tests
0nko Jan 17, 2025
cc5528b
Uncomment the code
0nko Jan 17, 2025
3114d36
Simplify and fix the edit mode detection
0nko Jan 17, 2025
6355e1b
Optimize imports
0nko Jan 17, 2025
81dac77
Add a missing feature flag check
0nko Jan 17, 2025
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
27 changes: 13 additions & 14 deletions app/src/main/java/com/duckduckgo/app/browser/BrowserActivity.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,9 @@ 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.flow.distinctUntilChanged
import kotlinx.coroutines.flow.launchIn
import kotlinx.coroutines.flow.onEach
import kotlinx.coroutines.launch
import kotlinx.coroutines.runBlocking
import timber.log.Timber
Expand Down Expand Up @@ -168,13 +170,7 @@ open class BrowserActivity : DuckDuckGoActivity() {
_currentTab = value
}

private val isInEditMode = SwitcherFlow<Boolean>()

private val isSwipingEnabled by lazy {
combine(viewModel.isOnboardingCompleted, isInEditMode) { isOnboardingCompleted, isInEditMode ->
isOnboardingCompleted && !isInEditMode
}
}
private val isOmnibarInEditMode = SwitcherFlow<Boolean>()

private val viewModel: BrowserViewModel by bindViewModel()

Expand Down Expand Up @@ -531,12 +527,11 @@ 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.distinctUntilChanged()
.onEach {
viewModel.onOmnibarEditModeChanged(it)
}
}
.launchIn(lifecycleScope)
} else {
viewModel.selectedTab.observe(this) {
if (it != null) {
Expand All @@ -557,7 +552,7 @@ open class BrowserActivity : DuckDuckGoActivity() {
tabPager.postDelayed(TAB_SWIPING_OBSERVER_DELAY) {
currentTab?.isInEditMode?.let {
lifecycleScope.launch {
isInEditMode.switch(it)
isOmnibarInEditMode.switch(it)
}
}
}
Expand Down Expand Up @@ -771,6 +766,10 @@ open class BrowserActivity : DuckDuckGoActivity() {
} else {
showWebContent()
}

if (swipingTabsFeature.isEnabled) {
tabPager.isUserInputEnabled = viewState.isTabSwipingEnabled
}
}
}

Expand Down
12 changes: 5 additions & 7 deletions app/src/main/java/com/duckduckgo/app/browser/BrowserViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -88,7 +86,6 @@ class BrowserViewModel @Inject constructor(
private val showOnAppLaunchFeature: ShowOnAppLaunchFeature,
private val showOnAppLaunchOptionHandler: ShowOnAppLaunchOptionHandler,
private val swipingTabsFeature: SwipingTabsFeatureProvider,
userStageStore: UserStageStore,
) : ViewModel(),
CoroutineScope {

Expand All @@ -97,6 +94,7 @@ class BrowserViewModel @Inject constructor(

data class ViewState(
val hideWebContent: Boolean = true,
val isTabSwipingEnabled: Boolean = false,
)

sealed class Command {
Expand Down Expand Up @@ -136,10 +134,6 @@ class BrowserViewModel @Inject constructor(
tabs.indexOf(selectedTab)
}.filterNot { it == -1 }

val isOnboardingCompleted: Flow<Boolean> = userStageStore.currentAppStage
.distinctUntilChanged()
.map { it != AppStage.DAX_ONBOARDING }

private var dataClearingObserver = Observer<ApplicationClearDataState> { state ->
when (state) {
ApplicationClearDataState.INITIALIZING -> {
Expand Down Expand Up @@ -359,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)
}
}

/**
Expand Down
2 changes: 2 additions & 0 deletions app/src/main/java/com/duckduckgo/app/browser/omnibar/Omnibar.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
93 changes: 47 additions & 46 deletions app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayout.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -84,13 +85,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.CoroutineScope
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.SupervisorJob
import kotlinx.coroutines.coroutineScope
import kotlinx.coroutines.flow.launchIn
import kotlinx.coroutines.Job
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)
Expand Down Expand Up @@ -140,14 +138,24 @@ 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

private var decoration: Decoration? = null
private var stateBuffer: MutableList<StateChange> = 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) }
Expand Down Expand Up @@ -190,6 +198,8 @@ class OmnibarLayout @JvmOverloads constructor(
R.layout.view_new_omnibar
}
inflate(context, layout, this)

AndroidSupportInjection.inject(this)
}

private fun omnibarViews(): List<View> = listOf(
Expand Down Expand Up @@ -227,8 +237,6 @@ class OmnibarLayout @JvmOverloads constructor(
}
}

private var coroutineScope: CoroutineScope? = null

private val smoothProgressAnimator by lazy { SmoothProgressAnimator(pageLoadingIndicator) }

private val viewModel: OmnibarLayoutViewModel by lazy {
Expand All @@ -238,24 +246,30 @@ class OmnibarLayout @JvmOverloads constructor(
)[OmnibarLayoutViewModel::class.java]
}

// One OmnibarLayout is always removed from the view hierarchy, we should cancel any flow subscriptions
fun onRemoved() {
Copy link
Member Author

@0nko 0nko Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Either the bottom or the top omnibar is always removed but its ViewModel flow subscriptions would keep collecting & rendering state for no reason.

viewStateJob?.cancel()
commandJob?.cancel()
}

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!!)
if (!isViewModelSubscribed) {
viewStateJob = lifecycleOwner.lifecycleScope.launch {
viewModel.viewState.flowWithLifecycle(lifecycleOwner.lifecycle).collectLatest {
render(it)
}
}

viewModel.commands()
.onEach { processCommand(it) }
.launchIn(coroutineScope!!)
commandJob = lifecycleOwner.lifecycleScope.launch {
viewModel.commands().flowWithLifecycle(lifecycleOwner.lifecycle).collectLatest {
processCommand(it)
}
}

viewModel.onAttachedToWindow()
isViewModelSubscribed = true
}

if (decoration != null) {
decorateDeferred(decoration!!)
Expand Down Expand Up @@ -416,8 +430,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
}
}

Expand Down Expand Up @@ -612,33 +626,20 @@ class OmnibarLayout @JvmOverloads constructor(
null
}

// 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(
Expand Down
11 changes: 6 additions & 5 deletions app/src/main/java/com/duckduckgo/app/browser/omnibar/OmnibarLayoutViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -99,7 +98,8 @@ class OmnibarLayoutViewModel @Inject constructor(
val updateOmnibarText: Boolean = false,
val shouldMoveCaretToEnd: Boolean = false,
val shouldMoveCaretToStart: Boolean = false,
val tabs: List<TabEntity> = emptyList(),
Copy link
Member Author

Choose a reason for hiding this comment

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

This list was only used to count the tabs. Each tab would contain a list of all these entities and with lots of tabs this would unnecessarily take up a lot of memory.

val tabCount: Int = 0,
val hasUnreadTabs: Boolean = false,
val shouldUpdateTabsCount: Boolean = false,
val showVoiceSearch: Boolean = false,
val showClearButton: Boolean = false,
Expand All @@ -126,13 +126,14 @@ 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,
shouldUpdateTabsCount = tabs.size != it.tabCount && tabs.isNotEmpty(),
Copy link
Member Author

Choose a reason for hiding this comment

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

I think the original condition was wrong — we don’t ever want to show 0 (the initial ViewState value), because the actual count will always be at least 1.

tabCount = tabs.size,
hasUnreadTabs = tabs.firstOrNull { !it.viewed } != null,
)
}
}.flowOn(dispatcherProvider.io())
Expand Down
23 changes: 18 additions & 5 deletions app/src/test/java/com/duckduckgo/app/browser/BrowserViewModelTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -337,7 +351,6 @@ class BrowserViewModelTest {
skipUrlConversionOnNewTabFeature = skipUrlConversionOnNewTabFeature,
showOnAppLaunchFeature = fakeShowOnAppLaunchFeatureToggle,
showOnAppLaunchOptionHandler = showOnAppLaunchOptionHandler,
userStageStore = userStageStore,
swipingTabsFeature = swipingTabsFeatureProvider,
)
}
Expand Down
Loading
Loading