From dc52e7fc74332369d66443dce3f1ae933a205e63 Mon Sep 17 00:00:00 2001 From: Lars Vogel Date: Thu, 13 Aug 2026 14:54:05 +0200 Subject: [PATCH 1/2] Cover restoring the tree expansion state and the viewer element map Restoring the expansion state by tree path had thin coverage. The one test that exercised it, MultipleEqualElementsTreeViewerTest, ran without an element comparer, although the comparer decides how a tree path hashes and compares, and nothing checked what happens to the items the walk reaches after the last path to expand has been found. TreeViewerExpansionStateTest covers the round trip with and without a comparer, restoring a subset and an empty set, restoring from equal but distinct elements the way a content provider does after a refresh, and the state of the containers the walk passes once its work is done. The hash table a viewer keeps to map elements to items had no coverage of its own at all. It is package private, so StructuredViewerElementMapTest reaches it the way a client does, through a comparer that decides the hashes: several keys in one slot, keys whose hashes differ but share a slot, a table grown well past its initial capacity, and removal. Also gives TreeViewerExpansionTest a second shape, a few large containers with one of them expanded, which is where restoring is most expensive. --- .../jface/tests/viewers/AllViewersTests.java | 3 +- .../StructuredViewerElementMapTest.java | 213 ++++++++++++++ .../viewers/TreeViewerExpansionStateTest.java | 268 ++++++++++++++++++ .../performance/TreeViewerExpansionTest.java | 59 +++- 4 files changed, 539 insertions(+), 4 deletions(-) create mode 100644 tests/org.eclipse.jface.tests/src/org/eclipse/jface/tests/viewers/StructuredViewerElementMapTest.java create mode 100644 tests/org.eclipse.jface.tests/src/org/eclipse/jface/tests/viewers/TreeViewerExpansionStateTest.java diff --git a/tests/org.eclipse.jface.tests/src/org/eclipse/jface/tests/viewers/AllViewersTests.java b/tests/org.eclipse.jface.tests/src/org/eclipse/jface/tests/viewers/AllViewersTests.java index db11de55179..0b9b8a39c36 100644 --- a/tests/org.eclipse.jface.tests/src/org/eclipse/jface/tests/viewers/AllViewersTests.java +++ b/tests/org.eclipse.jface.tests/src/org/eclipse/jface/tests/viewers/AllViewersTests.java @@ -32,7 +32,8 @@ Bug256889TableViewerTest.class, Bug287765Test.class, Bug242231Test.class, EditingSupportValueMismatchTest.class, StyledStringBuilderTest.class, TreeViewerWithLimitTest.class, TreeViewerWithLimitCompatibilityTest.class, TableViewerWithLimitTest.class, - TableViewerWithLimitCompatibilityTest.class }) + TableViewerWithLimitCompatibilityTest.class, TreeViewerExpansionStateTest.class, + StructuredViewerElementMapTest.class }) public class AllViewersTests { } diff --git a/tests/org.eclipse.jface.tests/src/org/eclipse/jface/tests/viewers/StructuredViewerElementMapTest.java b/tests/org.eclipse.jface.tests/src/org/eclipse/jface/tests/viewers/StructuredViewerElementMapTest.java new file mode 100644 index 00000000000..015bf661de8 --- /dev/null +++ b/tests/org.eclipse.jface.tests/src/org/eclipse/jface/tests/viewers/StructuredViewerElementMapTest.java @@ -0,0 +1,213 @@ +/******************************************************************************* + * Copyright (c) 2026 Vogella GmbH and others. + * + * This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + * + * Contributors: + * Lars Vogel - initial API and implementation + *******************************************************************************/ +package org.eclipse.jface.tests.viewers; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertSame; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; + +import org.eclipse.jface.viewers.ArrayContentProvider; +import org.eclipse.jface.viewers.IElementComparer; +import org.eclipse.jface.viewers.LabelProvider; +import org.eclipse.jface.viewers.TableViewer; +import org.eclipse.swt.widgets.Item; +import org.eclipse.swt.widgets.Shell; +import org.eclipse.swt.widgets.Widget; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; + +/** + * Tests the hash table a viewer keeps to map elements to their items, through + * the comparer, which is the only way a client controls how elements hash. + *

+ * The interesting cases are the ones a table with chained buckets has to get + * right: several keys in the same slot, keys whose hashes differ but land in + * the same slot anyway, and a table that has outgrown its initial capacity. + */ +public class StructuredViewerElementMapTest { + + /** + * The capacity a viewer's element map starts with. Slot assignment is the + * hash modulo this, so hashes that differ by a multiple of it collide as long + * as the table has not grown. + */ + private static final int INITIAL_CAPACITY = 13; + + private Shell shell; + + private TableViewer viewer; + + private static class Element { + final String name; + final int hash; + + Element(String name, int hash) { + this.name = name; + this.hash = hash; + } + + @Override + public String toString() { + return name; + } + } + + /** + * Takes the hash from the element, so that a test can place elements in + * whichever slots it needs, and compares by name, so that equal but distinct + * elements are recognized. + */ + private static final IElementComparer COMPARER = new IElementComparer() { + + @Override + public boolean equals(Object a, Object b) { + if (a instanceof Element first && b instanceof Element second) { + return first.name.equals(second.name); + } + return a == null ? b == null : a.equals(b); + } + + @Override + public int hashCode(Object element) { + return element instanceof Element typed ? typed.hash : element.hashCode(); + } + }; + + @AfterEach + public void tearDown() { + if (shell != null) { + shell.dispose(); + shell = null; + } + viewer = null; + } + + private void createViewer(List input) { + shell = new Shell(); + shell.setSize(500, 500); + viewer = new TableViewer(shell); + viewer.setComparer(COMPARER); + viewer.setUseHashlookup(true); + viewer.setContentProvider(ArrayContentProvider.getInstance()); + viewer.setLabelProvider(new LabelProvider()); + viewer.setInput(input); + shell.open(); + } + + private void assertMapsToItemFor(Element element) { + Widget item = viewer.testFindItem(element); + assertNotNull(item, "no item mapped for " + element); + assertSame(element, ((Item) item).getData(), "wrong item mapped for " + element); + } + + /** + * Keys whose hashes are identical all end up in one slot and have to be told + * apart by the comparer. + */ + @Test + public void testFindsElementsWithIdenticalHashes() { + List input = List.of(new Element("a", 7), new Element("b", 7), new Element("c", 7)); + createViewer(input); + + for (Element element : input) { + assertMapsToItemFor(element); + } + assertNull(viewer.testFindItem(new Element("absent", 7)), "an absent element was found"); + } + + /** + * Keys whose hashes differ but fall into the same slot must not be confused, + * which is what a lookup that screens entries by their hash has to preserve. + */ + @Test + public void testFindsElementsWithCollidingSlots() { + List input = List.of(new Element("a", 1), new Element("b", 1 + INITIAL_CAPACITY), + new Element("c", 1 + 2 * INITIAL_CAPACITY)); + createViewer(input); + + for (Element element : input) { + assertMapsToItemFor(element); + } + // Same slot, no matching hash, and no matching name. + assertNull(viewer.testFindItem(new Element("absent", 1 + 3 * INITIAL_CAPACITY)), + "an absent element sharing a slot was found"); + } + + /** + * Growing past the initial capacity redistributes every key, so all of them + * have to remain findable afterwards. + */ + @Test + public void testFindsElementsAfterTableGrowth() { + List input = new ArrayList<>(); + for (int i = 0; i < 200; i++) { + // Spread over the slots and force several rounds of growth. + input.add(new Element("e" + i, i * 31)); + } + createViewer(input); + + for (Element element : input) { + assertMapsToItemFor(element); + } + } + + /** + * After a refresh hands the viewer equal but distinct objects, the map has to + * find them and the items have to hold the new objects rather than the stale + * ones. + */ + @Test + public void testFindsEqualButDistinctElementsAfterRefresh() { + List input = new ArrayList<>( + Arrays.asList(new Element("a", 3), new Element("b", 3), new Element("c", 9))); + createViewer(input); + + List replacements = new ArrayList<>(); + for (Element element : input) { + replacements.add(new Element(element.name, element.hash)); + } + input.clear(); + input.addAll(replacements); + viewer.refresh(); + + for (Element replacement : replacements) { + assertMapsToItemFor(replacement); + } + assertEquals(replacements.size(), viewer.getTable().getItemCount()); + } + + /** + * A removed element must not be reachable through the map any more. + */ + @Test + public void testRemovedElementIsNoLongerFound() { + List input = new ArrayList<>( + Arrays.asList(new Element("a", 5), new Element("b", 5), new Element("c", 5))); + createViewer(input); + + Element removed = input.remove(1); + viewer.remove(removed); + + assertNull(viewer.testFindItem(removed), "the removed element is still mapped"); + for (Element element : input) { + assertMapsToItemFor(element); + } + } + +} diff --git a/tests/org.eclipse.jface.tests/src/org/eclipse/jface/tests/viewers/TreeViewerExpansionStateTest.java b/tests/org.eclipse.jface.tests/src/org/eclipse/jface/tests/viewers/TreeViewerExpansionStateTest.java new file mode 100644 index 00000000000..6862aa436c5 --- /dev/null +++ b/tests/org.eclipse.jface.tests/src/org/eclipse/jface/tests/viewers/TreeViewerExpansionStateTest.java @@ -0,0 +1,268 @@ +/******************************************************************************* + * Copyright (c) 2026 Vogella GmbH and others. + * + * This program and the accompanying materials + * are made available under the terms of the Eclipse Public License 2.0 + * which accompanies this distribution, and is available at + * https://www.eclipse.org/legal/epl-2.0/ + * + * SPDX-License-Identifier: EPL-2.0 + * + * Contributors: + * Lars Vogel - initial API and implementation + *******************************************************************************/ +package org.eclipse.jface.tests.viewers; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.ArrayList; +import java.util.List; +import java.util.Objects; + +import org.eclipse.jface.viewers.IElementComparer; +import org.eclipse.jface.viewers.ITreeContentProvider; +import org.eclipse.jface.viewers.LabelProvider; +import org.eclipse.jface.viewers.TreePath; +import org.eclipse.jface.viewers.TreeViewer; +import org.eclipse.swt.widgets.Shell; +import org.eclipse.swt.widgets.TreeItem; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; + +/** + * Tests saving and restoring the expansion state of a tree by tree path, with + * and without an {@link IElementComparer}. + */ +public class TreeViewerExpansionStateTest { + + private Shell shell; + + private TreeViewer treeViewer; + + private Node root; + + private static class Node { + final String name; + final Node parent; + final List children = new ArrayList<>(); + + Node(String name, Node parent) { + this.name = name; + this.parent = parent; + if (parent != null) { + parent.children.add(this); + } + } + + @Override + public String toString() { + return name; + } + } + + /** + * Keys the nodes by their name instead of by their identity, so that the paths + * handed back to the viewer are compared through the comparer and not through + * {@link Object#equals(Object)}. + */ + private static final IElementComparer NAME_COMPARER = new IElementComparer() { + + @Override + public boolean equals(Object a, Object b) { + if (a instanceof Node first && b instanceof Node second) { + return first.name.equals(second.name); + } + return Objects.equals(a, b); + } + + @Override + public int hashCode(Object element) { + return element instanceof Node node ? node.name.hashCode() : element.hashCode(); + } + }; + + @AfterEach + public void tearDown() { + if (shell != null) { + shell.dispose(); + shell = null; + } + treeViewer = null; + root = null; + } + + /** + * Creates a viewer over a tree with the given number of children per level. + */ + private void createViewer(IElementComparer comparer, int... branching) { + shell = new Shell(); + shell.setSize(500, 500); + treeViewer = new TreeViewer(shell); + if (comparer != null) { + treeViewer.setComparer(comparer); + } + treeViewer.setUseHashlookup(true); + treeViewer.setContentProvider(new ITreeContentProvider() { + + @Override + public Object[] getElements(Object inputElement) { + return getChildren(inputElement); + } + + @Override + public Object[] getChildren(Object parentElement) { + return ((Node) parentElement).children.toArray(); + } + + @Override + public Object getParent(Object element) { + return ((Node) element).parent; + } + + @Override + public boolean hasChildren(Object element) { + return !((Node) element).children.isEmpty(); + } + }); + treeViewer.setLabelProvider(new LabelProvider()); + + root = new Node("root", null); + createChildren(root, 0, branching); + treeViewer.setInput(root); + shell.open(); + } + + private static void createChildren(Node parent, int level, int[] branching) { + if (level >= branching.length) { + return; + } + for (int i = 0; i < branching[level]; i++) { + createChildren(new Node(parent.name + "." + i, parent), level + 1, branching); + } + } + + @Test + public void testRestoreExpansionWithoutComparer() { + assertExpansionSurvivesRoundTrip(null); + } + + /** + * The comparer decides how a tree path hashes and compares, so restoring by + * path has to route through it rather than through the elements themselves. + */ + @Test + public void testRestoreExpansionWithComparer() { + assertExpansionSurvivesRoundTrip(NAME_COMPARER); + } + + private void assertExpansionSurvivesRoundTrip(IElementComparer comparer) { + createViewer(comparer, 3, 3, 3); + treeViewer.expandAll(); + + TreePath[] expanded = treeViewer.getExpandedTreePaths(); + assertTrue(expanded.length > 0, "Nothing was expanded"); + + treeViewer.collapseAll(); + assertEquals(0, treeViewer.getExpandedTreePaths().length, "collapseAll left paths expanded"); + + treeViewer.setExpandedTreePaths(expanded); + assertArrayEquals(expanded, treeViewer.getExpandedTreePaths(), "expansion was not restored"); + } + + /** + * Restoring a subset has to expand exactly that subset, whichever order the + * viewer happens to find the paths in while it walks the tree. + */ + @Test + public void testRestoreSubsetOfExpandedPaths() { + createViewer(NAME_COMPARER, 3, 3); + treeViewer.expandAll(); + + TreePath[] expanded = treeViewer.getExpandedTreePaths(); + assertEquals(3, expanded.length); + + TreePath[] subset = { expanded[0], expanded[2] }; + treeViewer.setExpandedTreePaths(subset); + assertArrayEquals(subset, treeViewer.getExpandedTreePaths(), "wrong subset expanded"); + } + + /** + * Once every path to expand has been found, the items the viewer has not + * reached yet still have to be collapsed, and their children have to survive so + * that a later restore can expand them again. + */ + @Test + public void testCollapsesItemsAfterExpandedSetIsExhausted() { + createViewer(NAME_COMPARER, 4, 3); + treeViewer.expandAll(); + + TreePath[] expanded = treeViewer.getExpandedTreePaths(); + assertEquals(4, expanded.length); + + // The first container stays expanded, so the walk runs out of paths to look + // for while three expanded containers are still ahead of it. + TreePath[] first = { expanded[0] }; + treeViewer.setExpandedTreePaths(first); + + assertArrayEquals(first, treeViewer.getExpandedTreePaths(), "only the first container should be expanded"); + + TreeItem[] containers = treeViewer.getTree().getItems(); + assertEquals(4, containers.length); + assertTrue(containers[0].getExpanded(), "the restored container should be expanded"); + for (int i = 1; i < containers.length; i++) { + assertFalse(containers[i].getExpanded(), "container " + i + " should have been collapsed"); + assertEquals(3, containers[i].getItemCount(), "the children of a collapsed container were lost"); + } + + // Everything can be expanded again from the state left behind. + treeViewer.setExpandedTreePaths(expanded); + assertArrayEquals(expanded, treeViewer.getExpandedTreePaths(), "expansion could not be restored again"); + } + + /** + * An empty set collapses the tree without disturbing the items. + */ + @Test + public void testRestoreEmptyExpansion() { + createViewer(NAME_COMPARER, 3, 3); + treeViewer.expandAll(); + assertEquals(3, treeViewer.getExpandedTreePaths().length); + + treeViewer.setExpandedTreePaths(new TreePath[0]); + assertEquals(0, treeViewer.getExpandedTreePaths().length, "the tree should be fully collapsed"); + assertEquals(3, treeViewer.getTree().getItemCount()); + } + + /** + * A path built from equal but distinct elements has to be recognized, which is + * the whole point of restoring by value after a refresh has replaced the model + * objects. + */ + @Test + public void testRestoreWithEqualButDistinctElements() { + createViewer(NAME_COMPARER, 3, 3); + treeViewer.expandAll(); + + TreePath[] expanded = treeViewer.getExpandedTreePaths(); + assertEquals(3, expanded.length); + treeViewer.collapseAll(); + + TreePath[] rebuilt = new TreePath[expanded.length]; + for (int i = 0; i < expanded.length; i++) { + Object[] segments = new Object[expanded[i].getSegmentCount()]; + for (int j = 0; j < segments.length; j++) { + // A fresh object that the comparer considers equal to the original. + segments[j] = new Node(((Node) expanded[i].getSegment(j)).name, null); + } + rebuilt[i] = new TreePath(segments); + } + + treeViewer.setExpandedTreePaths(rebuilt); + assertEquals(expanded.length, treeViewer.getExpandedTreePaths().length, + "equal but distinct elements were not recognized"); + } + +} diff --git a/tests/org.eclipse.ui.tests.performance/src/org/eclipse/jface/tests/performance/TreeViewerExpansionTest.java b/tests/org.eclipse.ui.tests.performance/src/org/eclipse/jface/tests/performance/TreeViewerExpansionTest.java index 935060a39d1..b0fa3392c2c 100644 --- a/tests/org.eclipse.ui.tests.performance/src/org/eclipse/jface/tests/performance/TreeViewerExpansionTest.java +++ b/tests/org.eclipse.ui.tests.performance/src/org/eclipse/jface/tests/performance/TreeViewerExpansionTest.java @@ -20,6 +20,7 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import java.util.ArrayList; +import java.util.Arrays; import java.util.List; import java.util.Locale; import java.util.Objects; @@ -52,6 +53,15 @@ public class TreeViewerExpansionTest extends ViewerTest { /** Children per level, from the top level down. */ private static final int[] BRANCHING = { 12, 12, 12 }; + /** + * A few large subtrees, the shape of a generated document whose top level is a + * handful of big containers. Restoring one expanded container leaves most of + * the materialized items with nothing left to match against. + */ + private static final int[] WIDE_BRANCHING = { 3, 600 }; + + private int[] branching = BRANCHING; + private TreeViewer treeViewer; private DeepHashElement root; @@ -181,11 +191,11 @@ protected Object getInitialInput() { return root; } - private static void createChildren(DeepHashElement parent, int level) { - if (level >= BRANCHING.length) { + private void createChildren(DeepHashElement parent, int level) { + if (level >= branching.length) { return; } - for (int i = 0; i < BRANCHING[level]; i++) { + for (int i = 0; i < branching[level]; i++) { DeepHashElement child = new DeepHashElement(parent.name + "." + i, parent); parent.children.add(child); createChildren(child, level + 1); @@ -212,6 +222,49 @@ public void testRestoreExpansionWithComparer() throws CoreException { measureRestoreExpansion(); } + /** + * Restores a small expanded set on a tree whose items are all materialized, the + * case of a large document the user has opened one container of. The viewer + * still walks every item, but runs out of paths to look for early on. + */ + @Test + public void testRestoreFewExpandedPaths() throws CoreException { + branching = WIDE_BRANCHING; + openBrowser(); + treeViewer.expandAll(); + processEvents(); + + TreePath[] all = treeViewer.getExpandedTreePaths(); + assertTrue(all.length > 2, "Not enough expandable containers"); + TreePath[] few = Arrays.copyOf(all, 2); + + // Reaching the target state through the viewer keeps the items of the + // collapsed containers alive, unlike collapseAll, which prunes them. + treeViewer.setExpandedTreePaths(few); + processEvents(); + treeViewer.setExpandedTreePaths(few); + processEvents(); + + AtomicLong visits = new AtomicLong(); + exercise(() -> { + DeepHashElement.hashVisits.set(0); + startMeasuring(); + treeViewer.setExpandedTreePaths(few); + stopMeasuring(); + visits.set(DeepHashElement.hashVisits.get()); + + processEvents(); + assertEquals(few.length, treeViewer.getExpandedTreePaths().length, + "The expansion state was not restored"); + }, MIN_ITERATIONS, ITERATIONS, JFacePerformanceSuite.MAX_TIME); + + int elements = root.size() - 1; + reportTimings("restore " + few.length + " of " + elements + " elements, all materialized"); + System.out.printf(Locale.ROOT, "%-48s %d recursive hash visits for %d elements (%.1f per element)%n", + getClass().getSimpleName() + " few expanded", visits.get(), elements, + visits.get() / (double) elements); + } + private void measureRestoreExpansion() throws CoreException { openBrowser(); treeViewer.expandAll(); From f9f9c77a0253c563304c200d2e98632e09b23c72 Mon Sep 17 00:00:00 2001 From: Lars Vogel Date: Thu, 13 Aug 2026 14:57:24 +0200 Subject: [PATCH 2/2] Hash tree paths incrementally when restoring the expansion state Restoring the expansion state walks every item of the tree and looks each one up in a table keyed by TreePath. A tree path hashes all of its segments, so an element whose hashCode is recursive over its children, as an LSP DocumentSymbol is, was hashed once for every descendant of every ancestor on its path. The cost of the walk grew with the square of the tree rather than with its size. A tree path hashes as the sum of its segments, which composes, so internalSetExpandedTreePaths now carries the parent's hash down the recursion and derives each child's from it with a single element hash. Once the set of paths to expand is exhausted the remaining items can only be collapsed, which needs neither a path nor its hash, so the walk switches to a plain collapse. CustomHashtable keeps the hash in its entries, which lets a lookup rule out an entry without comparing the keys and lets the table grow without hashing the keys again, and gains a remove that takes an already computed hash. Measured with TreeViewerExpansionTest, recursive hash visits and the minimum of 100 restores: fully expanded, 156 of 1884 elements 361056 -> 44676 visits, 8.3 -> 5.1 ms 2 of 1803 elements, all materialized 1087807 -> 4206 visits, 10.2 -> 0.2 ms --- .../jface/viewers/AbstractTreeViewer.java | 35 ++++++++-- .../jface/viewers/CustomHashtable.java | 69 +++++++++++++------ 2 files changed, 80 insertions(+), 24 deletions(-) diff --git a/bundles/org.eclipse.jface/src/org/eclipse/jface/viewers/AbstractTreeViewer.java b/bundles/org.eclipse.jface/src/org/eclipse/jface/viewers/AbstractTreeViewer.java index cb02a29cd77..aeb30120d57 100644 --- a/bundles/org.eclipse.jface/src/org/eclipse/jface/viewers/AbstractTreeViewer.java +++ b/bundles/org.eclipse.jface/src/org/eclipse/jface/viewers/AbstractTreeViewer.java @@ -2202,19 +2202,34 @@ private void internalSetExpanded(CustomHashtable expandedElements, * which are expanded * @param widget * the widget + * @param currentHash + * the hash {@link TreePath#hashCode(IElementComparer)} would return + * for currentPath */ private void internalSetExpandedTreePaths( CustomHashtable expandedTreePaths, Widget widget, - TreePath currentPath) { + TreePath currentPath, int currentHash, IElementComparer comparer) { Item[] items = getChildren(widget); for (Item item : items) { + if (expandedTreePaths.size() == 0) { + // Every path that had to be expanded has been found, so the rest of the + // tree can only be collapsed, which needs neither a path nor its hash. + internalCollapseSubtree(item); + continue; + } Object data = item.getData(); TreePath childPath = data == null ? null : currentPath .createChildPath(data); + int childHash = currentHash; if (data != null && childPath != null) { + // A tree path hashes as the sum of its segments, so the child's hash + // follows from the parent's. Computing it from the path instead would + // hash every segment again, which is what makes an element with an + // expensive hashCode cost the whole traversal. + childHash += comparer == null ? data.hashCode() : comparer.hashCode(data); // remove the element to avoid an infinite loop // if the same element appears on a child item - boolean expanded = expandedTreePaths.remove(childPath) != null; + boolean expanded = expandedTreePaths.remove(childPath, childHash) != null; if (expanded != getExpanded(item)) { if (expanded) { createChildren(item); @@ -2222,7 +2237,19 @@ private void internalSetExpandedTreePaths( setExpanded(item, expanded); } } - internalSetExpandedTreePaths(expandedTreePaths, item, childPath); + internalSetExpandedTreePaths(expandedTreePaths, item, childPath, childHash, comparer); + } + } + + /** + * Collapses the given item and everything below it. + */ + private void internalCollapseSubtree(Item item) { + if (item.getData() != null && getExpanded(item)) { + setExpanded(item, false); + } + for (Item child : getChildren(item)) { + internalCollapseSubtree(child); } } @@ -2677,7 +2704,7 @@ public int hashCode(Object element) { // equal elements, and those are in the set of elements to be expanded, // only the first item found for each element will be expanded. internalSetExpandedTreePaths(expandedTreePaths, getControl(), - new TreePath(new Object[0])); + new TreePath(new Object[0]), 0, comparer); } /** diff --git a/bundles/org.eclipse.jface/src/org/eclipse/jface/viewers/CustomHashtable.java b/bundles/org.eclipse.jface/src/org/eclipse/jface/viewers/CustomHashtable.java index 164c148b241..4c46c28ae1f 100644 --- a/bundles/org.eclipse.jface/src/org/eclipse/jface/viewers/CustomHashtable.java +++ b/bundles/org.eclipse.jface/src/org/eclipse/jface/viewers/CustomHashtable.java @@ -36,11 +36,19 @@ private static class HashMapEntry { Object key, value; + /** + * The key's hash, kept so that growing the table does not hash the keys again + * and so that a lookup can rule out an entry without comparing the keys. Both + * matter for elements whose hashCode and equals are expensive. + */ + final int hash; + HashMapEntry next; - HashMapEntry(Object theKey, Object theValue) { + HashMapEntry(Object theKey, Object theValue, int theHash) { key = theKey; value = theValue; + hash = theHash; } } @@ -239,22 +247,15 @@ public Enumeration elements() { * does not exist */ public Object get(Object key) { - int index = (hashCode(key) & 0x7FFFFFFF) % elementData.length; - HashMapEntry entry = elementData[index]; - while (entry != null) { - if (keyEquals(key, entry.key)) { - return entry.value; - } - entry = entry.next; - } - return null; + HashMapEntry entry = getEntry(key); + return entry == null ? null : entry.value; } private HashMapEntry getEntry(Object key) { - int index = (hashCode(key) & 0x7FFFFFFF) % elementData.length; - HashMapEntry entry = elementData[index]; + int hash = hashCode(key); + HashMapEntry entry = elementData[indexFor(hash)]; while (entry != null) { - if (keyEquals(key, entry.key)) { + if (entry.hash == hash && keyEquals(key, entry.key)) { return entry; } entry = entry.next; @@ -262,6 +263,13 @@ private HashMapEntry getEntry(Object key) { return null; } + /** + * Answers the slot the given hash belongs into. + */ + private int indexFor(int hash) { + return (hash & 0x7FFFFFFF) % elementData.length; + } + /** * Answers the hash code for the given key. */ @@ -308,15 +316,16 @@ public Enumeration keys() { */ public Object put(Object key, Object value) { if (key != null && value != null) { - int index = (hashCode(key) & 0x7FFFFFFF) % elementData.length; + int hash = hashCode(key); + int index = indexFor(hash); HashMapEntry entry = elementData[index]; - while (entry != null && !keyEquals(key, entry.key)) { + while (entry != null && !(entry.hash == hash && keyEquals(key, entry.key))) { entry = entry.next; } if (entry == null) { if (++elementCount > threshold) { rehash(); - index = (hashCode(key) & 0x7FFFFFFF) % elementData.length; + index = indexFor(hash); } if (index < firstSlot) { firstSlot = index; @@ -324,7 +333,7 @@ public Object put(Object key, Object value) { if (index > lastSlot) { lastSlot = index; } - entry = new HashMapEntry(key, value); + entry = new HashMapEntry(key, value, hash); entry.next = elementData[index]; elementData[index] = entry; return null; @@ -352,7 +361,7 @@ private void rehash() { for (int i = elementData.length; --i >= 0;) { HashMapEntry entry = elementData[i]; while (entry != null) { - int index = (hashCode(entry.key) & 0x7FFFFFFF) % length; + int index = (entry.hash & 0x7FFFFFFF) % length; if (index < firstSlot) { firstSlot = index; } @@ -377,10 +386,30 @@ private void rehash() { * did not exist */ public Object remove(Object key) { + if (elementCount == 0) { + return null; + } + return remove(key, hashCode(key)); + } + + /** + * Removes the key/value pair for the given key, whose hash the caller has + * already computed. The hash must be the one this table's comparer would + * produce for the key. + * + * @param key the key to remove + * @param hash the key's hash + * @return the value associated with the key, or null if the key + * did not exist + */ + public Object remove(Object key, int hash) { + if (elementCount == 0) { + return null; + } HashMapEntry last = null; - int index = (hashCode(key) & 0x7FFFFFFF) % elementData.length; + int index = indexFor(hash); HashMapEntry entry = elementData[index]; - while (entry != null && !keyEquals(key, entry.key)) { + while (entry != null && !(entry.hash == hash && keyEquals(key, entry.key))) { last = entry; entry = entry.next; }