TreeGrid: DragItem null after dragging root item to another hierarchy and then somewhere else

[sirbris]

Description

After I dragged a root item to another hierarchy and try to move it again, the draggedItems list in the GridDragStartEvent has a null item inside. The problem only seems to be with dragging root items to another hierarchy and then trying do drag that item again.

Side note: The refreshDataProvider is called, but maybe not in time (async)!? Calling it manually (e.g. via extra button) before dragging the just dropped item again seemed to help in my other test case.

Expected outcome

I would expect the dragItem to never be null.

Minimal reproducible example

public class MainView extends HorizontalLayout {

    TreeGrid<String> tree = new TreeGrid<>(String.class);
    List<String> draggedItems = null;

    public MainView() {
        setMargin(true);

        tree.removeAllColumns();
        tree.addHierarchyColumn(item -> item).setHeader("Column");
        tree.addColumn(item -> item.length() + "");
        tree.addComponentColumn(item -> VaadinIcon.ACCESSIBILITY.create());
        TreeData<String> treeData = new TreeData<>();
        TreeDataProvider<String> provider = new TreeDataProvider<>(treeData);
        tree.setDataProvider(provider);
        treeData.addRootItems("A", "B", "C");
        treeData.addItems("A", "A1", "A2");
        treeData.addItems("A1", "C1", "C2");
        treeData.addItems("C1", "D1", "D2");
        treeData.addItems("B", "B1", "B2");

        tree.expand("A");
        tree.getDataProvider().refreshAll();
        tree.getDataCommunicator().reset();
        add(tree);

        initDragAndDrop();
    }

    public void initDragAndDrop() {
        tree.setRowsDraggable(true);
        tree.setDropMode(GridDropMode.ON_TOP_OR_BETWEEN);
        tree.addDragStartListener(event -> {
            draggedItems = event.getDraggedItems();
        });
        tree.addDragEndListener(event -> {
            draggedItems = null;
        });

        tree.addDropListener(event -> {
            String dropItem = event.getDropTargetItem().orElse(null);
            if (draggedItems == null || draggedItems.size() == 0) {
                return;
            }
            handleDrop(event, dropItem);
        });
    }

    private void handleDrop(GridDropEvent<String> inEvent, String dropItem) {
        if (draggedItems == null) {
            return;
        }
        TreeData<String> treeData = tree.getTreeData();
        int dropIndex = 0;
        List<String> dropChildItems;
        boolean justAddAtTheEnd = false;
        String newParent = dropItem == null ? null : treeData.getParent(dropItem);

        if (dropItem == null || treeData.getChildren(dropItem).size() > 0 && inEvent.getDropLocation() == GridDropLocation.ON_TOP) {
            dropChildItems = treeData.getChildren(dropItem);
            dropIndex = dropChildItems.size();
            justAddAtTheEnd = true;
        } else {
            if (inEvent.getDropLocation() == GridDropLocation.BELOW && treeData.getChildren(dropItem).size() > 0 && tree.isExpanded(dropItem)) {
                // dropped on open folder below so insert into that folder
                dropChildItems = tree.getTreeData().getChildren(dropItem);
                newParent = dropItem;
                dropIndex = 0;
            } else {
                dropChildItems = tree.getTreeData().getChildren(treeData.getParent(dropItem));
                dropIndex = dropChildItems.indexOf(dropItem);
                if (inEvent.getDropLocation() == GridDropLocation.BELOW) {
                    dropIndex++;
                }
            }
        }

        try {
            for (String dragItem : draggedItems) {
                if (dragItem == null) {
                    Notification.show("drag item null"); // FIXME: shouldn't happen
                    return;
                }
                if (dragItem == dropItem) {
                    // dropped on itself
                    return;
                }
                // Make sure that the target is not in the subtree of the dragged item itself
                // ...
                if (justAddAtTheEnd) {
                    treeData.setParent(dragItem, dropItem);
                    tree.expand(dropItem);
                } else {
                    treeData.setParent(dragItem, newParent);

                    boolean toggle = true;
                    if (dropIndex >= dropChildItems.size()) {
                        dropIndex = dropChildItems.size() - 1;
                        toggle = false; // don't toggle items if dropped at the end
                    }
                    String sibling = dropChildItems.get(dropIndex);

                    treeData.moveAfterSibling(dragItem, sibling);
                    if (toggle) {
                        // toggle items with this so drop at index 0 is possible
                        treeData.moveAfterSibling(sibling, dragItem);
                    }
                }
            }
        } catch (Exception ex) {
            ex.printStackTrace();
        }
        tree.getDataCommunicator().reset();
        tree.getDataProvider().refreshAll();
    }

}

Steps to reproduce

Example 1:

1. Expand B
2. Drag C in B
3. Try to drag C -> drag item null

Example 2:

1. Drag/drop A1 above A to root
2. Drag A1 back in A to same place
3. Drag/drop A1 above A again to root -> drag item null

Environment

Vaadin version(s): 23.1.4

Browsers

Chrome, Firefox

[sirbris]

Example 3:

1. Drag C between A1 and A2
2. Now drag A2 on top of C -> A2 will be moved to root since the drop item C is recognized as null

[yuriy-fix]

Simplified reproducible example:

public class BugPage extends Div {

    String draggedItem;

    public BugPage() {
        TreeGrid<String> grid = new TreeGrid<>();
        grid.addHierarchyColumn(item -> item);

        // DATA
        TreeData<String> treeData = new TreeData<>();
        treeData.addRootItems("A", "B", "C");
        treeData.addItems("B", "B1");

        TreeDataProvider<String> provider = new TreeDataProvider<>(treeData);
        grid.setDataProvider(provider);

        // SHOULD BE EXPANDED
        grid.expand("B");

        // DRAG AND DROP
        grid.setRowsDraggable(true);
        grid.setDropMode(GridDropMode.ON_TOP);

        grid.addDragStartListener(event -> {

            // TO REPRODUCE:
            //  1. MOVE ITEM "C" TO ITEM "B"
            //  2. MOVE ITEM "C" TO ITEM "A"

            // BUG REPRODUCED IF TRUE
            System.out.println(event.getDraggedItems().contains(null));

            draggedItem = event.getDraggedItems().get(0);
        });

        grid.addDropListener(event -> {
            if (draggedItem == null) {
                // TO AVOID NPE IN CONSOLE, SO THE LOG ABOVE IS VISIBLE
                return;
            }

            String dropTargetItem = event.getDropTargetItem().orElse(null);
            treeData.setParent(draggedItem, dropTargetItem);
            provider.refreshAll();
        });

        grid.addDragEndListener(event -> draggedItem = null);

        // CALLING `refreshAll` VIA BUTTON CLICK AFTER 1. STEP (MOVING ITEM "C" TO "B") SOLVES IT
        Button button = new Button("Refresh all", e -> provider.refreshAll());
        add(button);

        add(grid);
    }
}

[yuriy-fix]

Without D&D logic it would look like this:

TreeGrid<String> grid = new TreeGrid<>();
grid.addHierarchyColumn(item -> item);

// DATA
TreeData<String> treeData = new TreeData<>();
treeData.addRootItems("A", "B", "C");
treeData.addItems("B", "B1");

TreeDataProvider<String> provider = new TreeDataProvider<>(treeData);
grid.setDataProvider(provider);

// SHOULD BE EXPANDED
grid.expand("B");

// DIV
Div div = new Div();

Button prepareBtn = new Button("Step 1", e -> {
    // ITEM IS AVAILABLE BEFORE CHANGING PARENT
    System.out.println(grid.getDataCommunicator().getKeyMapper().has("C"));

    treeData.setParent("C", "B");
    provider.refreshAll();
});

Button reproduceBtn = new Button("Step 2", e -> {
    if (!grid.getDataCommunicator().getKeyMapper().has("C")) {
        div.setText("Item is not available in the KeyMapper");
    }
});

add(grid, div, prepareBtn, reproduceBtn);

[yuriy-fix]

@mshabarov, it seems that issue could be fixed on the Flow side.

[vaadin-bot]

This ticket/PR has been released with Vaadin 24.2.0.rc1 and is also targeting the upcoming stable 24.2.0 version.

[vaadin-bot]

This ticket/PR has been released with Vaadin 23.3.26.

[vaadin-bot]

This ticket/PR has been released with Vaadin 24.3.0.alpha1 and is also targeting the upcoming stable 24.3.0 version.