From 944c9641a9ebe358a37fb0c6498c704b982c8a4b Mon Sep 17 00:00:00 2001 From: Valere Date: Thu, 17 Jun 2021 14:44:02 +0200 Subject: [PATCH] Code review --- .../org/matrix/android/sdk/SpaceOrderTest.kt | 37 +++++++------------ .../sdk/api/session/space/SpaceOrderUtils.kt | 23 +++++++++--- vector/build.gradle | 1 - 3 files changed, 32 insertions(+), 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 index e8fdf5472a..b9a92585c6 100644 --- 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>, orderCommand: List) = orderedSpaces.map { orderInfo -> orderInfo.first to (orderCommand.find { it.spaceId == orderInfo.first }?.order ?: orderInfo.second) } - .sortedWith(TestSpaceComparator()) + .sortedWith(testSpaceComparator) - private fun List>.assertSpaceOrdered() : List> { - assertEquals(this, this.sortedWith(TestSpaceComparator())) + private fun List>.assertSpaceOrdered(): List> { + assertEquals(this, this.sortedWith(testSpaceComparator)) return this } - class TestSpaceComparator : Comparator> { - - override fun compare(left: Pair?, right: Pair?): 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, 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 index 3385662a5e..844a5adcb4 100644 --- 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,6 +18,16 @@ 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( @@ -25,6 +35,9 @@ object SpaceOrderUtils { 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>, movedSpaceId: String, delta: Int): List { val movedIndex = orderedSpacesToOrderMap.indexOfFirst { it.first == movedSpaceId } if (movedIndex == -1) return emptyList() @@ -34,8 +47,6 @@ object SpaceOrderUtils { val nodesToReNumber = mutableListOf() var lowerBondOrder: String? = null - var afterSpace: Pair? = 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? = 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 index 67aed7cf15..21f62647c0 100644 --- 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"