Code review

element-hq · Jun 17, 2021 · 944c964 · 944c964
1 parent 1de74b1
commit 944c964
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 29 deletions.
diff --git a/matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/SpaceOrderTest.kt b/matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/SpaceOrderTest.kt
@@ -235,35 +235,26 @@ class SpaceOrderTest {
         Assert.assertEquals("roomId6", reOrdered[5].first)
     }
 
+    @Test
+    fun testComparator() {
+        listOf(
+                "roomId2" to "a",
+                "roomId1" to "b",
+                "roomId3" to null,
+                "roomId4" to null,
+        ).assertSpaceOrdered()
+    }
+
     private fun reOrderWithCommands(orderedSpaces: List<Pair<String, String?>>, orderCommand: List<SpaceOrderUtils.SpaceReOrderCommand>) =
             orderedSpaces.map { orderInfo ->
                 orderInfo.first to (orderCommand.find { it.spaceId == orderInfo.first }?.order ?: orderInfo.second)
             }
-                    .sortedWith(TestSpaceComparator())
+                    .sortedWith(testSpaceComparator)
 
-    private fun List<Pair<String, String?>>.assertSpaceOrdered() : List<Pair<String, String?>> {
-        assertEquals(this, this.sortedWith(TestSpaceComparator()))
+    private fun List<Pair<String, String?>>.assertSpaceOrdered(): List<Pair<String, String?>> {
+        assertEquals(this, this.sortedWith(testSpaceComparator))
         return this
     }
 
-    class TestSpaceComparator : Comparator<Pair<String, String?>> {
-
-        override fun compare(left: Pair<String, String?>?, right: Pair<String, String?>?): Int {
-            val leftOrder = left?.second
-            val rightOrder = right?.second
-            return if (leftOrder != null && rightOrder != null) {
-                leftOrder.compareTo(rightOrder)
-            } else {
-                if (leftOrder == null) {
-                    if (rightOrder == null) {
-                        compareValues(left?.first, right?.first)
-                    } else {
-                        1
-                    }
-                } else {
-                    -1
-                }
-            }
-        }
-    }
+    private val testSpaceComparator = compareBy<Pair<String, String?>, String?>(nullsLast()) { it.second }.thenBy { it.first }
 }
diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/space/SpaceOrderUtils.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/space/SpaceOrderUtils.kt
@@ -18,13 +18,26 @@ package org.matrix.android.sdk.api.session.space
 
 import org.matrix.android.sdk.api.util.StringOrderUtils
 
+/**
+ * Adds some utilities to compute correct string orders when ordering spaces.
+ * After moving a space (e.g via DnD), client should limit the number of room account data update.
+ * For example if the space is moved between two other spaces with orders, just update the moved space order by computing
+ * a mid point between the surrounding orders.
+ * If the space is moved after a space with no order, all the previous spaces should be then ordered,
+ * and the computed orders should be chosen so that there is enough gaps in between them to facilitate future re-order.
+ * Re numbering (i.e change all spaces m.space.order account data) should be avoided as much as possible,
+ * as the updates might not be atomic for other clients and would makes spaces jump around.
+ */
 object SpaceOrderUtils {
 
     data class SpaceReOrderCommand(
             val spaceId: String,
             val order: String
     )
 
+    /**
+     * Returns a minimal list of order change in order to re order the space list as per given move.
+     */
     fun orderCommandsForMove(orderedSpacesToOrderMap: List<Pair<String, String?>>, movedSpaceId: String, delta: Int): List<SpaceReOrderCommand> {
         val movedIndex = orderedSpacesToOrderMap.indexOfFirst { it.first == movedSpaceId }
         if (movedIndex == -1) return emptyList()
@@ -34,8 +47,6 @@ object SpaceOrderUtils {
 
         val nodesToReNumber = mutableListOf<String>()
         var lowerBondOrder: String? = null
-        var afterSpace: Pair<String, String?>? = null
-//        if (delta > 0) {
         var index = targetIndex
         while (index >= 0 && lowerBondOrder == null) {
             val node = orderedSpacesToOrderMap.getOrNull(index)
@@ -51,7 +62,9 @@ object SpaceOrderUtils {
             index--
         }
         nodesToReNumber.add(movedSpaceId)
-        afterSpace = if (orderedSpacesToOrderMap.indices.contains(targetIndex + 1)) orderedSpacesToOrderMap[targetIndex + 1] else null
+        val afterSpace: Pair<String, String?>? = if (orderedSpacesToOrderMap.indices.contains(targetIndex + 1)) {
+            orderedSpacesToOrderMap[targetIndex + 1]
+        } else null
 
         val defaultMaxOrder = CharArray(4) { StringOrderUtils.DEFAULT_ALPHABET.last() }
                 .joinToString("")
@@ -81,10 +94,10 @@ object SpaceOrderUtils {
                 }
             } ?: emptyList()
         } else {
-            return nodesToReNumber.mapIndexed { index, s ->
+            return nodesToReNumber.mapIndexed { i, s ->
                 SpaceReOrderCommand(
                         s,
-                        newOrder[index]
+                        newOrder[i]
                 )
             }
         }

diff --git a/vector/build.gradle b/vector/build.gradle
@@ -366,7 +366,6 @@ dependencies {
     implementation "com.jakewharton.rxbinding3:rxbinding-appcompat:$rxbinding_version"
     implementation "com.jakewharton.rxbinding3:rxbinding-material:$rxbinding_version"
 
-//    implementation fileTree(include: ['*.jar', '*.aar'], dir: 'libs')
     implementation("com.airbnb.android:epoxy:$epoxy_version")
     implementation "com.airbnb.android:epoxy-glide-preloading:$epoxy_version"
     kapt "com.airbnb.android:epoxy-processor:$epoxy_version"