From 92914c17304662225c15900a16f95b672c6d1e2e Mon Sep 17 00:00:00 2001 From: Pierre-Yves Nicolas <6371790+pynicolas@users.noreply.github.com> Date: Wed, 25 Mar 2026 14:35:17 +0100 Subject: [PATCH] Make ImageRepository thread-safe (#147) --- app/build.gradle.kts | 1 + .../java/org/fairscan/app/MainViewModel.kt | 33 +++++++------- .../org/fairscan/app/data/ImageRepository.kt | 38 ++++++---------- .../fairscan/app/domain/ExportPreparation.kt | 9 ++-- .../main/java/org/fairscan/app/domain/Page.kt | 4 +- .../app/ui/screens/export/ExportViewModel.kt | 6 +-- .../fairscan/app/data/ImageRepositoryTest.kt | 45 ++++++++++--------- gradle/libs.versions.toml | 3 ++ 8 files changed, 66 insertions(+), 73 deletions(-) diff --git a/app/build.gradle.kts b/app/build.gradle.kts index 11d86b5..1207117 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -142,6 +142,7 @@ dependencies { testImplementation(libs.junit) testImplementation(libs.assertj) + testImplementation(libs.kotlinx.coroutines.test) androidTestImplementation(libs.androidx.junit) androidTestImplementation(libs.androidx.espresso.core) androidTestImplementation(platform(libs.androidx.compose.bom)) diff --git a/app/src/main/java/org/fairscan/app/MainViewModel.kt b/app/src/main/java/org/fairscan/app/MainViewModel.kt index 6f8c872..687c202 100644 --- a/app/src/main/java/org/fairscan/app/MainViewModel.kt +++ b/app/src/main/java/org/fairscan/app/MainViewModel.kt @@ -21,7 +21,6 @@ import androidx.lifecycle.viewModelScope import kotlinx.collections.immutable.persistentListOf import kotlinx.collections.immutable.toImmutableList import kotlinx.coroutines.Dispatchers -import kotlinx.coroutines.asCoroutineDispatcher import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.SharingStarted import kotlinx.coroutines.flow.StateFlow @@ -33,27 +32,29 @@ import kotlinx.coroutines.withContext import org.fairscan.app.data.ImageRepository import org.fairscan.app.domain.CapturedPage import org.fairscan.app.domain.PageViewKey +import org.fairscan.app.domain.ScanPage import org.fairscan.app.ui.NavigationState import org.fairscan.app.ui.Screen import org.fairscan.app.ui.state.DocumentUiModel -import java.util.concurrent.Executors class MainViewModel(val imageRepository: ImageRepository, launchMode: LaunchMode): ViewModel() { - // TODO ImageRepository should be made thread-safe - private val repositoryDispatcher = Executors.newSingleThreadExecutor().asCoroutineDispatcher() - private val _navigationState = MutableStateFlow(NavigationState.initial(launchMode)) val currentScreen: StateFlow = _navigationState.map { it.current } .stateIn(viewModelScope, SharingStarted.Eagerly, _navigationState.value.current) - private val _pages = MutableStateFlow(imageRepository.pages()) + private val _pages = MutableStateFlow>(emptyList()) + + init { + viewModelScope.launch { + _pages.value = imageRepository.pages() + } + } + val documentUiModel: StateFlow = _pages.map { pages -> DocumentUiModel( - pageKeys = pages.map { p -> - PageViewKey(p.id, p.manualRotation) - }.toImmutableList(), + pageKeys = pages.map { it.key() }.toImmutableList(), imageLoader = ::getBitmap, thumbnailLoader = ::getThumbnail, ) @@ -73,7 +74,7 @@ class MainViewModel(val imageRepository: ImageRepository, launchMode: LaunchMode fun rotateImage(id: String, clockwise: Boolean) { viewModelScope.launch { - val pages = withContext(repositoryDispatcher) { + val pages = withContext(Dispatchers.IO) { imageRepository.rotate(id, clockwise) imageRepository.pages() } @@ -83,7 +84,7 @@ class MainViewModel(val imageRepository: ImageRepository, launchMode: LaunchMode fun movePage(id: String, newIndex: Int) { viewModelScope.launch { - val pages = withContext(repositoryDispatcher) { + val pages = withContext(Dispatchers.IO) { imageRepository.movePage(id, newIndex) imageRepository.pages() } @@ -93,7 +94,7 @@ class MainViewModel(val imageRepository: ImageRepository, launchMode: LaunchMode fun deletePage(id: String) { viewModelScope.launch { - val pages = withContext(repositoryDispatcher) { + val pages = withContext(Dispatchers.IO) { imageRepository.delete(id) imageRepository.pages() } @@ -104,7 +105,7 @@ class MainViewModel(val imageRepository: ImageRepository, launchMode: LaunchMode fun startNewDocument() { _pages.value = persistentListOf() viewModelScope.launch { - withContext(repositoryDispatcher) { + withContext(Dispatchers.IO) { imageRepository.clear() } } @@ -122,10 +123,8 @@ class MainViewModel(val imageRepository: ImageRepository, launchMode: LaunchMode fun handleImageCaptured(capturedPage: CapturedPage) { viewModelScope.launch { - val sourceJpeg = withContext(Dispatchers.IO) { - capturedPage.sourceJpeg.await() - } - val pages = withContext(repositoryDispatcher) { + val pages = withContext(Dispatchers.IO) { + val sourceJpeg = capturedPage.sourceJpeg.await() imageRepository.add( capturedPage.pageJpeg, sourceJpeg, diff --git a/app/src/main/java/org/fairscan/app/data/ImageRepository.kt b/app/src/main/java/org/fairscan/app/data/ImageRepository.kt index 45688eb..e03ed85 100644 --- a/app/src/main/java/org/fairscan/app/data/ImageRepository.kt +++ b/app/src/main/java/org/fairscan/app/data/ImageRepository.kt @@ -14,6 +14,8 @@ */ package org.fairscan.app.data +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock import kotlinx.serialization.json.Json import kotlinx.serialization.json.decodeFromJsonElement import kotlinx.serialization.json.int @@ -56,6 +58,8 @@ class ImageRepository( if (!exists()) mkdirs() } + private val mutex = Mutex() + private val metadataFile = File(scanDir, "document.json") private val json = Json { @@ -121,17 +125,17 @@ class ImageRepository( metadataFile.writeText(json.encodeToString(metadata)) } - fun pages(): List = + suspend fun pages(): List = mutex.withLock { pages.pages().mapNotNull { runCatching { val manualRotation = Rotation.fromDegrees(it.manualRotationDegrees) ScanPage(it.id, manualRotation, it.toMetadata()) }.getOrNull() } + } - private fun page(id: String): PageV2? = pages.get(id) - - fun add(pageBytes: ByteArray, sourceBytes: ByteArray, metadata: PageMetadata) { + suspend fun add(pageBytes: ByteArray, sourceBytes: ByteArray, metadata: PageMetadata) + = mutex.withLock { val id = "${System.currentTimeMillis()}" val fileName = "$id.jpg" val file = File(scanDir, fileName) @@ -164,8 +168,8 @@ class ImageRepository( fun PageV2.workFileName() = workFileName(id, manualRotationDegrees) - fun rotate(id: String, clockwise: Boolean) { - val page = pages.get(id) ?: return + suspend fun rotate(id: String, clockwise: Boolean) = mutex.withLock { + val page = pages.get(id) ?: return@withLock val delta = if (clockwise) Rotation.R90 else Rotation.R270 val newRotation = Rotation.fromDegrees(page.manualRotationDegrees).add(delta) @@ -194,13 +198,6 @@ class ImageRepository( return (if (file.exists()) file else null)?.readBytes() } - fun jpegBytes(id: String): ByteArray? { - val page = page(id) - if (page == null) return null - val file = File(scanDir, page.workFileName()) - return (if (file.exists()) file else null)?.readBytes() - } - fun sourceJpegBytes(id: String): ByteArray? { val file = getSourceFile(id) return if (file.exists()) file.readBytes() else null @@ -211,10 +208,7 @@ class ImageRepository( } fun getThumbnail(key: PageViewKey): ByteArray? { - val thumbFile = getThumbnailFile(key) - if (thumbFile == null) { - return null - } + val thumbFile = File(thumbnailDir, workFileName(key)) if (!thumbFile.exists()) { val workFile = File(scanDir, workFileName(key)) if (!workFile.exists()) return null @@ -228,16 +222,12 @@ class ImageRepository( transformations.resize(originalFile, thumbFile, thumbnailSizePx) } - private fun getThumbnailFile(key: PageViewKey): File? { - return File(thumbnailDir, workFileName(key)) - } - - fun movePage(id: String, newIndex: Int) { + suspend fun movePage(id: String, newIndex: Int) = mutex.withLock { pages.move(id, newIndex) saveMetadata() } - fun delete(id: String) { + suspend fun delete(id: String) = mutex.withLock { pages.delete(id) saveMetadata() @@ -250,7 +240,7 @@ class ImageRepository( ?.forEach { it.delete() } } - fun clear() { + suspend fun clear() = mutex.withLock { pages.clear() saveMetadata() // "empty" json file diff --git a/app/src/main/java/org/fairscan/app/domain/ExportPreparation.kt b/app/src/main/java/org/fairscan/app/domain/ExportPreparation.kt index bb3369f..eaf4273 100644 --- a/app/src/main/java/org/fairscan/app/domain/ExportPreparation.kt +++ b/app/src/main/java/org/fairscan/app/domain/ExportPreparation.kt @@ -20,20 +20,19 @@ import org.fairscan.imageprocessing.resizeForMaxPixels import org.fairscan.imageprocessing.scaledTo import org.opencv.core.Mat import org.opencv.core.MatOfByte -import org.opencv.core.MatOfInt import org.opencv.imgcodecs.Imgcodecs -fun jpegsForExport( +suspend fun jpegsForExport( imageRepository: ImageRepository, exportQuality: ExportQuality ): Sequence { val pages = imageRepository.pages().asSequence() return when (exportQuality) { - ExportQuality.BALANCED -> pages.mapNotNull { imageRepository.jpegBytes(it.id) } + ExportQuality.BALANCED -> pages.mapNotNull { imageRepository.jpegBytes(it.key()) } ExportQuality.LOW -> pages.mapNotNull { page -> - imageRepository.jpegBytes(page.id)?.let { jpeg -> + imageRepository.jpegBytes(page.key())?.let { jpeg -> resizeJpegBytesForMaxPixels( jpegBytes = jpeg, maxPixels = exportQuality.maxPixels.toDouble(), @@ -49,7 +48,7 @@ fun jpegsForExport( if (sourceJpegBytes != null && pageMetadata != null) prepareJpegForHigh(sourceJpegBytes, pageMetadata, manualRotation, exportQuality) else - imageRepository.jpegBytes(page.id) + imageRepository.jpegBytes(page.key()) } } } diff --git a/app/src/main/java/org/fairscan/app/domain/Page.kt b/app/src/main/java/org/fairscan/app/domain/Page.kt index 72bc5fe..0a197c2 100644 --- a/app/src/main/java/org/fairscan/app/domain/Page.kt +++ b/app/src/main/java/org/fairscan/app/domain/Page.kt @@ -26,7 +26,9 @@ data class ScanPage( val id: String, val manualRotation: Rotation, val metadata: PageMetadata?, -) +) { + fun key(): PageViewKey = PageViewKey(id, manualRotation) +} data class PageViewKey( val pageId: String, diff --git a/app/src/main/java/org/fairscan/app/ui/screens/export/ExportViewModel.kt b/app/src/main/java/org/fairscan/app/ui/screens/export/ExportViewModel.kt index ce8d073..b9d85ba 100644 --- a/app/src/main/java/org/fairscan/app/ui/screens/export/ExportViewModel.kt +++ b/app/src/main/java/org/fairscan/app/ui/screens/export/ExportViewModel.kt @@ -119,10 +119,8 @@ class ExportViewModel(container: AppContainer, val imageRepository: ImageReposit } } - private fun currentPageKeys(): ImmutableList = - imageRepository.pages().map { - PageViewKey(it.id, it.manualRotation) - }.toImmutableList() + private suspend fun currentPageKeys(): ImmutableList = + imageRepository.pages().map { it.key() }.toImmutableList() fun prepareExportIfNeeded() { ensureValidFilename() diff --git a/app/src/test/java/org/fairscan/app/data/ImageRepositoryTest.kt b/app/src/test/java/org/fairscan/app/data/ImageRepositoryTest.kt index 266a74f..12aaa6e 100644 --- a/app/src/test/java/org/fairscan/app/data/ImageRepositoryTest.kt +++ b/app/src/test/java/org/fairscan/app/data/ImageRepositoryTest.kt @@ -16,6 +16,7 @@ package org.fairscan.app.data import kotlinx.collections.immutable.PersistentList import kotlinx.collections.immutable.toPersistentList +import kotlinx.coroutines.test.runTest import org.assertj.core.api.Assertions.assertThat import org.fairscan.app.domain.PageMetadata import org.fairscan.app.domain.PageViewKey @@ -60,7 +61,7 @@ class ImageRepositoryTest { } @Test - fun add_image() { + fun add_image() = runTest { val repo = repo() assertThat(repo.imageIds()).isEmpty() val bytes = byteArrayOf(101, 102, 103) @@ -82,7 +83,7 @@ class ImageRepositoryTest { } @Test - fun delete_image() { + fun delete_image() = runTest { val repo = repo() val bytes = byteArrayOf(101, 102, 103) repo.add(bytes, byteArrayOf(51), metadata1) @@ -98,28 +99,28 @@ class ImageRepositoryTest { } @Test - fun delete_unknown_id() { + fun delete_unknown_id() = runTest { val repo = repo() repo.delete("x") assertThat(repo.imageIds()).isEmpty() } @Test - fun `should find existing files at initialization with no json`() { + fun `should find existing files at initialization with no json`() = runTest { scanDir().mkdirs() File(scanDir(), "1.jpg").writeBytes(byteArrayOf(101, 102, 103)) assertThat(repo().imageIds()).containsExactly("1") } @Test - fun `should find existing files at initialization if json is invalid`() { + fun `should find existing files at initialization if json is invalid`() = runTest { writeDocumentDotJson("xxx") File(scanDir(), "1.jpg").writeBytes(byteArrayOf(101, 102, 103)) assertThat(repo().imageIds()).containsExactly("1") } @Test - fun `no json and two files with same id`() { + fun `no json and two files with same id`() = runTest { scanDir().mkdirs() File(scanDir(), "1768153479486.jpg").writeBytes(byteArrayOf(101, 102, 103)) File(scanDir(), "1768153479486-270.jpg").writeBytes(byteArrayOf(105, 106, 107)) @@ -128,24 +129,24 @@ class ImageRepositoryTest { } @Test - fun `should find existing files at initialization with no json and with rotation`() { + fun `should find existing files at initialization with no json and with rotation`() = runTest { scanDir().mkdirs() val bytes = byteArrayOf(101, 102, 103) File(scanDir(), "1-90.jpg").writeBytes(bytes) val repo = repo() assertThat(repo.imageIds()).containsExactly("1") - assertThat(repo.jpegBytes("1")).isEqualTo(bytes) + assertThat(repo.jpegBytes(PageViewKey("1", R0))).isEqualTo(bytes) } @Test - fun `should filter pages in json at initialization`() { + fun `should filter pages in json at initialization`() = runTest { writeDocumentDotJson("""{"pages":[{"file":"1.jpg"}, {"file":"2.jpg"}]}""") File(scanDir(), "2.jpg").writeBytes(byteArrayOf(101, 102, 103)) assertThat(repo().imageIds()).containsExactly("2") } @Test - fun `should rename rotated files with no base file`() { + fun `should rename rotated files with no base file`() = runTest { scanDir().mkdirs() val bytes = byteArrayOf(105, 106, 107) File(scanDir(), "123-90.jpg").writeBytes(bytes) @@ -157,24 +158,24 @@ class ImageRepositoryTest { } @Test - fun `should rename rotated files with no base file but listed in json`() { + fun `should rename rotated files with no base file but listed in json`() = runTest { writeDocumentDotJson("""{"pages":[{"file":"1-90.jpg"}]}""") val bytes = byteArrayOf(105, 106, 107) File(scanDir(), "1-90.jpg").writeBytes(bytes) val repo = repo() assertThat(repo.imageIds()).containsExactly("1") - assertThat(repo.jpegBytes("1")).isEqualTo(bytes) + assertThat(repo.jpegBytes(PageViewKey("1", R0))).isEqualTo(bytes) } @Test - fun `should return null on invalid id`() { + fun `should return null on invalid id`() = runTest { val repo = repo() assertThat(repo.imageIds()).isEmpty() - assertThat(repo.jpegBytes("x")).isNull() + assertThat(repo.jpegBytes(PageViewKey("x", R0))).isNull() } @Test - fun `clear should delete pages`() { + fun `clear should delete pages`() = runTest { val bytes = byteArrayOf(101, 102, 103) val repo1 = repo() repo1.add(bytes, byteArrayOf(51), metadata1) @@ -189,7 +190,7 @@ class ImageRepositoryTest { } @Test - fun rotate() { + fun rotate() = runTest { val repo = repo() repo.add(byteArrayOf(101, 102, 103), byteArrayOf(51), metadata1) assertThat(repo.pages().last().metadata).isEqualTo(metadata1) @@ -207,14 +208,14 @@ class ImageRepositoryTest { } @Test - fun rotate_unknown_id() { + fun rotate_unknown_id() = runTest { val repo = repo() repo.rotate("x", true) assertThat(repo.imageIds()).isEmpty() } @Test - fun movePage() { + fun movePage() = runTest { val repo = repo() repo.add(byteArrayOf(101), byteArrayOf(51), metadata1) Thread.sleep(1L) // to avoid file name clashes @@ -229,7 +230,7 @@ class ImageRepositoryTest { } @Test - fun move_unknown_id() { + fun move_unknown_id() = runTest { val repo = repo() repo.movePage("x", 0) assertThat(repo.imageIds()).isEmpty() @@ -250,7 +251,7 @@ class ImageRepositoryTest { } @Test - fun `pages with invalid metadata should be skipped`() { + fun `pages with invalid metadata should be skipped`() = runTest { val bytes = byteArrayOf(105, 106, 107) writeDocumentDotJson("""{"version":2, "pages":[{"id":"1", "manualRotationDegrees":90}]}""") @@ -265,7 +266,7 @@ class ImageRepositoryTest { } @Test - fun last_added_source_file() { + fun last_added_source_file() = runTest { val repo = repo() assertThat(repo.lastAddedSourceFile()).isNull() repo.add(byteArrayOf(101), byteArrayOf(51), metadata1) @@ -298,6 +299,6 @@ class ImageRepositoryTest { File(scanDir(), "document.json").writeText(json) } - fun ImageRepository.imageIds(): PersistentList = + suspend fun ImageRepository.imageIds(): PersistentList = pages().map { it.id }.toPersistentList() } diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index f140a54..25451bc 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -24,6 +24,7 @@ protobufJavaLite = "4.34.1" kotlinSerialization = "1.10.0" reorderable = "3.0.0" jetbrainsKotlinJvm = "2.3.10" +coroutines-test = "1.10.2" [libraries] androidx-core-ktx = { group = "androidx.core", name = "core-ktx", version.ref = "coreKtx" } @@ -62,6 +63,8 @@ zoomable = { group = "net.engawapg.lib", name = "zoomable", version.ref = "zooma reorderable = { module = "sh.calvin.reorderable:reorderable", version.ref = "reorderable" } aboutlibraries-compose-m3 = { module = "com.mikepenz:aboutlibraries-compose-m3", version.ref = "aboutLibraries" } kotlinx-serialization-json = { module = "org.jetbrains.kotlinx:kotlinx-serialization-json", version.ref = "kotlinSerialization" } +kotlinx-coroutines-test = { group = "org.jetbrains.kotlinx", name = "kotlinx-coroutines-test", version.ref = "coroutines-test" } + assertj = { group="org.assertj", name="assertj-core", version.ref = "assertj" }