Update naming, KDoc, tests

app/src/androidTest/kotlin/at/bitfire/davdroid/sync/LocalTestCollection.kt

@@ -7,11 +7,11 @@ package at.bitfire.davdroid.sync
 import at.bitfire.davdroid.db.SyncState
 import at.bitfire.davdroid.resource.LocalCollection
 
-class LocalTestCollection: LocalCollection<LocalTestResource> {
+class LocalTestCollection(
+ override val collectionUrl: String = "http://example.com/test/"
+): LocalCollection<LocalTestResource> {
 
  override val tag = "LocalTestCollection"
- override val collectionUrl: String
- get() = "https://example.com"
  override val title = "Local Test Collection"
 
  override var lastSyncState: SyncState? = null

app/src/androidTest/kotlin/at/bitfire/davdroid/sync/SyncerTest.kt

@@ -89,9 +89,11 @@ class SyncerTest {
 
  // Should delete the localCollection if dbCollection (remote) does not exist
  val localCollections = mutableListOf(localCollection)
- syncer.updateCollections(localCollections, dbCollections = emptyMap())
+ val result = syncer.updateCollections(mockk(), localCollections, emptyMap())
  verify(exactly = 1) { localCollection.deleteCollection() }
- assertTrue(localCollections.isEmpty())
+
+ // Updated local collection list should be empty
+ assertTrue(result.isEmpty())
  }
 
  @Test
@@ -101,27 +103,28 @@ class SyncerTest {
  val dbCollections = mapOf("http://update.the/collection".toHttpUrl() to dbCollection)
  every { dbCollection.url } returns "http://update.the/collection".toHttpUrl()
  every { localCollection.collectionUrl } returns "http://update.the/collection"
+ every { localCollection.title } returns "The Local Collection"
 
- // Should update the localCollection if it exists ...
- val localCollections = mutableListOf(localCollection)
- val newCollections = syncer.updateCollections(localCollections, dbCollections)
+ // Should update the localCollection if it exists
+ val result = syncer.updateCollections(mockk(), listOf(localCollection), dbCollections)
  verify(exactly = 1) { syncer.update(localCollection, dbCollection) }
- // ... and remove it from the "new found" collections which are to be created
- assertArrayEquals(arrayOf(localCollection), localCollections.toTypedArray())
- assertTrue(newCollections.isEmpty())
+
+ // Updated local collection list should be same as input
+ assertArrayEquals(arrayOf(localCollection), result.toTypedArray())
  }
 
  @Test
  fun testUpdateCollections_findsNewCollection() {
  val dbCollection = mockk<Collection>()
- val dbCollections = mapOf("http://newly.found/collection".toHttpUrl() to dbCollection)
  every { dbCollection.url } returns "http://newly.found/collection".toHttpUrl()
+ val dbCollections = mapOf(dbCollection.url to dbCollection)
 
  // Should return the new collection, because it was not updated
- val localCollections = mutableListOf<LocalTestCollection>()
- val newCollections = syncer.updateCollections(localCollections, dbCollections)
- assertEquals(dbCollection, newCollections["http://newly.found/collection".toHttpUrl()])
- assertTrue(localCollections.isEmpty())
+ val result = syncer.updateCollections(mockk(), emptyList(), dbCollections)
+
+ // Updated local collection list contain new entry
+ assertEquals(1, result.size)
+ assertEquals(dbCollection.url.toString(), result[0].collectionUrl)
  }
 
 
@@ -130,12 +133,11 @@ class SyncerTest {
  val provider = mockk<ContentProviderClient>()
  val localCollection = mockk<LocalTestCollection>()
  val dbCollection = mockk<Collection>()
- val dbCollections = mapOf("http://newly.found/collection".toHttpUrl() to dbCollection)
  every { syncer.create(provider, dbCollection) } returns localCollection
  every { dbCollection.url } returns "http://newly.found/collection".toHttpUrl()
 
  // Should return list of newly created local collections
- val newLocalCollections = syncer.createLocalCollections(provider, dbCollections)
+ val newLocalCollections = syncer.createLocalCollections(provider, listOf(dbCollection))
  assertEquals(listOf(localCollection), newLocalCollections)
  }
 
@@ -190,7 +192,7 @@ class SyncerTest {
  emptyList()
 
  override fun create(provider: ContentProviderClient, remoteCollection: Collection): LocalTestCollection =
- LocalTestCollection()
+ LocalTestCollection(remoteCollection.url.toString())
 
  override fun syncCollection(
  provider: ContentProviderClient,

app/src/main/kotlin/at/bitfire/davdroid/sync/Syncer.kt

@@ -124,14 +124,15 @@ abstract class Syncer<CollectionType: LocalCollection<*>>(
  /**
  * Updates and deletes local collections. Specifically:
  *
- * - Deletes local collections if corresponding database collections are missing.
  * - Updates local collections with possibly new info from corresponding database collections.
- * - Determines new database collections to be also created as local collections.
+ * - Deletes local collections if corresponding database collections are missing.
+ * - Creates local collections if database collections without local match are available.
  *
- * @param localCollections The local collections to be updated or deleted.
- * @param dbCollections The database collections possibly containing new information
+ * @param provider Content provider client, used to create local collections
+ * @param localCollections The current local collections
+ * @param dbCollections The current database collections, possibly containing new information
  *
- * @return New found database collections to be created in provider
+ * @return Updated list of local collections (obsolete collections removed, new collections added)
  */
  @VisibleForTesting
  internal fun updateCollections(
@@ -140,37 +141,43 @@ abstract class Syncer<CollectionType: LocalCollection<*>>(
  dbCollections: Map<HttpUrl, Collection>
  ): List<CollectionType> {
  // create mutable copies of input
- val newLocalCollections = localCollections.toMutableList()
+ val updatedLocalCollections = localCollections.toMutableList()
  val newDbCollections = dbCollections.toMutableMap()
 
  for (localCollection in localCollections) {
  val dbCollection = dbCollections[localCollection.collectionUrl?.toHttpUrlOrNull()]
  if (dbCollection == null) {
- // Collection not available in db = on server (anymore), delete
- logger.info("Deleting local collection ${localCollection.title}")
+ // Collection not available in db = on server (anymore), delete and remove from the updated list
+ logger.fine("Deleting local collection ${localCollection.title}")
  localCollection.deleteCollection()
- newLocalCollections -= localCollection
+ updatedLocalCollections -= localCollection
  } else {
  // Collection exists locally, update local collection and remove it from "to be created" map
+ logger.fine("Updating local collection ${localCollection.title} with $dbCollection")
  update(localCollection, dbCollection)
  newDbCollections -= dbCollection.url
  }
  }
 
  // Create local collections which are in DB, but don't exist locally yet
- val newProviderCollections = createLocalCollections(provider, newDbCollections.values.toList())
- newLocalCollections.addAll(newProviderCollections) // Add the newly created collections
+ if (newDbCollections.isNotEmpty()) {
+ val toBeCreated = newDbCollections.values.toList()
+ logger.log(Level.FINE, "Creating new local collections", toBeCreated.toTypedArray())
+ val newLocalCollections = createLocalCollections(provider, toBeCreated)
+ // Add the newly created collections to the updated list
+ updatedLocalCollections.addAll(newLocalCollections)
+ }
 
- return newLocalCollections
+ return updatedLocalCollections
  }
 
  /**
  * Creates new local collections from database collections.
  *
- * @param provider Content provider client to access local collections.
- * @param dbCollections Database collections to be created as local collections.
+ * @param provider Content provider client to access local collections
+ * @param dbCollections Database collections to be created as local collections
  *
- * @return Newly created local collections.
+ * @return Newly created local collections
  */
  @VisibleForTesting
  internal fun createLocalCollections(