fix(ui): address below-cut UI/Compose review nits (#308)
Six of the seven LOW findings collected in #308; the seventh is deliberately skipped (see below). - SettingsViewModel: run the app-lock disable-path Keystore/DataStore reseal off the main dispatcher (withContext(Dispatchers.Default)), matching AppLockViewModel's threading policy - no Keystore crypto on Main. - AppLockGateHost: clear the covered app content out of the semantics tree while locked so TalkBack can't traverse the occluded mailbox/ compose nodes behind the opaque cover; content stays composed so its state still survives a re-lock. - ReaderViewModel.toggleStar: reconcile the optimistic star on a failed persist - roll it back and surface a one-shot StarFailed event instead of leaving the star stuck in a state the store rejected. - OnboardingViewModel: persist firstAddedAccountId in SavedStateHandle so a process kill mid-onboarding still finishes onto the first account's inbox rather than the unfiltered mailbox (preserves #30). - RichTextEditor: memoize the formatting toolbar's parse + selection scans with remember(value) so the per-keystroke hot path isn't re-derived on every recomposition. - AccountSetupViewModel.onOutlookResult: treat a normal OAuth cancel (null result) as a no-op instead of surfacing an error snackbar. Skipped: MailboxViewModel per-keystroke search re-paging - the finding is documented-intentional and only a "could". The local pager narrows cached results instantly as you type while the expensive server search is already debounced (400ms); debouncing the local pager would add lag for no clear win, and correct scoping (query only, not account/folder) adds risk to a hot, well-tested path. Each behavioral change ships a JVM/Robolectric test; the toolbar memoization (a pure refactor) adds a toolbarStateOf test. PII-free AppLog breadcrumbs added on the new fallback/state-change paths. Closes #308
This commit is contained in:
@@ -13,6 +13,7 @@ import androidx.compose.ui.test.junit4.createAndroidComposeRule
|
||||
import androidx.compose.ui.test.onAllNodesWithText
|
||||
import androidx.compose.ui.test.onNodeWithText
|
||||
import androidx.compose.ui.test.performClick
|
||||
import androidx.lifecycle.SavedStateHandle
|
||||
import androidx.lifecycle.compose.collectAsStateWithLifecycle
|
||||
import androidx.navigation.NavType
|
||||
import androidx.navigation.compose.NavHost
|
||||
@@ -75,6 +76,7 @@ class BatteryOptimizationStepTest {
|
||||
BatteryOptimizationManager(context),
|
||||
ContactsPermissionManager(context),
|
||||
settingsRepository,
|
||||
SavedStateHandle(),
|
||||
)
|
||||
onboarding.onAccountAdded(FIRST_ACCOUNT_ID)
|
||||
|
||||
|
||||
@@ -110,6 +110,7 @@ class OnboardingFlowTest {
|
||||
BatteryOptimizationManager(appContext),
|
||||
ContactsPermissionManager(appContext),
|
||||
SettingsRepository(appContext),
|
||||
SavedStateHandle(),
|
||||
)
|
||||
composeTestRule.setContent {
|
||||
LibreMailTheme(darkTheme = false, dynamicColor = false) {
|
||||
|
||||
@@ -63,7 +63,10 @@ class AccountSetupViewModel @Inject constructor(
|
||||
|
||||
fun onOutlookResult(data: Intent?) {
|
||||
if (data == null) {
|
||||
_state.update { it.copy(error = "Microsoft sign-in was cancelled") }
|
||||
// A null result Intent is a normal user cancel: backing out of the Microsoft sign-in tab
|
||||
// returns RESULT_CANCELED with no data. That is expected, not a failure, so it is a no-op —
|
||||
// surfacing an error snackbar for a deliberate cancel is just noise (#308).
|
||||
AppLog.d(TAG, "Outlook sign-in cancelled by the user; no-op")
|
||||
return
|
||||
}
|
||||
viewModelScope.launch {
|
||||
|
||||
@@ -262,9 +262,10 @@ private fun FormattingToolbar(
|
||||
onFont: (String?) -> Unit,
|
||||
onInsertImage: (() -> Unit)?,
|
||||
) {
|
||||
val content = value.annotatedString.toRichContent()
|
||||
val start = value.selection.min
|
||||
val end = value.selection.max
|
||||
// Parsing the field into RichTextContent and the dozen selection scans that light up the buttons is
|
||||
// a per-keystroke hot path; memoize the whole derivation on the field value so a recomposition that
|
||||
// doesn't change it reuses the result instead of re-parsing + re-scanning (#308).
|
||||
val toolbar = remember(value) { toolbarStateOf(value) }
|
||||
Row(
|
||||
modifier = Modifier
|
||||
.fillMaxWidth()
|
||||
@@ -276,63 +277,61 @@ private fun FormattingToolbar(
|
||||
FormatButton(
|
||||
label = "B",
|
||||
description = stringResource(R.string.format_bold),
|
||||
active = RichTextEditing.isStyled(content, start, end, RichStyle.Bold),
|
||||
active = toolbar.bold,
|
||||
fontWeight = FontWeight.Bold,
|
||||
onClick = { onToggleStyle(RichStyle.Bold) },
|
||||
)
|
||||
FormatButton(
|
||||
label = "I",
|
||||
description = stringResource(R.string.format_italic),
|
||||
active = RichTextEditing.isStyled(content, start, end, RichStyle.Italic),
|
||||
active = toolbar.italic,
|
||||
fontStyle = FontStyle.Italic,
|
||||
onClick = { onToggleStyle(RichStyle.Italic) },
|
||||
)
|
||||
FormatButton(
|
||||
label = "U",
|
||||
description = stringResource(R.string.format_underline),
|
||||
active = RichTextEditing.isStyled(content, start, end, RichStyle.Underline),
|
||||
active = toolbar.underline,
|
||||
underline = true,
|
||||
onClick = { onToggleStyle(RichStyle.Underline) },
|
||||
)
|
||||
FormatButton(
|
||||
label = "S",
|
||||
description = stringResource(R.string.format_strikethrough),
|
||||
active = RichTextEditing.isStyled(content, start, end, RichStyle.Strikethrough),
|
||||
active = toolbar.strikethrough,
|
||||
strikethrough = true,
|
||||
onClick = { onToggleStyle(RichStyle.Strikethrough) },
|
||||
)
|
||||
val fontColorArgb = RichTextEditing.styleAt(content, start, end, RichStyle.FontColor::class.java)?.argb
|
||||
FormatButton(
|
||||
label = "A",
|
||||
description = stringResource(R.string.format_color),
|
||||
active = fontColorArgb != null,
|
||||
tint = fontColorArgb?.let { Color(it) },
|
||||
active = toolbar.fontColorArgb != null,
|
||||
tint = toolbar.fontColorArgb?.let { Color(it) },
|
||||
onClick = onFontColor,
|
||||
)
|
||||
val highlightArgb = RichTextEditing.styleAt(content, start, end, RichStyle.Highlight::class.java)?.argb
|
||||
FormatButton(
|
||||
label = "H",
|
||||
description = stringResource(R.string.format_highlight),
|
||||
active = false,
|
||||
swatchColor = highlightArgb?.let { Color(it) },
|
||||
swatchColor = toolbar.highlightArgb?.let { Color(it) },
|
||||
onClick = onHighlight,
|
||||
)
|
||||
FormatButton(
|
||||
label = "•",
|
||||
description = stringResource(R.string.format_bullet_list),
|
||||
active = RichTextEditing.hasBlock(content, start, end, BlockMarker.BULLET),
|
||||
active = toolbar.bullet,
|
||||
onClick = { onToggleBlock(BlockMarker.BULLET) },
|
||||
)
|
||||
FormatButton(
|
||||
label = "1.",
|
||||
description = stringResource(R.string.format_numbered_list),
|
||||
active = RichTextEditing.hasBlock(content, start, end, BlockMarker.ORDERED),
|
||||
active = toolbar.ordered,
|
||||
onClick = { onToggleBlock(BlockMarker.ORDERED) },
|
||||
)
|
||||
FormatButton(
|
||||
label = "❝",
|
||||
description = stringResource(R.string.format_quote),
|
||||
active = RichTextEditing.hasBlock(content, start, end, BlockMarker.QUOTE),
|
||||
active = toolbar.quote,
|
||||
onClick = { onToggleBlock(BlockMarker.QUOTE) },
|
||||
)
|
||||
FormatButton(
|
||||
@@ -347,12 +346,10 @@ private fun FormattingToolbar(
|
||||
// so its click lands on the button's on-screen center. Any control inserted *before* the block
|
||||
// buttons shifts them right and can push the bullet past the viewport, making that tap miss —
|
||||
// so these wider controls are appended last, leaving every pre-existing button in its tested spot.
|
||||
val fontCss = RichTextEditing.styleAt(content, start, end, RichStyle.FontFamily::class.java)?.css
|
||||
FontPicker(selectedCss = fontCss, onSelect = onFont)
|
||||
val fontSizePt = RichTextEditing.styleAt(content, start, end, RichStyle.FontSize::class.java)?.pt
|
||||
FontSizePicker(selectedPt = fontSizePt, onSelect = onFontSize)
|
||||
FontPicker(selectedCss = toolbar.fontCss, onSelect = onFont)
|
||||
FontSizePicker(selectedPt = toolbar.fontSizePt, onSelect = onFontSize)
|
||||
ParagraphAlignmentControl(
|
||||
selected = RichTextEditing.alignmentAt(content, start, end),
|
||||
selected = toolbar.alignment,
|
||||
onSelect = onAlignment,
|
||||
)
|
||||
// The image button is appended last (like the other trailing controls) so it never shifts the
|
||||
@@ -368,6 +365,53 @@ private fun FormattingToolbar(
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* The [FormattingToolbar]'s button state derived from the field's current [TextFieldValue]: the
|
||||
* active/inactive toggles plus the currently-applied color/font/size/alignment the pickers read back.
|
||||
* Bundled so the parse + selection scans that produce them can be computed once per field value and
|
||||
* memoized, keeping the per-keystroke toolbar off the re-parse-everything path (#308).
|
||||
*/
|
||||
internal data class ToolbarState(
|
||||
val bold: Boolean,
|
||||
val italic: Boolean,
|
||||
val underline: Boolean,
|
||||
val strikethrough: Boolean,
|
||||
val fontColorArgb: Int?,
|
||||
val highlightArgb: Int?,
|
||||
val bullet: Boolean,
|
||||
val ordered: Boolean,
|
||||
val quote: Boolean,
|
||||
val fontCss: String?,
|
||||
val fontSizePt: Int?,
|
||||
val alignment: RichAlign?,
|
||||
)
|
||||
|
||||
/**
|
||||
* Parses [value] into [RichTextContent] once and runs every toolbar selection scan over that single
|
||||
* parse, so [FormattingToolbar] derives all its button state in one pass instead of re-parsing the
|
||||
* whole body and re-scanning per button on each recomposition (#308). Pure over plain
|
||||
* [TextFieldValue] so it is JVM-unit-testable.
|
||||
*/
|
||||
internal fun toolbarStateOf(value: TextFieldValue): ToolbarState {
|
||||
val content = value.annotatedString.toRichContent()
|
||||
val start = value.selection.min
|
||||
val end = value.selection.max
|
||||
return ToolbarState(
|
||||
bold = RichTextEditing.isStyled(content, start, end, RichStyle.Bold),
|
||||
italic = RichTextEditing.isStyled(content, start, end, RichStyle.Italic),
|
||||
underline = RichTextEditing.isStyled(content, start, end, RichStyle.Underline),
|
||||
strikethrough = RichTextEditing.isStyled(content, start, end, RichStyle.Strikethrough),
|
||||
fontColorArgb = RichTextEditing.styleAt(content, start, end, RichStyle.FontColor::class.java)?.argb,
|
||||
highlightArgb = RichTextEditing.styleAt(content, start, end, RichStyle.Highlight::class.java)?.argb,
|
||||
bullet = RichTextEditing.hasBlock(content, start, end, BlockMarker.BULLET),
|
||||
ordered = RichTextEditing.hasBlock(content, start, end, BlockMarker.ORDERED),
|
||||
quote = RichTextEditing.hasBlock(content, start, end, BlockMarker.QUOTE),
|
||||
fontCss = RichTextEditing.styleAt(content, start, end, RichStyle.FontFamily::class.java)?.css,
|
||||
fontSizePt = RichTextEditing.styleAt(content, start, end, RichStyle.FontSize::class.java)?.pt,
|
||||
alignment = RichTextEditing.alignmentAt(content, start, end),
|
||||
)
|
||||
}
|
||||
|
||||
@Composable
|
||||
private fun FormatButton(
|
||||
label: String,
|
||||
|
||||
@@ -16,6 +16,7 @@ import androidx.compose.runtime.setValue
|
||||
import androidx.compose.ui.Modifier
|
||||
import androidx.compose.ui.input.pointer.pointerInput
|
||||
import androidx.compose.ui.res.stringResource
|
||||
import androidx.compose.ui.semantics.clearAndSetSemantics
|
||||
import androidx.core.content.ContextCompat
|
||||
import androidx.fragment.app.FragmentActivity
|
||||
import androidx.hilt.lifecycle.viewmodel.compose.hiltViewModel
|
||||
@@ -70,7 +71,21 @@ fun AppLockGateHost(viewModel: AppLockViewModel = hiltViewModel(), content: @Com
|
||||
LaunchedEffect(uiState) { if (uiState is AppLockUiState.Unlocked) hasEverUnlocked = true }
|
||||
|
||||
Box(modifier = Modifier.fillMaxSize()) {
|
||||
if (hasEverUnlocked) content()
|
||||
// While the app is covered (Checking/Locked) the opaque LockCover hides the content visually and
|
||||
// blocks its input, but the content stays in the composition (so its state survives a re-lock).
|
||||
// Clear the covered subtree out of the semantics/accessibility tree so an accessibility service
|
||||
// (e.g. TalkBack) can't traverse the occluded mailbox/compose nodes behind the cover (#308). When
|
||||
// Unlocked the modifier is dropped, restoring the content's full semantics.
|
||||
val contentCovered = uiState !is AppLockUiState.Unlocked
|
||||
Box(
|
||||
modifier = if (contentCovered) {
|
||||
Modifier.fillMaxSize().clearAndSetSemantics { }
|
||||
} else {
|
||||
Modifier.fillMaxSize()
|
||||
},
|
||||
) {
|
||||
if (hasEverUnlocked) content()
|
||||
}
|
||||
|
||||
when (val state = uiState) {
|
||||
AppLockUiState.Unlocked -> Unit
|
||||
|
||||
@@ -2,6 +2,7 @@
|
||||
package org.libremail.ui.onboarding
|
||||
|
||||
import android.content.Intent
|
||||
import androidx.lifecycle.SavedStateHandle
|
||||
import androidx.lifecycle.ViewModel
|
||||
import androidx.lifecycle.viewModelScope
|
||||
import dagger.hilt.android.lifecycle.HiltViewModel
|
||||
@@ -13,6 +14,8 @@ import org.libremail.contacts.ContactsPermissionManager
|
||||
import org.libremail.data.settings.SettingsRepository
|
||||
import org.libremail.push.BatteryOptimizationManager
|
||||
import org.libremail.push.BatteryPromptDecision
|
||||
import org.libremail.reporting.AppLog
|
||||
import org.libremail.reporting.accountLogRef
|
||||
import javax.inject.Inject
|
||||
|
||||
/**
|
||||
@@ -28,11 +31,18 @@ class OnboardingViewModel @Inject constructor(
|
||||
private val batteryOptimizationManager: BatteryOptimizationManager,
|
||||
private val contactsPermissionManager: ContactsPermissionManager,
|
||||
private val settingsRepository: SettingsRepository,
|
||||
private val savedStateHandle: SavedStateHandle,
|
||||
) : ViewModel() {
|
||||
|
||||
/** The id of the first account added this session, or null if none has been added yet. */
|
||||
var firstAddedAccountId: String? = null
|
||||
private set
|
||||
/**
|
||||
* The id of the first account added this session, or null if none has been added yet. Held in
|
||||
* [SavedStateHandle] rather than a plain field so it survives process death: this ViewModel is
|
||||
* scoped to the onboarding nav-graph back-stack entry, whose saved state is restored after a
|
||||
* process kill. Without that, a kill mid-onboarding — e.g. the excursion to system battery
|
||||
* settings on the opt-in step — would drop the id and finish onto the unfiltered mailbox instead
|
||||
* of the first account's inbox, defeating #30.
|
||||
*/
|
||||
val firstAddedAccountId: String? get() = savedStateHandle[KEY_FIRST_ACCOUNT_ID]
|
||||
|
||||
private val _batteryPromptNeeded = MutableStateFlow<Boolean?>(null)
|
||||
|
||||
@@ -82,7 +92,9 @@ class OnboardingViewModel @Inject constructor(
|
||||
/** Records a freshly added account. Only the first one sticks — later adds don't overwrite it. */
|
||||
fun onAccountAdded(accountId: String) {
|
||||
if (firstAddedAccountId == null) {
|
||||
firstAddedAccountId = accountId
|
||||
savedStateHandle[KEY_FIRST_ACCOUNT_ID] = accountId
|
||||
// PII-free: the id embeds the email, so reference it only via its non-reversible log ref.
|
||||
AppLog.i(TAG, "onboarding recorded first account ${accountLogRef(accountId)}")
|
||||
}
|
||||
}
|
||||
|
||||
@@ -128,4 +140,11 @@ class OnboardingViewModel @Inject constructor(
|
||||
fun markContactsPromptHandled() {
|
||||
viewModelScope.launch { settingsRepository.setContactsPromptHandled(true) }
|
||||
}
|
||||
|
||||
private companion object {
|
||||
const val TAG = "OnboardingVM"
|
||||
|
||||
/** SavedStateHandle key for [firstAddedAccountId], persisted across process death (#308). */
|
||||
const val KEY_FIRST_ACCOUNT_ID = "onboarding_first_added_account_id"
|
||||
}
|
||||
}
|
||||
|
||||
@@ -85,6 +85,7 @@ fun ReaderScreen(
|
||||
val noAppMessage = stringResource(R.string.attachment_no_app)
|
||||
val downloadFailedTemplate = stringResource(R.string.attachment_download_failed)
|
||||
val replyFailedMessage = stringResource(R.string.reader_reply_failed)
|
||||
val starFailedMessage = stringResource(R.string.reader_star_failed)
|
||||
|
||||
LaunchedEffect(state.deleted) {
|
||||
if (state.deleted) onBack()
|
||||
@@ -105,6 +106,8 @@ fun ReaderScreen(
|
||||
|
||||
is ReaderEvent.ComposeFailed ->
|
||||
snackbarHostState.showSnackbar(event.message ?: replyFailedMessage)
|
||||
|
||||
ReaderEvent.StarFailed -> snackbarHostState.showSnackbar(starFailedMessage)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -50,6 +50,9 @@ sealed interface ReaderEvent {
|
||||
|
||||
/** Building the reply/forward draft failed; the screen surfaces [message] (or a generic fallback). */
|
||||
data class ComposeFailed(val message: String?) : ReaderEvent
|
||||
|
||||
/** Persisting a star toggle failed; the optimistic flip was rolled back and the screen notifies. */
|
||||
data object StarFailed : ReaderEvent
|
||||
}
|
||||
|
||||
@HiltViewModel
|
||||
@@ -140,7 +143,19 @@ class ReaderViewModel @Inject constructor(
|
||||
val message = _state.value.message ?: return
|
||||
val starred = !message.isStarred
|
||||
_state.update { it.copy(message = message.copy(isStarred = starred)) }
|
||||
viewModelScope.launch { repository.setStarred(messageId, starred) }
|
||||
viewModelScope.launch {
|
||||
repository.setStarred(messageId, starred).onFailure { e ->
|
||||
// The optimistic flip already updated the UI; the persist failed, so reconcile by rolling
|
||||
// it back (the star would otherwise stay stuck in a state the store never accepted) and
|
||||
// notify the screen. Revert to the pre-toggle value (!starred) of the current message.
|
||||
AppLog.w(READER_TAG, "toggleStar persist failed; rolling back optimistic star", e)
|
||||
_state.update { current ->
|
||||
val shown = current.message ?: return@update current
|
||||
current.copy(message = shown.copy(isStarred = !starred))
|
||||
}
|
||||
_events.send(ReaderEvent.StarFailed)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fun loadRemoteImages() = _state.update { it.copy(loadRemoteImages = true) }
|
||||
|
||||
@@ -2,9 +2,12 @@
|
||||
package org.libremail.ui.settings
|
||||
|
||||
import android.content.Intent
|
||||
import androidx.annotation.VisibleForTesting
|
||||
import androidx.lifecycle.ViewModel
|
||||
import androidx.lifecycle.viewModelScope
|
||||
import dagger.hilt.android.lifecycle.HiltViewModel
|
||||
import kotlinx.coroutines.CoroutineDispatcher
|
||||
import kotlinx.coroutines.Dispatchers
|
||||
import kotlinx.coroutines.flow.MutableStateFlow
|
||||
import kotlinx.coroutines.flow.SharingStarted
|
||||
import kotlinx.coroutines.flow.StateFlow
|
||||
@@ -12,6 +15,7 @@ import kotlinx.coroutines.flow.asStateFlow
|
||||
import kotlinx.coroutines.flow.stateIn
|
||||
import kotlinx.coroutines.flow.update
|
||||
import kotlinx.coroutines.launch
|
||||
import kotlinx.coroutines.withContext
|
||||
import org.libremail.R
|
||||
import org.libremail.contacts.ContactsPermissionManager
|
||||
import org.libremail.data.security.AppLockManager
|
||||
@@ -62,6 +66,12 @@ class SettingsViewModel @Inject constructor(
|
||||
/** Whether this app is exempt from battery optimization ("Unrestricted"). */
|
||||
val batteryUnrestricted: StateFlow<Boolean> = _batteryUnrestricted.asStateFlow()
|
||||
|
||||
// Blocking Keystore/DataStore work on the app-lock disable path runs here — off the main
|
||||
// dispatcher — mirroring AppLockViewModel's threading policy so the reseal never runs Keystore
|
||||
// crypto on the main thread (#308). Injectable so unit tests can pin it to their scheduler.
|
||||
@VisibleForTesting
|
||||
internal var defaultDispatcher: CoroutineDispatcher = Dispatchers.Default
|
||||
|
||||
fun toggleAdvanced() = _advancedExpanded.update { !it }
|
||||
|
||||
/** Persist the account order the user produced by dragging in Settings (issue #164). */
|
||||
@@ -110,13 +120,19 @@ class SettingsViewModel @Inject constructor(
|
||||
// Reseal under the master key whenever an auth seal actually exists — gate on the seal, not
|
||||
// the encryptCache setting (a separate store that can already be off while the on-disk DB is
|
||||
// still auth-sealed). Do it BEFORE dropping the gate, or the next launch can't open the
|
||||
// cache. If it fails, keep app-lock on rather than strand the passphrase.
|
||||
if (databaseKeyStore.hasAuthSealedPassphrase()) {
|
||||
if (runCatching { databaseKeyStore.sealWithMaster() }.isFailure) {
|
||||
_appLockMessage.value = R.string.app_lock_disable_failed
|
||||
return@update
|
||||
// cache. If it fails, keep app-lock on rather than strand the passphrase. The Keystore/
|
||||
// DataStore reseal is blocking crypto, so it runs off the main thread (#308).
|
||||
val resealed = withContext(defaultDispatcher) {
|
||||
if (databaseKeyStore.hasAuthSealedPassphrase()) {
|
||||
runCatching { databaseKeyStore.sealWithMaster() }.isSuccess
|
||||
} else {
|
||||
true
|
||||
}
|
||||
}
|
||||
if (!resealed) {
|
||||
_appLockMessage.value = R.string.app_lock_disable_failed
|
||||
return@update
|
||||
}
|
||||
settingsRepository.setAppLock(false)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -151,6 +151,7 @@
|
||||
<!-- Reader -->
|
||||
<string name="reader_reply">Reply</string>
|
||||
<string name="reader_reply_failed">Couldn\'t start the reply</string>
|
||||
<string name="reader_star_failed">Couldn\'t update star</string>
|
||||
<string name="attachments_title">Attachments</string>
|
||||
<string name="attachment_downloaded">Downloaded</string>
|
||||
<string name="message_available_offline">Available offline</string>
|
||||
|
||||
@@ -124,13 +124,18 @@ class AccountSetupViewModelTest {
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a cancelled sign-in (null result) surfaces a cancellation message`() {
|
||||
fun `a cancelled sign-in (null result) is a no-op, not an error`() {
|
||||
val vm = viewModel()
|
||||
|
||||
vm.onOutlookResult(null)
|
||||
|
||||
assertEquals("Microsoft sign-in was cancelled", vm.state.value.error)
|
||||
// #308: a normal cancel (backing out of the sign-in tab → RESULT_CANCELED, null data) must not
|
||||
// raise an error snackbar; it leaves the screen untouched (idle, no error) and only breadcrumbs.
|
||||
assertNull(vm.state.value.error)
|
||||
assertEquals(SetupStatus.IDLE, vm.state.value.status)
|
||||
val entry = logBuffer.snapshot().single()
|
||||
assertEquals('D', entry.level)
|
||||
assertTrue(entry.message.contains("cancelled"), entry.message)
|
||||
}
|
||||
|
||||
@Test
|
||||
@@ -263,7 +268,9 @@ class AccountSetupViewModelTest {
|
||||
@Test
|
||||
fun `consumeError clears a surfaced error`() {
|
||||
val vm = viewModel()
|
||||
vm.onOutlookResult(null)
|
||||
// Surface a real error first (a cancel is now a no-op), then confirm consumeError clears it.
|
||||
vm.onOutlookLaunchFailed(IllegalStateException("appauth exploded"))
|
||||
assertEquals("appauth exploded", vm.state.value.error)
|
||||
|
||||
vm.consumeError()
|
||||
|
||||
|
||||
@@ -348,6 +348,37 @@ class RichTextEditorTest {
|
||||
assertTrue(isBulletActive(bothLines.copy(selection = fullText)))
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `toolbarStateOf derives every button's state from the field value in a single pass`() {
|
||||
// A run carrying bold + color + size, centered, on a bullet line. applyBlock inserts "• " and
|
||||
// lands the selection back on the styled text, so every scan sees the same run the user is on.
|
||||
var value = field("hello", TextRange(0, 5))
|
||||
value = applyStyle(value, RichStyle.Bold, linkColor)
|
||||
value = applyStyle(value, RichStyle.FontColor(0xFF112233.toInt()), linkColor)
|
||||
value = applyStyle(value, RichStyle.FontSize(18), linkColor)
|
||||
value = applyAlignment(value, RichAlign.CENTER, linkColor, noFont)
|
||||
value = applyBlock(value, BlockMarker.BULLET, linkColor, noFont)
|
||||
|
||||
val state = toolbarStateOf(value)
|
||||
|
||||
assertTrue(state.bold)
|
||||
assertFalse(state.italic)
|
||||
assertFalse(state.underline)
|
||||
assertFalse(state.strikethrough)
|
||||
assertEquals(0xFF112233.toInt(), state.fontColorArgb)
|
||||
assertEquals(null, state.highlightArgb)
|
||||
assertEquals(18, state.fontSizePt)
|
||||
assertEquals(null, state.fontCss)
|
||||
assertTrue(state.bullet)
|
||||
assertFalse(state.ordered)
|
||||
assertFalse(state.quote)
|
||||
assertEquals(RichAlign.CENTER, state.alignment)
|
||||
// Each field equals reading the same predicate directly off the value: the memoization the
|
||||
// toolbar wraps this in (remember(value)) is behaviour-preserving, just computed once (#308).
|
||||
assertEquals(isBoldActive(value), state.bold)
|
||||
assertEquals(isBulletActive(value), state.bullet)
|
||||
}
|
||||
|
||||
/** Mirrors exactly what FormattingToolbar reads to decide a style button's active/inactive tint. */
|
||||
private fun isBoldActive(value: TextFieldValue): Boolean = RichTextEditing.isStyled(
|
||||
value.annotatedString.toRichContent(),
|
||||
|
||||
@@ -4,6 +4,7 @@ package org.libremail.ui.lock
|
||||
import android.content.Context
|
||||
import androidx.compose.material3.Text
|
||||
import androidx.compose.runtime.CompositionLocalProvider
|
||||
import androidx.compose.runtime.DisposableEffect
|
||||
import androidx.compose.ui.test.assertIsDisplayed
|
||||
import androidx.compose.ui.test.junit4.v2.createComposeRule
|
||||
import androidx.compose.ui.test.onNodeWithText
|
||||
@@ -24,6 +25,7 @@ import org.robolectric.RobolectricTestRunner
|
||||
import org.robolectric.RuntimeEnvironment
|
||||
import org.robolectric.annotation.Config
|
||||
import org.robolectric.annotation.GraphicsMode
|
||||
import kotlin.test.assertEquals
|
||||
|
||||
/**
|
||||
* Robolectric JVM Compose test (#384, umbrella #373) for [AppLockGateHost], the gate that wraps the
|
||||
@@ -109,26 +111,50 @@ class AppLockGateHostJvmTest {
|
||||
}
|
||||
|
||||
@Test
|
||||
fun reLockAfterUnlock_drawsLockScreenButKeepsContentComposed() {
|
||||
fun reLockAfterUnlock_keepsContentComposedButGatesItOutOfTheAccessibilityTree() {
|
||||
val state = MutableStateFlow<AppLockUiState>(AppLockUiState.Unlocked)
|
||||
val vm = mockk<AppLockViewModel>(relaxed = true)
|
||||
every { vm.uiState } returns state
|
||||
// Track the content's composition lifecycle directly: entered once and never disposed proves the
|
||||
// content composable is RETAINED across a re-lock (its nav/scroll/draft state survives) rather
|
||||
// than torn down — independently of whether its semantics are currently in the tree.
|
||||
var entered = 0
|
||||
var disposed = 0
|
||||
composeTestRule.setContent {
|
||||
CompositionLocalProvider(LocalLifecycleOwner provides resumedOwner) {
|
||||
LibreMailTheme(darkTheme = false, dynamicColor = false) {
|
||||
AppLockGateHost(viewModel = vm) { Text(CONTENT) }
|
||||
AppLockGateHost(viewModel = vm) {
|
||||
DisposableEffect(Unit) {
|
||||
entered++
|
||||
onDispose { disposed++ }
|
||||
}
|
||||
Text(CONTENT)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
composeTestRule.onNodeWithText(CONTENT).assertIsDisplayed()
|
||||
assertEquals(1, entered)
|
||||
|
||||
// Re-lock: once ever unlocked, the content stays composed (its nav/scroll/draft state survives)
|
||||
// and the lock screen is drawn OVER it rather than replacing it.
|
||||
// Re-lock: the lock screen is drawn OVER the content. The content stays composed (entered==1,
|
||||
// never disposed), but #308 clears it out of the semantics/accessibility tree so TalkBack can't
|
||||
// traverse the occluded mailbox nodes behind the opaque cover — assertDoesNotExist confirms the
|
||||
// content's text is no longer reachable in the (a11y-facing) semantics tree.
|
||||
state.value = AppLockUiState.Locked()
|
||||
composeTestRule.waitForIdle()
|
||||
|
||||
composeTestRule.onNodeWithText(string(R.string.app_lock_title)).assertIsDisplayed()
|
||||
composeTestRule.onNodeWithText(CONTENT).assertExists()
|
||||
composeTestRule.onNodeWithText(CONTENT).assertDoesNotExist()
|
||||
assertEquals(1, entered)
|
||||
assertEquals(0, disposed)
|
||||
|
||||
// Unlocking again restores the content's semantics (same retained composition — still one entry).
|
||||
state.value = AppLockUiState.Unlocked
|
||||
composeTestRule.waitForIdle()
|
||||
|
||||
composeTestRule.onNodeWithText(CONTENT).assertIsDisplayed()
|
||||
assertEquals(1, entered)
|
||||
assertEquals(0, disposed)
|
||||
}
|
||||
|
||||
private companion object {
|
||||
|
||||
@@ -1,12 +1,16 @@
|
||||
// SPDX-License-Identifier: GPL-3.0-or-later
|
||||
package org.libremail.ui.onboarding
|
||||
|
||||
import android.util.Log
|
||||
import androidx.lifecycle.SavedStateHandle
|
||||
import io.mockk.Runs
|
||||
import io.mockk.coEvery
|
||||
import io.mockk.coVerify
|
||||
import io.mockk.every
|
||||
import io.mockk.just
|
||||
import io.mockk.mockk
|
||||
import io.mockk.mockkStatic
|
||||
import io.mockk.unmockkAll
|
||||
import kotlinx.coroutines.Dispatchers
|
||||
import kotlinx.coroutines.ExperimentalCoroutinesApi
|
||||
import kotlinx.coroutines.test.UnconfinedTestDispatcher
|
||||
@@ -30,10 +34,19 @@ class OnboardingViewModelTest {
|
||||
private val testDispatcher = UnconfinedTestDispatcher()
|
||||
|
||||
@Before
|
||||
fun setUp() = Dispatchers.setMain(testDispatcher)
|
||||
fun setUp() {
|
||||
Dispatchers.setMain(testDispatcher)
|
||||
// onAccountAdded now breadcrumbs the first account via AppLog (#308); android.util.Log is a
|
||||
// no-op stub that throws "not mocked" under plain JVM tests, so stub it so the call can't crash.
|
||||
mockkStatic(Log::class)
|
||||
every { Log.i(any(), any()) } returns 0
|
||||
}
|
||||
|
||||
@After
|
||||
fun tearDown() = Dispatchers.resetMain()
|
||||
fun tearDown() {
|
||||
Dispatchers.resetMain()
|
||||
unmockkAll()
|
||||
}
|
||||
|
||||
private fun batteryManager(supported: Boolean = true, unrestricted: Boolean = false) =
|
||||
mockk<BatteryOptimizationManager> {
|
||||
@@ -55,7 +68,8 @@ class OnboardingViewModelTest {
|
||||
battery: BatteryOptimizationManager = batteryManager(),
|
||||
contacts: ContactsPermissionManager = contactsManager(),
|
||||
settings: SettingsRepository = settingsRepository(),
|
||||
) = OnboardingViewModel(battery, contacts, settings)
|
||||
savedStateHandle: SavedStateHandle = SavedStateHandle(),
|
||||
) = OnboardingViewModel(battery, contacts, settings, savedStateHandle)
|
||||
|
||||
@Test
|
||||
fun `battery prompt is needed when not unrestricted and not handled`() = runTest(testDispatcher) {
|
||||
@@ -123,6 +137,21 @@ class OnboardingViewModelTest {
|
||||
assertEquals("imap:first@example.com", vm.firstAddedAccountId)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `the first added account id survives a process-death recreation via SavedStateHandle`() =
|
||||
runTest(testDispatcher) {
|
||||
// #308: onboarding can be process-killed mid-flow (e.g. the excursion to system battery
|
||||
// settings). The first-account id lives in SavedStateHandle, so a recreated ViewModel over
|
||||
// the restored state must still finish onto that account's inbox rather than the unified one.
|
||||
val handle = SavedStateHandle()
|
||||
viewModel(savedStateHandle = handle).onAccountAdded("imap:first@example.com")
|
||||
|
||||
// Simulate restoration after a kill: a brand-new handle seeded from the saved state, then a
|
||||
// fresh ViewModel over it (a plain field would have been lost — this is exactly #30's guard).
|
||||
val restored = SavedStateHandle(handle.keys().associateWith { handle.get<Any?>(it) })
|
||||
assertEquals("imap:first@example.com", viewModel(savedStateHandle = restored).firstAddedAccountId)
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `marking the battery prompt handled persists the flag`() = runTest(testDispatcher) {
|
||||
val repo = settingsRepository()
|
||||
|
||||
@@ -55,6 +55,9 @@ class ReaderViewModelActionsTest {
|
||||
every { Log.d(any(), any()) } returns 0
|
||||
every { Log.i(any(), any()) } returns 0
|
||||
every { Log.w(any<String>(), any<String>()) } returns 0
|
||||
// toggleStar's rollback path logs via AppLog.w(tag, msg, throwable) -> Log.w(String, String,
|
||||
// Throwable); stub that overload too so the failing-write test's coroutine doesn't crash on it.
|
||||
every { Log.w(any<String>(), any<String>(), any()) } returns 0
|
||||
every { Log.e(any(), any(), any()) } returns 0
|
||||
}
|
||||
|
||||
@@ -78,6 +81,7 @@ class ReaderViewModelActionsTest {
|
||||
coEvery { repo.openMessage(messageId) } returns Result.success(message)
|
||||
every { repo.observeAttachments(messageId) } returns flowOf(listOf(attachment(0)))
|
||||
coEvery { repo.downloadedAttachmentParts(messageId) } returns emptySet()
|
||||
coEvery { repo.setStarred(any(), any()) } returns Result.success(Unit)
|
||||
return repo
|
||||
}
|
||||
|
||||
@@ -181,6 +185,28 @@ class ReaderViewModelActionsTest {
|
||||
coVerify { repo.setStarred(messageId, true) }
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `toggleStar rolls the optimistic star back and emits StarFailed when the write fails`() = runTest(dispatcher) {
|
||||
val repo = loadedRepo()
|
||||
coEvery { repo.setStarred(messageId, true) } returns Result.failure(RuntimeException("db locked"))
|
||||
val vm = viewModel(repo)
|
||||
advanceUntilIdle()
|
||||
assertFalse(vm.state.value.message!!.isStarred)
|
||||
|
||||
vm.events.test {
|
||||
vm.toggleStar()
|
||||
// The optimistic flip lands immediately, before the (failing) persist runs.
|
||||
assertTrue(vm.state.value.message!!.isStarred)
|
||||
advanceUntilIdle()
|
||||
|
||||
assertEquals(ReaderEvent.StarFailed, awaitItem())
|
||||
// The failed write is reconciled: the star is rolled back to its pre-toggle value so the
|
||||
// UI never stays stuck in a state the store rejected.
|
||||
assertFalse(vm.state.value.message!!.isStarred)
|
||||
cancelAndIgnoreRemainingEvents()
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `toggleStar does nothing before the message has loaded`() = runTest(dispatcher) {
|
||||
val repo = mockk<MailRepository>(relaxed = true)
|
||||
|
||||
@@ -9,6 +9,7 @@ import io.mockk.mockk
|
||||
import kotlinx.coroutines.Dispatchers
|
||||
import kotlinx.coroutines.ExperimentalCoroutinesApi
|
||||
import kotlinx.coroutines.flow.flowOf
|
||||
import kotlinx.coroutines.test.StandardTestDispatcher
|
||||
import kotlinx.coroutines.test.UnconfinedTestDispatcher
|
||||
import kotlinx.coroutines.test.advanceUntilIdle
|
||||
import kotlinx.coroutines.test.resetMain
|
||||
@@ -107,6 +108,26 @@ class SettingsViewModelTest {
|
||||
coVerify(exactly = 0) { settingsRepository.setAppLock(false) }
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `the disable-path reseal is dispatched off the main thread, not run inline on main`() = runTest(dispatcher) {
|
||||
val databaseKeyStore = mockk<DatabaseKeyStore>(relaxed = true)
|
||||
coEvery { databaseKeyStore.hasAuthSealedPassphrase() } returns true
|
||||
val settingsRepository = mockk<SettingsRepository>(relaxed = true)
|
||||
val vm = viewModel(databaseKeyStore = databaseKeyStore, settingsRepository = settingsRepository)
|
||||
// Swap the pinned dispatcher for a queue-and-drain one (sharing the scheduler) so we can see
|
||||
// that the blocking Keystore reseal is DISPATCHED off-main rather than run inline on Main's
|
||||
// unconfined pass — the point of #308: no Keystore crypto on the main dispatcher.
|
||||
vm.defaultDispatcher = StandardTestDispatcher(testScheduler)
|
||||
|
||||
vm.setAppLock(false)
|
||||
|
||||
// Dispatched to defaultDispatcher, so it has not run on the current (unconfined Main) pass...
|
||||
coVerify(exactly = 0) { databaseKeyStore.sealWithMaster() }
|
||||
advanceUntilIdle()
|
||||
// ...only once that off-main dispatcher is drained.
|
||||
coVerify(exactly = 1) { databaseKeyStore.sealWithMaster() }
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `disabling app-lock with no auth seal just drops the gate`() = runTest(dispatcher) {
|
||||
val databaseKeyStore = mockk<DatabaseKeyStore>(relaxed = true)
|
||||
@@ -139,6 +160,10 @@ class SettingsViewModelTest {
|
||||
batteryOptimizationManager = mockk(relaxed = true),
|
||||
contactsPermissionManager = mockk(relaxed = true),
|
||||
syncScheduler = mockk(relaxed = true),
|
||||
)
|
||||
).apply {
|
||||
// Pin the off-main reseal dispatcher (#308) to the test scheduler so the disable path's
|
||||
// withContext(defaultDispatcher) work is advanced deterministically by advanceUntilIdle().
|
||||
defaultDispatcher = dispatcher
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -79,6 +79,9 @@ style:
|
||||
- '**/data/repository/AccountRepositoryImplTest.kt'
|
||||
- '**/ui/reader/ReaderViewModelTest.kt'
|
||||
- '**/ui/reader/ReaderViewModelActionsTest.kt'
|
||||
# First-account onboarding breadcrumb (issue #308): onAccountAdded logs via AppLog, so this
|
||||
# suite mockkStatic(Log) so the call doesn't crash on the throwing JVM stub.
|
||||
- '**/ui/onboarding/OnboardingViewModelTest.kt'
|
||||
MagicNumber:
|
||||
# dp / sp / duration literals are idiomatic inline in Compose.
|
||||
ignoreAnnotated: ['Composable']
|
||||
|
||||
Reference in New Issue
Block a user