Implement #2785: resort groups using drag & drop

CHANGELOG.md

@@ -14,6 +14,7 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `#
 - We continued to improve the new groups interface:
   - You can now again select multiple groups (and a few related settings were added to the preferences) [#2786](https://github.com/JabRef/jabref/issues/2786).
   - We further improved performance of group operations, especially of the new filter feature [#2852](https://github.com/JabRef/jabref/issues/2852). 
+  - It is now possible to resort groups using drag & drop [#2785](https://github.com/JabRef/jabref/issues/2785).
 - The entry editor got a fresh coat of paint:
   - Homogenize the size of text fields.
   - The buttons were changed to icons.

src/main/java/org/jabref/gui/groups/DroppingMouseLocation.java

@@ -0,0 +1,10 @@
+package org.jabref.gui.groups;
+
+/**
+ * The mouse location within the cell when the dropping gesture occurs.
+ */
+enum DroppingMouseLocation {
+    BOTTOM,
+    CENTER,
+    TOP
+}

src/main/java/org/jabref/gui/groups/GroupNodeViewModel.java

@@ -254,6 +254,51 @@ public void moveTo(GroupNodeViewModel target) {
         //panel.getUndoManager().addEdit(new UndoableMoveGroup(this.groupsRoot, moveChange));
         //panel.markBaseChanged();
         //frame.output(Localization.lang("Moved group \"%0\".", node.getNode().getGroup().getName()));
+    }
+
+    public void moveTo(GroupTreeNode target, int targetIndex) {
+        getGroupNode().moveTo(target, targetIndex);
+    }
+
+    public Optional<GroupTreeNode> getParent() {
+        return groupNode.getParent();
+    }
+
+    public void draggedOn(GroupNodeViewModel target, DroppingMouseLocation mouseLocation) {
+        Optional<GroupTreeNode> targetParent = target.getParent();
+        if (targetParent.isPresent()) {
+            int targetIndex = target.getPositionInParent();
+
+            // In case we want to move an item in the same parent
+            // and the item is moved down, we need to adjust the target index
+            if (targetParent.equals(getParent())) {
+                int sourceIndex = this.getPositionInParent();
+                if (sourceIndex < targetIndex) {
+                    targetIndex--;
+                }
+            }
+
+            // Different actions depending on where the user releases the drop in the target row
+            // Bottom + top -> insert source row before / after this row
+            // Center -> add as child
+            switch (mouseLocation) {
+                case BOTTOM:
+                    this.moveTo(targetParent.get(), targetIndex + 1);
+                    break;
+                case CENTER:
+                    this.moveTo(target);
+                    break;
+                case TOP:
+                    this.moveTo(targetParent.get(), targetIndex);
+                    break;
+            }
+        } else {
+            // No parent = root -> just add
+            this.moveTo(target);
+        }
+    }
 
+    private int getPositionInParent() {
+        return groupNode.getPositionInParent();
     }
 }

src/main/java/org/jabref/gui/groups/GroupTree.css

@@ -34,6 +34,24 @@
     -fx-font-size: 12px;
 }
 
+.tree-table-row-cell:dragOver-bottom {
+    -fx-border-color: #eea82f;
+    -fx-border-width: 0 0 2 0;
+    -fx-padding: 0 0 -2 0;
+}
+
+.tree-table-row-cell:dragOver-center {
+    -fx-border-color: #eea82f;
+    -fx-border-width: 2 2 2 2;
+    -fx-padding: -2 -2 -2 -2;
+}
+
+.tree-table-row-cell:dragOver-top {
+    -fx-border-color: #eea82f;
+    -fx-border-width: 2 0 0 0;
+    -fx-padding: -2 0 0 0;
+}
+
 .tree-table-row-cell:sub > .tree-table-cell {
     -fx-padding: 0.20em 0em 0.20em 0em;
 }

src/main/java/org/jabref/gui/groups/GroupTreeController.java

@@ -25,6 +25,7 @@
 import javafx.scene.control.TreeTableRow;
 import javafx.scene.control.TreeTableView;
 import javafx.scene.input.ClipboardContent;
+import javafx.scene.input.DragEvent;
 import javafx.scene.input.Dragboard;
 import javafx.scene.input.TransferMode;
 import javafx.scene.layout.StackPane;
@@ -60,6 +61,12 @@ public class GroupTreeController extends AbstractController<GroupTreeViewModel>
     @Inject private DialogService dialogService;
     @Inject private TaskExecutor taskExecutor;
 
+    private static void removePseudoClasses(TreeTableRow<GroupNodeViewModel> row, PseudoClass... pseudoClasses) {
+        for (PseudoClass pseudoClass : pseudoClasses) {
+            row.pseudoClassStateChanged(pseudoClass, false);
+        }
+    }
+
     @FXML
     public void initialize() {
         viewModel = new GroupTreeViewModel(stateManager, dialogService, taskExecutor);
@@ -141,6 +148,11 @@ public void initialize() {
         // Set pseudo-classes to indicate if row is root or sub-item ( > 1 deep)
         PseudoClass rootPseudoClass = PseudoClass.getPseudoClass("root");
         PseudoClass subElementPseudoClass = PseudoClass.getPseudoClass("sub");
+
+        // Pseudo-classes for drag and drop
+        PseudoClass dragOverBottom = PseudoClass.getPseudoClass("dragOver-bottom");
+        PseudoClass dragOverCenter = PseudoClass.getPseudoClass("dragOver-center");
+        PseudoClass dragOverTop = PseudoClass.getPseudoClass("dragOver-top");
         groupTree.setRowFactory(treeTable -> {
             TreeTableRow<GroupNodeViewModel> row = new TreeTableRow<>();
             row.treeItemProperty().addListener((ov, oldTreeItem, newTreeItem) -> {
@@ -183,9 +195,25 @@ public void initialize() {
                 Dragboard dragboard = event.getDragboard();
                 if ((event.getGestureSource() != row) && row.getItem().acceptableDrop(dragboard)) {
                     event.acceptTransferModes(TransferMode.MOVE, TransferMode.LINK);
+
+                    removePseudoClasses(row, dragOverBottom, dragOverCenter, dragOverTop);
+                    switch (getDroppingMouseLocation(row, event)) {
+                        case BOTTOM:
+                            row.pseudoClassStateChanged(dragOverBottom, true);
+                            break;
+                        case CENTER:
+                            row.pseudoClassStateChanged(dragOverCenter, true);
+                            break;
+                        case TOP:
+                            row.pseudoClassStateChanged(dragOverTop, true);
+                            break;
+                    }
                 }
                 event.consume();
             });
+            row.setOnDragExited(event -> {
+                removePseudoClasses(row, dragOverBottom, dragOverCenter, dragOverTop);
+            });
 
             row.setOnDragDropped(event -> {
                 Dragboard dragboard = event.getDragboard();
@@ -195,7 +223,7 @@ public void initialize() {
                     Optional<GroupNodeViewModel> source = viewModel.rootGroupProperty().get()
                             .getChildByPath(pathToSource);
                     if (source.isPresent()) {
-                        source.get().moveTo(row.getItem());
+                        source.get().draggedOn(row.getItem(), getDroppingMouseLocation(row, event));
                         success = true;
                     }
                 }
@@ -315,4 +343,17 @@ private void setupClearButtonField(CustomTextField customTextField) {
             LOGGER.error("Failed to decorate text field with clear button", ex);
         }
     }
+
+    /**
+     * Determines where the mouse is in the given row.
+     */
+    private DroppingMouseLocation getDroppingMouseLocation(TreeTableRow<GroupNodeViewModel> row, DragEvent event) {
+        if ((row.getHeight() * 0.25) > event.getY()) {
+            return DroppingMouseLocation.TOP;
+        } else if ((row.getHeight() * 0.75) < event.getY()) {
+            return DroppingMouseLocation.BOTTOM;
+        } else {
+            return DroppingMouseLocation.CENTER;
+        }
+    }
 }

src/main/java/org/jabref/gui/util/RecursiveTreeItem.java

@@ -70,28 +70,28 @@ private void addChildrenListener(T value) {
         children = new FilteredList<>(childrenFactory.call(value));
         children.predicateProperty().bind(Bindings.createObjectBinding(() -> this::showNode, filter));
 
-        children.forEach(this::addAsChild);
+        addAsChildren(children, 0);
 
         children.addListener((ListChangeListener<T>) change -> {
             while (change.next()) {
 
                 if (change.wasRemoved()) {
                     change.getRemoved().forEach(t-> {
-                        final List<TreeItem<T>> itemsToRemove = RecursiveTreeItem.this.getChildren().stream().filter(treeItem -> treeItem.getValue().equals(t)).collect(Collectors.toList());
-
-                        RecursiveTreeItem.this.getChildren().removeAll(itemsToRemove);
+                        final List<TreeItem<T>> itemsToRemove = getChildren().stream().filter(treeItem -> treeItem.getValue().equals(t)).collect(Collectors.toList());
+                        getChildren().removeAll(itemsToRemove);
                     });
                 }
 
                 if (change.wasAdded()) {
-                    change.getAddedSubList().forEach(this::addAsChild);
+                    addAsChildren(change.getAddedSubList(), change.getFrom());
                 }
             }
         });
     }
 
-    private boolean addAsChild(T child) {
-        return RecursiveTreeItem.this.getChildren().add(new RecursiveTreeItem<>(child, getGraphic(), childrenFactory, expandedProperty, filter));
+    private void addAsChildren(List<? extends T> children, int startIndex) {
+        List<RecursiveTreeItem<T>> treeItems = children.stream().map(child -> new RecursiveTreeItem<>(child, getGraphic(), childrenFactory, expandedProperty, filter)).collect(Collectors.toList());
+        getChildren().addAll(startIndex, treeItems);
     }
 
     private boolean showNode(T t) {

src/main/java/org/jabref/model/util/TreeCollector.java

@@ -40,7 +40,7 @@ private TreeCollector(Function<T, List<T>> getChildren, BiConsumer<T, T> addChil
     }
 
     public static <T extends TreeNode<T>> TreeCollector<T> mergeIntoTree(BiPredicate<T, T> equivalence) {
-        return new TreeCollector<T>(
+        return new TreeCollector<>(
                 TreeNode::getChildren,
                 (parent, child) -> child.moveTo(parent),
                 equivalence);

src/test/java/org/jabref/gui/groups/GroupNodeViewModelTest.java

@@ -1,5 +1,7 @@
 package org.jabref.gui.groups;
 
+import java.util.Arrays;
+
 import javafx.collections.FXCollections;
 import javafx.collections.ObservableList;
 
@@ -87,7 +89,71 @@ public void treeOfAutomaticKeywordGroupIsCombined() throws Exception {
         assertEquals(expected, groupViewModel.getChildren());
     }
 
+    @Test
+    public void draggedOnTopOfGroupAddsBeforeIt() throws Exception {
+        GroupNodeViewModel rootViewModel = getViewModelForGroup(new WordKeywordGroup("root", GroupHierarchyType.INCLUDING, "keywords", "A", true, ',', true));
+        WordKeywordGroup groupA = new WordKeywordGroup("A", GroupHierarchyType.INCLUDING, "keywords", "A", true, ',', true);
+        WordKeywordGroup groupB = new WordKeywordGroup("B", GroupHierarchyType.INCLUDING, "keywords", "A > B", true, ',', true);
+        WordKeywordGroup groupC = new WordKeywordGroup("C", GroupHierarchyType.INCLUDING, "keywords", "A > B > B1", true, ',', true);
+        GroupNodeViewModel groupAViewModel = getViewModelForGroup(rootViewModel.addSubgroup(groupA));
+        GroupNodeViewModel groupBViewModel = getViewModelForGroup(rootViewModel.addSubgroup(groupB));
+        GroupNodeViewModel groupCViewModel = getViewModelForGroup(rootViewModel.addSubgroup(groupC));
+
+        groupCViewModel.draggedOn(groupBViewModel, DroppingMouseLocation.TOP);
+
+        assertEquals(Arrays.asList(groupAViewModel, groupCViewModel, groupBViewModel), rootViewModel.getChildren());
+    }
+
+    @Test
+    public void draggedOnBottomOfGroupAddsAfterIt() throws Exception {
+        GroupNodeViewModel rootViewModel = getViewModelForGroup(new WordKeywordGroup("root", GroupHierarchyType.INCLUDING, "keywords", "A", true, ',', true));
+        WordKeywordGroup groupA = new WordKeywordGroup("A", GroupHierarchyType.INCLUDING, "keywords", "A", true, ',', true);
+        WordKeywordGroup groupB = new WordKeywordGroup("B", GroupHierarchyType.INCLUDING, "keywords", "A > B", true, ',', true);
+        WordKeywordGroup groupC = new WordKeywordGroup("C", GroupHierarchyType.INCLUDING, "keywords", "A > B > B1", true, ',', true);
+        GroupNodeViewModel groupAViewModel = getViewModelForGroup(rootViewModel.addSubgroup(groupA));
+        GroupNodeViewModel groupBViewModel = getViewModelForGroup(rootViewModel.addSubgroup(groupB));
+        GroupNodeViewModel groupCViewModel = getViewModelForGroup(rootViewModel.addSubgroup(groupC));
+
+        groupCViewModel.draggedOn(groupAViewModel, DroppingMouseLocation.BOTTOM);
+
+        assertEquals(Arrays.asList(groupAViewModel, groupCViewModel, groupBViewModel), rootViewModel.getChildren());
+    }
+
+    @Test
+    public void draggedOnBottomOfGroupAddsAfterItWhenSourceGroupWasBefore() throws Exception {
+        GroupNodeViewModel rootViewModel = getViewModelForGroup(new WordKeywordGroup("root", GroupHierarchyType.INCLUDING, "keywords", "A", true, ',', true));
+        WordKeywordGroup groupA = new WordKeywordGroup("A", GroupHierarchyType.INCLUDING, "keywords", "A", true, ',', true);
+        WordKeywordGroup groupB = new WordKeywordGroup("B", GroupHierarchyType.INCLUDING, "keywords", "A > B", true, ',', true);
+        WordKeywordGroup groupC = new WordKeywordGroup("C", GroupHierarchyType.INCLUDING, "keywords", "A > B > B1", true, ',', true);
+        GroupNodeViewModel groupAViewModel = getViewModelForGroup(rootViewModel.addSubgroup(groupA));
+        GroupNodeViewModel groupBViewModel = getViewModelForGroup(rootViewModel.addSubgroup(groupB));
+        GroupNodeViewModel groupCViewModel = getViewModelForGroup(rootViewModel.addSubgroup(groupC));
+
+        groupAViewModel.draggedOn(groupBViewModel, DroppingMouseLocation.BOTTOM);
+
+        assertEquals(Arrays.asList(groupBViewModel, groupAViewModel, groupCViewModel), rootViewModel.getChildren());
+    }
+
+    @Test
+    public void draggedOnTopOfGroupAddsBeforeItWhenSourceGroupWasBefore() throws Exception {
+        GroupNodeViewModel rootViewModel = getViewModelForGroup(new WordKeywordGroup("root", GroupHierarchyType.INCLUDING, "keywords", "A", true, ',', true));
+        WordKeywordGroup groupA = new WordKeywordGroup("A", GroupHierarchyType.INCLUDING, "keywords", "A", true, ',', true);
+        WordKeywordGroup groupB = new WordKeywordGroup("B", GroupHierarchyType.INCLUDING, "keywords", "A > B", true, ',', true);
+        WordKeywordGroup groupC = new WordKeywordGroup("C", GroupHierarchyType.INCLUDING, "keywords", "A > B > B1", true, ',', true);
+        GroupNodeViewModel groupAViewModel = getViewModelForGroup(rootViewModel.addSubgroup(groupA));
+        GroupNodeViewModel groupBViewModel = getViewModelForGroup(rootViewModel.addSubgroup(groupB));
+        GroupNodeViewModel groupCViewModel = getViewModelForGroup(rootViewModel.addSubgroup(groupC));
+
+        groupAViewModel.draggedOn(groupCViewModel, DroppingMouseLocation.TOP);
+
+        assertEquals(Arrays.asList(groupBViewModel, groupAViewModel, groupCViewModel), rootViewModel.getChildren());
+    }
+
     private GroupNodeViewModel getViewModelForGroup(AbstractGroup group) {
         return new GroupNodeViewModel(databaseContext, stateManager, taskExecutor, group);
     }
+
+    private GroupNodeViewModel getViewModelForGroup(GroupTreeNode group) {
+        return new GroupNodeViewModel(databaseContext, stateManager, taskExecutor, group);
+    }
 }

src/test/java/org/jabref/model/TreeNodeTest.java

@@ -172,6 +172,36 @@ public void moveToInSameLevelAddsAtEnd() {
         assertEquals(Arrays.asList(child2, child1), root.getChildren());
     }
 
+    @Test
+    public void moveToInSameLevelWhenNodeWasBeforeTargetIndex() {
+        TreeNodeTestData.TreeNodeMock root = new TreeNodeTestData.TreeNodeMock();
+        TreeNodeTestData.TreeNodeMock child1 = new TreeNodeTestData.TreeNodeMock();
+        TreeNodeTestData.TreeNodeMock child2 = new TreeNodeTestData.TreeNodeMock();
+        TreeNodeTestData.TreeNodeMock child3 = new TreeNodeTestData.TreeNodeMock();
+        root.addChild(child1);
+        root.addChild(child2);
+        root.addChild(child3);
+
+        child1.moveTo(root, 1);
+
+        assertEquals(Arrays.asList(child2, child1, child3), root.getChildren());
+    }
+
+    @Test
+    public void moveToInSameLevelWhenNodeWasAfterTargetIndex() {
+        TreeNodeTestData.TreeNodeMock root = new TreeNodeTestData.TreeNodeMock();
+        TreeNodeTestData.TreeNodeMock child1 = new TreeNodeTestData.TreeNodeMock();
+        TreeNodeTestData.TreeNodeMock child2 = new TreeNodeTestData.TreeNodeMock();
+        TreeNodeTestData.TreeNodeMock child3 = new TreeNodeTestData.TreeNodeMock();
+        root.addChild(child1);
+        root.addChild(child2);
+        root.addChild(child3);
+
+        child3.moveTo(root, 1);
+
+        assertEquals(Arrays.asList(child1, child3, child2), root.getChildren());
+    }
+
     @Test
     public void getPathFromRootInSimpleTree() {
         TreeNodeTestData.TreeNodeMock root = new TreeNodeTestData.TreeNodeMock();