Make ImageRepository thread-safe (#147)

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))

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<Screen> = _navigationState.map { it.current }
         .stateIn(viewModelScope, SharingStarted.Eagerly, _navigationState.value.current)
 
-    private val _pages = MutableStateFlow(imageRepository.pages())
+    private val _pages = MutableStateFlow<List<ScanPage>>(emptyList())
+
+    init {
+        viewModelScope.launch {
+            _pages.value = imageRepository.pages()
+        }
+    }
+
     val documentUiModel: StateFlow<DocumentUiModel> =
         _pages.map { pages ->
             DocumentUiModel(
-                pageKeys = pages.map { p ->
+                pageKeys = pages.map { it.key() }.toImmutableList(),
-                    PageViewKey(p.id, p.manualRotation)
-                }.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) {
+            val pages = withContext(Dispatchers.IO) {
-                capturedPage.sourceJpeg.await()
+                val sourceJpeg = capturedPage.sourceJpeg.await()
-            }
-            val pages = withContext(repositoryDispatcher) {
                 imageRepository.add(
                     capturedPage.pageJpeg,
                     sourceJpeg,

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<ScanPage> =
+    suspend fun pages(): List<ScanPage> = 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)
+    suspend fun add(pageBytes: ByteArray, sourceBytes: ByteArray, metadata: PageMetadata)
-
+    = mutex.withLock {
-    fun add(pageBytes: ByteArray, sourceBytes: ByteArray, metadata: PageMetadata) {
         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) {
+    suspend fun rotate(id: String, clockwise: Boolean) = mutex.withLock {
-        val page = pages.get(id) ?: return
+        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)
+        val thumbFile = File(thumbnailDir, workFileName(key))
-        if (thumbFile == null) {
-            return null
-        }
         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? {
+    suspend fun movePage(id: String, newIndex: Int) = mutex.withLock {
-        return File(thumbnailDir, workFileName(key))
-    }
-
-    fun movePage(id: String, newIndex: Int) {
         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
 

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<ByteArray> {
 
     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())
         }
     }
 }

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,

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<PageViewKey> =
+    private suspend fun currentPageKeys(): ImmutableList<PageViewKey> =
-        imageRepository.pages().map {
+        imageRepository.pages().map { it.key() }.toImmutableList()
-            PageViewKey(it.id, it.manualRotation)
-        }.toImmutableList()
 
     fun prepareExportIfNeeded() {
         ensureValidFilename()

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<String> =
+    suspend fun ImageRepository.imageIds(): PersistentList<String> =
         pages().map { it.id }.toPersistentList()
 }

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" }