diff --git a/app/app/src/main/java/org/modg/bookshelf/AppContainer.kt b/app/app/src/main/java/org/modg/bookshelf/AppContainer.kt index 7040ceb..25baf8b 100644 --- a/app/app/src/main/java/org/modg/bookshelf/AppContainer.kt +++ b/app/app/src/main/java/org/modg/bookshelf/AppContainer.kt @@ -1,6 +1,7 @@ package org.modg.bookshelf import android.content.Context +import java.io.File import java.util.concurrent.TimeUnit import kotlinx.coroutines.flow.first import kotlinx.coroutines.runBlocking @@ -12,6 +13,7 @@ import okhttp3.OkHttpClient import okhttp3.logging.HttpLoggingInterceptor import org.modg.bookshelf.data.local.BookshelfDatabase import org.modg.bookshelf.data.metadata.BookSearchRepository +import org.modg.bookshelf.data.repo.CoverDownloader import org.modg.bookshelf.data.metadata.MetadataRepository import org.modg.bookshelf.diagnostics.CrashReporter import org.modg.bookshelf.data.prefs.SettingsStore @@ -121,6 +123,12 @@ class AppContainer(private val context: Context) { val metadataRepository by lazy { MetadataRepository(metadataHttpClient, json, BuildConfig.GOOGLE_BOOKS_API_KEY) } + /** + * Cover preloads for the save sheets. [metadataHttpClient], not [okHttpClient]: covers + * come from third-party image hosts, which must never see our PocketBase token. + */ + val coverDownloader by lazy { CoverDownloader(metadataHttpClient, File(context.cacheDir, "cover-preload")) } + /** Library screen's online title/author search — shares the same client/timeouts/key as [metadataRepository]. */ val bookSearchRepository by lazy { BookSearchRepository(metadataHttpClient, json, BuildConfig.GOOGLE_BOOKS_API_KEY) } diff --git a/app/app/src/main/java/org/modg/bookshelf/BookshelfApplication.kt b/app/app/src/main/java/org/modg/bookshelf/BookshelfApplication.kt index b7e220b..a4e71e9 100644 --- a/app/app/src/main/java/org/modg/bookshelf/BookshelfApplication.kt +++ b/app/app/src/main/java/org/modg/bookshelf/BookshelfApplication.kt @@ -16,5 +16,9 @@ class BookshelfApplication : Application() { // See CrashReporter's KDoc: born from an on-device crash right after // the previous commit that could never be reproduced off-device. appContainer.crashReporter.install() + // Preloaded covers from a process that died with a save sheet open. Off the main + // thread (file listing); files this process creates are newer and never swept. + val processStart = System.currentTimeMillis() + Thread { appContainer.coverDownloader.sweepStale(olderThanMillis = processStart) }.start() } } diff --git a/app/app/src/main/java/org/modg/bookshelf/data/metadata/TextNormalization.kt b/app/app/src/main/java/org/modg/bookshelf/data/metadata/TextNormalization.kt index 603d06f..fe0c8b6 100644 --- a/app/app/src/main/java/org/modg/bookshelf/data/metadata/TextNormalization.kt +++ b/app/app/src/main/java/org/modg/bookshelf/data/metadata/TextNormalization.kt @@ -12,6 +12,11 @@ import java.text.Normalizer object TextNormalization { private val leadingArticles = setOf("the", "a", "an") + // Compiled once: the library search normalizes every owned book's fields, so an inline + // Regex(...) here was being recompiled thousands of times per keystroke. + private val combiningMarks = Regex("\\p{Mn}+") + private val whitespaceRun = Regex("\\s+") + /** * Lowercase, diacritics stripped (NFD decompose + drop combining marks), every * non-alphanumeric character turned into a space, whitespace collapsed. "J.R.R." @@ -19,9 +24,9 @@ object TextNormalization { */ fun normalizeForMatching(raw: String): String { val decomposed = Normalizer.normalize(raw, Normalizer.Form.NFD) - val noDiacritics = decomposed.replace(Regex("\\p{Mn}+"), "") + val noDiacritics = decomposed.replace(combiningMarks, "") val spaced = noDiacritics.map { c -> if (c.isLetterOrDigit()) c else ' ' }.joinToString("") - return spaced.lowercase().trim().replace(Regex("\\s+"), " ") + return spaced.lowercase().trim().replace(whitespaceRun, " ") } fun tokens(raw: String): List { diff --git a/app/app/src/main/java/org/modg/bookshelf/data/repo/BookRepository.kt b/app/app/src/main/java/org/modg/bookshelf/data/repo/BookRepository.kt index 6843731..de8d2a6 100644 --- a/app/app/src/main/java/org/modg/bookshelf/data/repo/BookRepository.kt +++ b/app/app/src/main/java/org/modg/bookshelf/data/repo/BookRepository.kt @@ -7,14 +7,11 @@ import kotlinx.coroutines.withContext import kotlinx.serialization.json.Json import kotlinx.serialization.encodeToString import kotlinx.serialization.decodeFromString -import okhttp3.OkHttpClient -import okhttp3.Request import org.modg.bookshelf.data.local.BookDao import org.modg.bookshelf.data.local.BookEntity import org.modg.bookshelf.data.local.IdGenerator import org.modg.bookshelf.data.local.SyncState import java.io.File -import java.io.IOException private val authorsJson = Json { ignoreUnknownKeys = true } @@ -31,7 +28,6 @@ fun decodeAuthors(json: String): List = class BookRepository( private val bookDao: BookDao, private val context: Context, - private val downloadClient: OkHttpClient = OkHttpClient(), ) { fun observeAll(): Flow> = bookDao.observeAll() fun observeAllByRecentlyAdded(): Flow> = bookDao.observeAllByRecentlyAdded() @@ -44,10 +40,13 @@ class BookRepository( suspend fun countByShelf(shelfId: String): Int = bookDao.countByShelf(shelfId) /** - * Creates a new book. If [coverSourceUrl] is set, attempts to download the - * cover to app-private storage now (best-effort; failures are swallowed — - * the SyncEngine will simply have nothing to upload, and the UI still has - * [coverSourceUrl] to fall back on for display). + * Creates a new book. [coverFile] is a cover the caller already downloaded (see + * [CoverPreload]); it is COPIED into app-private storage as [BookEntity.localCoverPath] + * for SyncEngine to upload. This used to download [coverSourceUrl] itself before + * writing the row, which made every save wait on the image host and silently saved + * the book with no cover file when that download failed. The caller keeps ownership + * of [coverFile] (a copy, not a move, so a failed write leaves the preload intact for + * a retry). [coverSourceUrl] is still recorded — the UI falls back to it for display. */ suspend fun createBook( title: String, @@ -60,35 +59,42 @@ class BookRepository( pageCount: Int? = null, description: String? = null, coverSourceUrl: String? = null, + coverFile: File? = null, shelfId: String? = null, notes: String? = null, addedBy: String? = null, ): String { val id = IdGenerator.newId() val now = System.currentTimeMillis() - val localCoverPath = coverSourceUrl?.let { downloadCoverBestEffort(id, it) } - bookDao.upsert( - BookEntity( - id = id, - title = title, - subtitle = subtitle, - authorsJson = encodeAuthors(authors), - isbn13 = isbn13, - isbn10 = isbn10, - publisher = publisher, - publishedDate = publishedDate, - pageCount = pageCount, - description = description, - coverSourceUrl = coverSourceUrl, - shelfId = shelfId, - notes = notes, - addedBy = addedBy, - createdAt = now, - updatedAt = now, - syncState = SyncState.PENDING_CREATE, - localCoverPath = localCoverPath, - ), - ) + val localCover = coverFile?.let { copyCover(id, it) } + try { + bookDao.upsert( + BookEntity( + id = id, + title = title, + subtitle = subtitle, + authorsJson = encodeAuthors(authors), + isbn13 = isbn13, + isbn10 = isbn10, + publisher = publisher, + publishedDate = publishedDate, + pageCount = pageCount, + description = description, + coverSourceUrl = coverSourceUrl, + shelfId = shelfId, + notes = notes, + addedBy = addedBy, + createdAt = now, + updatedAt = now, + syncState = SyncState.PENDING_CREATE, + localCoverPath = localCover?.absolutePath, + ), + ) + } catch (e: Throwable) { + // No row points at the copy, so nothing would ever upload or delete it. + localCover?.delete() + throw e + } return id } @@ -131,19 +137,8 @@ class BookRepository( ) } - private suspend fun downloadCoverBestEffort(id: String, url: String): String? = withContext(Dispatchers.IO) { - try { - val request = Request.Builder().url(url).build() - downloadClient.newCall(request).execute().use { response -> - if (!response.isSuccessful) return@withContext null - val bytes = response.body.bytes() - val dir = File(context.filesDir, "covers").apply { mkdirs() } - val file = File(dir, "$id.jpg") - file.writeBytes(bytes) - file.absolutePath - } - } catch (e: IOException) { - null - } + private suspend fun copyCover(id: String, source: File): File = withContext(Dispatchers.IO) { + val dir = File(context.filesDir, "covers").apply { mkdirs() } + source.copyTo(File(dir, "$id.jpg"), overwrite = true) } } diff --git a/app/app/src/main/java/org/modg/bookshelf/data/repo/CoverPreload.kt b/app/app/src/main/java/org/modg/bookshelf/data/repo/CoverPreload.kt new file mode 100644 index 0000000..e078b88 --- /dev/null +++ b/app/app/src/main/java/org/modg/bookshelf/data/repo/CoverPreload.kt @@ -0,0 +1,164 @@ +package org.modg.bookshelf.data.repo + +import java.io.File +import java.io.IOException +import java.util.UUID +import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.Job +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.flow.asStateFlow +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.launch +import kotlinx.coroutines.withContext +import okhttp3.OkHttpClient +import okhttp3.Request +import org.modg.bookshelf.data.metadata.SourceResult + +/** Outcome of one cover download. [Failed.reason] is safe to show the user. */ +sealed interface CoverFetch { + data class Ready(val file: File) : CoverFetch + data class Failed(val reason: String) : CoverFetch +} + +/** + * Downloads a cover into [preloadDir] so it can be handed to [BookRepository.createBook] + * at save time. Save used to download the cover itself, BEFORE writing the row, so every + * save sat waiting on the image host (covers.openlibrary.org redirects to archive.org and + * is often slow) even though the sheet was already showing that very cover. Downloading + * as soon as the cover URL is known means the file is usually there by the time Save is + * tapped, and a failure can be shown while the user is still on the sheet. + * + * Not Coil's disk cache: that's Coil's private performance detail (eviction, cache keys, + * format can all change), and we need a file we own to upload to the server. + */ +class CoverDownloader( + private val httpClient: OkHttpClient, + private val preloadDir: File, + private val ioDispatcher: CoroutineDispatcher = Dispatchers.IO, +) { + /** Never throws except [CancellationException]; no file is left behind on any non-success path. */ + suspend fun download(url: String): CoverFetch { + preloadDir.mkdirs() + val file = File(preloadDir, UUID.randomUUID().toString()) + return try { + withContext(ioDispatcher) { fetchInto(file, url) } + } catch (e: CancellationException) { + // Also covers a cancel that lands AFTER the bytes were written: withContext then + // throws on its way out, past any catch inside the block. + file.delete() + throw e + } + } + + private fun fetchInto(file: File, url: String): CoverFetch = try { + val request = Request.Builder().url(url).build() + httpClient.newCall(request).execute().use { response -> + val bytes = if (response.isSuccessful) response.body.bytes() else null + when { + bytes == null -> CoverFetch.Failed(SourceResult.fromHttpCode(response.code).reason) + bytes.isEmpty() -> CoverFetch.Failed("empty image") + else -> { + file.writeBytes(bytes) + CoverFetch.Ready(file) + } + } + } + } catch (e: IOException) { + file.delete() + CoverFetch.Failed(SourceResult.fromException(e).reason) + } catch (e: Throwable) { + file.delete() + CoverFetch.Failed("unexpected: ${e.javaClass.simpleName}") + } + + /** + * Deletes preloads left behind by a previous process (killed or crashed with a sheet + * open). Only files older than [olderThanMillis] — the current process's start — so a + * preload started by this process can never be swept out from under its sheet. + */ + fun sweepStale(olderThanMillis: Long) { + preloadDir.listFiles()?.forEach { if (it.lastModified() < olderThanMillis) it.delete() } + } +} + +/** What a sheet/screen shows about its cover download. Every non-[None] state carries the URL it is for. */ +sealed interface CoverPreloadState { + data object None : CoverPreloadState + data class Loading(val url: String) : CoverPreloadState + data class Ready(val url: String, val file: File) : CoverPreloadState + data class Failed(val url: String, val reason: String) : CoverPreloadState +} + +/** + * At most ONE cover download for one owner (a sheet or screen) — it starts when a result is + * opened, never for every row in a result list. Owns the downloaded file until [discard]: + * call it on Skip/dismiss/leaving the screen AND after a successful save (createBook copies + * the file), so nothing is left behind for books that weren't saved. + */ +class CoverPreload( + private val downloader: CoverDownloader, + private val scope: CoroutineScope, +) { + private val _state = MutableStateFlow(CoverPreloadState.None) + val state: StateFlow = _state.asStateFlow() + private var job: Job? = null + + /** Starts downloading [url]; a no-op if that URL is already loading or ready. Null/blank clears. */ + fun start(url: String?) { + if (url.isNullOrBlank()) { + discard() + return + } + when (val current = _state.value) { + is CoverPreloadState.Loading -> if (current.url == url) return + is CoverPreloadState.Ready -> if (current.url == url) return + else -> Unit + } + discard() + val loading = CoverPreloadState.Loading(url) + _state.value = loading + job = scope.launch { + val result = downloader.download(url) + if (_state.value != loading) { + // Superseded or discarded while downloading — the file is nobody's. + (result as? CoverFetch.Ready)?.file?.delete() + return@launch + } + _state.value = when (result) { + is CoverFetch.Ready -> CoverPreloadState.Ready(url, result.file) + is CoverFetch.Failed -> CoverPreloadState.Failed(url, result.reason) + } + } + } + + fun retry() { + val failed = _state.value as? CoverPreloadState.Failed ?: return + discard() + start(failed.url) + } + + /** + * Waits for any in-flight download to finish. A Ready file that has since vanished + * (the preload lives in the cache dir, which Android may clear) is downloaded again. + */ + suspend fun awaitSettled(): CoverPreloadState { + val settled = _state.first { it !is CoverPreloadState.Loading } + if (settled is CoverPreloadState.Ready && !settled.file.exists()) { + discard() + start(settled.url) + return _state.first { it !is CoverPreloadState.Loading } + } + return settled + } + + fun discard() { + job?.cancel() + job = null + (_state.value as? CoverPreloadState.Ready)?.file?.delete() + _state.value = CoverPreloadState.None + } +} diff --git a/app/app/src/main/java/org/modg/bookshelf/ui/add/AddBookScreen.kt b/app/app/src/main/java/org/modg/bookshelf/ui/add/AddBookScreen.kt index 992d947..00997a8 100644 --- a/app/app/src/main/java/org/modg/bookshelf/ui/add/AddBookScreen.kt +++ b/app/app/src/main/java/org/modg/bookshelf/ui/add/AddBookScreen.kt @@ -37,9 +37,11 @@ import androidx.lifecycle.viewmodel.initializer import androidx.lifecycle.viewmodel.viewModelFactory import org.modg.bookshelf.AppContainer import org.modg.bookshelf.ui.components.BookshelfScaffold +import org.modg.bookshelf.ui.components.CoverPreloadNotice import org.modg.bookshelf.ui.components.PaperSurface import org.modg.bookshelf.ui.components.PrimaryButton import org.modg.bookshelf.ui.components.SecondaryButton +import org.modg.bookshelf.ui.components.saveButtonLabel import org.modg.bookshelf.ui.scan.DuplicateStatus import org.modg.bookshelf.ui.scan.ShelfPicker @@ -71,6 +73,7 @@ fun AddBookScreen( bookRepository = container.bookRepository, locationRepository = container.locationRepository, settingsStore = container.settingsStore, + coverDownloader = container.coverDownloader, initialDraft = BookDraft.fromRouteArgs( title = initialTitle, isbn = initialIsbn, @@ -103,6 +106,7 @@ fun AddBookScreen( onSave = { viewModel.save(onSaved) }, onSaveAndAddAnother = viewModel::saveAndAddAnother, onBack = onBack, + onRetryCover = viewModel::retryCover, ), ) } @@ -127,6 +131,7 @@ data class AddBookCallbacks( val onSave: () -> Unit, val onSaveAndAddAnother: () -> Unit, val onBack: () -> Unit, + val onRetryCover: () -> Unit = {}, ) @Composable @@ -304,6 +309,8 @@ fun AddBookContent(state: AddBookUiState, callbacks: AddBookCallbacks, modifier: onShelfSelected = callbacks.onShelfSelected, ) + CoverPreloadNotice(state = state.cover, onRetry = callbacks.onRetryCover) + if (state.saveError != null) { Text( text = "Couldn't save: ${state.saveError}", @@ -324,7 +331,7 @@ fun AddBookContent(state: AddBookUiState, callbacks: AddBookCallbacks, modifier: modifier = Modifier.weight(1f), ) PrimaryButton( - text = if (state.isSaving) "Saving…" else "Save", + text = saveButtonLabel(state.isSaving, state.cover), enabled = state.canSave, onClick = callbacks.onSave, modifier = Modifier.weight(1f), diff --git a/app/app/src/main/java/org/modg/bookshelf/ui/add/AddBookViewModel.kt b/app/app/src/main/java/org/modg/bookshelf/ui/add/AddBookViewModel.kt index 96f8dbc..ea8527c 100644 --- a/app/app/src/main/java/org/modg/bookshelf/ui/add/AddBookViewModel.kt +++ b/app/app/src/main/java/org/modg/bookshelf/ui/add/AddBookViewModel.kt @@ -16,6 +16,9 @@ import org.modg.bookshelf.data.local.BookcaseEntity import org.modg.bookshelf.data.local.ShelfEntity import org.modg.bookshelf.data.prefs.SettingsStore import org.modg.bookshelf.data.repo.BookRepository +import org.modg.bookshelf.data.repo.CoverDownloader +import org.modg.bookshelf.data.repo.CoverPreload +import org.modg.bookshelf.data.repo.CoverPreloadState import org.modg.bookshelf.data.repo.LocationRepository import org.modg.bookshelf.ui.scan.DuplicateCheck import org.modg.bookshelf.ui.scan.DuplicateStatus @@ -32,6 +35,7 @@ data class AddBookUiState( val saveError: String? = null, val savedCount: Int = 0, val lastSavedTitle: String? = null, + val cover: CoverPreloadState = CoverPreloadState.None, ) { val isbnError: String? get() = if (validateIsbn(draft.isbn) is IsbnFieldState.Invalid) { @@ -57,6 +61,7 @@ class AddBookViewModel( private val bookRepository: BookRepository, locationRepository: LocationRepository, private val settingsStore: SettingsStore, + coverDownloader: CoverDownloader, initialDraft: BookDraft = BookDraft(), ) : ViewModel() { @@ -71,6 +76,12 @@ class AddBookViewModel( ) private val _formState = MutableStateFlow(FormState(draft = initialDraft)) + + /** + * Only a draft carried in from an online search hit ("Edit details") has a cover URL — + * there's no cover field on this form — so this is usually [CoverPreloadState.None]. + */ + private val coverPreload = CoverPreload(coverDownloader, viewModelScope) private var duplicateCheckJob: Job? = null val bookcases: StateFlow> = locationRepository.observeBookcases() @@ -87,8 +98,8 @@ class AddBookViewModel( // collecting (e.g. Compose's collectAsState, or a test using .first()); a bare // .value read with no live collector sees only the seed passed to stateIn. val uiState: StateFlow = combine( - _formState, bookcases, shelves, recentShelfId, - ) { form, cases, shelfList, recent -> + _formState, bookcases, shelves, recentShelfId, coverPreload.state, + ) { form, cases, shelfList, recent, cover -> AddBookUiState( draft = form.draft, bookcases = cases, @@ -100,6 +111,7 @@ class AddBookViewModel( saveError = form.saveError, savedCount = form.savedCount, lastSavedTitle = form.lastSavedTitle, + cover = cover, ) }.stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), AddBookUiState(draft = initialDraft)) @@ -113,8 +125,11 @@ class AddBookViewModel( if (remembered != null) _formState.update { it.copy(selectedShelfId = remembered) } } if (initialDraft.isbn.isNotBlank()) scheduleDuplicateCheck(initialDraft.isbn) + coverPreload.start(initialDraft.coverSourceUrl) } + fun retryCover() = coverPreload.retry() + fun onTitleChanged(value: String) = updateDraft { it.copy(title = value) } fun onSubtitleChanged(value: String) = updateDraft { it.copy(subtitle = value) } fun onAuthorsChanged(value: String) = updateDraft { it.copy(authors = value) } @@ -145,8 +160,16 @@ class AddBookViewModel( return } duplicateCheckJob = viewModelScope.launch { - val existing = bookRepository.findByIsbn13(isbn13) - _formState.update { it.copy(duplicate = DuplicateCheck.check(existing)) } + // A Room failure here would otherwise crash the app from a keystroke. The check + // is only a warning, so on failure there is simply no warning. + val duplicate = try { + DuplicateCheck.check(bookRepository.findByIsbn13(isbn13)) + } catch (e: CancellationException) { + throw e + } catch (e: Throwable) { + DuplicateStatus.New + } + _formState.update { it.copy(duplicate = duplicate) } } } @@ -177,17 +200,26 @@ class AddBookViewModel( */ internal suspend fun performSave(clearAfterSave: Boolean): String? { if (_formState.value.isSaving) return null + val coverAtTap = coverPreload.state.value _formState.update { it.copy(isSaving = true, saveError = null) } val form = _formState.value val draft = form.draft - return try { + val savedTitle = draft.title.trim() + val bookId = try { + // Same cover rule as the scan sheet (see ScanViewModel.performSave): a download + // that fails while Save waits stops the save; one that had already failed when + // Save (labelled "Save without cover") was tapped does not. + val cover = coverPreload.awaitSettled() + if (cover is CoverPreloadState.Failed && coverAtTap !is CoverPreloadState.Failed) { + _formState.update { it.copy(isSaving = false) } + return null + } val isbnValidation = validateIsbn(draft.isbn) val isbn13 = (isbnValidation as? IsbnFieldState.Valid)?.isbn13 val isbn10 = (isbnValidation as? IsbnFieldState.Valid)?.isbn10 val pageCount = (validatePages(draft.pages) as? PagesFieldState.Valid)?.pages val authors = draft.authors.split(",").map(String::trim).filter(String::isNotEmpty) - val savedTitle = draft.title.trim() - val bookId = bookRepository.createBook( + bookRepository.createBook( title = savedTitle, subtitle = draft.subtitle.trim().ifEmpty { null }, authors = authors, @@ -198,30 +230,46 @@ class AddBookViewModel( pageCount = pageCount, description = draft.description.trim().ifEmpty { null }, coverSourceUrl = draft.coverSourceUrl.trim().ifEmpty { null }, + coverFile = (cover as? CoverPreloadState.Ready)?.file, shelfId = form.selectedShelfId, ) - rememberShelf(form.selectedShelfId) - _formState.update { - it.copy( - draft = if (clearAfterSave) BookDraft() else it.draft, - isSaving = false, - saveError = null, - savedCount = it.savedCount + 1, - lastSavedTitle = savedTitle, - ) - } - if (clearAfterSave) scheduleDuplicateCheck("") // the ISBN field just cleared; the warning must too - bookId } catch (e: CancellationException) { + _formState.update { it.copy(isSaving = false) } throw e } catch (e: Throwable) { _formState.update { it.copy(isSaving = false, saveError = e.javaClass.simpleName) } - null + return null + } + // Everything below runs only once the book is written. None of it may report a + // failure: "couldn't save" for a saved book invites a retry that creates a duplicate. + coverPreload.discard() // createBook copied it; "add another" starts a draft with no cover + rememberShelf(form.selectedShelfId) + _formState.update { + it.copy( + draft = if (clearAfterSave) BookDraft() else it.draft, + isSaving = false, + saveError = null, + savedCount = it.savedCount + 1, + lastSavedTitle = savedTitle, + ) + } + if (clearAfterSave) scheduleDuplicateCheck("") // the ISBN field just cleared; the warning must too + return bookId + } + + /** A remembered-shelf write that fails is swallowed — the book is already saved (see [performSave]). */ + private suspend fun rememberShelf(shelfId: String?) { + if (shelfId == null) return // "Not shelved" must never overwrite the memory — it isn't a shelf + try { + settingsStore.setLastShelfId(shelfId) + } catch (e: CancellationException) { + throw e + } catch (e: Throwable) { + // Losing the "recent shelf" shortcut is not worth an error on a saved book. } } - /** "Not shelved" (null) must never overwrite the memory — it isn't a shelf. */ - private suspend fun rememberShelf(shelfId: String?) { - if (shelfId != null) settingsStore.setLastShelfId(shelfId) + override fun onCleared() { + coverPreload.discard() } } diff --git a/app/app/src/main/java/org/modg/bookshelf/ui/components/CoverPreloadNotice.kt b/app/app/src/main/java/org/modg/bookshelf/ui/components/CoverPreloadNotice.kt new file mode 100644 index 0000000..9e126fd --- /dev/null +++ b/app/app/src/main/java/org/modg/bookshelf/ui/components/CoverPreloadNotice.kt @@ -0,0 +1,42 @@ +package org.modg.bookshelf.ui.components + +import androidx.compose.foundation.layout.Row +import androidx.compose.foundation.layout.fillMaxWidth +import androidx.compose.foundation.layout.padding +import androidx.compose.material3.MaterialTheme +import androidx.compose.material3.Text +import androidx.compose.material3.TextButton +import androidx.compose.runtime.Composable +import androidx.compose.ui.Alignment +import androidx.compose.ui.Modifier +import androidx.compose.ui.unit.dp +import org.modg.bookshelf.data.repo.CoverPreloadState + +/** + * The one visible trace of a cover preload: nothing while it loads or once it's ready (the + * sheet already shows the cover via Coil), and a failure line with Retry when it failed — + * so the user finds out while still on the sheet, not after the book is saved without one. + */ +@Composable +fun CoverPreloadNotice(state: CoverPreloadState, onRetry: () -> Unit, modifier: Modifier = Modifier) { + if (state !is CoverPreloadState.Failed) return + Row(modifier = modifier.fillMaxWidth().padding(top = 8.dp), verticalAlignment = Alignment.CenterVertically) { + Text( + text = "Couldn't download the cover — ${state.reason}", + style = MaterialTheme.typography.bodySmall, + color = MaterialTheme.colorScheme.error, + modifier = Modifier.weight(1f), + ) + TextButton(onClick = onRetry) { Text("Retry") } + } +} + +/** + * Save's label. After a failed cover download, saving goes ahead without the cover file + * (the user decided: retry OR save without it, never stuck mid-box), so the button says so. + */ +fun saveButtonLabel(isSaving: Boolean, cover: CoverPreloadState, idle: String = "Save"): String = when { + isSaving -> "Saving…" + cover is CoverPreloadState.Failed -> "$idle without cover" + else -> idle +} diff --git a/app/app/src/main/java/org/modg/bookshelf/ui/library/LibraryModels.kt b/app/app/src/main/java/org/modg/bookshelf/ui/library/LibraryModels.kt index a819e97..b54d089 100644 --- a/app/app/src/main/java/org/modg/bookshelf/ui/library/LibraryModels.kt +++ b/app/app/src/main/java/org/modg/bookshelf/ui/library/LibraryModels.kt @@ -68,12 +68,15 @@ sealed interface OnlineSearchState { data class Searching(val query: String) : OnlineSearchState /** - * [inLibrary] is the FULL "In your library" set once a search has run — a superset - * of [LocalBookMatcher]'s plain text match, since [org.modg.bookshelf.data.metadata.SearchResultMerger] - * also pulls in an owned book reached only via matching one of the online hits (task: - * "an online hit that is a different edition of an owned book belongs in the library - * section"). [openLibraryFailure]/[googleBooksFailure] are that source's - * [org.modg.bookshelf.data.metadata.SourceResult.Failed.reason]-equivalent, null when + * [inLibrary] is what the "In your library" section shows once a search has run — a + * superset of [LocalBookMatcher]'s plain text match, since [org.modg.bookshelf.data.metadata.SearchResultMerger] + * also pulls in an owned book reached only via one of the online hits (task: "an + * online hit that is a different edition of an owned book belongs in the library + * section"). The shelf/bookcase filter and sort are applied to it; owned matches the + * filter hides are counted in [inLibraryOutsideFilter] instead of silently vanishing — + * the merge still (correctly) keeps them out of the online section, so without the count + * a book you own on another shelf would appear in neither. + * [openLibraryFailure]/[googleBooksFailure] are that source's failure reason, null when * that source answered (whether or not it had anything) — the UI keys off these to * show a per-source failure line with Retry while still showing the other source's * [online] results, and to tell "both failed" (retry affordance, never "no results") @@ -85,6 +88,7 @@ sealed interface OnlineSearchState { val online: List, val openLibraryFailure: String?, val googleBooksFailure: String?, + val inLibraryOutsideFilter: Int = 0, ) : OnlineSearchState } diff --git a/app/app/src/main/java/org/modg/bookshelf/ui/library/LibraryScreen.kt b/app/app/src/main/java/org/modg/bookshelf/ui/library/LibraryScreen.kt index 8ced445..2275236 100644 --- a/app/app/src/main/java/org/modg/bookshelf/ui/library/LibraryScreen.kt +++ b/app/app/src/main/java/org/modg/bookshelf/ui/library/LibraryScreen.kt @@ -68,13 +68,16 @@ import org.modg.bookshelf.data.local.ShelfEntity import org.modg.bookshelf.data.metadata.BookMetadata import org.modg.bookshelf.data.metadata.OnlineBook import org.modg.bookshelf.data.repo.decodeAuthors +import org.modg.bookshelf.data.repo.CoverPreloadState import org.modg.bookshelf.ui.components.BookCover +import org.modg.bookshelf.ui.components.CoverPreloadNotice import org.modg.bookshelf.ui.components.EmptyState import org.modg.bookshelf.ui.components.BookshelfScaffold import org.modg.bookshelf.ui.components.GoldDivider import org.modg.bookshelf.ui.components.PaperSurface import org.modg.bookshelf.ui.components.PrimaryButton import org.modg.bookshelf.ui.components.SecondaryButton +import org.modg.bookshelf.ui.components.saveButtonLabel import org.modg.bookshelf.ui.components.SyncStatus import org.modg.bookshelf.ui.components.SyncStatusBar import org.modg.bookshelf.ui.scan.ShelfPicker @@ -116,6 +119,7 @@ fun LibraryScreen( settingsStore = container.settingsStore, bookSearchRepository = container.bookSearchRepository, metadataRepository = container.metadataRepository, + coverDownloader = container.coverDownloader, initialShelfId = shelfIdFilter, ) } @@ -129,6 +133,7 @@ fun LibraryScreen( val onlineSearch by viewModel.onlineSearch.collectAsState() val saveSheet by viewModel.saveSheet.collectAsState() val recentShelfId by viewModel.recentShelfId.collectAsState() + val cover by viewModel.cover.collectAsState() BookshelfScaffold( title = "Bookshelf", @@ -183,7 +188,11 @@ fun LibraryScreen( when { query.isNotBlank() -> LibrarySearchResultsContent( query = query, - localMatches = uiState.books, + // Once an online search has run, the merged set — it includes owned + // editions reached only through an online hit, which the plain text + // match can't find and the merge has already removed from "Online". + localMatches = (onlineSearch as? OnlineSearchState.Done)?.inLibrary ?: uiState.books, + inLibraryOutsideFilter = (onlineSearch as? OnlineSearchState.Done)?.inLibraryOutsideFilter ?: 0, onlineSearch = onlineSearch, onBookClick = onBookClick, onSubmitOnlineSearch = viewModel::submitOnlineSearch, @@ -235,8 +244,10 @@ fun LibraryScreen( recentShelfId = recentShelfId, onShelfSelected = viewModel::onSaveSheetShelfSelected, onSave = viewModel::saveOnlineResult, - onEditDetails = { onEditOnlineResult(sheetState.book, sheetState.enrichment) }, + onEditDetails = { viewModel.takeForEditing()?.let { (book, enrichment) -> onEditOnlineResult(book, enrichment) } }, onSkip = viewModel::dismissSaveSheet, + cover = cover, + onRetryCover = viewModel::retryCover, ) } } @@ -408,6 +419,7 @@ internal fun LibrarySearchResultsContent( onOnlineResultClick: (OnlineBook) -> Unit, onEnterByHand: () -> Unit, modifier: Modifier = Modifier, + inLibraryOutsideFilter: Int = 0, ) { Column( modifier = modifier @@ -439,6 +451,20 @@ internal fun LibrarySearchResultsContent( } } + if (inLibraryOutsideFilter > 0) { + Text( + text = if (inLibraryOutsideFilter == 1) { + "1 more in your library, outside the current filter" + } else { + "$inLibraryOutsideFilter more in your library, outside the current filter" + }, + style = MaterialTheme.typography.bodySmall, + color = MaterialTheme.colorScheme.onSurfaceVariant, + // Extra top space: at 4dp it sat under the last card's author and read as its byline. + modifier = Modifier.padding(start = 16.dp, end = 16.dp, top = 12.dp, bottom = 4.dp), + ) + } + GoldDivider(modifier = Modifier.padding(horizontal = 16.dp, vertical = 8.dp)) SearchSectionHeader(text = "Online") OnlineSearchSection( @@ -632,6 +658,8 @@ internal fun OnlineResultSaveSheet( onSave: () -> Unit, onEditDetails: () -> Unit, onSkip: () -> Unit, + cover: CoverPreloadState = CoverPreloadState.None, + onRetryCover: () -> Unit = {}, ) { val book = state.book Column(modifier = Modifier.fillMaxWidth().padding(16.dp)) { @@ -674,12 +702,13 @@ internal fun OnlineResultSaveSheet( recentShelfId = recentShelfId, onShelfSelected = onShelfSelected, ) + CoverPreloadNotice(state = cover, onRetry = onRetryCover) Row(modifier = Modifier.fillMaxWidth().padding(top = 16.dp), horizontalArrangement = Arrangement.spacedBy(12.dp)) { SecondaryButton(text = "Skip", onClick = onSkip, modifier = Modifier.weight(1f)) SecondaryButton(text = "Edit details", onClick = onEditDetails, modifier = Modifier.weight(1f)) } PrimaryButton( - text = if (state.isSaving) "Saving…" else "Save", + text = saveButtonLabel(state.isSaving, cover), enabled = !state.isSaving, onClick = onSave, modifier = Modifier.fillMaxWidth().padding(top = 12.dp), diff --git a/app/app/src/main/java/org/modg/bookshelf/ui/library/LibraryViewModel.kt b/app/app/src/main/java/org/modg/bookshelf/ui/library/LibraryViewModel.kt index 22b7e20..91e1dad 100644 --- a/app/app/src/main/java/org/modg/bookshelf/ui/library/LibraryViewModel.kt +++ b/app/app/src/main/java/org/modg/bookshelf/ui/library/LibraryViewModel.kt @@ -3,6 +3,8 @@ package org.modg.bookshelf.ui.library import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.Job import kotlinx.coroutines.flow.MutableStateFlow @@ -10,6 +12,7 @@ import kotlinx.coroutines.flow.SharingStarted import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.flow.combine +import kotlinx.coroutines.flow.flowOn import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.stateIn import kotlinx.coroutines.flow.update @@ -17,6 +20,7 @@ import kotlinx.coroutines.launch import org.modg.bookshelf.data.local.BookEntity import org.modg.bookshelf.data.local.BookcaseEntity import org.modg.bookshelf.data.local.ShelfEntity +import org.modg.bookshelf.data.metadata.BookMetadata import org.modg.bookshelf.data.metadata.BookSearchRepository import org.modg.bookshelf.data.metadata.IsbnUtils import org.modg.bookshelf.data.metadata.LookupResult @@ -27,6 +31,9 @@ import org.modg.bookshelf.data.metadata.SearchResultMerger import org.modg.bookshelf.data.metadata.SearchSourceResult import org.modg.bookshelf.data.prefs.SettingsStore import org.modg.bookshelf.data.repo.BookRepository +import org.modg.bookshelf.data.repo.CoverDownloader +import org.modg.bookshelf.data.repo.CoverPreload +import org.modg.bookshelf.data.repo.CoverPreloadState import org.modg.bookshelf.data.repo.LocationRepository import org.modg.bookshelf.data.repo.SyncEngine import org.modg.bookshelf.data.repo.SyncResult @@ -49,7 +56,10 @@ class LibraryViewModel( private val settingsStore: SettingsStore, private val bookSearchRepository: BookSearchRepository, private val metadataRepository: MetadataRepository, + coverDownloader: CoverDownloader, initialShelfId: String?, + /** Where the per-keystroke match and the search merge run — off the main thread. Tests pass their own. */ + computeDispatcher: CoroutineDispatcher = Dispatchers.Default, ) : ViewModel() { private val _query = MutableStateFlow("") @@ -81,6 +91,12 @@ class LibraryViewModel( private val allBooks: StateFlow> = bookRepository.observeAll() .stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), emptyList()) + /** [allBooks] normalized once per change, not once per keystroke — see [LocalBookMatcher.IndexedBook]. */ + private val allBooksIndex: StateFlow> = allBooks + .map { LocalBookMatcher.index(it) } + .flowOn(computeDispatcher) + .stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), emptyList()) + private val hasAnyBooks: StateFlow = allBooks .map { it.isNotEmpty() } .stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), true) @@ -91,9 +107,9 @@ class LibraryViewModel( * single whole-string LIKE for this screen, which couldn't match "hobbit tolkien" * at all. A blank query means "browse everything", not "match nothing". */ - private val localMatches: StateFlow> = combine(_query, allBooks) { queryValue, books -> - if (queryValue.isBlank()) books else LocalBookMatcher.match(books, queryValue) - }.stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), emptyList()) + private val localMatches: StateFlow> = combine(_query, allBooksIndex) { queryValue, index -> + if (queryValue.isBlank()) index.map { it.book } else LocalBookMatcher.matchIndexed(index, queryValue) + }.flowOn(computeDispatcher).stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), emptyList()) private val filteredSortedBooks: StateFlow> = combine( localMatches, @@ -129,22 +145,46 @@ class LibraryViewModel( * sync) immediately migrates itself from "Online" into "In your library" via the * normal Room flow, with no hand-written "move it over" step. */ - val onlineSearch: StateFlow = combine(_onlineSearchPhase, allBooks) { phase, books -> + val onlineSearch: StateFlow = combine( + _onlineSearchPhase, + allBooksIndex, + _filter, + _sortOption, + shelfIdsByBookcase, + ) { phase, index, filterValue, sort, shelfMap -> when (phase) { is OnlineSearchPhase.NotSearched -> OnlineSearchState.NotSearched is OnlineSearchPhase.Searching -> OnlineSearchState.Searching(phase.query) - is OnlineSearchPhase.Done -> buildDoneState(phase.hits, books) + is OnlineSearchPhase.Done -> buildDoneState(phase.hits, index, filterValue, sort, shelfMap) } - }.stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), OnlineSearchState.NotSearched) + }.flowOn(computeDispatcher).stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), OnlineSearchState.NotSearched) - private fun buildDoneState(raw: RawHits, books: List): OnlineSearchState.Done { + /** + * The merge runs against the WHOLE library (ownership doesn't depend on the filter), + * then the shelf/bookcase filter and sort are applied to what "In your library" shows. + */ + private fun buildDoneState( + raw: RawHits, + index: List, + filterValue: LibraryFilter, + sort: LibrarySortOption, + shelfMap: Map>, + ): OnlineSearchState.Done { val onlineIsbns = (raw.openLibrary.asSequence() + raw.googleBooks.asSequence()) .flatMap { it.isbn13s } .toSet() - val isbnLinked = books.filter { book -> bookIsbn13s(book).any { it in onlineIsbns } } - val candidates = (LocalBookMatcher.match(books, raw.query) + isbnLinked).distinctBy { it.id } + val isbnLinked = index.map { it.book }.filter { book -> bookIsbn13s(book).any { it in onlineIsbns } } + val candidates = (LocalBookMatcher.matchIndexed(index, raw.query) + isbnLinked).distinctBy { it.id } val merged = SearchResultMerger.merge(candidates, raw.openLibrary, raw.googleBooks) - return OnlineSearchState.Done(raw.query, merged.inLibrary, merged.online, raw.openLibraryFailure, raw.googleBooksFailure) + val visible = LibraryFilterLogic.apply(merged.inLibrary, filterValue, shelfMap) + return OnlineSearchState.Done( + query = raw.query, + inLibrary = LibrarySort.sort(visible, sort), + online = merged.online, + openLibraryFailure = raw.openLibraryFailure, + googleBooksFailure = raw.googleBooksFailure, + inLibraryOutsideFilter = merged.inLibrary.size - visible.size, + ) } private fun bookIsbn13s(book: BookEntity): List = @@ -156,6 +196,10 @@ class LibraryViewModel( val saveSheet: StateFlow = _saveSheet.asStateFlow() private var enrichmentJob: Job? = null + /** The save sheet's cover, downloading from the moment the sheet opens — one result at a time, never the whole list. */ + private val coverPreload = CoverPreload(coverDownloader, viewModelScope) + val cover: StateFlow = coverPreload.state + /** Surfaced as the save sheet's shelf picker default, same as the scan/add-by-hand sheets. */ val recentShelfId: StateFlow = settingsStore.lastShelfId .stateIn(viewModelScope, SharingStarted.WhileSubscribed(5_000), null) @@ -257,6 +301,7 @@ class LibraryViewModel( fun onOnlineResultTapped(book: OnlineBook) { enrichmentJob?.cancel() _saveSheet.value = OnlineSaveSheetState.Shown(book = book, selectedShelfId = recentShelfId.value) + coverPreload.start(resolveOnlineBookFields(book, enrichment = null).coverUrl) val isbn13 = book.isbn13 ?: return enrichmentJob = viewModelScope.launch { val result = metadataRepository.lookup(isbn13) @@ -264,16 +309,33 @@ class LibraryViewModel( _saveSheet.update { state -> if (state is OnlineSaveSheetState.Shown && state.book == book) state.copy(enrichment = result.metadata) else state } + // A no-op unless enrichment supplied the only cover (the hit's own wins, see resolveOnlineBookFields). + val shown = _saveSheet.value as? OnlineSaveSheetState.Shown + if (shown?.book == book) coverPreload.start(resolveOnlineBookFields(book, shown.enrichment).coverUrl) } } } + fun retryCover() = coverPreload.retry() + + /** + * "Edit details": closes the sheet and hands its book to the add-by-hand screen. It must + * close here — left open, the sheet was still up (Save enabled) on returning to the + * library after saving that same book from the add screen, one tap from a duplicate. + */ + fun takeForEditing(): Pair? { + val shown = _saveSheet.value as? OnlineSaveSheetState.Shown ?: return null + dismissSaveSheet() + return shown.book to shown.enrichment + } + fun onSaveSheetShelfSelected(shelfId: String?) { _saveSheet.update { if (it is OnlineSaveSheetState.Shown) it.copy(selectedShelfId = shelfId) else it } } fun dismissSaveSheet() { enrichmentJob?.cancel() + coverPreload.discard() _saveSheet.value = OnlineSaveSheetState.Hidden } @@ -291,11 +353,24 @@ class LibraryViewModel( internal suspend fun performSaveOnlineResult(): String? { val state = _saveSheet.value as? OnlineSaveSheetState.Shown ?: return null if (state.isSaving) return null + val coverAtTap = coverPreload.state.value _saveSheet.update { if (it is OnlineSaveSheetState.Shown) it.copy(isSaving = true, saveError = null) else it } + var shelfId = state.selectedShelfId - val fields = resolveOnlineBookFields(state.book, state.enrichment) - return try { - val id = bookRepository.createBook( + val id = try { + // Same cover rule as the scan sheet (see ScanViewModel.performSave): a download + // that fails while Save waits stops the save; one that had already failed when + // "Save without cover" was tapped does not. + val cover = coverPreload.awaitSettled() + if (cover is CoverPreloadState.Failed && coverAtTap !is CoverPreloadState.Failed) { + _saveSheet.update { if (it is OnlineSaveSheetState.Shown) it.copy(isSaving = false) else it } + return null + } + // Read the sheet again: enrichment may have landed during the wait. + val latest = _saveSheet.value as? OnlineSaveSheetState.Shown ?: state + val fields = resolveOnlineBookFields(latest.book, latest.enrichment) + shelfId = latest.selectedShelfId + bookRepository.createBook( title = fields.title, subtitle = fields.subtitle, authors = fields.authors, @@ -306,23 +381,39 @@ class LibraryViewModel( pageCount = fields.pageCount, description = fields.description, coverSourceUrl = fields.coverUrl, - shelfId = state.selectedShelfId, + coverFile = (cover as? CoverPreloadState.Ready)?.takeIf { it.url == fields.coverUrl }?.file, + shelfId = shelfId, ) - rememberShelf(state.selectedShelfId) - enrichmentJob?.cancel() - _saveSheet.value = OnlineSaveSheetState.Hidden - id } catch (e: CancellationException) { + _saveSheet.update { if (it is OnlineSaveSheetState.Shown) it.copy(isSaving = false) else it } throw e } catch (e: Throwable) { _saveSheet.update { if (it is OnlineSaveSheetState.Shown) it.copy(isSaving = false, saveError = e.javaClass.simpleName) else it } - null + return null + } + // The book is written: nothing below may report a failure (a retry would duplicate it). + dismissSaveSheet() // also discards the preload — createBook copied it + rememberShelf(shelfId) + return id + } + + /** + * "Not shelved" (null) must never overwrite the memory — it isn't a shelf. Runs after the + * book is written, so a failure is swallowed rather than reported (see [performSaveOnlineResult]). + */ + private suspend fun rememberShelf(shelfId: String?) { + if (shelfId == null) return + try { + settingsStore.setLastShelfId(shelfId) + } catch (e: CancellationException) { + throw e + } catch (e: Throwable) { + // Losing the "recent shelf" shortcut is not worth an error on a saved book. } } - /** "Not shelved" (null) must never overwrite the memory — it isn't a shelf. */ - private suspend fun rememberShelf(shelfId: String?) { - if (shelfId != null) settingsStore.setLastShelfId(shelfId) + override fun onCleared() { + coverPreload.discard() } /** Manual trigger — app start (see init) and pull-to-refresh, per SPEC "Sync design". */ diff --git a/app/app/src/main/java/org/modg/bookshelf/ui/library/LocalBookMatcher.kt b/app/app/src/main/java/org/modg/bookshelf/ui/library/LocalBookMatcher.kt index c491bf6..4e55770 100644 --- a/app/app/src/main/java/org/modg/bookshelf/ui/library/LocalBookMatcher.kt +++ b/app/app/src/main/java/org/modg/bookshelf/ui/library/LocalBookMatcher.kt @@ -14,28 +14,44 @@ import org.modg.bookshelf.data.repo.decodeAuthors object LocalBookMatcher { /** - * Normalizes [query] and every candidate field, splits the query on whitespace, - * and requires EVERY token to appear in at least one of title/subtitle/authors/ - * isbn13/isbn10 (different tokens may match different fields). Matching also - * checks a space-stripped form of each field so an initials-heavy name like - * "J.R.R. Tolkien" (normalized to "j r r tolkien", per [TextNormalization]) is - * still found by the squashed query token "jrr", not just "j" or "r". + * One book's searchable text, normalized once. Normalizing every field of every book + * on each keystroke (Unicode decomposition + regex, on the main thread) measured ~60ms + * per keystroke for 2,000 books on the dev server, so a phone would visibly lag; + * [index] is rebuilt only when the book list changes. */ - fun match(books: List, query: String): List { - val queryTokens = TextNormalization.tokens(query) - if (queryTokens.isEmpty()) return emptyList() - return books.filter { book -> matches(book, queryTokens) } + class IndexedBook internal constructor(val book: BookEntity, private val haystacks: List) { + private val squashed = haystacks.map { it.replace(" ", "") } + + internal fun matches(queryTokens: List): Boolean = + queryTokens.all { token -> haystacks.any { it.contains(token) } || squashed.any { it.contains(token) } } } - private fun matches(book: BookEntity, queryTokens: List): Boolean { - val haystacks = buildList { - add(TextNormalization.normalizeForMatching(book.title)) - book.subtitle?.let { add(TextNormalization.normalizeForMatching(it)) } - decodeAuthors(book.authorsJson).forEach { add(TextNormalization.normalizeForMatching(it)) } - book.isbn13?.let { add(TextNormalization.normalizeForMatching(it)) } - book.isbn10?.let { add(TextNormalization.normalizeForMatching(it)) } - } - val squashed = haystacks.map { it.replace(" ", "") } - return queryTokens.all { token -> haystacks.any { it.contains(token) } || squashed.any { it.contains(token) } } + fun index(books: List): List = books.map { book -> + IndexedBook( + book, + buildList { + add(TextNormalization.normalizeForMatching(book.title)) + book.subtitle?.let { add(TextNormalization.normalizeForMatching(it)) } + decodeAuthors(book.authorsJson).forEach { add(TextNormalization.normalizeForMatching(it)) } + book.isbn13?.let { add(TextNormalization.normalizeForMatching(it)) } + book.isbn10?.let { add(TextNormalization.normalizeForMatching(it)) } + }, + ) } + + /** + * Normalizes [query], splits it on whitespace, and requires EVERY token to appear in at + * least one of title/subtitle/authors/isbn13/isbn10 (different tokens may match different + * fields). Matching also checks a space-stripped form of each field so an initials-heavy + * name like "J.R.R. Tolkien" (normalized to "j r r tolkien", per [TextNormalization]) is + * still found by the squashed query token "jrr", not just "j" or "r". + */ + fun matchIndexed(index: List, query: String): List { + val queryTokens = TextNormalization.tokens(query) + if (queryTokens.isEmpty()) return emptyList() + return index.filter { it.matches(queryTokens) }.map { it.book } + } + + /** Convenience for callers without an index (tests, one-off matches). */ + fun match(books: List, query: String): List = matchIndexed(index(books), query) } diff --git a/app/app/src/main/java/org/modg/bookshelf/ui/scan/ScanModels.kt b/app/app/src/main/java/org/modg/bookshelf/ui/scan/ScanModels.kt index f6ce03d..5643091 100644 --- a/app/app/src/main/java/org/modg/bookshelf/ui/scan/ScanModels.kt +++ b/app/app/src/main/java/org/modg/bookshelf/ui/scan/ScanModels.kt @@ -55,6 +55,9 @@ object ScanMetadataOutcome { } } +/** The Found sheet's Save button: [isSaving] disables it; [error] names the exception class of a failed save. */ +data class ScanSaveStatus(val isSaving: Boolean = false, val error: String? = null) + /** SPEC.md "Continuous mode: ... a running 'added this session' count." */ data class ScanSessionState(val savedCount: Int = 0) { fun withSave(): ScanSessionState = copy(savedCount = savedCount + 1) diff --git a/app/app/src/main/java/org/modg/bookshelf/ui/scan/ScanScreen.kt b/app/app/src/main/java/org/modg/bookshelf/ui/scan/ScanScreen.kt index 10cf6da..2296671 100644 --- a/app/app/src/main/java/org/modg/bookshelf/ui/scan/ScanScreen.kt +++ b/app/app/src/main/java/org/modg/bookshelf/ui/scan/ScanScreen.kt @@ -71,6 +71,9 @@ import org.modg.bookshelf.ui.components.EmptyState import org.modg.bookshelf.ui.components.PrimaryButton import org.modg.bookshelf.ui.components.SecondaryButton import org.modg.bookshelf.ui.components.ShelfPickerSheet +import org.modg.bookshelf.ui.components.CoverPreloadNotice +import org.modg.bookshelf.ui.components.saveButtonLabel +import org.modg.bookshelf.data.repo.CoverPreloadState /** * SPEC.md "scan" screen: camera + reticle, on-hit bottom sheet, continuous @@ -92,12 +95,15 @@ fun ScanScreen( locationRepository = container.locationRepository, metadataRepository = container.metadataRepository, settingsStore = container.settingsStore, + coverDownloader = container.coverDownloader, ) } }, ) val sheetState by viewModel.sheetState.collectAsState() + val cover by viewModel.cover.collectAsState() + val saveStatus by viewModel.saveStatus.collectAsState() val sessionState by viewModel.sessionState.collectAsState() val rejectedMessage by viewModel.rejectedMessage.collectAsState() val torchEnabled by viewModel.scannerController.torchEnabled.collectAsState() @@ -171,6 +177,10 @@ fun ScanScreen( onShelfSelected = viewModel::selectShelf, onSave = { viewModel.save(state.isbn13, state.metadata) }, onSkip = { viewModel.skip() }, + cover = cover, + onRetryCover = viewModel::retryCover, + isSaving = saveStatus.isSaving, + saveError = saveStatus.error, ) } is ScanSheetState.NotFound -> ModalBottomSheet( @@ -330,6 +340,10 @@ internal fun FoundBookSheet( onShelfSelected: (String?) -> Unit, onSave: () -> Unit, onSkip: () -> Unit, + cover: CoverPreloadState = CoverPreloadState.None, + onRetryCover: () -> Unit = {}, + isSaving: Boolean = false, + saveError: String? = null, ) { Column(modifier = Modifier.fillMaxWidth().padding(16.dp)) { Box( @@ -362,9 +376,23 @@ internal fun FoundBookSheet( recentShelfId = recentShelfId, onShelfSelected = onShelfSelected, ) + CoverPreloadNotice(state = cover, onRetry = onRetryCover) + if (saveError != null) { + Text( + text = "Couldn't save: $saveError", + style = MaterialTheme.typography.bodyMedium, + color = MaterialTheme.colorScheme.error, + modifier = Modifier.padding(top = 8.dp), + ) + } Row(modifier = Modifier.fillMaxWidth().padding(top = 16.dp), horizontalArrangement = Arrangement.spacedBy(12.dp)) { SecondaryButton(text = "Skip", onClick = onSkip, modifier = Modifier.weight(1f)) - PrimaryButton(text = "Save", onClick = onSave, modifier = Modifier.weight(1f)) + PrimaryButton( + text = saveButtonLabel(isSaving, cover), + enabled = !isSaving, + onClick = onSave, + modifier = Modifier.weight(1f), + ) } } } diff --git a/app/app/src/main/java/org/modg/bookshelf/ui/scan/ScanViewModel.kt b/app/app/src/main/java/org/modg/bookshelf/ui/scan/ScanViewModel.kt index fcce674..95a4aff 100644 --- a/app/app/src/main/java/org/modg/bookshelf/ui/scan/ScanViewModel.kt +++ b/app/app/src/main/java/org/modg/bookshelf/ui/scan/ScanViewModel.kt @@ -18,6 +18,9 @@ import org.modg.bookshelf.data.metadata.IsbnUtils import org.modg.bookshelf.data.metadata.MetadataRepository import org.modg.bookshelf.data.prefs.SettingsStore import org.modg.bookshelf.data.repo.BookRepository +import org.modg.bookshelf.data.repo.CoverDownloader +import org.modg.bookshelf.data.repo.CoverPreload +import org.modg.bookshelf.data.repo.CoverPreloadState import org.modg.bookshelf.data.repo.LocationRepository /** @@ -31,12 +34,21 @@ class ScanViewModel( locationRepository: LocationRepository, private val metadataRepository: MetadataRepository, private val settingsStore: SettingsStore, + coverDownloader: CoverDownloader, val scannerController: ScannerController = ScannerController(), ) : ViewModel() { private val _sheetState = MutableStateFlow(ScanSheetState.Hidden) val sheetState: StateFlow = _sheetState.asStateFlow() + /** The Found sheet's cover, downloading from the moment the lookup succeeds — see [CoverPreload]. */ + private val coverPreload = CoverPreload(coverDownloader, viewModelScope) + val cover: StateFlow = coverPreload.state + + private val _saveStatus = MutableStateFlow(ScanSaveStatus()) + /** The Found sheet's Save: in flight (disables the button) and the last failure, if any. */ + val saveStatus: StateFlow = _saveStatus.asStateFlow() + private val _sessionState = MutableStateFlow(ScanSessionState()) val sessionState: StateFlow = _sessionState.asStateFlow() @@ -86,7 +98,9 @@ class ScanViewModel( try { val duplicate = DuplicateCheck.check(bookRepository.findByIsbn13(isbn13)) val result = metadataRepository.lookup(isbn13) - _sheetState.value = ScanMetadataOutcome.from(isbn13, result, duplicate) + val outcome = ScanMetadataOutcome.from(isbn13, result, duplicate) + _sheetState.value = outcome + if (outcome is ScanSheetState.Found) coverPreload.start(outcome.metadata.coverUrl) } catch (e: CancellationException) { // Normal control flow (sheet dismissed / screen left mid-lookup) — // must propagate, never be reported as a failed lookup. @@ -131,27 +145,57 @@ class ScanViewModel( viewModelScope.launch { performSave(isbn13, metadata) } } + fun retryCover() = coverPreload.retry() + /** * The actual save-from-metadata logic, split out from [save] so tests can await it * directly (as a plain suspend call) instead of racing [viewModelScope]'s launch. + * Returns whether the book was saved. + * + * Waits for the cover preload if it's still running, so the cover file is written + * with the row. If the download fails DURING that wait the save stops and the sheet + * shows the failure (Retry, or Save again to go ahead without it); if it had already + * failed when Save was tapped, the user saw "Save without cover" and meant it. + * + * Guarded like wave 8's lookup path, which this save path wasn't: [Throwable] is + * caught so a Room failure can't crash the app, [CancellationException] rethrown + * first, and a save already in flight makes a second tap a no-op. */ - internal suspend fun performSave(isbn13: String, metadata: BookMetadata) { + internal suspend fun performSave(isbn13: String, metadata: BookMetadata): Boolean { + if (_saveStatus.value.isSaving) return false + val coverAtTap = coverPreload.state.value + _saveStatus.value = ScanSaveStatus(isSaving = true) val shelfId = _selectedShelfId.value - bookRepository.createBook( - title = metadata.title ?: "Untitled", - subtitle = metadata.subtitle, - authors = metadata.authors, - isbn13 = metadata.isbn13 ?: isbn13, - isbn10 = metadata.isbn10, - publisher = metadata.publisher, - publishedDate = metadata.publishedDate, - pageCount = metadata.pageCount, - description = metadata.description, - coverSourceUrl = metadata.coverUrl, - shelfId = shelfId, - ) + try { + val cover = coverPreload.awaitSettled() + if (cover is CoverPreloadState.Failed && coverAtTap !is CoverPreloadState.Failed) { + _saveStatus.value = ScanSaveStatus() + return false + } + bookRepository.createBook( + title = metadata.title ?: "Untitled", + subtitle = metadata.subtitle, + authors = metadata.authors, + isbn13 = metadata.isbn13 ?: isbn13, + isbn10 = metadata.isbn10, + publisher = metadata.publisher, + publishedDate = metadata.publishedDate, + pageCount = metadata.pageCount, + description = metadata.description, + coverSourceUrl = metadata.coverUrl, + coverFile = (cover as? CoverPreloadState.Ready)?.file, + shelfId = shelfId, + ) + } catch (e: CancellationException) { + _saveStatus.value = ScanSaveStatus() + throw e + } catch (e: Throwable) { + _saveStatus.value = ScanSaveStatus(error = e.javaClass.simpleName) + return false + } rememberShelf(shelfId) recordSave() + return true } /** Save from the manual-entry form shown when metadata lookup misses (SPEC: pre-filled with the scanned ISBN). */ @@ -172,9 +216,20 @@ class ScanViewModel( recordSave() } - /** "Not shelved" (null) must never overwrite the memory — it isn't a shelf. */ + /** + * "Not shelved" (null) must never overwrite the memory — it isn't a shelf. Called only + * AFTER the book is written, and a failure here is swallowed: reporting "couldn't save" + * for a book that WAS saved would invite a retry that creates a duplicate. + */ private suspend fun rememberShelf(shelfId: String?) { - if (shelfId != null) settingsStore.setLastShelfId(shelfId) + if (shelfId == null) return + try { + settingsStore.setLastShelfId(shelfId) + } catch (e: CancellationException) { + throw e + } catch (e: Throwable) { + // The book is saved; losing the "recent shelf" shortcut is not worth an error. + } } private fun recordSave() { @@ -188,9 +243,15 @@ class ScanViewModel( fun dismissSheet() { _sheetState.value = ScanSheetState.Hidden + coverPreload.discard() // saved (createBook copied it) or skipped — either way, not needed + _saveStatus.value = ScanSaveStatus() scannerController.resetDebounce() } + override fun onCleared() { + coverPreload.discard() + } + private companion object { /** How long a rejected-barcode message stays on screen before it auto-clears. */ const val REJECTED_MESSAGE_MILLIS = 3000L diff --git a/app/app/src/test/java/org/modg/bookshelf/data/repo/BookRepositoryCoverTest.kt b/app/app/src/test/java/org/modg/bookshelf/data/repo/BookRepositoryCoverTest.kt new file mode 100644 index 0000000..8777cd2 --- /dev/null +++ b/app/app/src/test/java/org/modg/bookshelf/data/repo/BookRepositoryCoverTest.kt @@ -0,0 +1,97 @@ +package org.modg.bookshelf.data.repo + +import androidx.room.Room +import androidx.test.core.app.ApplicationProvider +import java.io.File +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.test.runTest +import org.junit.After +import org.junit.Assert.assertArrayEquals +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Assert.fail +import org.junit.Before +import org.junit.Rule +import org.junit.Test +import org.junit.rules.TemporaryFolder +import org.junit.runner.RunWith +import org.modg.bookshelf.data.local.BookDao +import org.modg.bookshelf.data.local.BookEntity +import org.modg.bookshelf.data.local.BookshelfDatabase +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config + +/** + * [BookRepository.createBook] no longer downloads anything: it takes an already-preloaded + * cover file and copies it into app storage for SyncEngine to upload. + */ +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [34]) +class BookRepositoryCoverTest { + + // Not context.cacheDir: a file written there in @Before was observed missing by the test body. + @get:Rule + val tmp = TemporaryFolder() + + private lateinit var db: BookshelfDatabase + private lateinit var context: android.content.Context + private lateinit var preloaded: File + + @Before + fun setUp() { + context = ApplicationProvider.getApplicationContext() + db = Room.inMemoryDatabaseBuilder(context, BookshelfDatabase::class.java) + .allowMainThreadQueries() + .build() + preloaded = tmp.newFile("preloaded-cover").apply { writeBytes(byteArrayOf(1, 2, 3)) } + } + + @After + fun tearDown() { + db.close() + } + + @Test + fun `a cover file is copied into app storage and recorded as localCoverPath`() = runTest { + val repository = BookRepository(db.bookDao(), context) + + val id = repository.createBook(title = "Dune", coverSourceUrl = "https://x/c.jpg", coverFile = preloaded) + + val saved = checkNotNull(repository.getById(id)) + val local = File(checkNotNull(saved.localCoverPath)) + assertEquals(File(context.filesDir, "covers/$id.jpg"), local) + assertArrayEquals(byteArrayOf(1, 2, 3), local.readBytes()) + assertEquals("https://x/c.jpg", saved.coverSourceUrl) + assertTrue("a copy: the caller still owns the preload", preloaded.exists()) + } + + @Test + fun `no cover file means no localCoverPath and no download attempt`() = runTest { + val repository = BookRepository(db.bookDao(), context) + + val id = repository.createBook(title = "Dune", coverSourceUrl = "https://x/c.jpg") + + assertNull(checkNotNull(repository.getById(id)).localCoverPath) + } + + @Test + fun `a failed write deletes the copied cover, leaving nothing unowned in app storage`() = runTest { + val repository = BookRepository(ThrowingUpsertBookDao(db.bookDao()), context) + + try { + repository.createBook(title = "Dune", coverFile = preloaded) + fail("expected the write failure to propagate") + } catch (e: IllegalStateException) { + // expected + } + + assertEquals(0, File(context.filesDir, "covers").listFiles()?.size ?: 0) + assertTrue(preloaded.exists()) + assertTrue(db.bookDao().observeAll().first().isEmpty()) + } + + private class ThrowingUpsertBookDao(private val delegate: BookDao) : BookDao by delegate { + override suspend fun upsert(book: BookEntity) = throw IllegalStateException("disk full") + } +} diff --git a/app/app/src/test/java/org/modg/bookshelf/data/repo/CoverPreloadTest.kt b/app/app/src/test/java/org/modg/bookshelf/data/repo/CoverPreloadTest.kt new file mode 100644 index 0000000..9972a68 --- /dev/null +++ b/app/app/src/test/java/org/modg/bookshelf/data/repo/CoverPreloadTest.kt @@ -0,0 +1,206 @@ +package org.modg.bookshelf.data.repo + +import java.io.File +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.SupervisorJob +import kotlinx.coroutines.cancel +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.runBlocking +import kotlinx.coroutines.withTimeout +import org.junit.After +import org.junit.Assert.assertArrayEquals +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Assert.fail +import org.junit.Before +import org.junit.Rule +import org.junit.Test +import org.junit.rules.TemporaryFolder + +/** + * The cover preload that replaced save-time downloading: the file must be ready (or the + * failure visible) before Save, and no file may outlive the sheet that asked for it. + */ +class CoverPreloadTest { + + @get:Rule + val tmp = TemporaryFolder() + + private val host = FakeCoverHost() + private lateinit var preloadDir: File + private val scope = CoroutineScope(SupervisorJob() + Dispatchers.Unconfined) + + @Before + fun setUp() { + preloadDir = File(tmp.root, "cover-preload") + } + + @After + fun tearDown() { + host.release() + scope.cancel() + } + + private val url = "https://covers.example.org/b/id/42-M.jpg" + + // --- CoverDownloader --- + + @Test + fun `a successful download writes the bytes into the preload dir`() = runBlocking { + host.serve(url, byteArrayOf(9, 8, 7)) + + val result = host.downloader(preloadDir).download(url) + + val file = (result as CoverFetch.Ready).file + assertEquals(preloadDir, file.parentFile) + assertArrayEquals(byteArrayOf(9, 8, 7), file.readBytes()) + } + + @Test + fun `an http error is Failed with the status in the reason, and leaves no file`() = runBlocking { + host.fail(url, 503) + + val result = host.downloader(preloadDir).download(url) + + assertTrue(result is CoverFetch.Failed) + assertTrue((result as CoverFetch.Failed).reason.contains("503")) + assertEquals(0, preloadDir.listFiles()?.size ?: 0) + } + + @Test + fun `a transport failure is Failed, not a crash, and leaves no file`() = runBlocking { + host.reset(url) + + val result = host.downloader(preloadDir).download(url) + + assertTrue(result is CoverFetch.Failed) + assertEquals(0, preloadDir.listFiles()?.size ?: 0) + } + + @Test + fun `sweepStale deletes only files older than the cutoff`() { + preloadDir.mkdirs() + val old = File(preloadDir, "old").apply { writeBytes(byteArrayOf(1)); setLastModified(1_000) } + val fresh = File(preloadDir, "fresh").apply { writeBytes(byteArrayOf(1)); setLastModified(5_000) } + + host.downloader(preloadDir).sweepStale(olderThanMillis = 3_000) + + assertFalse(old.exists()) + assertTrue(fresh.exists()) + } + + // --- CoverPreload --- + + @Test + fun `start downloads, and starting the same url again does not download twice`() = runBlocking { + host.serve(url) + val preload = CoverPreload(host.downloader(preloadDir), scope) + + preload.start(url) + preload.start(url) + + val ready = preload.awaitSettled() as CoverPreloadState.Ready + assertTrue(ready.file.exists()) + assertEquals(1, host.requestCount(url)) + } + + @Test + fun `a different url replaces the preload and deletes the old file`() = runBlocking { + val other = "https://covers.example.org/b/id/43-M.jpg" + host.serve(url) + host.serve(other) + val preload = CoverPreload(host.downloader(preloadDir), scope) + + preload.start(url) + val first = (preload.awaitSettled() as CoverPreloadState.Ready).file + preload.start(other) + val second = preload.awaitSettled() as CoverPreloadState.Ready + + assertFalse(first.exists()) + assertEquals(other, second.url) + assertTrue(second.file.exists()) + } + + @Test + fun `discard deletes the downloaded file`() = runBlocking { + host.serve(url) + val preload = CoverPreload(host.downloader(preloadDir), scope) + preload.start(url) + val file = (preload.awaitSettled() as CoverPreloadState.Ready).file + + preload.discard() + + assertFalse(file.exists()) + assertEquals(CoverPreloadState.None, preload.state.value) + } + + @Test + fun `a failed preload can be retried`() = runBlocking { + host.reset(url) + val preload = CoverPreload(host.downloader(preloadDir), scope) + preload.start(url) + assertTrue(preload.awaitSettled() is CoverPreloadState.Failed) + + host.serve(url) + preload.retry() + + assertTrue(preload.awaitSettled() is CoverPreloadState.Ready) + } + + @Test + fun `awaitSettled waits for a download still in flight`() = runBlocking { + host.serve(url) + host.hold() + val preload = CoverPreload(host.downloader(preloadDir, Dispatchers.IO), scope) + preload.start(url) + assertTrue(preload.state.value is CoverPreloadState.Loading) + + host.release() + val settled = withTimeout(10_000) { preload.awaitSettled() } + + assertTrue(settled is CoverPreloadState.Ready) + } + + @Test + fun `a download discarded while in flight leaves no file behind`() = runBlocking { + host.serve(url) + host.hold() + val preload = CoverPreload(host.downloader(preloadDir, Dispatchers.IO), scope) + preload.start(url) + pollUntil { host.requestCount(url) == 1 } // blocked inside the HTTP call + + preload.discard() + host.release() + // The blocked call returns on its IO thread after cancellation and must clean up after itself. + pollUntil { (preloadDir.listFiles()?.size ?: 0) == 0 } + Thread.sleep(200) // and nothing lands afterwards + + assertEquals(CoverPreloadState.None, preload.state.value) + assertEquals(0, preloadDir.listFiles()?.size ?: 0) + } + + /** Bounded: a condition that never comes true fails the test instead of hanging the build. */ + private fun pollUntil(condition: () -> Boolean) { + repeat(500) { + if (condition()) return + Thread.sleep(10) + } + fail("condition not met within 5s") + } + + @Test + fun `a ready file that has vanished is downloaded again rather than handed to save`() = runBlocking { + host.serve(url) + val preload = CoverPreload(host.downloader(preloadDir), scope) + preload.start(url) + (preload.awaitSettled() as CoverPreloadState.Ready).file.delete() // e.g. Android cleared the cache dir + + val settled = preload.awaitSettled() as CoverPreloadState.Ready + + assertTrue(settled.file.exists()) + assertEquals(2, host.requestCount(url)) + assertEquals(url, preload.state.first().let { (it as CoverPreloadState.Ready).url }) + } +} diff --git a/app/app/src/test/java/org/modg/bookshelf/data/repo/FakeCoverHost.kt b/app/app/src/test/java/org/modg/bookshelf/data/repo/FakeCoverHost.kt new file mode 100644 index 0000000..9570c27 --- /dev/null +++ b/app/app/src/test/java/org/modg/bookshelf/data/repo/FakeCoverHost.kt @@ -0,0 +1,68 @@ +package org.modg.bookshelf.data.repo + +import java.io.File +import java.io.IOException +import java.util.concurrent.ConcurrentHashMap +import java.util.concurrent.CountDownLatch +import java.util.concurrent.TimeUnit +import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.Dispatchers +import okhttp3.MediaType.Companion.toMediaType +import okhttp3.OkHttpClient +import okhttp3.Protocol +import okhttp3.Response +import okhttp3.ResponseBody.Companion.toResponseBody + +/** + * An image host for cover-preload tests, answered by an interceptor (no sockets). Each URL + * gets bytes, an HTTP error code, or a thrown IOException; anything unregistered is a 404. + * [hold] makes downloads block until [release], to test Save waiting on a preload. + */ +class FakeCoverHost { + private sealed interface Answer { + class Bytes(val bytes: ByteArray) : Answer + class Code(val code: Int) : Answer + class Throw(val e: IOException) : Answer + } + + private val answers = ConcurrentHashMap() + private val requests = ConcurrentHashMap() + @Volatile private var gate: CountDownLatch? = null + + fun serve(url: String, bytes: ByteArray = byteArrayOf(1, 2, 3, 4)) { answers[url] = Answer.Bytes(bytes) } + fun fail(url: String, code: Int) { answers[url] = Answer.Code(code) } + fun reset(url: String, message: String = "connection reset") { answers[url] = Answer.Throw(IOException(message)) } + fun requestCount(url: String): Int = requests[url] ?: 0 + + fun hold() { gate = CountDownLatch(1) } + fun release() { gate?.countDown() } + + val client: OkHttpClient = OkHttpClient.Builder().addInterceptor { chain -> + val request = chain.request() + val url = request.url.toString() + requests.merge(url, 1, Int::plus) + gate?.await(10, TimeUnit.SECONDS) + when (val answer = answers[url]) { + is Answer.Bytes -> response(request, 200, answer.bytes) + is Answer.Code -> response(request, answer.code, ByteArray(0)) + is Answer.Throw -> throw answer.e + null -> response(request, 404, ByteArray(0)) + } + }.build() + + /** + * Downloads run on the calling thread by default, so no IO-thread download can resume + * into a later test after this one's Dispatchers.resetMain. Tests that [hold] must pass + * a real dispatcher instead (a held download would block the test thread itself). + */ + fun downloader(preloadDir: File, dispatcher: CoroutineDispatcher = Dispatchers.Unconfined): CoverDownloader = + CoverDownloader(client, preloadDir, dispatcher) + + private fun response(request: okhttp3.Request, code: Int, bytes: ByteArray) = Response.Builder() + .request(request) + .protocol(Protocol.HTTP_1_1) + .code(code) + .message("fake") + .body(bytes.toResponseBody("image/jpeg".toMediaType())) + .build() +} diff --git a/app/app/src/test/java/org/modg/bookshelf/ui/add/AddBookViewModelTest.kt b/app/app/src/test/java/org/modg/bookshelf/ui/add/AddBookViewModelTest.kt index 32a5a4f..3458de8 100644 --- a/app/app/src/test/java/org/modg/bookshelf/ui/add/AddBookViewModelTest.kt +++ b/app/app/src/test/java/org/modg/bookshelf/ui/add/AddBookViewModelTest.kt @@ -24,6 +24,8 @@ import org.modg.bookshelf.data.local.BookEntity import org.modg.bookshelf.data.local.BookshelfDatabase import org.modg.bookshelf.data.prefs.SettingsStore import org.modg.bookshelf.data.repo.BookRepository +import org.modg.bookshelf.data.repo.CoverPreloadState +import org.modg.bookshelf.data.repo.FakeCoverHost import org.modg.bookshelf.data.repo.LocationRepository import org.modg.bookshelf.data.repo.decodeAuthors import org.modg.bookshelf.ui.scan.DuplicateStatus @@ -42,6 +44,7 @@ class AddBookViewModelTest { private lateinit var viewModel: AddBookViewModel private lateinit var shelfId: String private lateinit var context: android.content.Context + private val coverHost = FakeCoverHost() @Before fun setUp() = runTest { @@ -64,6 +67,7 @@ class AddBookViewModelTest { shelfId = locationRepository.createShelf(bookcaseId, label = "Top shelf") viewModel = AddBookViewModel( + coverDownloader = coverHost.downloader(java.io.File(context.cacheDir, "cover-preload")), bookRepository = bookRepository, locationRepository = locationRepository, settingsStore = settingsStore, @@ -267,6 +271,7 @@ class AddBookViewModelTest { settingsStore.setLastShelfId(shelfId) val vm = AddBookViewModel( + coverDownloader = coverHost.downloader(java.io.File(context.cacheDir, "cover-preload")), bookRepository = bookRepository, locationRepository = locationRepository, settingsStore = settingsStore, @@ -276,7 +281,95 @@ class AddBookViewModelTest { assertEquals(shelfId, preSelected.selectedShelfId) } + // --- review fixes (2026-09-16) --- + + private val coverUrl = "https://covers.example.org/b/id/42-M.jpg" + + private fun viewModelWithDraft(draft: BookDraft, dispatcher: kotlinx.coroutines.CoroutineDispatcher = Dispatchers.Unconfined) = + AddBookViewModel( + coverDownloader = coverHost.downloader(java.io.File(context.cacheDir, "cover-preload"), dispatcher), + bookRepository = bookRepository, + locationRepository = locationRepository, + settingsStore = settingsStore, + initialDraft = draft, + ) + + @Test + fun `a cover carried in from a search hit is preloaded and saved as the book's cover file`() = runTest { + coverHost.serve(coverUrl, byteArrayOf(7, 7)) + val vm = viewModelWithDraft(BookDraft(title = "Dune", coverSourceUrl = coverUrl)) + val preloaded = (vm.uiState.first { it.cover is CoverPreloadState.Ready }.cover as CoverPreloadState.Ready).file + + val bookId = checkNotNull(vm.performSave(clearAfterSave = false)) + + val saved = checkNotNull(bookRepository.getById(bookId)) + assertTrue(java.io.File(checkNotNull(saved.localCoverPath)).readBytes().contentEquals(byteArrayOf(7, 7))) + assertTrue("the preload is deleted once createBook has copied it", !preloaded.exists()) + } + + @Test + fun `save waits for a cover download that is still running`() = runTest { + coverHost.serve(coverUrl) + coverHost.hold() + val vm = viewModelWithDraft(BookDraft(title = "Dune", coverSourceUrl = coverUrl), Dispatchers.IO) + Thread { Thread.sleep(200); coverHost.release() }.start() + + val bookId = checkNotNull(vm.performSave(clearAfterSave = false)) + + assertTrue(checkNotNull(bookRepository.getById(bookId)).localCoverPath != null) + } + + @Test + fun `a cover that already failed when Save was tapped does not block the save`() = runTest { + coverHost.reset(coverUrl) + val vm = viewModelWithDraft(BookDraft(title = "Dune", coverSourceUrl = coverUrl)) + vm.uiState.first { it.cover is CoverPreloadState.Failed } + + val bookId = checkNotNull(vm.performSave(clearAfterSave = false)) + + val saved = checkNotNull(bookRepository.getById(bookId)) + assertNull(saved.localCoverPath) + assertEquals(coverUrl, saved.coverSourceUrl) + } + + @Test + fun `a cover that fails while Save waits stops the save and keeps the form`() = runTest { + coverHost.fail(coverUrl, 404) + coverHost.hold() + val vm = viewModelWithDraft(BookDraft(title = "Dune", coverSourceUrl = coverUrl), Dispatchers.IO) + Thread { Thread.sleep(200); coverHost.release() }.start() + + assertNull(vm.performSave(clearAfterSave = false)) + + val state = vm.uiState.first { it.cover is CoverPreloadState.Failed } + assertEquals("Dune", state.draft.title) + assertTrue(!state.isSaving) + assertTrue(bookRepository.observeAll().first().isEmpty()) + } + + @Test + fun `a failing duplicate lookup shows no warning instead of crashing`() = runTest { + val vm = AddBookViewModel( + coverDownloader = coverHost.downloader(java.io.File(context.cacheDir, "cover-preload")), + bookRepository = BookRepository(ThrowingFindByIsbnBookDao(db.bookDao(), IllegalStateException("disk full")), context), + locationRepository = locationRepository, + settingsStore = settingsStore, + ) + + vm.onIsbnChanged("9780441013593") // must not throw out of the launched check + + assertEquals(DuplicateStatus.New, vm.uiState.first().duplicate) + } + + private class ThrowingFindByIsbnBookDao( + private val delegate: BookDao, + private val toThrow: Throwable, + ) : BookDao by delegate { + override suspend fun findByIsbn13(isbn13: String): BookEntity? = throw toThrow + } + private fun viewModelWithThrowingCreateBook(toThrow: Throwable): AddBookViewModel = AddBookViewModel( + coverDownloader = coverHost.downloader(java.io.File(context.cacheDir, "cover-preload")), bookRepository = BookRepository(ThrowingUpsertBookDao(db.bookDao(), toThrow), context), locationRepository = locationRepository, settingsStore = settingsStore, diff --git a/app/app/src/test/java/org/modg/bookshelf/ui/library/LibraryViewModelTest.kt b/app/app/src/test/java/org/modg/bookshelf/ui/library/LibraryViewModelTest.kt index f6ac234..c41df80 100644 --- a/app/app/src/test/java/org/modg/bookshelf/ui/library/LibraryViewModelTest.kt +++ b/app/app/src/test/java/org/modg/bookshelf/ui/library/LibraryViewModelTest.kt @@ -34,6 +34,9 @@ import org.modg.bookshelf.data.metadata.MetadataRepository import org.modg.bookshelf.data.metadata.OnlineBook import org.modg.bookshelf.data.prefs.SettingsStore import org.modg.bookshelf.data.repo.BookRepository +import org.modg.bookshelf.data.repo.CoverDownloader +import org.modg.bookshelf.data.repo.CoverPreloadState +import org.modg.bookshelf.data.repo.FakeCoverHost import org.modg.bookshelf.data.repo.LocationRepository import org.modg.bookshelf.data.repo.SyncEngine import org.modg.bookshelf.data.repo.decodeAuthors @@ -59,6 +62,7 @@ class LibraryViewModelTest { private lateinit var locationRepository: LocationRepository private lateinit var bookRepository: BookRepository private lateinit var context: android.content.Context + private val coverHost = FakeCoverHost() @Before fun setUp() { @@ -78,7 +82,12 @@ class LibraryViewModelTest { Dispatchers.resetMain() } - private fun viewModel(searchHttpClient: OkHttpClient = OkHttpClient()): LibraryViewModel = LibraryViewModel( + private fun viewModel( + searchHttpClient: OkHttpClient = OkHttpClient(), + coverDownloader: CoverDownloader = coverHost.downloader(java.io.File(context.cacheDir, "cover-preload")), + ): LibraryViewModel = LibraryViewModel( + coverDownloader = coverDownloader, + computeDispatcher = Dispatchers.Main, bookRepository = bookRepository, locationRepository = locationRepository, syncEngine = SyncEngine(ApiProvider { null }, db.bookDao(), db.bookcaseDao(), db.shelfDao(), settingsStore), @@ -200,6 +209,7 @@ class LibraryViewModelTest { coverUrl = "https://example.com/cover.jpg", isbn13 = "9780441013593", ) + coverHost.serve("https://example.com/cover.jpg") vm.onOnlineResultTapped(onlineBook) vm.onSaveSheetShelfSelected(shelfId) @@ -241,6 +251,8 @@ class LibraryViewModelTest { fun `a throwing save keeps the sheet open with an error and does not throw`() = runTest { val throwingBookRepository = BookRepository(ThrowingUpsertBookDao(db.bookDao(), IllegalStateException("disk full")), context) val vm = LibraryViewModel( + coverDownloader = coverHost.downloader(java.io.File(context.cacheDir, "cover-preload")), + computeDispatcher = Dispatchers.Main, bookRepository = throwingBookRepository, locationRepository = locationRepository, syncEngine = SyncEngine(ApiProvider { null }, db.bookDao(), db.bookcaseDao(), db.shelfDao(), settingsStore), @@ -263,6 +275,8 @@ class LibraryViewModelTest { @Test fun `a CancellationException during save propagates rather than being swallowed`() = runTest { val vm = LibraryViewModel( + coverDownloader = coverHost.downloader(java.io.File(context.cacheDir, "cover-preload")), + computeDispatcher = Dispatchers.Main, bookRepository = BookRepository(ThrowingUpsertBookDao(db.bookDao(), CancellationException("scope cancelled")), context), locationRepository = locationRepository, syncEngine = SyncEngine(ApiProvider { null }, db.bookDao(), db.bookcaseDao(), db.shelfDao(), settingsStore), @@ -294,6 +308,94 @@ class LibraryViewModelTest { assertEquals(1, bookRepository.observeAll().first().size) } + // --- review fixes (2026-09-16) --- + + @Test + fun `an owned edition reached only through an online ISBN is shown in inLibrary, not dropped`() = runTest { + // Its own text doesn't match the query at all; only the OL work's ISBN list ties it in. + bookRepository.createBook(title = "Warrior of the Hittites", isbn13 = "9781883937386") + val vm = viewModel(fakeSearchClient(olBody = olHit("Hittite Warrior", "Joanne Williamson", isbn = "1883937388"), gbBody = gbEmpty())) + vm.uiState.first { it.books.isNotEmpty() } // the owned book has loaded from Room before the search merges against it + vm.onQueryChange("hittite williamson") + vm.submitOnlineSearch() + + val done = vm.onlineSearch.first { it is OnlineSearchState.Done } as OnlineSearchState.Done + + assertEquals(listOf("Warrior of the Hittites"), done.inLibrary.map { it.title }) + assertTrue(done.online.isEmpty()) + } + + @Test + fun `the shelf filter narrows inLibrary and counts what it hides instead of dropping it`() = runTest { + val bookcaseId = locationRepository.createBookcase(name = "Living Room") + val top = locationRepository.createShelf(bookcaseId, label = "Top") + val bottom = locationRepository.createShelf(bookcaseId, label = "Bottom") + bookRepository.createBook(title = "Dune", authors = listOf("Frank Herbert"), shelfId = top) + bookRepository.createBook(title = "Dune Messiah", authors = listOf("Frank Herbert"), shelfId = bottom) + val vm = viewModel(fakeSearchClient(olBody = olEmpty(), gbBody = gbEmpty())) + vm.onFilterChange(LibraryFilter.Shelf(top)) + vm.onQueryChange("dune herbert") + vm.submitOnlineSearch() + + val done = vm.onlineSearch.first { it is OnlineSearchState.Done && it.inLibraryOutsideFilter > 0 } as OnlineSearchState.Done + + assertEquals(listOf("Dune"), done.inLibrary.map { it.title }) + assertEquals(1, done.inLibraryOutsideFilter) + } + + @Test + fun `Edit details closes the sheet, so returning to the library can't save the same book twice`() = runTest { + val cover = "https://covers.example.org/dune.jpg" + coverHost.serve(cover) + val vm = viewModel() + val book = OnlineBook(title = "Dune", coverUrl = cover) + vm.onOnlineResultTapped(book) + val preloaded = (vm.cover.first { it is CoverPreloadState.Ready } as CoverPreloadState.Ready).file + + val taken = vm.takeForEditing() + + assertEquals(book, taken?.first) + assertEquals(OnlineSaveSheetState.Hidden, vm.saveSheet.value) + assertTrue("the sheet's preloaded cover must not be left behind", !preloaded.exists()) + } + + @Test + fun `save waits for a cover still downloading and stores the cover file with the book`() = runTest { + val cover = "https://covers.example.org/dune.jpg" + coverHost.serve(cover, byteArrayOf(4, 2)) + coverHost.hold() + val vm = viewModel(coverDownloader = coverHost.downloader(java.io.File(context.cacheDir, "cover-preload"), Dispatchers.IO)) + vm.onOnlineResultTapped(OnlineBook(title = "Dune", coverUrl = cover)) + assertTrue(vm.cover.value is CoverPreloadState.Loading) + Thread { Thread.sleep(200); coverHost.release() }.start() + + val bookId = checkNotNull(vm.performSaveOnlineResult()) + + val saved = checkNotNull(bookRepository.getById(bookId)) + val localCover = java.io.File(checkNotNull(saved.localCoverPath)) + assertTrue(localCover.readBytes().contentEquals(byteArrayOf(4, 2))) + assertEquals(CoverPreloadState.None, vm.cover.value) + } + + @Test + fun `a cover that fails while save waits stops the save, and saving again goes ahead without it`() = runTest { + val cover = "https://covers.example.org/dune.jpg" + coverHost.fail(cover, 503) + coverHost.hold() + val vm = viewModel(coverDownloader = coverHost.downloader(java.io.File(context.cacheDir, "cover-preload"), Dispatchers.IO)) + vm.onOnlineResultTapped(OnlineBook(title = "Dune", coverUrl = cover)) + Thread { Thread.sleep(200); coverHost.release() }.start() + + assertNull("the user hasn't seen the failure yet — don't save past it", vm.performSaveOnlineResult()) + assertTrue(vm.cover.value is CoverPreloadState.Failed) + assertTrue(bookRepository.observeAll().first().isEmpty()) + + val bookId = checkNotNull(vm.performSaveOnlineResult()) // tapped "Save without cover" + val saved = checkNotNull(bookRepository.getById(bookId)) + assertNull(saved.localCoverPath) + assertEquals(cover, saved.coverSourceUrl) + } + private class ThrowingUpsertBookDao( private val delegate: BookDao, private val toThrow: Throwable, diff --git a/app/app/src/test/java/org/modg/bookshelf/ui/scan/ScanViewModelTest.kt b/app/app/src/test/java/org/modg/bookshelf/ui/scan/ScanViewModelTest.kt index 786f8ad..a4f41cd 100644 --- a/app/app/src/test/java/org/modg/bookshelf/ui/scan/ScanViewModelTest.kt +++ b/app/app/src/test/java/org/modg/bookshelf/ui/scan/ScanViewModelTest.kt @@ -3,7 +3,12 @@ package org.modg.bookshelf.ui.scan import androidx.room.Room import androidx.test.core.app.ApplicationProvider import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.flow.first +import kotlinx.coroutines.test.UnconfinedTestDispatcher +import kotlinx.coroutines.test.resetMain +import kotlinx.coroutines.test.setMain import kotlinx.coroutines.test.runTest import kotlinx.serialization.json.Json import okhttp3.OkHttpClient @@ -21,6 +26,10 @@ import org.modg.bookshelf.data.metadata.BookMetadata import org.modg.bookshelf.data.metadata.MetadataRepository import org.modg.bookshelf.data.prefs.SettingsStore import org.modg.bookshelf.data.repo.BookRepository +import org.modg.bookshelf.data.repo.CoverPreloadState +import org.modg.bookshelf.data.repo.FakeCoverHost +import kotlinx.coroutines.async +import okhttp3.ResponseBody.Companion.toResponseBody import org.modg.bookshelf.data.repo.LocationRepository import org.robolectric.RobolectricTestRunner import org.robolectric.annotation.Config @@ -31,6 +40,7 @@ import org.robolectric.annotation.Config * so it survives to the next scanning session — except "Not shelved" (null), * which must never overwrite what's already remembered. */ +@OptIn(ExperimentalCoroutinesApi::class) @RunWith(RobolectricTestRunner::class) @Config(sdk = [34]) class ScanViewModelTest { @@ -41,9 +51,13 @@ class ScanViewModelTest { private lateinit var locationRepository: LocationRepository private lateinit var shelfId: String private lateinit var context: android.content.Context + private val coverHost = FakeCoverHost() @Before fun setUp() = runTest { + // The cover preload launches on viewModelScope (Dispatchers.Main); unconfined so it + // runs without idling Robolectric's main looper. + Dispatchers.setMain(UnconfinedTestDispatcher()) context = ApplicationProvider.getApplicationContext() db = Room.inMemoryDatabaseBuilder(context, BookshelfDatabase::class.java) .allowMainThreadQueries() @@ -54,6 +68,7 @@ class ScanViewModelTest { shelfId = locationRepository.createShelf(bookcaseId, label = "Top shelf") viewModel = ScanViewModel( + coverDownloader = coverHost.downloader(java.io.File(context.cacheDir, "cover-preload")), bookRepository = BookRepository(db.bookDao(), context), locationRepository = locationRepository, metadataRepository = MetadataRepository(OkHttpClient(), Json { ignoreUnknownKeys = true }), @@ -64,6 +79,7 @@ class ScanViewModelTest { @After fun tearDown() { db.close() + Dispatchers.resetMain() } @Test @@ -131,7 +147,126 @@ class ScanViewModelTest { } } + // --- review fixes (2026-09-16): cover preload + a guarded save --- + + @Test + fun `a successful lookup preloads its cover, and save stores it as the book's cover file`() = runTest { + val olFixture = fixture("openlibrary_success.json") + val coverUrl = "https://covers.openlibrary.org/b/id/675832-L.jpg" // the fixture's reported cover.large + coverHost.serve(coverUrl, byteArrayOf(5, 5, 5)) + val vm = ScanViewModel( + bookRepository = BookRepository(db.bookDao(), context), + locationRepository = locationRepository, + metadataRepository = MetadataRepository(fakeMetadataClient(olFixture), Json { ignoreUnknownKeys = true }), + settingsStore = settingsStore, + coverDownloader = coverHost.downloader(java.io.File(context.cacheDir, "cover-preload")), + ) + + vm.runLookup("9780201558029") + val found = vm.sheetState.value as ScanSheetState.Found + val preloaded = (vm.cover.first { it is CoverPreloadState.Ready } as CoverPreloadState.Ready).file + assertTrue(vm.performSave(found.isbn13, found.metadata)) + + val saved = db.bookDao().findByIsbn13("9780201558029") + checkNotNull(saved) + assertTrue(java.io.File(checkNotNull(saved.localCoverPath)).readBytes().contentEquals(byteArrayOf(5, 5, 5))) + assertTrue("the preload is discarded once saved", !preloaded.exists()) + assertEquals(ScanSheetState.Hidden, vm.sheetState.value) + } + + @Test + fun `a failing save is reported on the sheet instead of crashing`() = runTest { + val vm = viewModelWithThrowingUpsert(IllegalStateException("disk full")) + + val saved = vm.performSave("9780765326355", BookMetadata(title = "The Way of Kings")) // must not throw + + assertTrue(!saved) + assertEquals(ScanSaveStatus(isSaving = false, error = "IllegalStateException"), vm.saveStatus.value) + } + + @Test + fun `a CancellationException during save propagates`() = runTest { + val vm = viewModelWithThrowingUpsert(CancellationException("scope cancelled")) + try { + vm.performSave("9780765326355", BookMetadata(title = "The Way of Kings")) + fail("expected CancellationException to propagate") + } catch (e: CancellationException) { + // expected + } + } + + @Test + fun `a second tap while a save is in flight does not create a second book`() = runTest { + // Hold the first save inside its database write, so the second tap deterministically + // lands while it is in flight (not after it finished and closed the sheet). + val gate = kotlinx.coroutines.CompletableDeferred() + val vm = ScanViewModel( + bookRepository = BookRepository(GatedUpsertBookDao(db.bookDao(), gate), context), + locationRepository = locationRepository, + metadataRepository = MetadataRepository(OkHttpClient(), Json { ignoreUnknownKeys = true }), + settingsStore = settingsStore, + coverDownloader = coverHost.downloader(java.io.File(context.cacheDir, "cover-preload")), + ) + val first = kotlinx.coroutines.CoroutineScope(Dispatchers.Unconfined).async { + vm.performSave("9780765326355", BookMetadata(title = "The Way of Kings")) + } + assertTrue(vm.saveStatus.value.isSaving) + + val secondSaved = vm.performSave("9780765326355", BookMetadata(title = "The Way of Kings")) + gate.complete(Unit) + + assertTrue(!secondSaved) + assertTrue(first.await()) + assertEquals(1, db.bookDao().observeAll().first().size) + } + + private class GatedUpsertBookDao( + private val delegate: BookDao, + private val gate: kotlinx.coroutines.CompletableDeferred, + ) : BookDao by delegate { + override suspend fun upsert(book: BookEntity) { + gate.await() + delegate.upsert(book) + } + } + + private fun viewModelWithThrowingUpsert(toThrow: Throwable): ScanViewModel = ScanViewModel( + bookRepository = BookRepository(ThrowingUpsertBookDao(db.bookDao(), toThrow), context), + locationRepository = locationRepository, + metadataRepository = MetadataRepository(OkHttpClient(), Json { ignoreUnknownKeys = true }), + settingsStore = settingsStore, + coverDownloader = coverHost.downloader(java.io.File(context.cacheDir, "cover-preload")), + ) + + private class ThrowingUpsertBookDao( + private val delegate: BookDao, + private val toThrow: Throwable, + ) : BookDao by delegate { + override suspend fun upsert(book: BookEntity) = throw toThrow + } + + /** Open Library answers with [olBody]; Google Books has nothing. No sockets. */ + private fun fakeMetadataClient(olBody: String): OkHttpClient = OkHttpClient.Builder() + .addInterceptor { chain -> + val request = chain.request() + val (code, body) = if (request.url.host.contains("openlibrary")) 200 to olBody else 200 to """{"totalItems": 0}""" + okhttp3.Response.Builder() + .request(request) + .protocol(okhttp3.Protocol.HTTP_1_1) + .code(code) + .message("") + .body(body.toResponseBody(null)) + .build() + } + .build() + + private fun fixture(name: String): String = + checkNotNull(javaClass.classLoader?.getResourceAsStream("fixtures/$name")) { "missing fixture $name" } + .bufferedReader() + .readText() + private fun viewModelWithThrowingBookDao(toThrow: Throwable): ScanViewModel = ScanViewModel( + coverDownloader = coverHost.downloader(java.io.File(context.cacheDir, "cover-preload")), bookRepository = BookRepository(ThrowingFindByIsbnBookDao(db.bookDao(), toThrow), context), locationRepository = locationRepository, metadataRepository = MetadataRepository(OkHttpClient(), Json { ignoreUnknownKeys = true }), diff --git a/app/app/src/test/java/org/modg/bookshelf/ui/screens/LibraryScreenPaparazziTest.kt b/app/app/src/test/java/org/modg/bookshelf/ui/screens/LibraryScreenPaparazziTest.kt index 9d6b5e6..e52d144 100644 --- a/app/app/src/test/java/org/modg/bookshelf/ui/screens/LibraryScreenPaparazziTest.kt +++ b/app/app/src/test/java/org/modg/bookshelf/ui/screens/LibraryScreenPaparazziTest.kt @@ -32,6 +32,7 @@ import org.junit.Rule import org.junit.Test import org.modg.bookshelf.data.local.BookEntity import org.modg.bookshelf.data.metadata.OnlineBook +import org.modg.bookshelf.data.repo.CoverPreloadState import org.modg.bookshelf.data.metadata.SearchSource import org.modg.bookshelf.ui.components.BookshelfScaffold import org.modg.bookshelf.ui.components.EmptyState @@ -74,12 +75,15 @@ class LibraryScreenPaparazziTest { fun librarySearchBothSectionsLight() = snapshotBoth("library-search-both-sections") { SearchShell( localMatches = listOf(ScreenFixtures.books[1]), // "The Hobbit" -- already owned + // Owned "The Hobbit" is NOT repeated online — the merge puts it in exactly one + // section. The count line is an owned match hidden by the shelf filter. onlineSearch = OnlineSearchState.Done( query = "hobbit", inLibrary = listOf(ScreenFixtures.books[1]), - online = onlineResults, + online = onlineResults.drop(1), openLibraryFailure = null, googleBooksFailure = null, + inLibraryOutsideFilter = 1, ), ) } @@ -122,6 +126,26 @@ class LibraryScreenPaparazziTest { } } + @Test + fun libraryOnlineSaveSheetCoverFailedLight() = snapshotBoth("library-online-save-sheet-cover-failed") { + Surface { + OnlineResultSaveSheet( + state = OnlineSaveSheetState.Shown( + book = onlineResults[0], + selectedShelfId = ScreenFixtures.topShelf.id, + ), + bookcases = ScreenFixtures.bookcases, + shelves = ScreenFixtures.shelves, + recentShelfId = ScreenFixtures.topShelf.id, + onShelfSelected = {}, + onSave = {}, + onEditDetails = {}, + onSkip = {}, + cover = CoverPreloadState.Failed("https://covers.openlibrary.org/b/id/1-M.jpg", "tls connection reset"), + ) + } + } + /** Two online hits: one merged from both sources, one Open-Library-only -- the shape [SearchResultMerger] actually produces. */ private val onlineResults = listOf( OnlineBook( @@ -179,6 +203,7 @@ class LibraryScreenPaparazziTest { LibrarySearchResultsContent( query = "hobbit", localMatches = localMatches, + inLibraryOutsideFilter = (onlineSearch as? OnlineSearchState.Done)?.inLibraryOutsideFilter ?: 0, onlineSearch = onlineSearch, onBookClick = {}, onSubmitOnlineSearch = {}, diff --git a/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_LibraryScreenPaparazziTest_libraryOnlineSaveSheetCoverFailedLight_library-online-save-sheet-cover-failed-dark.png b/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_LibraryScreenPaparazziTest_libraryOnlineSaveSheetCoverFailedLight_library-online-save-sheet-cover-failed-dark.png new file mode 100644 index 0000000..de8e353 Binary files /dev/null and b/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_LibraryScreenPaparazziTest_libraryOnlineSaveSheetCoverFailedLight_library-online-save-sheet-cover-failed-dark.png differ diff --git a/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_LibraryScreenPaparazziTest_libraryOnlineSaveSheetCoverFailedLight_library-online-save-sheet-cover-failed-light.png b/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_LibraryScreenPaparazziTest_libraryOnlineSaveSheetCoverFailedLight_library-online-save-sheet-cover-failed-light.png new file mode 100644 index 0000000..67b4d4f Binary files /dev/null and b/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_LibraryScreenPaparazziTest_libraryOnlineSaveSheetCoverFailedLight_library-online-save-sheet-cover-failed-light.png differ diff --git a/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_LibraryScreenPaparazziTest_librarySearchBothSectionsLight_library-search-both-sections-dark.png b/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_LibraryScreenPaparazziTest_librarySearchBothSectionsLight_library-search-both-sections-dark.png index f3b71dd..5951ed5 100644 Binary files a/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_LibraryScreenPaparazziTest_librarySearchBothSectionsLight_library-search-both-sections-dark.png and b/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_LibraryScreenPaparazziTest_librarySearchBothSectionsLight_library-search-both-sections-dark.png differ diff --git a/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_LibraryScreenPaparazziTest_librarySearchBothSectionsLight_library-search-both-sections-light.png b/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_LibraryScreenPaparazziTest_librarySearchBothSectionsLight_library-search-both-sections-light.png index 8d24023..333b2e2 100644 Binary files a/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_LibraryScreenPaparazziTest_librarySearchBothSectionsLight_library-search-both-sections-light.png and b/app/app/src/test/snapshots/images/org.modg.bookshelf.ui.screens_LibraryScreenPaparazziTest_librarySearchBothSectionsLight_library-search-both-sections-light.png differ diff --git a/docs/HANDOFF.md b/docs/HANDOFF.md index 7cfe00e..5ebe193 100644 --- a/docs/HANDOFF.md +++ b/docs/HANDOFF.md @@ -766,3 +766,72 @@ Logs: `logs/wave-chain.log`, `logs/gate-.log`, `logs/WAVE9-DONE`, `logs/WA **The gate is not acceptance.** Still owed by the orchestrator for BOTH waves: read each `.summary`, read the diff, eyeball the new Paparazzi PNGs, check CancellationException is rethrown in every new catch, and check wave 10's dedupe judgement call in its report. + +## Waves 9 + 10 — REVIEWED 2026-09-16; accepted after a review-fix round +Chain finished 2026-09-15 18:28Z: `b600df6` (wave 9, 241 tests), `cf82cbc` (wave 10, 308). +Orchestrator independently re-ran assembleDebug / testDebugUnitTest / verifyPaparazziDebug on +both — green — then read the diffs and PNGs. The gate was right that they built; it could not +see these, all fixed directly by the orchestrator in the review-fix commit that follows: + +1. **"In your library" never showed the merged set.** The screen passed the plain text match + (`uiState.books`) instead of `OnlineSearchState.Done.inLibrary`, so an owned edition reached + only via an online ISBN was removed from "Online" and shown NOWHERE; wave 10's VM test + asserted `inLibrary` but nothing rendered it. Now rendered; the shelf filter applies to it + and hidden owned matches are counted ("1 more in your library, outside the current filter"). +2. **"Edit details" left the save sheet open** -> save from the add screen, Back, sheet still up + with Save enabled -> duplicate book. `LibraryViewModel.takeForEditing()` closes it. +3. **`rememberShelf` inside the save `try`** (add + online save): a failed prefs write after a + successful insert showed "Couldn't save" -> retry -> duplicate. Now after, and swallowed. +4. **Per-keystroke search on the main thread**: two regexes recompiled per call, every book + re-normalized per keystroke (~60ms/keystroke at 2,000 books on this server, measured with + jshell). Precompiled regexes, `LocalBookMatcher.index` rebuilt only when books change, + `flowOn(Dispatchers.Default)`. +5. **Add-by-hand duplicate check unguarded** (a Room throw from a keystroke would crash). Guarded. +6. **Scan screen save was unguarded** (wave 8's known gap): now catch/rethrow-cancellation, + in-flight guard, error line. `performSaveManualEntry` (post-lookup NotFound sheet) is STILL + unguarded — its sheet has no error UI; left as is. + +### Cover preload — design decided WITH the user, 2026-09-16 +User-reported: saving had a distinct delay even with the cover already visible on the sheet. +Cause: `createBook` downloaded the cover (fresh OkHttpClient) BEFORE writing the row, and on +failure silently saved with no `localCoverPath`. User rejected "save first, fetch cover in the +background" (a kill/crash loses the upload; a failure has nowhere to be shown). Their design: +- **Preload** (`data/repo/CoverPreload.kt`): the download starts when a sheet/screen gets a + cover URL — scan Found sheet, online save sheet on open (ONE result, never the whole list — + the user asked), add screen when "Edit details" carries a cover. Into `cacheDir/cover-preload/`. +- **Save waits** for it if still running; `createBook(coverFile=)` COPIES it to + `filesDir/covers/.jpg` (copy, so a failed insert leaves the preload for a retry; the copy + is deleted on insert failure). +- **Failure = Option A (user's choice)**: failure line + Retry on the sheet; Save becomes + "Save without cover". A download that fails WHILE Save waits stops the save (the user hasn't + seen it yet); one that had already failed when tapped proceeds without the file. +- **Cleanup**: preload discarded on skip/dismiss/save/onCleared; `BookshelfApplication` sweeps + files older than process start. NOT Coil's disk cache — public API, but Coil's eviction and + keys aren't a contract; the user agreed it wasn't worth depending on. +- Downloads use `metadataHttpClient` (never the PocketBase-token client). + +### Still open after review (not fixed — judgement calls or out of scope) +- Wave 9 has NO worker report (worker hit its wall clock mid-verification, see hazard #11). Its + undocumented calls: small EditNote FAB above Scan; no "More fields…" on the in-scan sheet; + `imePadding` rather than the prompt's `safeDrawingPadding` — needs an on-device look. +- Dedupe (wave 10's own analysis, agreed): false merges for same-author colon series + ("Star Wars: X"/"Star Wars: Y") and same-title same-surname authors; one bad edge chains via + union-find; misses translations, surname spelling variants, author-less books. +- Retry re-runs BOTH sources (an OL failure spends a Google Books request). +- Search toolbar still renders as a blank bar in Paparazzi (pre-existing), so no snapshot shows + a query. Online result rows are a non-lazy Column (≤ ~40 thumbnails per search; accepted). + +### HAZARD #11 — the lease did NOT stop a freeze; the wall clock counts frozen time +Chain took the lease 2026-09-13 17:30 (expire 3600s). The sprite froze ~17:45 — BEFORE the +first renewal, lease still valid — and stayed frozen ~26.5h until the user connected 09-14 +20:21. No reboot (uptime continuous); processes paused (`ps` start times shifted by the gap). +It froze again during K-manual-fix (09-14 20:37 -> 23:43 -> 09-15 11:27 log gaps). +Consequences: (a) an unattended chain only progresses while someone is connected — do not +promise the user overnight progress; (b) `run-task.sh`'s 24h `wall=` counts the freeze, so +K-manual was killed as "WALL CLOCK EXCEEDED" seconds after waking, mid-test-run, with no report. +The 8 red tests the gate then saw were its own unfinished tests. If this recurs, measure elapsed +time with the monotonic clock (`/proc/uptime`), which does not advance while frozen. + +Also seen 2026-09-16: Claude Code's background-task runner killed Gradle runs "because the system +is running low on memory" even with ~6GB available (8GB box, no swap; Kotlin daemon ~2.1GB, +Gradle daemon ~1.2GB). Foreground runs of the same command succeeded. diff --git a/logs/CHAIN-DONE b/logs/CHAIN-DONE new file mode 100644 index 0000000..0ce91e4 --- /dev/null +++ b/logs/CHAIN-DONE @@ -0,0 +1 @@ +chain complete 2026-09-15T18:28:39+00:00 diff --git a/logs/WAVE10-DONE b/logs/WAVE10-DONE new file mode 100644 index 0000000..6e30d50 --- /dev/null +++ b/logs/WAVE10-DONE @@ -0,0 +1,4 @@ +=== WAVE 10 (L-search) passed the chain gate 2026-09-15T18:28:39+00:00 === +commit cf82cbc (pushed); tests 308; worker $10.452535, 157 turns +Mechanical gate only. Orchestrator must still review: read logs/L-search.summary, +eyeball new Paparazzi PNGs, and read the diff (git show --stat cf82cbc).