From a84dd5620f14dd956acbeb35ed2bb6fe90faced9 Mon Sep 17 00:00:00 2001 From: jee-hyun-kim Date: Mon, 15 Apr 2024 21:21:28 +0900 Subject: [PATCH] Implement Tree.RemoveStyle --- yorkie/proto/yorkie/v1/resources.proto | 1 + .../dev/yorkie/document/json/JsonTreeTest.kt | 29 +++++++ .../dev/yorkie/api/OperationConverter.kt | 6 +- .../dev/yorkie/document/crdt/CrdtTree.kt | 33 ++++++++ .../kotlin/dev/yorkie/document/crdt/Rht.kt | 57 ++++++++++++-- .../dev/yorkie/document/crdt/TreeInfo.kt | 2 + .../dev/yorkie/document/json/JsonTree.kt | 27 ++++++- .../document/operation/OperationInfo.kt | 3 +- .../document/operation/TreeStyleOperation.kt | 38 ++++++--- .../kotlin/dev/yorkie/api/ConverterTest.kt | 3 +- .../dev/yorkie/document/crdt/RhtTest.kt | 11 +++ .../dev/yorkie/document/json/JsonTreeTest.kt | 77 ++++++++++++++++++- 12 files changed, 263 insertions(+), 24 deletions(-) diff --git a/yorkie/proto/yorkie/v1/resources.proto b/yorkie/proto/yorkie/v1/resources.proto index ad27ca5c8..557833c7e 100644 --- a/yorkie/proto/yorkie/v1/resources.proto +++ b/yorkie/proto/yorkie/v1/resources.proto @@ -133,6 +133,7 @@ message Operation { TreePos to = 3; map attributes = 4; TimeTicket executed_at = 5; + repeated string attributes_to_remove = 6; } oneof body { diff --git a/yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeTest.kt b/yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeTest.kt index 0b95696d2..10b5ab5a3 100644 --- a/yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeTest.kt +++ b/yorkie/src/androidTest/kotlin/dev/yorkie/document/json/JsonTreeTest.kt @@ -2070,6 +2070,35 @@ class JsonTreeTest { } } + @Test + fun test_sync_content_with_remove_style() { + withTwoClientsAndDocuments(syncMode = Manual) { c1, c2, d1, d2, _ -> + updateAndSync( + Updater(c1, d1) { root, _ -> + root.setNewTree( + "t", + element("doc") { + element("p") { + attr { "italic" to "true" } + text { "hello" } + } + }, + ) + }, + Updater(c2, d2), + ) + assertTreesXmlEquals("

hello

", d1, d2) + + updateAndSync( + Updater(c1, d1) { root, _ -> + root.rootTree().removeStyle(0, 1, listOf("italic")) + }, + Updater(c2, d2), + ) + assertTreesXmlEquals("

hello

", d1, d2) + } + } + companion object { fun JsonObject.rootTree() = getAs("t") diff --git a/yorkie/src/main/kotlin/dev/yorkie/api/OperationConverter.kt b/yorkie/src/main/kotlin/dev/yorkie/api/OperationConverter.kt index ac7c2b10d..081413172 100644 --- a/yorkie/src/main/kotlin/dev/yorkie/api/OperationConverter.kt +++ b/yorkie/src/main/kotlin/dev/yorkie/api/OperationConverter.kt @@ -106,8 +106,9 @@ internal fun List.toOperations(): List { parentCreatedAt = it.treeStyle.parentCreatedAt.toTimeTicket(), fromPos = it.treeStyle.from.toCrdtTreePos(), toPos = it.treeStyle.to.toCrdtTreePos(), - attributes = it.treeStyle.attributesMap.toMap(), + attributes = it.treeStyle.attributesMap, executedAt = it.treeStyle.executedAt.toTimeTicket(), + attributesToRemove = it.treeStyle.attributesToRemoveList, ) else -> throw IllegalArgumentException("unimplemented operation") @@ -226,9 +227,10 @@ internal fun Operation.toPBOperation(): PBOperation { from = operation.fromPos.toPBTreePos() to = operation.toPos.toPBTreePos() executedAt = operation.executedAt.toPBTimeTicket() - operation.attributes.forEach { (key, value) -> + operation.attributes?.forEach { (key, value) -> attributes[key] = value } + operation.attributesToRemove?.forEach { attributesToRemove.add(it) } } } } diff --git a/yorkie/src/main/kotlin/dev/yorkie/document/crdt/CrdtTree.kt b/yorkie/src/main/kotlin/dev/yorkie/document/crdt/CrdtTree.kt index f26140706..a0e9cdbdf 100644 --- a/yorkie/src/main/kotlin/dev/yorkie/document/crdt/CrdtTree.kt +++ b/yorkie/src/main/kotlin/dev/yorkie/document/crdt/CrdtTree.kt @@ -383,6 +383,35 @@ internal class CrdtTree( } } + fun removeStyle( + range: TreePosRange, + attributeToRemove: List, + executedAt: TimeTicket, + ): List { + val (fromParent, fromLeft) = findNodesAndSplitText(range.first, executedAt) + val (toParent, toLeft) = findNodesAndSplitText(range.second, executedAt) + return buildList { + traverseInPosRange(fromParent, fromLeft, toParent, toLeft) { (node, _), _ -> + if (!node.isRemoved && !node.isText && attributeToRemove.isNotEmpty()) { + attributeToRemove.forEach { key -> + node.removeAttribute(key, executedAt) + } + add( + TreeChange( + type = TreeChangeType.RemoveStyle, + from = toIndex(fromParent, fromLeft), + to = toIndex(toParent, toLeft), + fromPath = toPath(fromParent, fromLeft), + toPath = toPath(toParent, toLeft), + actorID = executedAt.actorID, + attributesToRemove = attributeToRemove, + ), + ) + } + } + } + } + private fun traverseAll( node: CrdtTreeNode, depth: Int = 0, @@ -772,6 +801,10 @@ internal data class CrdtTreeNode private constructor( _attributes.set(key, value, executedAt) } + fun removeAttribute(key: String, executedAt: TimeTicket) { + _attributes.remove(key, executedAt) + } + /** * Marks the node as removed. */ diff --git a/yorkie/src/main/kotlin/dev/yorkie/document/crdt/Rht.kt b/yorkie/src/main/kotlin/dev/yorkie/document/crdt/Rht.kt index fa5ae9755..93359882a 100644 --- a/yorkie/src/main/kotlin/dev/yorkie/document/crdt/Rht.kt +++ b/yorkie/src/main/kotlin/dev/yorkie/document/crdt/Rht.kt @@ -8,8 +8,9 @@ import dev.yorkie.document.time.TimeTicket.Companion.compareTo * For more details about RHT: * @link http://csl.skku.edu/papers/jpdc11.pdf */ -internal class Rht : Iterable { +internal class Rht : Collection { private val nodeMapByKey = mutableMapOf() + private var numberOfRemovedElements = 0 val nodeKeyValueMap: Map get() { @@ -25,14 +26,41 @@ internal class Rht : Iterable { ) { val prev = nodeMapByKey[key] if (prev?.executedAt < executedAt) { - val node = Node(key, value, executedAt) + if (prev?.isRemoved == false) { + numberOfRemovedElements-- + } + val node = Node(key, value, executedAt, false) nodeMapByKey[key] = node } } + /** + * Removes the Element of the given [key]. + */ + fun remove(key: String, executedAt: TimeTicket): String { + val prev = nodeMapByKey[key] + return when { + prev == null -> { + numberOfRemovedElements++ + nodeMapByKey[key] = Node(key, "", executedAt, true) + "" + } + + prev.executedAt < executedAt -> { + if (!prev.isRemoved) { + numberOfRemovedElements++ + } + nodeMapByKey[key] = Node(key, prev.value, executedAt, true) + if (prev.isRemoved) "" else prev.value + } + + else -> "" + } + } + operator fun get(key: String): String? = nodeMapByKey[key]?.value - fun has(key: String): Boolean = key in nodeMapByKey + fun has(key: String): Boolean = nodeMapByKey[key]?.isRemoved == false fun deepCopy(): Rht { val rht = Rht() @@ -47,15 +75,23 @@ internal class Rht : Iterable { * Converts the given [Rht] to XML String. */ fun toXml(): String { - return nodeKeyValueMap.entries.joinToString(" ") { (key, value) -> - "$key=\"$value\"" - } + return nodeMapByKey.filterValues { !it.isRemoved }.entries + .joinToString(" ") { (key, node) -> + "$key=\"${node.value}\"" + } } override fun iterator(): Iterator { return nodeMapByKey.values.iterator() } + override val size: Int + get() = nodeMapByKey.size - numberOfRemovedElements + + override fun containsAll(elements: Collection): Boolean = elements.all { contains(it) } + + override fun contains(element: Node): Boolean = nodeMapByKey[element.key]?.isRemoved == false + override fun equals(other: Any?): Boolean { if (other !is Rht) { return false @@ -67,5 +103,12 @@ internal class Rht : Iterable { return nodeMapByKey.hashCode() } - data class Node(val key: String, val value: String, val executedAt: TimeTicket) + override fun isEmpty(): Boolean = size == 0 + + data class Node( + val key: String, + val value: String, + val executedAt: TimeTicket, + val isRemoved: Boolean, + ) } diff --git a/yorkie/src/main/kotlin/dev/yorkie/document/crdt/TreeInfo.kt b/yorkie/src/main/kotlin/dev/yorkie/document/crdt/TreeInfo.kt index 8ba5aef49..15c3618f2 100644 --- a/yorkie/src/main/kotlin/dev/yorkie/document/crdt/TreeInfo.kt +++ b/yorkie/src/main/kotlin/dev/yorkie/document/crdt/TreeInfo.kt @@ -61,10 +61,12 @@ internal data class TreeChange( val toPath: List, val value: List? = null, val attributes: Map? = null, + val attributesToRemove: List? = null, val splitLevel: Int = 0, ) internal enum class TreeChangeType { Content, Style, + RemoveStyle, } diff --git a/yorkie/src/main/kotlin/dev/yorkie/document/json/JsonTree.kt b/yorkie/src/main/kotlin/dev/yorkie/document/json/JsonTree.kt index f3f29577d..fba177201 100644 --- a/yorkie/src/main/kotlin/dev/yorkie/document/json/JsonTree.kt +++ b/yorkie/src/main/kotlin/dev/yorkie/document/json/JsonTree.kt @@ -70,8 +70,33 @@ public class JsonTree internal constructor( target.createdAt, range.first, range.second, - attributes.toMap(), ticket, + attributes.toMap(), + ), + ) + } + + public fun removeStyle( + fromIndex: Int, + toIndex: Int, + attributesToRemove: List, + ) { + require(fromIndex <= toIndex) { + "from should be less than or equal to to" + } + + val fromPos = target.findPos(fromIndex) + val toPos = target.findPos(toIndex) + val executedAt = context.issueTimeTicket() + target.removeStyle(fromPos to toPos, attributesToRemove, executedAt) + + context.push( + TreeStyleOperation( + target.createdAt, + fromPos, + toPos, + executedAt, + attributesToRemove = attributesToRemove, ), ) } diff --git a/yorkie/src/main/kotlin/dev/yorkie/document/operation/OperationInfo.kt b/yorkie/src/main/kotlin/dev/yorkie/document/operation/OperationInfo.kt index 30a821c9e..42e50cc02 100644 --- a/yorkie/src/main/kotlin/dev/yorkie/document/operation/OperationInfo.kt +++ b/yorkie/src/main/kotlin/dev/yorkie/document/operation/OperationInfo.kt @@ -92,7 +92,8 @@ public sealed class OperationInfo { val from: Int, val to: Int, val fromPath: List, - val attributes: Map, + val attributes: Map = emptyMap(), + val attributesToRemove: List = emptyList(), override var path: String = INITIAL_PATH, ) : OperationInfo(), TreeOperationInfo diff --git a/yorkie/src/main/kotlin/dev/yorkie/document/operation/TreeStyleOperation.kt b/yorkie/src/main/kotlin/dev/yorkie/document/operation/TreeStyleOperation.kt index d926717d7..9682bc830 100644 --- a/yorkie/src/main/kotlin/dev/yorkie/document/operation/TreeStyleOperation.kt +++ b/yorkie/src/main/kotlin/dev/yorkie/document/operation/TreeStyleOperation.kt @@ -14,8 +14,9 @@ internal data class TreeStyleOperation( override val parentCreatedAt: TimeTicket, val fromPos: CrdtTreePos, val toPos: CrdtTreePos, - val attributes: Map, override var executedAt: TimeTicket, + val attributes: Map? = null, + val attributesToRemove: List? = null, ) : Operation() { override val effectedCreatedAt = parentCreatedAt @@ -29,16 +30,33 @@ internal data class TreeStyleOperation( YorkieLogger.e(TAG, "fail to execute, only Tree can execute edit") return emptyList() } - val changes = tree.style(fromPos to toPos, attributes.toMap(), executedAt) - return changes.map { - TreeStyleOpInfo( - it.from, - it.to, - it.fromPath, - it.attributes.orEmpty(), - root.createPath(parentCreatedAt), - ) + return when { + attributes?.isNotEmpty() == true -> { + tree.style(fromPos to toPos, attributes, executedAt).map { + TreeStyleOpInfo( + it.from, + it.to, + it.fromPath, + it.attributes.orEmpty(), + path = root.createPath(parentCreatedAt), + ) + } + } + + attributesToRemove?.isNotEmpty() == true -> { + tree.removeStyle(fromPos to toPos, attributesToRemove, executedAt).map { + TreeStyleOpInfo( + it.from, + it.to, + it.fromPath, + attributesToRemove = it.attributesToRemove.orEmpty(), + path = root.createPath(parentCreatedAt), + ) + } + } + + else -> emptyList() } } diff --git a/yorkie/src/test/kotlin/dev/yorkie/api/ConverterTest.kt b/yorkie/src/test/kotlin/dev/yorkie/api/ConverterTest.kt index 47ab52202..72806f330 100644 --- a/yorkie/src/test/kotlin/dev/yorkie/api/ConverterTest.kt +++ b/yorkie/src/test/kotlin/dev/yorkie/api/ConverterTest.kt @@ -261,8 +261,9 @@ class ConverterTest { CrdtTreeNodeID(InitialTimeTicket, 10), CrdtTreeNodeID(InitialTimeTicket, 10), ), - mapOf("a" to "b"), InitialTimeTicket, + mapOf("a" to "b"), + listOf("a"), ) val converted = listOf( addOperation.toPBOperation(), diff --git a/yorkie/src/test/kotlin/dev/yorkie/document/crdt/RhtTest.kt b/yorkie/src/test/kotlin/dev/yorkie/document/crdt/RhtTest.kt index cf82946e7..19c4dd0c3 100644 --- a/yorkie/src/test/kotlin/dev/yorkie/document/crdt/RhtTest.kt +++ b/yorkie/src/test/kotlin/dev/yorkie/document/crdt/RhtTest.kt @@ -51,6 +51,17 @@ class RhtTest { } } + @Test + fun `should handle remove`() { + target.set(TEST_KEY, TEST_VALUE, TimeTicket.InitialTimeTicket) + assertEquals(TEST_VALUE, target[TEST_KEY]) + assertEquals(1, target.size) + + target.remove(TEST_KEY, TimeTicket.MaxTimeTicket) + assertFalse(target.has(TEST_KEY)) + assertTrue(target.isEmpty()) + } + private fun Rht.toTestString(): String { return nodeKeyValueMap.entries.joinToString("") { "${it.key}:${it.value}" } } diff --git a/yorkie/src/test/kotlin/dev/yorkie/document/json/JsonTreeTest.kt b/yorkie/src/test/kotlin/dev/yorkie/document/json/JsonTreeTest.kt index 254f6d93a..fd8243b27 100644 --- a/yorkie/src/test/kotlin/dev/yorkie/document/json/JsonTreeTest.kt +++ b/yorkie/src/test/kotlin/dev/yorkie/document/json/JsonTreeTest.kt @@ -324,7 +324,7 @@ class JsonTreeTest { 1, listOf(0), mapOf("a" to "b"), - "$.t", + path = "$.t", ), ), actualOperationDeferred.await(), @@ -390,7 +390,7 @@ class JsonTreeTest { 3, listOf(0, 0, 0), mapOf("a" to "b"), - "$.t", + path = "$.t", ), ), actualOperationDeferred.await(), @@ -708,6 +708,79 @@ class JsonTreeTest { }.await() } + @Test + fun `should handle remove style`() = runTest { + val doc = Document(Document.Key("")) + fun JsonObject.tree() = getAs("t") + + doc.updateAsync { root, _ -> + root.setNewTree( + "t", + element("doc") { + element("p") { + element("span") { + attr { "bold" to "true" } + text { "hello" } + } + } + }, + ) + }.await() + assertEquals( + "

hello

", + doc.getRoot().tree().toXml(), + ) + + doc.updateAsync { root, _ -> + root.tree().removeStyle(1, 8, listOf("bold")) + }.await() + assertEquals("

hello

", doc.getRoot().tree().toXml()) + } + + @Test + fun `should handle removal of attributes that do not exist`() = runTest { + val doc = Document(Document.Key("")) + fun JsonObject.tree() = getAs("t") + + doc.updateAsync { root, _ -> + root.setNewTree( + "t", + element("doc") { + element("p") { + element("span") { + attr { "bold" to "true" } + text { "hello" } + } + element("span") { + text { "hi" } + } + } + }, + ) + }.await() + + assertEquals( + "

hellohi

", + doc.getRoot().tree().toXml(), + ) + + doc.updateAsync { root, _ -> + root.tree().removeStyle(1, 12, listOf("italic")) + }.await() + assertEquals( + "

hellohi

", + doc.getRoot().tree().toXml(), + ) + + doc.updateAsync { root, _ -> + root.tree().removeStyle(1, 8, listOf("italic", "bold")) + }.await() + assertEquals( + "

hellohi

", + doc.getRoot().tree().toXml(), + ) + } + companion object { private val DummyContext = ChangeContext( ChangeID.InitialChangeID,