Address comments for PR 6440

collect_app/src/main/java/org/odk/collect/android/formmanagement/FormsDataService.kt

@@ -58,7 +58,7 @@ class FormsDataService(
         progressReporter: (Int, Int) -> Unit,
         isCancelled: () -> Boolean
     ): Map<ServerFormDetails, FormDownloadException?> {
-        var results = mutableMapOf<ServerFormDetails, FormDownloadException?>()
+        val results = mutableMapOf<ServerFormDetails, FormDownloadException?>()
 
         val projectDependencyModule = projectDependencyModuleFactory.create(projectId)
         projectDependencyModule.formsLock.withLock { acquiredLock ->

collect_app/src/test/java/org/odk/collect/android/external/FormUriActivityTest.kt

@@ -62,7 +62,7 @@ import org.odk.collect.settings.InMemSettingsProvider
 import org.odk.collect.settings.SettingsProvider
 import org.odk.collect.settings.keys.ProtectedProjectKeys
 import org.odk.collect.shared.TempFiles
-import org.odk.collect.shared.locks.ThreadSafeBooleanChangeLock
+import org.odk.collect.shared.locks.BooleanChangeLock
 import org.odk.collect.shared.strings.UUIDGenerator
 import org.odk.collect.testshared.FakeScheduler
 import java.io.File
@@ -85,7 +85,7 @@ class FormUriActivityTest {
     private val savepointsRepositoryProvider = mock<SavepointsRepositoryProvider>().apply {
         whenever(create()).thenReturn(savepointsRepository)
     }
-    private val changeLock = ThreadSafeBooleanChangeLock()
+    private val changeLock = BooleanChangeLock()
     private val changeLockProvider = ChangeLockProvider { changeLock }
 
     @get:Rule
@@ -1052,7 +1052,7 @@ class FormUriActivityTest {
             ).build()
         )
 
-        changeLock.tryLock()
+        changeLock.lock()
         launcherRule.launchForResult<FormUriActivity>(getBlankFormIntent(project.uuid, form.dbId))
         fakeScheduler.flush()
 
@@ -1074,7 +1074,7 @@ class FormUriActivityTest {
             ).build()
         )
 
-        changeLock.tryLock()
+        changeLock.lock()
         val scenario = launcherRule.launchForResult<FormUriActivity>(getBlankFormIntent(project.uuid, form.dbId))
         fakeScheduler.flush()
 
@@ -1103,7 +1103,7 @@ class FormUriActivityTest {
                 .build()
         )
 
-        changeLock.tryLock()
+        changeLock.lock()
         launcherRule.launchForResult<FormUriActivity>(getSavedIntent(project.uuid, instance.dbId))
         fakeScheduler.flush()
 
@@ -1134,7 +1134,7 @@ class FormUriActivityTest {
                 .build()
         )
 
-        changeLock.tryLock()
+        changeLock.lock()
         val scenario = launcherRule.launchForResult<FormUriActivity>(getSavedIntent(project.uuid, instance.dbId))
         fakeScheduler.flush()
 

collect_app/src/test/java/org/odk/collect/android/formentry/FormEntryViewModelTest.java

@@ -36,7 +36,7 @@
 import org.odk.collect.androidshared.data.Consumable;
 import org.odk.collect.forms.FormsRepository;
 import org.odk.collect.formstest.InMemFormsRepository;
-import org.odk.collect.shared.locks.ThreadSafeBooleanChangeLock;
+import org.odk.collect.shared.locks.BooleanChangeLock;
 import org.odk.collect.testshared.FakeScheduler;
 
 import java.io.FileNotFoundException;
@@ -54,7 +54,7 @@ public class FormEntryViewModelTest {
     private FakeScheduler scheduler;
     private final FormSessionRepository formSessionRepository = new InMemFormSessionRepository();
     private final FormsRepository formsRepository = new InMemFormsRepository();
-    private final ChangeLocks changeLocks = new ChangeLocks(new ThreadSafeBooleanChangeLock(), new ThreadSafeBooleanChangeLock());
+    private final ChangeLocks changeLocks = new ChangeLocks(new BooleanChangeLock(), new BooleanChangeLock());
 
     @Rule
     public InstantTaskExecutorRule instantTaskExecutorRule = new InstantTaskExecutorRule();

collect_app/src/test/java/org/odk/collect/android/formlists/blankformlist/BlankFormListViewModelTest.kt

@@ -25,7 +25,7 @@ import org.odk.collect.formstest.FormUtils
 import org.odk.collect.formstest.InMemInstancesRepository
 import org.odk.collect.settings.enums.FormUpdateMode
 import org.odk.collect.settings.keys.ProjectKeys
-import org.odk.collect.shared.locks.ThreadSafeBooleanChangeLock
+import org.odk.collect.shared.locks.BooleanChangeLock
 import org.odk.collect.shared.settings.InMemSettings
 import org.odk.collect.testshared.FakeScheduler
 import org.odk.collect.testshared.getOrAwaitValue
@@ -47,7 +47,7 @@ class BlankFormListViewModelTest {
     private val changeLockProvider: ChangeLockProvider = mock()
     private val projectId = "projectId"
 
-    private val changeLock = ThreadSafeBooleanChangeLock()
+    private val changeLock = BooleanChangeLock()
     private lateinit var viewModel: BlankFormListViewModel
 
     @Test

collect_app/src/test/java/org/odk/collect/android/formmanagement/FormsDataServiceTest.kt

@@ -32,7 +32,7 @@ import org.odk.collect.forms.FormSourceException
 import org.odk.collect.formstest.FormUtils
 import org.odk.collect.projects.Project
 import org.odk.collect.settings.keys.ProjectKeys
-import org.odk.collect.shared.locks.ThreadSafeBooleanChangeLock
+import org.odk.collect.shared.locks.BooleanChangeLock
 import org.odk.collect.shared.strings.Md5.getMd5Hash
 
 @RunWith(AndroidJUnit4::class)
@@ -50,7 +50,7 @@ class FormsDataServiceTest {
     private val notifier = mock<Notifier>()
     private val analytics = mock<Analytics>()
 
-    private val changeLockProvider = ChangeLockProvider { ThreadSafeBooleanChangeLock() }
+    private val changeLockProvider = ChangeLockProvider { BooleanChangeLock() }
 
     private val formSource = mock<FormSource> {
         on { fetchFormList() } doReturn emptyList()
@@ -113,8 +113,8 @@ class FormsDataServiceTest {
     fun `downloadUpdates() does nothing when change lock is locked`() {
         val isSyncing = formsDataService.isSyncing(project.uuid)
 
-        val changeLock = changeLockProvider.create(project.uuid).formsLock as ThreadSafeBooleanChangeLock
-        changeLock.tryLock()
+        val changeLock = changeLockProvider.create(project.uuid).formsLock
+        changeLock.lock()
 
         isSyncing.recordValues { projectValues ->
             formsDataService.downloadUpdates(project.uuid)
@@ -130,8 +130,8 @@ class FormsDataServiceTest {
     fun `matchFormsWithServer() does nothing when change lock is locked`() {
         val isSyncing = formsDataService.isSyncing(project.uuid)
 
-        val changeLock = changeLockProvider.create(project.uuid).formsLock as ThreadSafeBooleanChangeLock
-        changeLock.tryLock()
+        val changeLock = changeLockProvider.create(project.uuid).formsLock
+        changeLock.lock()
 
         isSyncing.recordValues { projectValues ->
             formsDataService.matchFormsWithServer(project.uuid)
@@ -149,8 +149,8 @@ class FormsDataServiceTest {
      */
     @Test
     fun `matchFormsWithServer() returns false when change lock is locked`() {
-        val changeLock = changeLockProvider.create(project.uuid).formsLock as ThreadSafeBooleanChangeLock
-        changeLock.tryLock()
+        val changeLock = changeLockProvider.create(project.uuid).formsLock
+        changeLock.lock()
 
         assertThat(formsDataService.matchFormsWithServer(project.uuid), equalTo(false))
     }
@@ -224,8 +224,8 @@ class FormsDataServiceTest {
     fun `update() does nothing when change lock is locked`() {
         val isSyncing = formsDataService.isSyncing(project.uuid)
 
-        val changeLock = changeLockProvider.create(project.uuid).formsLock as ThreadSafeBooleanChangeLock
-        changeLock.tryLock()
+        val changeLock = changeLockProvider.create(project.uuid).formsLock
+        changeLock.lock()
 
         isSyncing.recordValues { projectValues ->
             formsDataService.update(project.uuid)

collect_app/src/test/java/org/odk/collect/android/instancemanagement/InstancesDataServiceTest.kt

@@ -28,7 +28,7 @@ import org.odk.collect.formstest.InMemInstancesRepository
 import org.odk.collect.formstest.InstanceFixtures
 import org.odk.collect.projects.ProjectDependencyFactory
 import org.odk.collect.settings.keys.ProjectKeys
-import org.odk.collect.shared.locks.ThreadSafeBooleanChangeLock
+import org.odk.collect.shared.locks.BooleanChangeLock
 import org.odk.collect.shared.settings.InMemSettings
 import java.io.File
 
@@ -39,7 +39,7 @@ class InstancesDataServiceTest {
         it.save(ProjectKeys.KEY_SERVER_URL, "http://example.com")
     }
 
-    private val changeLocks = ChangeLocks(ThreadSafeBooleanChangeLock(), ThreadSafeBooleanChangeLock())
+    private val changeLocks = ChangeLocks(BooleanChangeLock(), BooleanChangeLock())
     private val formsRepository = InMemFormsRepository()
     private val instancesRepository = InMemInstancesRepository()
 
@@ -74,7 +74,7 @@ class InstancesDataServiceTest {
 
     @Test
     fun `instances should not be deleted if the instances database is locked`() {
-        (projectDependencyModule.instancesLock as ThreadSafeBooleanChangeLock).tryLock()
+        projectDependencyModule.instancesLock.lock()
         val result = instancesDataService.deleteInstances("projectId", longArrayOf(1))
         assertThat(result, equalTo(false))
     }

collect_app/src/test/java/org/odk/collect/android/projects/ProjectDeleterTest.kt

@@ -24,7 +24,7 @@ import org.odk.collect.settings.keys.MetaKeys
 import org.odk.collect.settings.keys.ProjectKeys
 import org.odk.collect.settings.keys.ProtectedProjectKeys
 import org.odk.collect.shared.TempFiles
-import org.odk.collect.shared.locks.ThreadSafeBooleanChangeLock
+import org.odk.collect.shared.locks.BooleanChangeLock
 import java.io.File
 
 class ProjectDeleterTest {
@@ -44,8 +44,8 @@ class ProjectDeleterTest {
         whenever(getProjectRootDirPath(project1.uuid)).thenReturn("")
     }
     private val changeLockProvider = mock<ChangeLockProvider> {
-        on { getFormLock(any()) } doReturn ThreadSafeBooleanChangeLock()
-        on { getInstanceLock(any()) } doReturn ThreadSafeBooleanChangeLock()
+        on { getFormLock(any()) } doReturn BooleanChangeLock()
+        on { getInstanceLock(any()) } doReturn BooleanChangeLock()
     }
     private val deleter = ProjectDeleter(
         projectsRepository,
@@ -152,8 +152,8 @@ class ProjectDeleterTest {
 
     @Test
     fun `If there are running background jobs that use blank forms the project should not be deleted`() {
-        val formChangeLock = ThreadSafeBooleanChangeLock().apply {
-            tryLock()
+        val formChangeLock = BooleanChangeLock().apply {
+            lock()
         }
         whenever(changeLockProvider.getFormLock(any())).thenReturn(formChangeLock)
 
@@ -165,8 +165,8 @@ class ProjectDeleterTest {
 
     @Test
     fun `If there are running background jobs that use saved forms the project should not be deleted`() {
-        val changeLock = ThreadSafeBooleanChangeLock().apply {
-            tryLock()
+        val changeLock = BooleanChangeLock().apply {
+            lock()
         }
         whenever(changeLockProvider.getInstanceLock(any())).thenReturn(changeLock)
 

collect_app/src/test/java/org/odk/collect/android/projects/ProjectResetterTest.kt

@@ -33,7 +33,7 @@ import org.odk.collect.projects.Project
 import org.odk.collect.settings.SettingsProvider
 import org.odk.collect.settings.keys.ProjectKeys
 import org.odk.collect.settings.keys.ProtectedProjectKeys
-import org.odk.collect.shared.locks.ThreadSafeBooleanChangeLock
+import org.odk.collect.shared.locks.BooleanChangeLock
 import org.odk.collect.shared.settings.Settings
 import java.io.File
 
@@ -49,7 +49,7 @@ class ProjectResetterTest {
     private lateinit var anotherProjectId: String
 
     private val propertyManager = mock<PropertyManager>()
-    private val changeLockProvider = ChangeLockProvider { ThreadSafeBooleanChangeLock() }
+    private val changeLockProvider = ChangeLockProvider { BooleanChangeLock() }
 
     @Before
     fun setup() {
@@ -202,7 +202,7 @@ class ProjectResetterTest {
         saveTestInstanceFiles(currentProjectId)
         setupTestInstancesDatabase(currentProjectId)
 
-        (changeLockProvider.create(currentProjectId).instancesLock as ThreadSafeBooleanChangeLock).tryLock()
+        changeLockProvider.create(currentProjectId).instancesLock.lock()
         val failedResetActions = projectResetter.reset(listOf(ProjectResetter.ResetAction.RESET_INSTANCES))
         assertEquals(1, failedResetActions.size)
 

shared/src/main/java/org/odk/collect/shared/locks/BooleanChangeLock.kt

@@ -0,0 +1,40 @@
+package org.odk.collect.shared.locks
+
+import java.util.function.Function
+
+class BooleanChangeLock : ChangeLock {
+    private var locked = false
+
+    override fun <T> withLock(function: Function<Boolean, T>): T {
+        val acquired = tryLock()
+
+        return try {
+            function.apply(acquired)
+        } finally {
+            if (acquired) {
+                unlock()
+            }
+        }
+    }
+
+    override fun tryLock(): Boolean {
+        if (locked) {
+            return false
+        } else {
+            locked = true
+            return true
+        }
+    }
+
+    override fun lock() {
+        if (locked) {
+            throw IllegalStateException()
+        } else {
+            locked = true
+        }
+    }
+
+    override fun unlock() {
+        locked = false
+    }
+}

shared/src/main/java/org/odk/collect/shared/locks/ChangeLock.kt

@@ -12,38 +12,7 @@ interface ChangeLock {
 
     fun tryLock(): Boolean
 
-    fun unlock()
-}
-
-class ThreadSafeBooleanChangeLock : ChangeLock {
-    private var locked = false
-
-    override fun <T> withLock(function: Function<Boolean, T>): T {
-        val acquired = tryLock()
+    fun lock()
 
-        return try {
-            function.apply(acquired)
-        } finally {
-            if (acquired) {
-                unlock()
-            }
-        }
-    }
-
-    override fun tryLock(): Boolean {
-        return synchronized(this) {
-            if (locked) {
-                false
-            } else {
-                locked = true
-                true
-            }
-        }
-    }
-
-    override fun unlock() {
-        synchronized(this) {
-            locked = false
-        }
-    }
+    fun unlock()
 }

shared/src/main/java/org/odk/collect/shared/locks/ThreadSafeBooleanChangeLock.kt

@@ -0,0 +1,46 @@
+package org.odk.collect.shared.locks
+
+import java.util.function.Function
+
+class ThreadSafeBooleanChangeLock : ChangeLock {
+    private var locked = false
+
+    override fun <T> withLock(function: Function<Boolean, T>): T {
+        val acquired = tryLock()
+
+        return try {
+            function.apply(acquired)
+        } finally {
+            if (acquired) {
+                unlock()
+            }
+        }
+    }
+
+    override fun tryLock(): Boolean {
+        return synchronized(this) {
+            if (locked) {
+                false
+            } else {
+                locked = true
+                true
+            }
+        }
+    }
+
+    override fun lock() {
+        synchronized(this) {
+            if (locked) {
+                throw IllegalStateException()
+            } else {
+                locked = true
+            }
+        }
+    }
+
+    override fun unlock() {
+        synchronized(this) {
+            locked = false
+        }
+    }
+}

shared/src/test/java/org/odk/collect/shared/locks/BooleanChangeLockTest.kt

@@ -0,0 +1,7 @@
+package org.odk.collect.shared.locks
+
+class BooleanChangeLockTest : ChangeLockTest() {
+    override fun buildSubject(): ChangeLock {
+        return BooleanChangeLock()
+    }
+}