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.

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

[PM-13626] Remember last opened view for 5 minutes #4574

Open
wants to merge 32 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
8007888
Add AppResumeManager, SettingsDiskSource methods and unlock check forโ€ฆ
aj-rosado Jan 3, 2025
876df9a
Add generator screen to AppResume
aj-rosado Jan 3, 2025
0d1565a
Added SendShortcut and Send to AppResumeManager
aj-rosado Jan 8, 2025
885e4b5
Add App Resume logic to the Search
aj-rosado Jan 10, 2025
7937c22
Verification Code app resume navigation
aj-rosado Jan 13, 2025
cf91608
Merge branch 'main' into PM-13626/remember-last-opened-view
aj-rosado Jan 13, 2025
fa0cdd9
saving last lock timestamp and improved AppResume logic
aj-rosado Jan 14, 2025
c934288
improved app resume clarity
aj-rosado Jan 14, 2025
b374725
Refactor SettingsDiskSource appResume methods to receive/return an Apโ€ฆ
aj-rosado Jan 15, 2025
286ecd5
Add tests for AppResume functionality
aj-rosado Jan 15, 2025
54826d6
Merge branch 'main' into PM-13626/remember-last-opened-view
aj-rosado Jan 16, 2025
f98e93c
Fixed tests
aj-rosado Jan 16, 2025
bbbd31f
Fixed SearchScreenTest
aj-rosado Jan 16, 2025
2bd72a4
Added tests for RootNavVM VaultUnlockNavBarVM and VaultVM
aj-rosado Jan 16, 2025
7438b48
Improved readibility, null validations, clock usage,
aj-rosado Jan 20, 2025
565659f
Added null validation FakeSettingsDiskSource and test to null scenario
aj-rosado Jan 20, 2025
126fabf
addressed pr comments
aj-rosado Jan 21, 2025
422bb7f
Use composition local and producer and consumer effects to make code โ€ฆ
dseverns-livefront Jan 20, 2025
5c27d51
Merge branch 'main' into PM-13626/remember-last-opened-view
aj-rosado Jan 22, 2025
edf4f91
Addressed PR comments to improve readibility
aj-rosado Jan 22, 2025
fa94b47
Merge branch 'change-to-use-common-code' into PM-13626/remember-last-โ€ฆ
aj-rosado Jan 22, 2025
309a249
Added AppResumeStateManager logic to Send, Generator and Verificationโ€ฆ
aj-rosado Jan 23, 2025
459ea5e
Added documentation to AppResumeStateManager methods
aj-rosado Jan 23, 2025
b3ff6a0
Removed unnecessary setResumeScreen on GeneratorViewModel
aj-rosado Jan 23, 2025
6b559c8
added AppResumeStateManager tests
aj-rosado Jan 23, 2025
b9c6574
MainActivity onCreate suppress LongMethod
aj-rosado Jan 23, 2025
55bc827
AppResumeManager ensuring clearing data when should 5 minutes passed โ€ฆ
aj-rosado Jan 23, 2025
5b7bcd0
Removed AppResumeManager from old implementation VM tests
aj-rosado Jan 24, 2025
3548304
Merge branch 'main' into PM-13626/remember-last-opened-view
aj-rosado Jan 24, 2025
c9a7ec6
Addressed PR comments
aj-rosado Jan 24, 2025
ca5dbaf
Merge branch 'main' into PM-13626/remember-last-opened-view
aj-rosado Jan 24, 2025
2f50cea
Added tests to mainviewmodel
aj-rosado Jan 24, 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
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import com.x8bit.bitwarden.data.auth.datasource.disk.model.PendingAuthRequestJso
import com.x8bit.bitwarden.data.auth.datasource.disk.model.UserStateJson
import com.x8bit.bitwarden.data.vault.datasource.network.model.SyncResponseJson
import kotlinx.coroutines.flow.Flow
import java.time.Instant

/**
* Primary access point for disk information.
Expand Down Expand Up @@ -352,4 +353,14 @@ interface AuthDiskSource {
* Stores the new device notice state for the given [userId].
*/
fun storeNewDeviceNoticeState(userId: String, newState: NewDeviceNoticeState?)

/**
* Gets the last lock timestamp for the given [userId].
*/
fun getLastLockTimestamp(userId: String): Instant?

/**
* Stores the last lock timestamp for the given [userId].
*/
fun storeLastLockTimestamp(userId: String, lastLockTimestamp: Instant?)
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.MutableSharedFlow
import kotlinx.coroutines.flow.onSubscription
import kotlinx.serialization.json.Json
import java.time.Instant
import java.util.UUID

// These keys should be encrypted
Expand Down Expand Up @@ -49,6 +50,7 @@ private const val USES_KEY_CONNECTOR = "usesKeyConnector"
private const val ONBOARDING_STATUS_KEY = "onboardingStatus"
private const val SHOW_IMPORT_LOGINS_KEY = "showImportLogins"
private const val NEW_DEVICE_NOTICE_STATE = "newDeviceNoticeState"
private const val LAST_LOCK_TIMESTAMP = "lastLockTimestamp"

/**
* Primary implementation of [AuthDiskSource].
Expand Down Expand Up @@ -154,6 +156,7 @@ class AuthDiskSourceImpl(
storeIsTdeLoginComplete(userId = userId, isTdeLoginComplete = null)
storeAuthenticatorSyncUnlockKey(userId = userId, authenticatorSyncUnlockKey = null)
storeShowImportLogins(userId = userId, showImportLogins = null)
storeLastLockTimestamp(userId = userId, lastLockTimestamp = null)

// Do not remove the DeviceKey or PendingAuthRequest on logout, these are persisted
// indefinitely unless the TDE flow explicitly removes them.
Expand Down Expand Up @@ -502,6 +505,19 @@ class AuthDiskSourceImpl(
)
}

override fun getLastLockTimestamp(userId: String): Instant? {
return getLong(key = LAST_LOCK_TIMESTAMP.appendIdentifier(userId))?.let {
Instant.ofEpochMilli(it)
}
}

override fun storeLastLockTimestamp(userId: String, lastLockTimestamp: Instant?) {
david-livefront marked this conversation as resolved.
Show resolved Hide resolved
putLong(
key = LAST_LOCK_TIMESTAMP.appendIdentifier(userId),
value = lastLockTimestamp?.toEpochMilli(),
)
}

private fun generateAndStoreUniqueAppId(): String =
UUID
.randomUUID()
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package com.x8bit.bitwarden.data.platform.datasource.disk

import com.x8bit.bitwarden.data.platform.manager.model.AppResumeScreenData
import com.x8bit.bitwarden.data.platform.repository.model.UriMatchType
import com.x8bit.bitwarden.data.platform.repository.model.VaultTimeoutAction
import com.x8bit.bitwarden.ui.platform.feature.settings.appearance.model.AppLanguage
Expand Down Expand Up @@ -356,4 +357,14 @@ interface SettingsDiskSource {
* Stores the given [count] completed create send actions for the device.
*/
fun storeCreateSendActionCount(count: Int?)

/**
* Stores the given [screenData] as the screen to resume to identified by [userId].
*/
fun storeAppResumeScreen(userId: String, screenData: AppResumeScreenData?)

/**
* Gets the screen data to resume to for the device identified by [userId] or null if no screen
*/
fun getAppResumeScreen(userId: String): AppResumeScreenData?
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.x8bit.bitwarden.data.platform.datasource.disk

import android.content.SharedPreferences
import com.x8bit.bitwarden.data.platform.manager.model.AppResumeScreenData
import com.x8bit.bitwarden.data.platform.repository.model.UriMatchType
import com.x8bit.bitwarden.data.platform.repository.model.VaultTimeoutAction
import com.x8bit.bitwarden.data.platform.repository.util.bufferedMutableSharedFlow
Expand All @@ -10,7 +11,6 @@ import com.x8bit.bitwarden.ui.platform.feature.settings.appearance.model.AppThem
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.MutableSharedFlow
import kotlinx.coroutines.flow.onSubscription
import kotlinx.serialization.encodeToString
import kotlinx.serialization.json.Json
import java.time.Instant

Expand Down Expand Up @@ -40,6 +40,7 @@ private const val IS_VAULT_REGISTERED_FOR_EXPORT = "isVaultRegisteredForExport"
private const val ADD_ACTION_COUNT = "addActionCount"
private const val COPY_ACTION_COUNT = "copyActionCount"
private const val CREATE_ACTION_COUNT = "createActionCount"
private const val RESUME_SCREEN = "resumeScreen"

/**
* Primary implementation of [SettingsDiskSource].
Expand Down Expand Up @@ -175,6 +176,7 @@ class SettingsDiskSourceImpl(
storeClearClipboardFrequencySeconds(userId = userId, frequency = null)
removeWithPrefix(prefix = ACCOUNT_BIOMETRIC_INTEGRITY_VALID_KEY.appendIdentifier(userId))
storeVaultRegisteredForExport(userId = userId, isRegistered = null)
storeAppResumeScreen(userId = userId, screenData = null)

// The following are intentionally not cleared so they can be
// restored after logging out and back in:
Expand Down Expand Up @@ -482,6 +484,16 @@ class SettingsDiskSourceImpl(
)
}

override fun storeAppResumeScreen(userId: String, screenData: AppResumeScreenData?) {
putString(
key = RESUME_SCREEN.appendIdentifier(userId),
value = screenData?.let { json.encodeToString(it) },
)
}

override fun getAppResumeScreen(userId: String): AppResumeScreenData? =
getString(RESUME_SCREEN.appendIdentifier(userId))?.let { json.decodeFromStringOrNull(it) }

private fun getMutableLastSyncFlow(
userId: String,
): MutableSharedFlow<Instant?> =
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
package com.x8bit.bitwarden.data.platform.manager

import com.x8bit.bitwarden.data.platform.manager.model.AppResumeScreenData
import com.x8bit.bitwarden.data.platform.manager.model.SpecialCircumstance

/**
* Manages the screen from which the app should be resumed after unlock.
*/
interface AppResumeManager {

/**
* Sets the screen from which the app should be resumed after unlock.
*
* @param screenData The screen identifier (e.g., "HomeScreen", "SettingsScreen").
*/
fun setResumeScreen(screenData: AppResumeScreenData)

/**
* Gets the screen from which the app should be resumed after unlock.
*
* @return The screen identifier, or an empty string if not set.
*/
fun getResumeScreen(): AppResumeScreenData?

/**
* Gets the special circumstance associated with the resume screen for the current user.
*
* @return The special circumstance, or null if no special circumstance
* is associated with the resume screen.
*/
fun getResumeSpecialCircumstance(): SpecialCircumstance?

/**
* Clears the saved resume screen for the current user.
*/
fun clearResumeScreen()
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
package com.x8bit.bitwarden.data.platform.manager

import com.x8bit.bitwarden.data.auth.datasource.disk.AuthDiskSource
import com.x8bit.bitwarden.data.auth.repository.AuthRepository
import com.x8bit.bitwarden.data.platform.datasource.disk.SettingsDiskSource
import com.x8bit.bitwarden.data.platform.manager.model.AppResumeScreenData
import com.x8bit.bitwarden.data.platform.manager.model.SpecialCircumstance
import com.x8bit.bitwarden.data.vault.manager.VaultLockManager
import java.time.Clock

// 5 minutes
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment should not be needed. Or you could you make it a javadoc.

private const val UNLOCK_NAVIGATION_TIME_MS: Long = 5 * 60
Copy link
Collaborator

@dseverns-livefront dseverns-livefront Jan 21, 2025

Choose a reason for hiding this comment

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

โ›๏ธ I appreciate the comment here, but the _MS immediately makes me think milliseconds, UNLOCK_NAVIGATION_TIME_MINUTES can never go wrong being UBER explicit.

Copy link
Collaborator

@david-livefront david-livefront Jan 21, 2025

Choose a reason for hiding this comment

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

I think that is my bad, I told him to add _MS because I thought it was milliseconds.

It should be UNLOCK_NAVIGATION_TIME_SECONDS since the value is in seconds


/**
* Primary implementation of [AppResumeManager].
*/
class AppResumeManagerImpl(
private val settingsDiskSource: SettingsDiskSource,
private val authDiskSource: AuthDiskSource,
private val authRepository: AuthRepository,
private val vaultLockManager: VaultLockManager,
private val clock: Clock,
) : AppResumeManager {

override fun setResumeScreen(screenData: AppResumeScreenData) {
authRepository.activeUserId?.let {
settingsDiskSource.storeAppResumeScreen(
userId = it,
screenData = screenData,
)
}
}

override fun getResumeScreen(): AppResumeScreenData? {
return authRepository.activeUserId?.let { userId ->
settingsDiskSource.getAppResumeScreen(userId)
}
}

override fun getResumeSpecialCircumstance(): SpecialCircumstance? {
val userId = authRepository.activeUserId ?: return null
val timeNowMinus5Min = clock.instant().minusSeconds(UNLOCK_NAVIGATION_TIME_MS)
val lastLockTimestamp = authDiskSource.getLastLockTimestamp(
userId = userId,
) ?: return null
Copy link
Collaborator

Choose a reason for hiding this comment

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

formatting:

       val lastLockTimestamp = authDiskSource
           .getLastLockTimestamp(userId = userId)
           ?: return null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

val lastLockTimestamp = authDiskSource
            .getLastLockTimestamp(
                userId = userId
            ) ?: return null

If the ")" is on the bottom line, formatter will format it like above

Copy link
Collaborator

Choose a reason for hiding this comment

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

The ?: should be on its own line for proper chaining.

This would also be fine:

val lastLockTimestamp = authDiskSource
    .getLastLockTimestamp(
        userId = userId
    )
    ?: return null


if (timeNowMinus5Min.isAfter(lastLockTimestamp)) {
clearResumeScreen()
return null
david-livefront marked this conversation as resolved.
Show resolved Hide resolved
}
return when (val resumeScreenData = getResumeScreen()) {
AppResumeScreenData.GeneratorScreen -> SpecialCircumstance.GeneratorShortcut
AppResumeScreenData.SendScreen -> SpecialCircumstance.SendShortcut
is AppResumeScreenData.SearchScreen -> SpecialCircumstance.SearchShortcut(
searchTerm = resumeScreenData.searchTerm,
)

AppResumeScreenData.VerificationCodeScreen -> {
SpecialCircumstance.VerificationCodeShortcut
david-livefront marked this conversation as resolved.
Show resolved Hide resolved
}

else -> null
}
}

override fun clearResumeScreen() {
val userId = authRepository.activeUserId ?: return
if (vaultLockManager.isVaultUnlocked(userId = userId)) {
settingsDiskSource.storeAppResumeScreen(
userId = userId,
screenData = null,
)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import com.x8bit.bitwarden.data.platform.datasource.disk.legacy.LegacyAppCenterM
import com.x8bit.bitwarden.data.platform.datasource.network.authenticator.RefreshAuthenticator
import com.x8bit.bitwarden.data.platform.datasource.network.service.EventService
import com.x8bit.bitwarden.data.platform.datasource.network.service.PushService
import com.x8bit.bitwarden.data.platform.manager.AppResumeManager
import com.x8bit.bitwarden.data.platform.manager.AppResumeManagerImpl
import com.x8bit.bitwarden.data.platform.manager.AppStateManager
import com.x8bit.bitwarden.data.platform.manager.AppStateManagerImpl
import com.x8bit.bitwarden.data.platform.manager.AssetManager
Expand Down Expand Up @@ -64,6 +66,7 @@ import com.x8bit.bitwarden.data.platform.repository.EnvironmentRepository
import com.x8bit.bitwarden.data.platform.repository.ServerConfigRepository
import com.x8bit.bitwarden.data.platform.repository.SettingsRepository
import com.x8bit.bitwarden.data.vault.datasource.disk.VaultDiskSource
import com.x8bit.bitwarden.data.vault.manager.VaultLockManager
import com.x8bit.bitwarden.data.vault.repository.VaultRepository
import dagger.Module
import dagger.Provides
Expand Down Expand Up @@ -329,4 +332,22 @@ object PlatformManagerModule {
autofillEnabledManager = autofillEnabledManager,
accessibilityEnabledManager = accessibilityEnabledManager,
)

@Provides
@Singleton
fun provideAppResumeManager(
settingsDiskSource: SettingsDiskSource,
authDiskSource: AuthDiskSource,
authRepository: AuthRepository,
vaultLockManager: VaultLockManager,
clock: Clock,
): AppResumeManager {
return AppResumeManagerImpl(
settingsDiskSource = settingsDiskSource,
authDiskSource = authDiskSource,
authRepository = authRepository,
vaultLockManager = vaultLockManager,
clock = clock,
)
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package com.x8bit.bitwarden.data.platform.manager.model

import kotlinx.serialization.Serializable

/**
* Data class representing the Screen Data for app resume.
*/
@Serializable
sealed class AppResumeScreenData {

/**
* Data object representing the Generator screen for app resume.
*/
@Serializable
data object GeneratorScreen : AppResumeScreenData()

/**
* Data object representing the Send screen for app resume.
*/
@Serializable
data object SendScreen : AppResumeScreenData()

/**
* Data class representing the Search screen for app resume.
*/
@Serializable
data class SearchScreen(val searchTerm: String) : AppResumeScreenData()

/**
* Data object representing the Verification Code screen for app resume.
*/
@Serializable
data object VerificationCodeScreen : AppResumeScreenData()
}
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,24 @@ sealed class SpecialCircumstance : Parcelable {
@Parcelize
data object AccountSecurityShortcut : SpecialCircumstance()

/**
* Deeplink to the Send.
*/
@Parcelize
data object SendShortcut : SpecialCircumstance()

/**
* Deeplink to the Search.
*/
@Parcelize
data class SearchShortcut(val searchTerm: String) : SpecialCircumstance()

/**
* Deeplink to the Verification Code.
*/
@Parcelize
data object VerificationCodeShortcut : SpecialCircumstance()

/**
* A subset of [SpecialCircumstance] that are only relevant in a pre-login state and should be
* cleared after a successful login.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package com.x8bit.bitwarden.data.vault.manager

import com.bitwarden.core.InitUserCryptoMethod
import com.bitwarden.crypto.Kdf
import com.bitwarden.sdk.AuthClient
import com.x8bit.bitwarden.data.vault.manager.model.VaultStateEvent
import com.x8bit.bitwarden.data.vault.repository.model.VaultUnlockData
import com.x8bit.bitwarden.data.vault.repository.model.VaultUnlockResult
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ import kotlinx.coroutines.flow.onCompletion
import kotlinx.coroutines.flow.onEach
import kotlinx.coroutines.flow.update
import kotlinx.coroutines.launch
import java.time.Clock
import kotlin.time.Duration.Companion.minutes

/**
Expand All @@ -70,6 +71,7 @@ class VaultLockManagerImpl(
private val userLogoutManager: UserLogoutManager,
private val trustedDeviceManager: TrustedDeviceManager,
dispatcherManager: DispatcherManager,
private val clock: Clock,
) : VaultLockManager {
private val unconfinedScope = CoroutineScope(dispatcherManager.unconfined)

Expand Down Expand Up @@ -268,6 +270,10 @@ class VaultLockManagerImpl(
)
if (!wasVaultLocked) {
mutableVaultStateEventSharedFlow.tryEmit(VaultStateEvent.Locked(userId = userId))
authDiskSource.storeLastLockTimestamp(
userId = userId,
lastLockTimestamp = clock.instant(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

๐Ÿ‘

)
}
}

Expand Down
Loading