diff --git a/app/src/main/java/com/markleaf/notes/feature/editor/EditorScreen.kt b/app/src/main/java/com/markleaf/notes/feature/editor/EditorScreen.kt index a128316..7919b03 100644 --- a/app/src/main/java/com/markleaf/notes/feature/editor/EditorScreen.kt +++ b/app/src/main/java/com/markleaf/notes/feature/editor/EditorScreen.kt @@ -110,6 +110,7 @@ import kotlinx.coroutines.flow.collectLatest import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.first import kotlinx.coroutines.withContext +import kotlinx.coroutines.withTimeoutOrNull import kotlinx.coroutines.delay import kotlinx.coroutines.launch import kotlin.math.ceil @@ -138,6 +139,9 @@ fun EditorScreen( var shouldRequestEditorFocus by remember(noteId) { mutableStateOf(noteId == null) } val editorFocusRequester = remember(noteId) { FocusRequester() } var isPreviewMode by remember(noteId) { mutableStateOf(false) } + // True when the settings read timed out and the note opened on defaults. + // The position recorder consults it — see [recordsPosition] (#204). + var openedOnFallbackSettings by remember(noteId) { mutableStateOf(false) } var isFocusMode by remember(noteId) { mutableStateOf(false) } var isFormattingExpanded by remember(noteId) { mutableStateOf(false) } var showDeleteConfirm by remember(noteId) { mutableStateOf(false) } @@ -316,7 +320,17 @@ fun EditorScreen( // snapshot, which starts on the default before DataStore emits) so // an existing note honours "open notes in preview" on its very first // frame instead of flashing edit (#200). New notes stay in edit. - val persistedSettings = settingsRepository.settings.first() + // + // Bounded, because this suspends before the note is shown at all: a + // DataStore read that never returns would hang note-open with no + // way out, and the whole point of reading it here is a detail of + // which mode the note opens in (#204). Falling back to the defaults + // opens in edit at the top, which is the safe answer — it shows the + // note and puts the caret somewhere harmless. + val readSettings = + withTimeoutOrNull(SETTINGS_READ_TIMEOUT_MS) { settingsRepository.settings.first() } + openedOnFallbackSettings = readSettings == null + val persistedSettings = readSettings ?: AppSettings() val openInPreview = persistedSettings.openNotesInPreview val loadedNote = repo.getNote(noteId) val content = loadedNote?.contentMarkdown.orEmpty() @@ -346,12 +360,7 @@ fun EditorScreen( OpenNotesAt.LAST_POSITION -> lastPosition?.let { PreviewScrollRequest(it.previewIndex, animate = false) } } - // Only land in preview when there is something to preview. A - // brand-new note is created first and then opened with a real id - // (the FAB path in MarkleafNavHost), so `noteId != null` alone would - // drop an empty note into preview with no editor/IME — honour "new - // notes still open for editing" by gating on content (#200). - isPreviewMode = openInPreview && content.isNotEmpty() + isPreviewMode = opensInPreview(openInPreview, content) shouldRequestEditorFocus = content.isEmpty() isLoaded = true // Remember this note as the launch target for the opt-in @@ -444,9 +453,9 @@ fun EditorScreen( // write at the end of it rather than one per keystroke, and written while // the screen is still composed so it survives the process being killed // rather than depending on a tidy exit. - LaunchedEffect(noteId, isLoaded, appSettings.openNotesAt) { + LaunchedEffect(noteId, isLoaded, appSettings.openNotesAt, openedOnFallbackSettings) { if (noteId == null || !isLoaded) return@LaunchedEffect - if (appSettings.openNotesAt != OpenNotesAt.LAST_POSITION) return@LaunchedEffect + if (!recordsPosition(appSettings.openNotesAt, openedOnFallbackSettings)) return@LaunchedEffect snapshotFlow { Triple( isPreviewMode, @@ -1132,6 +1141,52 @@ private const val MAX_TAG_SUGGESTIONS = 8 */ private const val POSITION_WRITE_DEBOUNCE_MS = 1_000L +/** + * How long the note-open path waits for DataStore before opening on defaults. + * + * Generous rather than tight: the read normally completes in milliseconds, and + * a device slow enough to need a second is a device where opening on the wrong + * mode is worse than waiting. This is a stuck-forever guard, not a latency + * budget (#204). + */ +private const val SETTINGS_READ_TIMEOUT_MS = 2_000L + +/** + * Whether a note opens in preview: only when the setting is on *and* the note + * has something to preview. + * + * The content check is not a nicety. A brand-new note is created first and then + * opened with a real id (the FAB path in `MarkleafNavHost`), so the id alone + * cannot tell a new note from an existing one — without this an empty note + * would land in preview with no editor and no keyboard, and "new notes still + * open for editing" would be broken (#200). + * + * Extracted so that rule has a test rather than living only in a load effect + * that needs a device to exercise (#204). + */ +internal fun opensInPreview(openNotesInPreview: Boolean, content: String): Boolean = + openNotesInPreview && content.isNotEmpty() + +/** + * Whether this screen may record where the note was left. + * + * The setting has to be on, and — the part that is easy to miss — the note must + * not have opened on fallback settings. When the settings read times out the + * note opens at the top on defaults; DataStore may then emit the real settings + * a moment later, flipping `openNotesAt` to `LAST_POSITION` and starting the + * recorder from that fallback state. Its first debounced write would store + * caret 0 and overwrite the position the user actually left, without them + * having touched anything (#204). + * + * Skipping the write is the conservative answer: we could not read what the + * user asked for, so we do not overwrite what we already knew. Reopening the + * note takes the normal path. + */ +internal fun recordsPosition( + openNotesAt: OpenNotesAt, + openedOnFallbackSettings: Boolean +): Boolean = openNotesAt == OpenNotesAt.LAST_POSITION && !openedOnFallbackSettings + /** * A preview scroll waiting for the preview to be built. [animate] separates the * two callers: restoring where a note was left should already be there when the diff --git a/app/src/test/java/com/markleaf/notes/feature/editor/OpenModeTest.kt b/app/src/test/java/com/markleaf/notes/feature/editor/OpenModeTest.kt new file mode 100644 index 0000000..3d30900 --- /dev/null +++ b/app/src/test/java/com/markleaf/notes/feature/editor/OpenModeTest.kt @@ -0,0 +1,72 @@ +package com.markleaf.notes.feature.editor + +import com.markleaf.notes.data.settings.OpenNotesAt +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * Which mode a note opens in when "open notes in preview" is on (#200). + * + * The rule lived inline in the editor's load effect, where the only thing + * exercising it was someone opening a note on a device (#204). It is one + * boolean and one emptiness check, and getting it wrong is not subtle — a new + * note that opens in preview has no editor and no keyboard, so the FAB appears + * to do nothing. + */ +class OpenModeTest { + + @Test + fun `setting off always opens in edit`() { + assertFalse(opensInPreview(openNotesInPreview = false, content = "# A note")) + assertFalse(opensInPreview(openNotesInPreview = false, content = "")) + } + + @Test + fun `an existing note opens in preview when the setting is on`() { + assertTrue(opensInPreview(openNotesInPreview = true, content = "# A note")) + } + + /** + * The case the guard exists for. The FAB creates the note first and opens + * it with a real id, so the id cannot distinguish a new note from an + * existing one — only its content can. + */ + @Test + fun `a just-created note opens in edit even with the setting on`() { + assertFalse(opensInPreview(openNotesInPreview = true, content = "")) + } + + /** + * Whitespace is content: the user typed it, and a note holding a newline + * has something the preview can render. Emptiness is the "brand new" + * signal, not blankness. + */ + @Test + fun `whitespace counts as content`() { + assertTrue(opensInPreview(openNotesInPreview = true, content = "\n")) + assertTrue(opensInPreview(openNotesInPreview = true, content = " ")) + } + + // ---- position recording ---- + + @Test + fun `position is recorded only when the setting selects it`() { + assertTrue(recordsPosition(OpenNotesAt.LAST_POSITION, openedOnFallbackSettings = false)) + assertFalse(recordsPosition(OpenNotesAt.TOP, openedOnFallbackSettings = false)) + assertFalse(recordsPosition(OpenNotesAt.BOTTOM, openedOnFallbackSettings = false)) + } + + /** + * The trap the timeout introduced. A note that opened on fallback settings + * sits at caret 0; if DataStore then emits the real settings and flips + * `openNotesAt` to LAST_POSITION, the recorder would start from that + * fallback state and its first debounced write would store 0 over the + * position the user actually left — without them touching anything. + */ + @Test + fun `a note opened on fallback settings never records its position`() { + assertFalse(recordsPosition(OpenNotesAt.LAST_POSITION, openedOnFallbackSettings = true)) + assertFalse(recordsPosition(OpenNotesAt.TOP, openedOnFallbackSettings = true)) + } +} diff --git a/app/src/testDebug/java/com/markleaf/notes/feature/editor/EditorTopAppBarLockTest.kt b/app/src/testDebug/java/com/markleaf/notes/feature/editor/EditorTopAppBarLockTest.kt index 18ecf19..3a7934b 100644 --- a/app/src/testDebug/java/com/markleaf/notes/feature/editor/EditorTopAppBarLockTest.kt +++ b/app/src/testDebug/java/com/markleaf/notes/feature/editor/EditorTopAppBarLockTest.kt @@ -26,15 +26,20 @@ class EditorTopAppBarLockTest { @get:Rule val composeRule = createComposeRule() - @Test - fun tapFlipsPreviewAndLongPressFlipsLockIndependently() { - var previewToggles = 0 - var lockToggles = 0 + private var previewToggles = 0 + private var lockToggles = 0 + + /** + * Renders the bar and returns the view-toggle node. In edit mode the toggle + * offers "Preview"; in preview mode it offers "Edit" — the label names the + * destination, not the current state. + */ + private fun toggle(isPreviewMode: Boolean = false, isViewModeLocked: Boolean = false) = run { composeRule.setContent { MarkleafTheme(dynamicColor = false) { EditorTopAppBar( title = "New Note", - isPreviewMode = false, + isPreviewMode = isPreviewMode, isFocusMode = false, showMore = true, moreExpanded = false, @@ -46,14 +51,17 @@ class EditorTopAppBarLockTest { onOpenMore = {}, onDismissMore = {}, moreMenuContent = {}, - isViewModeLocked = false, + isViewModeLocked = isViewModeLocked, onToggleLock = { lockToggles++ } ) } } + composeRule.onNodeWithContentDescription(if (isPreviewMode) "Edit" else "Preview") + } - // While in edit mode the toggle offers "Preview". - val toggle = composeRule.onNodeWithContentDescription("Preview") + @Test + fun tapFlipsPreviewAndLongPressFlipsLockIndependently() { + val toggle = toggle() toggle.performClick() assertEquals(1, previewToggles) @@ -63,4 +71,69 @@ class EditorTopAppBarLockTest { assertEquals(1, lockToggles) assertEquals(1, previewToggles) } + + /** + * The other order, which is the one at risk. The lock gesture runs in the + * Initial pass and consumes the release so the underlying IconButton does + * not also see a tap — if that consumption leaked past its own gesture, the + * *next* tap would be swallowed and the button would look dead. + */ + @Test + fun aTapAfterALongPressStillFlipsPreview() { + val toggle = toggle() + + toggle.performTouchInput { longClick() } + assertEquals(1, lockToggles) + assertEquals(0, previewToggles) + + toggle.performClick() + assertEquals(1, previewToggles) + assertEquals(1, lockToggles) + } + + /** Locking pins the mode; it does not disable the control that flips it. */ + @Test + fun tapAndLongPressStillWorkWhileLocked() { + val toggle = toggle(isViewModeLocked = true) + + toggle.performClick() + assertEquals(1, previewToggles) + + // Long-pressing a locked toggle unlocks it — same gesture, same handler. + toggle.performTouchInput { longClick() } + assertEquals(1, lockToggles) + assertEquals(1, previewToggles) + } + + /** + * Repeated long-presses each register. The lock is a toggle, so a second + * press has to reach the handler rather than being eaten as a repeat. + */ + @Test + fun repeatedLongPressesEachFire() { + val toggle = toggle() + + toggle.performTouchInput { longClick() } + toggle.performTouchInput { longClick() } + toggle.performTouchInput { longClick() } + + assertEquals(3, lockToggles) + assertEquals(0, previewToggles) + } + + /** + * The control keeps its label in preview mode, which is what a screen + * reader announces before either gesture. Losing it would leave the lock + * reachable only by sighted trial and error. + */ + @Test + fun theToggleIsReachableByItsLabelInPreviewMode() { + val toggle = toggle(isPreviewMode = true) + + toggle.performTouchInput { longClick() } + assertEquals(1, lockToggles) + + toggle.performClick() + assertEquals(1, previewToggles) + } }