From fc208da14d1c8f12a71f002075be2cb33f243a45 Mon Sep 17 00:00:00 2001 From: Soroosh Taefi Date: Tue, 3 Nov 2020 09:37:28 +0200 Subject: [PATCH] fix: Sync HierarchicalDataCommunicator's expand state with client side (#9275) HierarchicalDataCommunicator's expanded state were not being synchronised with client side and TreeGrid's expanded nodes were being collapsed after re-attaching. Warranty: Sync TreeGrid expanded items state with the client side when detaching and reattaching Fixes: https://github.com/vaadin/flow/issues/9175 (cherry picked from commit bb51a2b5d3e04e28994f1fc619e7cf0848596625) --- .../HierarchicalDataCommunicator.java | 13 ++ .../provider/hierarchy/HierarchyMapper.java | 27 +++- .../HierarchicalCommunicatorTest.java | 85 +++++++++- .../HierarchyMapperWithDataTest.java | 152 ++++++++++++++++++ 4 files changed, 265 insertions(+), 12 deletions(-) diff --git a/flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalDataCommunicator.java b/flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalDataCommunicator.java index 4f1b2f65634..a39d3032fe5 100644 --- a/flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalDataCommunicator.java +++ b/flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalDataCommunicator.java @@ -158,6 +158,19 @@ public void reset() { HierarchicalUpdate update = arrayUpdater .startUpdate(getHierarchyMapper().getRootSize()); update.enqueue("$connector.ensureHierarchy"); + + Collection expandedItems = getHierarchyMapper().getExpandedItems(); + update.enqueue("$connector.expandItems", + expandedItems + .stream() + .map(getKeyMapper()::key) + .map(key -> { + JsonObject json = Json.createObject(); + json.put("key", key); + return json; + }).collect( + JsonUtils.asArray())); + requestFlush(update); } } diff --git a/flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchyMapper.java b/flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchyMapper.java index 02043b1e4c7..bdb21284fad 100644 --- a/flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchyMapper.java +++ b/flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchyMapper.java @@ -16,6 +16,7 @@ package com.vaadin.flow.data.provider.hierarchy; import java.io.Serializable; +import java.util.Collection; import java.util.Collections; import java.util.Comparator; import java.util.HashMap; @@ -63,7 +64,7 @@ public class HierarchyMapper implements Serializable { private List backEndSorting; private Comparator inMemorySorting; - private Set expandedItemIds = new HashSet<>(); + private Map expandedItems = new HashMap<>(); /** * Constructs a new HierarchyMapper. @@ -132,7 +133,7 @@ public boolean isExpanded(T item) { // Root nodes are always visible. return true; } - return expandedItemIds.contains(getDataProvider().getId(item)); + return expandedItems.containsKey(getDataProvider().getId(item)); } /** @@ -177,7 +178,7 @@ public Range expand(T item, Integer position) { private boolean doExpand(T item) { boolean expanded = false; if (!isExpanded(item) && hasChildren(item)) { - expandedItemIds.add(getDataProvider().getId(item)); + expandedItems.put(getDataProvider().getId(item), item); expanded = true; } return expanded; @@ -194,7 +195,7 @@ public boolean collapse(T item) { return false; } if (isExpanded(item)) { - expandedItemIds.remove(getDataProvider().getId(item)); + expandedItems.remove(getDataProvider().getId(item)); return true; } return false; @@ -217,7 +218,7 @@ public Range collapse(T item, Integer position) { removedRows = Range.withLength(position + 1, (int) getHierarchy(item, false).count()); } - expandedItemIds.remove(getDataProvider().getId(item)); + expandedItems.remove(getDataProvider().getId(item)); } return removedRows; } @@ -439,7 +440,7 @@ protected void removeChildren(Object id) { iterator.remove(); } } - expandedItemIds.remove(id); + expandedItems.remove(id); invalidatedChildren.stream().map(getDataProvider()::getId) .forEach(x -> { removeChildren(x); @@ -616,7 +617,7 @@ private Stream combineParentAndChildStreams(T parent, Stream children, public void destroyAllData() { childMap.clear(); parentIdMap.clear(); - expandedItemIds.clear(); + expandedItems.clear(); } /** @@ -625,6 +626,16 @@ public void destroyAllData() { * @return {@code true} if there is any expanded items. */ public boolean hasExpandedItems() { - return !expandedItemIds.isEmpty(); + return !expandedItems.isEmpty(); + } + + /** + * Returns the expanded items in form of an unmodifiable collection. + * + * @return an unmodifiable {@code Collection} containing the expanded + * items. + */ + public Collection getExpandedItems() { + return Collections.unmodifiableCollection(expandedItems.values()); } } diff --git a/flow-data/src/test/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalCommunicatorTest.java b/flow-data/src/test/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalCommunicatorTest.java index 35f07deb000..b2c22faec56 100644 --- a/flow-data/src/test/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalCommunicatorTest.java +++ b/flow-data/src/test/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalCommunicatorTest.java @@ -15,12 +15,11 @@ */ package com.vaadin.flow.data.provider.hierarchy; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; - import java.io.Serializable; import java.util.ArrayList; +import java.util.HashMap; import java.util.List; +import java.util.Map; import org.junit.Assert; import org.junit.Before; @@ -36,7 +35,11 @@ import com.vaadin.flow.internal.StateNode; import com.vaadin.flow.internal.StateTree; +import elemental.json.JsonArray; +import elemental.json.JsonObject; import elemental.json.JsonValue; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; public class HierarchicalCommunicatorTest { @@ -54,6 +57,9 @@ public class HierarchicalCommunicatorTest { private List enqueueFunctions = new ArrayList<>(); + private Map enqueueFunctionsWithParams = + new HashMap<>(); + private class UpdateQueue implements HierarchicalUpdate { @Override public void clear(int start, int length) { @@ -89,6 +95,13 @@ public void commit() { } } + private class UpdateQueueWithArguments extends UpdateQueue { + @Override + public void enqueue(String name, Serializable... arguments) { + enqueueFunctionsWithParams.put(name, arguments); + } + } + private final HierarchicalArrayUpdater arrayUpdater = new HierarchicalArrayUpdater() { @Override public HierarchicalUpdate startUpdate(int sizeChange) { @@ -100,6 +113,18 @@ public void initialize() { } }; + private final HierarchicalArrayUpdater arrayUpdaterWithArguments = + new HierarchicalArrayUpdater() { + @Override + public HierarchicalUpdate startUpdate(int sizeChange) { + return new UpdateQueueWithArguments(); + } + + @Override + public void initialize() { + } + }; + @Before public void setUp() { ui = Mockito.mock(UI.class); @@ -188,9 +213,61 @@ public void reset_noDataControllers_hierarchicalUpdateIsCalled() { // any data controllers communicator.reset(); - Assert.assertEquals(1, enqueueFunctions.size()); + Assert.assertEquals(2, enqueueFunctions.size()); Assert.assertEquals("$connector.ensureHierarchy", enqueueFunctions.get(0)); + Assert.assertEquals("$connector.expandItems", + enqueueFunctions.get(1)); + } + + @Test + public void reset_expandSomeItems_updateContainsProperJsonObjectsToExpand() { + enqueueFunctionsWithParams = new HashMap<>(); + + TreeData hierarchyTreeData = new TreeData<>(); + hierarchyTreeData.addItem(null, "root"); + hierarchyTreeData.addItem("root", "first-1"); + hierarchyTreeData.addItem("root", "first-2"); + hierarchyTreeData.addItem("first-1", "second-1-1"); + hierarchyTreeData.addItem("first-2", "second-2-1"); + + TreeDataProvider treeDataProvider = + new TreeDataProvider<>(hierarchyTreeData); + + HierarchicalDataCommunicator dataCommunicator = + new HierarchicalDataCommunicator( + Mockito.mock(CompositeDataGenerator.class), + arrayUpdaterWithArguments, + json -> {}, Mockito.mock(StateNode.class), () -> null); + + dataCommunicator.setDataProvider(treeDataProvider, null); + + dataCommunicator.expand("root"); + dataCommunicator.expand("first-1"); + + dataCommunicator.reset(); + + + Assert.assertTrue(enqueueFunctionsWithParams + .containsKey("$connector.expandItems")); + JsonArray arguments = (JsonArray) enqueueFunctionsWithParams + .get("$connector.expandItems")[0]; + Assert.assertNotNull(arguments); + Assert.assertEquals(2, arguments.length()); + + JsonObject first1 = arguments.getObject(0); + JsonObject root = arguments.getObject(1); + + Assert.assertNotNull(first1); + Assert.assertNotNull(root); + + Assert.assertTrue(first1.hasKey("key")); + Assert.assertTrue(root.hasKey("key")); + + Assert.assertEquals(dataCommunicator.getKeyMapper().key("first-1"), + first1.getString("key")); + Assert.assertEquals(dataCommunicator.getKeyMapper().key("root"), + root.getString("key")); } } diff --git a/flow-data/src/test/java/com/vaadin/flow/data/provider/hierarchy/HierarchyMapperWithDataTest.java b/flow-data/src/test/java/com/vaadin/flow/data/provider/hierarchy/HierarchyMapperWithDataTest.java index ca15d04cf8d..914171d5169 100644 --- a/flow-data/src/test/java/com/vaadin/flow/data/provider/hierarchy/HierarchyMapperWithDataTest.java +++ b/flow-data/src/test/java/com/vaadin/flow/data/provider/hierarchy/HierarchyMapperWithDataTest.java @@ -16,6 +16,8 @@ package com.vaadin.flow.data.provider.hierarchy; import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; import java.util.Comparator; import java.util.List; import java.util.Objects; @@ -23,8 +25,11 @@ import java.util.stream.IntStream; import java.util.stream.Stream; +import org.junit.Assert; import org.junit.Before; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; import com.vaadin.flow.function.SerializablePredicate; import com.vaadin.flow.internal.Range; @@ -45,6 +50,9 @@ public class HierarchyMapperWithDataTest { private List roots; private int mapSize; + @Rule + public ExpectedException exceptionRule = ExpectedException.none(); + private void setupData() { mapSize = ROOT_COUNT; data = new TreeData<>(); @@ -224,6 +232,67 @@ public void fetchWithFilter() { verifyFetchIsCorrect(expectedResult, range); } + @Test + public void getExpandedItems_expandSomeItems_returnsCorrectExpandedItems() { + + TreeNode root = new TreeNode("root", null); + TreeNode second1 = new TreeNode("second-1", root); + TreeNode second2 = new TreeNode("second-2", root); + TreeNode third11 = new TreeNode("third-1-1", second1); + TreeNode third21 = new TreeNode("third-2-1", second2); + + HierarchicalDataProvider dataProvider = + new ThreeLevelStaticHierarchicalDataProvider(root, + new TreeNode[]{second1, second2}, + new TreeNode[]{third11, third21}); + + HierarchyMapper hierarchyMapper = new HierarchyMapper<>( + dataProvider + ); + + Collection expandedItems = hierarchyMapper.getExpandedItems(); + Assert.assertNotNull(expandedItems); + Assert.assertEquals(0L, expandedItems.size()); + + hierarchyMapper.expand(root); + hierarchyMapper.expand(second2); + + expandedItems = hierarchyMapper.getExpandedItems(); + Assert.assertNotNull(expandedItems); + Assert.assertEquals(2L, expandedItems.size()); + Assert.assertArrayEquals(new Object[]{"root", "second-2"}, + expandedItems.stream() + .map(TreeNode::getName) + .sorted().toArray()); + } + + @Test + public void getExpandedItems_tryToAddItemsToCollection_shouldThrowException() { + + exceptionRule.expect(UnsupportedOperationException.class); + + TreeNode root = new TreeNode("root", null); + TreeNode second1 = new TreeNode("second-1", root); + TreeNode second2 = new TreeNode("second-2", root); + TreeNode third11 = new TreeNode("third-1-1", second1); + TreeNode third21 = new TreeNode("third-2-1", second2); + + HierarchicalDataProvider dataProvider = + new ThreeLevelStaticHierarchicalDataProvider(root, + new TreeNode[]{second1, second2}, + new TreeNode[]{third11, third21}); + + HierarchyMapper hierarchyMapper = new HierarchyMapper<>( + dataProvider + ); + + hierarchyMapper.expand(root); + hierarchyMapper.expand(second1); + + Collection expandedItems = hierarchyMapper.getExpandedItems(); + expandedItems.add(new TreeNode("third-1")); + } + private void expand(Node node) { insertRows(mapper.expand(node, mapper.getIndexOf(node).orElse(null))); } @@ -277,4 +346,87 @@ public void insertRows(Range range) { 0 <= range.getStart() && range.getStart() <= mapSize); mapSize += range.length(); } + + private static class TreeNode { + private String name; + private TreeNode parent; + + public TreeNode(String name) { + this.name = name; + } + + public TreeNode(String name, TreeNode parent) { + this.name = name; + this.parent = parent; + } + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } + + public TreeNode getParent() { + return parent; + } + + public void setParent(TreeNode parent) { + this.parent = parent; + } + } + + private static class ThreeLevelStaticHierarchicalDataProvider + extends AbstractBackEndHierarchicalDataProvider { + + private TreeNode root; + private TreeNode[] secondLevelNodes; + private TreeNode[] thirdLevelNodes; + + public ThreeLevelStaticHierarchicalDataProvider( + TreeNode root, + TreeNode[] secondLevelNodes, + TreeNode[] thirdLevelNodes) { + this.root = root; + this.secondLevelNodes = secondLevelNodes; + this.thirdLevelNodes = thirdLevelNodes; + } + + @Override + public int getChildCount(HierarchicalQuery query) { + // query node is the root: + if (query.getParent() == null) { + return secondLevelNodes.length; + } + // query node is among the last(third) layer: + if (Arrays.stream(secondLevelNodes) + .anyMatch(node -> node == query.getParent())) { + return 0; + } + // count nodes of last(third) layer that are children of query's + // parent: + return (int) Arrays.stream(thirdLevelNodes) + .filter(node -> node.getParent() == query.getParent()) + .count(); + } + + @Override + public boolean hasChildren(TreeNode item) { + return item.getParent() == null || + Arrays.stream(secondLevelNodes) + .anyMatch(node -> node == item); + } + + @Override + protected Stream fetchChildrenFromBackEnd( + HierarchicalQuery query) { + if (query.getParent() == null) { + return Arrays.stream(secondLevelNodes); + } + return Arrays.stream(thirdLevelNodes) + .filter(node -> node.getParent() == query.getParent()); + } + } + }