From e148df86773e5e3d2ea6186b8e428b93c72856ec Mon Sep 17 00:00:00 2001 From: "Florine W. Dekker" Date: Fri, 10 Nov 2023 20:38:37 +0100 Subject: [PATCH] Fix JTemplateTree dnd and view-related bugs Mostly fixes #485, except for the reset causing UUID duplication. --- .../randomness/template/TemplateJTree.kt | 73 +++++--- .../randomness/template/TemplateJTreeModel.kt | 166 ++++++++++++++---- src/main/resources/randomness.properties | 2 +- .../fwdekker/randomness/InsertActionTest.kt | 2 +- 4 files changed, 177 insertions(+), 66 deletions(-) diff --git a/src/main/kotlin/com/fwdekker/randomness/template/TemplateJTree.kt b/src/main/kotlin/com/fwdekker/randomness/template/TemplateJTree.kt index f656c99ab..243551a5b 100644 --- a/src/main/kotlin/com/fwdekker/randomness/template/TemplateJTree.kt +++ b/src/main/kotlin/com/fwdekker/randomness/template/TemplateJTree.kt @@ -22,6 +22,7 @@ import com.intellij.ui.AnActionButton import com.intellij.ui.ColoredTreeCellRenderer import com.intellij.ui.CommonActionsPanel import com.intellij.ui.LayeredIcon +import com.intellij.ui.RowsDnDSupport.RefinedDropSupport.Position import com.intellij.ui.SimpleTextAttributes import com.intellij.ui.ToolbarDecorator import com.intellij.ui.TreeSpeedSearch @@ -144,6 +145,8 @@ class TemplateJTree( isRootVisible = false showsRootHandles = true selectionModel.selectionMode = TreeSelectionModel.SINGLE_TREE_SELECTION + myModel.viewIndexToModelIndex = { myModel.root.descendants.indexOf(getPathForRow(it).lastPathComponent) } + myModel.wrapDrop = ::runPreservingState setCellRenderer(CellRenderer()) setSelectionRow(0) @@ -250,7 +253,7 @@ class TemplateJTree( selectionPath = myModel.getPathToRoot( if (myModel.isLeaf(parent)) parent - else parent.children[min(oldIndex, parent.children.count() - 1)] + else parent.children[min(oldIndex, parent.children.lastIndex)] ) } @@ -275,47 +278,65 @@ class TemplateJTree( } } + /** + * Returns `true` if and only if [moveSchemeByOnePosition] can be invoked with these parameters. + * + * @see TemplateJTreeModel.canMoveRow + */ + fun canMoveSchemeByOnePosition(scheme: Scheme, moveDown: Boolean): Boolean { + val (fromIndex, toIndex, position) = getMoveDescriptor(scheme, moveDown) + return myModel.canMoveRow(fromIndex, toIndex, position) + } + /** * Moves [scheme] by one position; down if [moveDown] is `true, and up otherwise. * - * If a non-[Template] is moved up or down outside its parent's boundaries, it is moved to the next parent in that - * direction. + * @see TemplateJTreeModel.moveRow */ fun moveSchemeByOnePosition(scheme: Scheme, moveDown: Boolean) { val node = StateNode(scheme) runPreservingState { - myModel.exchangeRows( - myModel.root.descendants.indexOf(node), - getMoveTargetIndex(scheme, moveDown) - ) + val (fromIndex, toIndex, position) = getMoveDescriptor(scheme, moveDown) + myModel.moveRow(fromIndex, toIndex, position) } makeVisible(myModel.getPathToRoot(node)) } /** - * Returns `true` if and only if [moveSchemeByOnePosition] can be invoked with these parameters. + * Returns the arguments to pass to [TemplateJTreeModel.moveRow] or [TemplateJTreeModel.canMoveRow] to describe + * moving [scheme] up (if [moveDown] is `false`) or down (if [moveDown] is `true`). + * + * If an invalid move is returned, for example with negative indices, the move was not possible to begin with. + * + * @see canMoveSchemeByOnePosition + * @see moveSchemeByOnePosition */ - fun canMoveSchemeByOnePosition(scheme: Scheme, moveDown: Boolean) = - myModel.canExchangeRows( - myModel.root.descendants.indexOf(StateNode(scheme)), - getMoveTargetIndex(scheme, moveDown) - ) + @Suppress("detekt:CognitiveComplexMethod", "detekt:MagicNumber") // Complexity unavoidable, -2 is not magic + private fun getMoveDescriptor(scheme: Scheme, moveDown: Boolean): Triple { + val descendants = myModel.root.descendants + val templates = myModel.root.children - /** - * Returns the index to which [scheme] is moved (down if [moveDown] is `true`, or up otherwise) when - * [moveSchemeByOnePosition] is invoked, or an out-of-range index if [scheme] cannot be moved in that direction. - */ - private fun getMoveTargetIndex(scheme: Scheme, moveDown: Boolean): Int { - val node = StateNode(scheme) + val fromNode = StateNode(scheme) + val (toNode, position) = + if (scheme is Template) { + val index = templates.indexOf(fromNode) - val children = myModel.root.children - val descendants = myModel.root.descendants - val diff = if (moveDown) 1 else -1 + if (index == templates.lastIndex - 1) Pair(descendants.last(), Position.BELOW) + else Pair(templates.getOrNull(index + if (!moveDown) -1 else 2), Position.ABOVE) + } else { + val index = descendants.indexOf(fromNode) + val candidate = descendants.getOrNull(index + if (!moveDown) -2 else 1) + + when { + candidate == null || candidate.state !is Template -> Pair(candidate, Position.BELOW) + candidate.children.isEmpty() -> Pair(candidate, Position.INTO) + else -> Pair(candidate.children.first(), Position.ABOVE) + } + } - return if (scheme !is Template) descendants.indexOf(node) + diff - else descendants.indexOf(children.getOrNull(children.indexOf(node) + diff)) + return Triple(descendants.indexOf(fromNode), descendants.indexOf(toNode), position) } @@ -501,7 +522,7 @@ class TemplateJTree( override fun onChosen(value: Scheme?, finalChoice: Boolean) = when (value) { POPUP_STEP_SCHEMES[0] -> TemplatesPopupStep() - POPUP_STEP_SCHEMES[POPUP_STEP_SCHEMES.size - 1] -> ReferencesPopupStep() + POPUP_STEP_SCHEMES[POPUP_STEP_SCHEMES.lastIndex] -> ReferencesPopupStep() else -> super.onChosen(value, finalChoice) } @@ -514,7 +535,7 @@ class TemplateJTree( * Returns `true` if and only if [value] equals the [Template] or [TemplateReference] entry. */ override fun hasSubstep(value: Scheme?) = - value == POPUP_STEP_SCHEMES[0] || value == POPUP_STEP_SCHEMES[POPUP_STEP_SCHEMES.size - 1] + value == POPUP_STEP_SCHEMES[0] || value == POPUP_STEP_SCHEMES[POPUP_STEP_SCHEMES.lastIndex] /** * Returns a separator if [value] should be preceded by a separator, or `null` otherwise. diff --git a/src/main/kotlin/com/fwdekker/randomness/template/TemplateJTreeModel.kt b/src/main/kotlin/com/fwdekker/randomness/template/TemplateJTreeModel.kt index ffa55f91d..a4b568baa 100644 --- a/src/main/kotlin/com/fwdekker/randomness/template/TemplateJTreeModel.kt +++ b/src/main/kotlin/com/fwdekker/randomness/template/TemplateJTreeModel.kt @@ -4,7 +4,10 @@ import com.fwdekker.randomness.Bundle import com.fwdekker.randomness.Scheme import com.fwdekker.randomness.State import com.fwdekker.randomness.setAll +import com.intellij.ui.RowsDnDSupport.RefinedDropSupport +import com.intellij.ui.RowsDnDSupport.RefinedDropSupport.Position import com.intellij.util.ui.EditableModel +import javax.swing.JComponent import javax.swing.JTree import javax.swing.event.TreeModelEvent import javax.swing.event.TreeModelListener @@ -22,7 +25,9 @@ import javax.swing.tree.TreePath * @param list the list to be modeled */ @Suppress("detekt:TooManyFunctions") // Normal for Swing implementations -class TemplateJTreeModel(list: TemplateList = TemplateList(mutableListOf())) : TreeModel, EditableModel { +class TemplateJTreeModel( + list: TemplateList = TemplateList(mutableListOf()), +) : TreeModel, EditableModel, RefinedDropSupport { /** * The listeners that are informed when the state of the tree changes. */ @@ -38,6 +43,35 @@ class TemplateJTreeModel(list: TemplateList = TemplateList(mutableListOf())) : T */ private val nodes get() = listOf(root) + root.descendants + /** + * Converts the row index in the view to the corresponding row index in the model. + * + * For example, if the model has rows `[0, 3]` with children `[1, 2]` and `[4]`, respectively, and row `0` is + * collapsed in the view, then the view has rows `[0, 3, 4]` while the model has rows `[0, 1, 2, 3, 4]`, and this + * method functions as the map `{0: 0, 1: 3, 2: 4}`. Inputs outside the view's valid row indices are not supported. + * + * This field is used only in the methods [canDrop] and [drop] in order to implement [RefinedDropSupport], which + * assumes that the model knows view-based indices. + * + * @see RefinedDropSupport + */ + var viewIndexToModelIndex: (Int) -> Int = { it } + + /** + * A wrapper provided by the view and used by [TemplateJTreeModel.drop] to ensure the view's selection and expansion + * state are retained. + * + * This wrapper is invoked in the method [drop] with a lambda that performs the dropping when invoked. By default, + * the wrapper does nothing special, but can be overridden to perform some actions before and after dropping the + * [StateNode]. + * + * This field is used only in the method [drop] in order to implement [RefinedDropSupport], which assumes that the + * model can somehow retain the view's state. + * + * @see RefinedDropSupport + */ + var wrapDrop: (() -> Unit) -> Unit = { it() } + /** * Returns [root]. @@ -108,66 +142,122 @@ class TemplateJTreeModel(list: TemplateList = TemplateList(mutableListOf())) : T } /** - * Exchanges two consecutive rows, moving the node at row [oldIndex] to row [newIndex]. + * Use [moveRow] instead. + * + * @throws UnsupportedOperationException always + * @see moveRow + */ + @Throws(UnsupportedOperationException::class) + override fun exchangeRows(oldIndex: Int, newIndex: Int) = throw UnsupportedOperationException() + + /** + * Use [canMoveRow] instead. + * + * @throws UnsupportedOperationException always + * @see canMoveRow + */ + @Throws(UnsupportedOperationException::class) + override fun canExchangeRows(oldIndex: Int, newIndex: Int): Boolean = throw UnsupportedOperationException() + + /** + * Returns `true` if and only if [moveRow] can be invoked with the given parameters. * - * If [oldIndex] and [newIndex] both refer to a template or both refer to a scheme, then the node at [oldIndex] will - * be inserted so that it has the same index in its parent as the node at [newIndex] had before invoking this - * method. Otherwise, if [oldIndex] refers to a non-template scheme and [newIndex] refers to a template, then the - * non-template scheme is moved into the template at [newIndex]; specifically, if [oldIndex] is less than [newIndex] - * then the non-template scheme becomes the template's first child, otherwise it becomes the template's last child. + * @see moveRow */ - override fun exchangeRows(oldIndex: Int, newIndex: Int) { - require(canExchangeRows(oldIndex, newIndex)) { - Bundle("template_list.error.cannot_swap_rows", oldIndex, newIndex) + fun canMoveRow(fromIndex: Int, toIndex: Int, position: Position): Boolean { + val descendants = root.descendants + val fromNode = descendants.getOrNull(fromIndex) + val toNode = descendants.getOrNull(toIndex) + + return when { + fromIndex == toIndex || fromNode == null || toNode == null -> false + fromNode.state !is Template -> (toNode.state !is Template) xor (position == Position.INTO) + position != Position.ABOVE -> position == Position.BELOW && toIndex == descendants.lastIndex + else -> toNode.state is Template && toNode != root.children.run { getOrNull(indexOf(fromNode) + 1) } + } + } + + /** + * Moves the node at row [fromIndex] near the node at row [toIndex] as specified by [position]. + * + * If [fromIndex] refers to a [Template], then the [Template] can either be moved [Position.ABOVE] another + * [Template], or [Position.BELOW] the very last [Scheme] of the [TemplateList]. + * + * If [fromIndex] refers to a non-[Template] [Scheme], then the [Scheme] can either be moved [Position.INTO] a + * [Template] to append it to that [Template], or [Position.ABOVE] or [Position.BELOW] another non-[Template] + * [Scheme]. + * + * @throws IllegalArgumentException if [canMoveRow] returns `false` for the given arguments + */ + fun moveRow(fromIndex: Int, toIndex: Int, position: Position) { + require(canMoveRow(fromIndex, toIndex, position)) { + Bundle("template_list.error.cannot_move_row", fromIndex, position.name, toIndex) } - val nodeToMove = root.descendants[oldIndex] - val targetNode = root.descendants[newIndex] + val descendants = root.descendants + val fromNode = descendants[fromIndex] + val toNode = descendants[toIndex] - val targetNodeParent: StateNode - val targetIndexInParent: Int - if (nodeToMove.state !is Template && targetNode.state is Template) { - if (newIndex < oldIndex) { - targetNodeParent = root.children[root.children.indexOf(targetNode) - 1] - targetIndexInParent = targetNodeParent.children.count() - } else { - targetNodeParent = targetNode - targetIndexInParent = 0 + removeNode(fromNode) + + if (fromNode.state is Template) { + when (position) { + Position.ABOVE -> insertNode(root, fromNode, root.children.indexOf(toNode)) + Position.BELOW -> insertNode(root, fromNode) + Position.INTO -> error("Bug: 'canMoveRow' should have caught this case.") } } else { - targetNodeParent = getParentOf(targetNode)!! - targetIndexInParent = targetNodeParent.children.indexOf(targetNode) + val toNodeParent = getParentOf(toNode)!! + when (position) { + Position.ABOVE -> insertNode(toNodeParent, fromNode, toNodeParent.children.indexOf(toNode)) + Position.BELOW -> insertNodeAfter(toNodeParent, toNode, fromNode) + Position.INTO -> insertNode(toNode, fromNode) + } } - - removeNode(nodeToMove) - insertNode(targetNodeParent, nodeToMove, targetIndexInParent) } /** - * Returns `true` if and only if the node at row [oldIndex] can be moved to row [newIndex]. + * Invokes [canMoveRow] after converting [fromIndex] and [toIndex] using [viewIndexToModelIndex]. + * + * @see canMoveRow + * @see RefinedDropSupport */ - override fun canExchangeRows(oldIndex: Int, newIndex: Int): Boolean { - val oldNode = root.descendants.getOrNull(oldIndex) - val newNode = root.descendants.getOrNull(newIndex) + override fun canDrop(fromIndex: Int, toIndex: Int, position: Position) = + canMoveRow(viewIndexToModelIndex(fromIndex), viewIndexToModelIndex(toIndex), position) - return oldNode != null && newNode != null && - if (oldNode.state is Template) newNode.state is Template - else newIndex != 0 - } + /** + * Returns `true` if and only if the node at [fromIndex] can be moved [Position.INTO] the node at [toIndex]. + * + * @see canMoveRow + * @see RefinedDropSupport + */ + override fun isDropInto(component: JComponent, fromIndex: Int, toIndex: Int) = + canDrop(fromIndex, toIndex, Position.INTO) + + /** + * Invokes [moveRow] after converting [fromIndex] and [toIndex] using [viewIndexToModelIndex]. + * + * @see moveRow + * @see RefinedDropSupport + */ + override fun drop(fromIndex: Int, toIndex: Int, position: Position) = + wrapDrop { moveRow(viewIndexToModelIndex(fromIndex), viewIndexToModelIndex(toIndex), position) } /** - * Returns `true` if and only if [node] does not have children. + * Returns `true` if and only if [node] is contained in this model and [node] does not have children. * - * Throws an exception if [node] is not a [StateNode] or if [node] is not contained in this model. + * Throws an exception if [node] is not a [StateNode]. + * + * Unlike methods such as [getChild], this method does not throw an exception if [node] is not contained in this + * model, because [com.intellij.ui.tree.ui.DefaultTreeUI] calls this method on non-nodes during drag-and-drop. */ override fun isLeaf(node: Any): Boolean { require(node is StateNode) { Bundle("template_list.error.unknown_node_type", "node", node.javaClass.canonicalName) } - require(node in nodes) { Bundle("template_list.error.node_not_in_tree") } - return !node.canHaveChildren || node.children.isEmpty() + return node in nodes && (!node.canHaveChildren || node.children.isEmpty()) } /** diff --git a/src/main/resources/randomness.properties b/src/main/resources/randomness.properties index 08fbd971a..80b3f76fe 100644 --- a/src/main/resources/randomness.properties +++ b/src/main/resources/randomness.properties @@ -122,8 +122,8 @@ template.name.unknown= template.ui.name_option=&Name: template.title=Template template_list.error.cannot_insert_root='TemplateList' cannot have parent so cannot be inserted. +template_list.error.cannot_move_row=Cannot move row %1$d %2$s row %3$d. template_list.error.cannot_remove_root=Cannot remove root from model. -template_list.error.cannot_swap_rows=Cannot swap row at index %1$d with row at index %2$d. template_list.error.duplicate_name=Template names should be unique. Rename template '%1$s'. template_list.error.failed_to_save_settings=Failed To Save Settings template_list.error.infertile_parent=Cannot access child of parent that cannot have children. diff --git a/src/test/kotlin/com/fwdekker/randomness/InsertActionTest.kt b/src/test/kotlin/com/fwdekker/randomness/InsertActionTest.kt index 4c747dc93..50452a69b 100644 --- a/src/test/kotlin/com/fwdekker/randomness/InsertActionTest.kt +++ b/src/test/kotlin/com/fwdekker/randomness/InsertActionTest.kt @@ -14,7 +14,7 @@ import io.kotest.matchers.shouldBe /** * Integration tests for [InsertAction]. */ -@Suppress("detekt:FunctionMaxLength", "detekt:FunctionName", "detekt:TooManyFunctions") +@Suppress("detekt:FunctionMaxLength", "detekt:FunctionName", "detekt:TooManyFunctions") // Acceptable for tests class InsertActionTest : BasePlatformTestCase() { private lateinit var document: Document private lateinit var caretModel: CaretModel