Skip to content

Commit f32dbbc

Browse files
committed
Bugfix: Extremely slow, non-recursive search queries in JTree/JList
While at it dix slow deletion of paths in PathManagerUI
1 parent 94003af commit f32dbbc

8 files changed

Lines changed: 204 additions & 25 deletions

File tree

‎src/main/java/sc/fiji/snt/PathManagerUI.java‎

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@
2323
package sc.fiji.snt;
2424

2525
import com.jidesoft.swing.Searchable;
26-
import com.jidesoft.swing.TreeSearchable;
2726
import ij.ImagePlus;
2827
import ij.gui.Roi;
2928
import ij.measure.Calibration;
@@ -797,8 +796,10 @@ private void deletePaths(final Collection<Path> pathsToBeDeleted) {
797796
rebuild = true;
798797
}
799798
p.disconnectFromAll();
800-
pathAndFillManager.deletePath(p);
801799
}
800+
// NB: We use pathAndFillManager.deletePaths(Collection): it fires a single resetListeners() at the end,
801+
// so that PathManagerUI#setPathList() (that triggers a full HelpfulTreeModel rebuild) runs only once
802+
pathAndFillManager.deletePaths(pathsToBeDeleted);
802803
if (all) {
803804
pathAndFillManager.resetIDs();
804805
deselectAllTagsMenu();
@@ -1704,7 +1705,7 @@ private class HelpfulJTree extends JTree {
17041705

17051706
private static final long serialVersionUID = 1L;
17061707
private static final String ROOT_LABEL = "All Paths";
1707-
private final TreeSearchable searchable;
1708+
private final Searchable searchable;
17081709

17091710
public HelpfulJTree() {
17101711
super(new DefaultMutableTreeNode(HelpfulJTree.ROOT_LABEL));
@@ -1718,7 +1719,7 @@ public HelpfulJTree() {
17181719
// stream mode double-click can zoom to the path instead of expanding/collapsing
17191720
setToggleClickCount(0);
17201721
setRowHeight(getFontMetrics(getFont()).getHeight()); // otherwise viewport too small!?
1721-
searchable = new TreeSearchable(this);
1722+
searchable = SNTSearchableBar.newTreeSearchable(this);
17221723
final Timer timer = new Timer(400, ev -> getSNT().getUI().getRecorder(false)
17231724
.recordCmd(String.format("snt.getUI().getPathManager().applySelectionFilter(\"%s\")",
17241725
searchable.getSearchingText())));
@@ -3724,11 +3725,9 @@ public void execute(List<Path> selectedPaths, String cmd) {
37243725
return;
37253726
}
37263727

3727-
// Delete original paths
3728-
for (final Path p : orderedPaths) {
3729-
p.disconnectFromAll();
3730-
getPathAndFillManager().deletePath(p);
3731-
}
3728+
// Delete original paths (disconnect first, then a single batch deletion as w/ PathManagerUI#deletePaths(Collection)
3729+
orderedPaths.forEach(Path::disconnectFromAll);
3730+
getPathAndFillManager().deletePaths(orderedPaths);
37323731

37333732
// Add merged path and restore IDs
37343733
getPathAndFillManager().addPath(mergedPath, false, false);

‎src/main/java/sc/fiji/snt/SNTUI.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4467,7 +4467,7 @@ private JPanel renderingPanel() {
44674467
}
44684468

44694469
private JPanel colorOptionsPanel() {
4470-
final ColorChooserButton colorChooser1 = new ColorChooserButton(SNTPrefs.selectedPathColor(), "Selected: ");
4470+
final ColorChooserButton colorChooser1 = new ColorChooserButton(SNTPrefs.selectedPathColor(), " Selected: ");
44714471
colorChooser1.setName("Color for Selected Paths");
44724472
colorChooser1.addColorChangedListener(newColor -> {
44734473
if (SNTPrefs.deselectedPathColor().equals(newColor)) {
@@ -4514,9 +4514,13 @@ private JPanel colorOptionsPanel() {
45144514
toolbar.add(Box.createHorizontalGlue());
45154515
toolbar.add(colorChooser1);
45164516
toolbar.add(resetButton1);
4517+
GuiUtils.Buttons.addToGroup(List.of(colorChooser1, resetButton1));
4518+
toolbar.add(Box.createHorizontalStrut(InternalUtils.MARGIN * 2));
45174519
toolbar.addSeparator();
4520+
toolbar.add(Box.createHorizontalStrut(InternalUtils.MARGIN * 2));
45184521
toolbar.add(colorChooser2);
45194522
toolbar.add(resetButton2);
4523+
GuiUtils.Buttons.addToGroup(List.of(colorChooser2, resetButton2));
45204524
toolbar.add(Box.createHorizontalGlue());
45214525
final JPanel panel = new JPanel(new BorderLayout());
45224526
panel.add(toolbar, BorderLayout.CENTER);

‎src/main/java/sc/fiji/snt/gui/GuiUtils.java‎

Lines changed: 28 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@
3131
import com.formdev.flatlaf.util.UIScale;
3232
import com.jidesoft.plaf.LookAndFeelFactory;
3333
import com.jidesoft.popup.JidePopup;
34-
import com.jidesoft.swing.ListSearchable;
34+
import com.jidesoft.swing.Searchable;
3535
import com.jidesoft.swing.TableSearchable;
3636
import com.jidesoft.swing.event.SearchableEvent;
3737
import com.jidesoft.utils.ProductNames;
@@ -736,13 +736,10 @@ public int getNextMatch(final String prefix, final int startIndex, final Positio
736736
gbc.gridy++;
737737
panel.add(ScrollPanes.create(ta), gbc);
738738
if (choices.length > 4) {
739-
final ListSearchable searchable = new ListSearchable(list) {
740-
@Override
741-
protected String convertElementToString(Object object) {
742-
final int idx = listModel.indexOf(object);
743-
return (choices[idx] + " " + descriptions[idx]).replaceAll("\\r?\\n", " ");
744-
}
745-
};
739+
final Searchable searchable = SNTSearchableBar.newListSearchable(list, object -> {
740+
final int idx = listModel.indexOf(object);
741+
return (choices[idx] + " " + descriptions[idx]).replaceAll("\\r?\\n", " ");
742+
});
746743
final SNTSearchableBar searchableBar = new SNTSearchableBar(searchable);
747744
searchableBar.setVisibleButtons(SNTSearchableBar.SHOW_NAVIGATION | SNTSearchableBar.SHOW_STATUS);
748745
searchableBar.setHighlightAll(false); // only one item can be selected in the choice list
@@ -5058,6 +5055,24 @@ private static void expandNodesByLevel(final JTree tree, final TreePath path, in
50585055
}
50595056
}
50605057

5058+
/**
5059+
* Centers the row for {@code path} in its enclosing viewport in a single move.
5060+
*
5061+
* @param tree the tree being scrolled
5062+
* @param path the path to center in view
5063+
*/
5064+
public static void scrollDirectlyTo(final JTree tree, final TreePath path) {
5065+
tree.makeVisible(path); // expand collapsed ancestors, no scrolling yet
5066+
final Rectangle bounds = tree.getPathBounds(path);
5067+
if (bounds == null) return;
5068+
if (tree.getParent() instanceof JViewport viewport) {
5069+
final int y = Math.max(0, bounds.y - (viewport.getExtentSize().height - bounds.height) / 2);
5070+
viewport.setViewPosition(new Point(0, y));
5071+
} else {
5072+
tree.scrollRectToVisible(bounds);
5073+
}
5074+
}
5075+
50615076
}
50625077

50635078
public static class Buttons {
@@ -5069,6 +5084,11 @@ private Buttons() {}
50695084
{"Distinct", "Fire", "Ice", "Plasma", "Red-Green", "Spectrum", "Viridis"};
50705085

50715086

5087+
public static void addToGroup(final List<AbstractButton> buttons) {
5088+
final ButtonGroup buttonGroup = new ButtonGroup();
5089+
for (final AbstractButton button : buttons) buttonGroup.add(button);
5090+
}
5091+
50725092
public static ButtonGroup noneSelectedButtonGroup() {
50735093
return new ButtonGroup() {
50745094

‎src/main/java/sc/fiji/snt/gui/MeasureUI.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@
2323
package sc.fiji.snt.gui;
2424

2525
import com.jidesoft.swing.CheckBoxList;
26-
import com.jidesoft.swing.ListSearchable;
2726
import net.imagej.ImageJ;
2827
import org.apache.commons.math3.stat.descriptive.SummaryStatistics;
2928
import org.scijava.Context;
@@ -318,7 +317,7 @@ public void mouseClicked(final MouseEvent e) {
318317
});
319318

320319
// searchable
321-
final SNTSearchableBar searchableBar = new SNTSearchableBar(new ListSearchable(metricList),
320+
final SNTSearchableBar searchableBar = new SNTSearchableBar(metricList,
322321
"Search " + allMetrics.size() + " metrics");
323322
searchableBar.setVisibleButtons(SNTSearchableBar.SHOW_HIGHLIGHTS | SNTSearchableBar.SHOW_NAVIGATION);
324323
searchableBar.setVisible(true);

‎src/main/java/sc/fiji/snt/gui/OntologyBrowser.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@
2323
package sc.fiji.snt.gui;
2424

2525
import com.jidesoft.swing.CheckBoxTree;
26-
import com.jidesoft.swing.TreeSearchable;
2726
import org.scijava.util.ColorRGB;
2827
import sc.fiji.snt.annotation.AllenCompartment;
2928
import sc.fiji.snt.annotation.AllenUtils;
@@ -613,7 +612,7 @@ public class OntologyTab extends JPanel {
613612
tree.setExpandsSelectedPaths(true);
614613

615614
// Search bar
616-
searchableBar = new SNTSearchableBar(new TreeSearchable(tree));
615+
searchableBar = new SNTSearchableBar(tree);
617616
searchableBar.setHighlightAll(true);
618617
searchableBar.setShowMatchCount(true);
619618
searchableBar.setVisibleButtons(SNTSearchableBar.SHOW_NAVIGATION | SNTSearchableBar.SHOW_HIGHLIGHTS |
@@ -979,6 +978,9 @@ static class BrowserTree extends CheckBoxTree {
979978
BrowserTree(final DefaultTreeModel model) {
980979
super(model);
981980
setLargeModel(true);
981+
// A fixed row height is what actually lets setLargeModel() work. Without it, setLargeModel() alone
982+
// silently does nothing without it!
983+
setRowHeight(getFontMetrics(getFont()).getHeight());
982984
}
983985

984986
@Override

‎src/main/java/sc/fiji/snt/gui/PathManagerUISearchableBar.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,8 @@ public PathManagerUISearchableBar(final PathManagerUI pmui) {
7070
_extraButtons.add(createColorFilteringButton());
7171
_extraButtons.add(createTagFilteringButton());
7272
_extraButtons.add(createMorphoFilteringButton());
73-
_extraButtons.add(createSubFilteringButton());
73+
GuiUtils.Buttons.addToGroup(_extraButtons); // Organize buttons in group for FlatLaf's mouse over effect
74+
_extraButtons.add(createSubFilteringButton()); // excluded from group
7475
setVisibleButtons(SHOW_NAVIGATION | SHOW_STATUS | SHOW_HIGHLIGHTS);
7576
setStatusLabelPlaceholder(String.format("%d Path(s) listed", pmui.getPathAndFillManager().size()));
7677
_highlightsButton.setToolTipText("Select all paths matching the current search");

‎src/main/java/sc/fiji/snt/gui/SNTSearchableBar.java‎

Lines changed: 155 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,22 +22,26 @@
2222

2323
package sc.fiji.snt.gui;
2424

25+
import com.jidesoft.swing.ListSearchable;
2526
import com.jidesoft.swing.Searchable;
2627
import com.jidesoft.swing.SearchableBar;
2728
import com.jidesoft.swing.SearchableBarIconsFactory;
29+
import com.jidesoft.swing.TreeSearchable;
2830
import com.jidesoft.swing.WholeWordsSupport;
2931
import com.jidesoft.swing.event.SearchableEvent;
3032
import com.jidesoft.swing.event.SearchableListener;
3133

3234
import sc.fiji.snt.SNTUtils;
3335

3436
import javax.swing.*;
37+
import javax.swing.tree.TreePath;
3538
import java.awt.*;
3639
import java.awt.event.KeyEvent;
3740
import java.io.Serial;
3841
import java.util.ArrayList;
3942
import java.util.Collection;
4043
import java.util.List;
44+
import java.util.function.Function;
4145

4246
/**
4347
* Implements a SearchableBar following SNT's UI.
@@ -48,7 +52,7 @@ public class SNTSearchableBar extends SearchableBar {
4852

4953
@Serial
5054
private static final long serialVersionUID = 1L;
51-
private static final int DELAY_MS = 100;
55+
private static final int DELAY_MS = 200;
5256
protected List<AbstractButton> _extraButtons;
5357

5458
private String statusLabelPlaceholder;
@@ -61,6 +65,23 @@ public class SNTSearchableBar extends SearchableBar {
6165
protected boolean subFilteringEnabled;
6266
private final boolean compactNavigation;
6367

68+
69+
public SNTSearchableBar(final JTree jTree) {
70+
this(SNTSearchableBar.newTreeSearchable(jTree));
71+
}
72+
73+
public SNTSearchableBar(final JList<?> list) {
74+
this(SNTSearchableBar.newListSearchable(list));
75+
}
76+
77+
public SNTSearchableBar(final JTree jTree, final String placeholder) {
78+
this(SNTSearchableBar.newTreeSearchable(jTree), placeholder);
79+
}
80+
81+
public SNTSearchableBar(final JList<?> list, final String placeholder) {
82+
this(SNTSearchableBar.newListSearchable(list), placeholder);
83+
}
84+
6485
public SNTSearchableBar(final Searchable searchable) {
6586
this(searchable, "Type to select");
6687
}
@@ -81,6 +102,139 @@ public SNTSearchableBar(final Searchable searchable, final String placeholder) {
81102
init(placeholder); // should be the last call in the constructor
82103
}
83104

105+
/**
106+
* Creates a {@link TreeSearchable} that scrolls only to the current match rather than once per hit.
107+
* A plain {@code TreeSearchable} scrolls on every call to its {@code setSelectedIndex()}, and
108+
* {@code highlightAll()} (on by default here, see {@link #setHighlightAll(boolean)}) calls it once per
109+
* match, so a large/deep tree ends up scrolling repeatedly on every keystroke instead of just once.
110+
* <p>
111+
* Also turns on {@link TreeSearchable#setRecursive(boolean)}, which defaults to {@code false} in jide-oss: with
112+
* it off, matches are only looked for among the currently visible (expanded) rows, so a collapsed branch is
113+
* invisible to the search, not just to the eye
114+
*
115+
* @param tree the tree to be searched
116+
* @return a scroll-throttled, fully-recursive searchable wrapping {@code tree}
117+
*/
118+
public static Searchable newTreeSearchable(final JTree tree) {
119+
final TreeSearchable searchable = new TreeSearchable(tree) {
120+
private boolean suppressScroll;
121+
private TreePath firstMatch;
122+
123+
@Override
124+
protected void highlightAll() {
125+
firstMatch = null;
126+
suppressScroll = true;
127+
try {
128+
super.highlightAll();
129+
} finally {
130+
suppressScroll = false;
131+
}
132+
if (firstMatch != null) GuiUtils.JTrees.scrollDirectlyTo(tree, firstMatch);
133+
}
134+
135+
@Override
136+
public int findFromCursor(final String s) {
137+
// with match-count on (see setShowMatchCount()), this walks EVERY element from the start to count all
138+
// matches, calling setSelectedIndex() once per match; If left unsuppressed that alone scrolls through
139+
// the whole tree before the real, single search result is even selected!
140+
final boolean wasSuppressed = suppressScroll;
141+
suppressScroll = true;
142+
try {
143+
return super.findFromCursor(s);
144+
} finally {
145+
suppressScroll = wasSuppressed;
146+
}
147+
}
148+
149+
@Override
150+
protected void setSelectedIndex(final int index, final boolean incremental) {
151+
if (!suppressScroll) {
152+
super.setSelectedIndex(index, incremental);
153+
return;
154+
}
155+
// same selection logic as TreeSearchable#setSelectedIndex, minus
156+
// the scrollRowToVisible()/scrollPathToVisible() call
157+
if (!isRecursive()) {
158+
if (incremental) tree.addSelectionInterval(index, index);
159+
else tree.setSelectionRow(index);
160+
} else if (getElementAt(index) instanceof TreePath path) {
161+
if (firstMatch == null) firstMatch = path;
162+
if (incremental) tree.addSelectionPath(path);
163+
else tree.setSelectionPath(path);
164+
}
165+
}
166+
};
167+
searchable.setRecursive(true);
168+
return searchable;
169+
}
170+
171+
/**
172+
* Same as {@link #newListSearchable(JList, Function)}, with the default {@code toString()}-based element converter
173+
*
174+
* @param list the list to be searched
175+
* @return a scroll-throttled searchable wrapping {@code list}
176+
*/
177+
public static Searchable newListSearchable(final JList<?> list) {
178+
return newListSearchable(list, null);
179+
}
180+
181+
/**
182+
* Creates a {@link ListSearchable} with the same scroll-throttling and match-counting fixes as
183+
* {@link #newTreeSearchable(JTree)}.
184+
*
185+
* @param list the list to be searched
186+
* @param elementConverter optional; converts a list element to the string matched against, or {@code null} to use
187+
* the element's own {@code toString()}
188+
* @return a scroll-throttled searchable wrapping {@code list}
189+
*/
190+
public static Searchable newListSearchable(final JList<?> list, final Function<Object, String> elementConverter) {
191+
return new ListSearchable(list) {
192+
private boolean suppressScroll;
193+
private int firstMatch = -1;
194+
195+
@Override
196+
protected String convertElementToString(final Object object) {
197+
return (elementConverter != null) ? elementConverter.apply(object) : super.convertElementToString(object);
198+
}
199+
200+
@Override
201+
public int findFromCursor(final String s) {
202+
final boolean wasSuppressed = suppressScroll;
203+
suppressScroll = true;
204+
try {
205+
return super.findFromCursor(s);
206+
} finally {
207+
suppressScroll = wasSuppressed;
208+
}
209+
}
210+
211+
@Override
212+
protected void highlightAll() {
213+
firstMatch = -1;
214+
suppressScroll = true;
215+
try {
216+
super.highlightAll();
217+
} finally {
218+
suppressScroll = false;
219+
}
220+
if (firstMatch != -1) list.ensureIndexIsVisible(firstMatch);
221+
}
222+
223+
@Override
224+
public void setSelectedIndex(final int index, final boolean incremental) {
225+
if (!suppressScroll) {
226+
super.setSelectedIndex(index, incremental);
227+
return;
228+
}
229+
// same selection logic as ListSearchable#setSelectedIndex,
230+
// minus the ensureIndexIsVisible() call
231+
if (incremental) list.addSelectionInterval(index, index);
232+
else if (list.getSelectedIndex() != index) list.setSelectedIndex(index);
233+
if (firstMatch == -1 && index >= 0) firstMatch = index;
234+
}
235+
};
236+
}
237+
84238
@Override
85239
public int getMaxHistoryLength() {
86240
return 0; // disabled. We'll use builtinSearchHistory

0 commit comments

Comments
 (0)