diff --git a/diylc/diylc-core/src/main/resources/update.xml b/diylc/diylc-core/src/main/resources/update.xml index 5520a47e..3c0e3681 100644 --- a/diylc/diylc-core/src/main/resources/update.xml +++ b/diylc/diylc-core/src/main/resources/update.xml @@ -605,6 +605,10 @@ IMPROVEMENT Include 'Potentiometer (Miniature)' and 'Trimmer Potentiometer' in Guitar Wiring Analyzer + + IMPROVEMENT + Improve performance of 'Project Explorer' pane + BUG_FIX Fixed the issue of disappearing components in some edge cases diff --git a/diylc/diylc-swing/src/main/java/org/diylc/swing/plugins/explorer/ExplorerPane.java b/diylc/diylc-swing/src/main/java/org/diylc/swing/plugins/explorer/ExplorerPane.java index 21e0fe30..e08d4e98 100644 --- a/diylc/diylc-swing/src/main/java/org/diylc/swing/plugins/explorer/ExplorerPane.java +++ b/diylc/diylc-swing/src/main/java/org/diylc/swing/plugins/explorer/ExplorerPane.java @@ -5,6 +5,7 @@ import java.awt.Dimension; import java.awt.Font; import java.awt.GridBagConstraints; import java.awt.GridBagLayout; +import java.awt.event.HierarchyEvent; import java.awt.event.ItemEvent; import java.awt.event.ItemListener; import java.awt.event.MouseAdapter; @@ -14,6 +15,7 @@ import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.Comparator; +import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; @@ -67,6 +69,9 @@ public class ExplorerPane extends JPanel { private JComboBox sortBox; private ListSelectionListener selectionListener; + private boolean stale; + private ExplorerSnapshot snapshot; + public ExplorerPane(ISwingUI swingUI, IPlugInPort plugInPort) { super(); this.swingUI = swingUI; @@ -105,6 +110,12 @@ public class ExplorerPane extends JPanel { add(getSortBox(), gbc); setPreferredSize(new Dimension(240, 200)); + + addHierarchyListener(e -> { + if ((e.getChangeFlags() & HierarchyEvent.SHOWING_CHANGED) != 0 && isShowing()) { + refreshIfStale(); + } + }); } public JLabel getTitleLabel() { @@ -238,6 +249,13 @@ public class ExplorerPane extends JPanel { }); componentList.setModel(getComponentListModel()); componentList.setFont(DEFAULT_FONT); + // without a fixed height every content change re-measures each row through the HTML + // renderer, which dominates the cost of a refresh on large projects + componentList.setFixedCellHeight(new DefaultListCellRenderer() + .getListCellRendererComponent(componentList, + "[L0] Xg (Xg)", + 0, false, false) + .getPreferredSize().height); componentList.setSelectionMode(ListSelectionModel.MULTIPLE_INTERVAL_SELECTION); componentList.setCellRenderer(new ComponentListCellRenderer(plugInPort.getComponentTypes())); selectionListener = new ListSelectionListener() { @@ -324,10 +342,36 @@ public class ExplorerPane extends JPanel { public void setComponents(List> components, Collection> selectedComponents) { - this.getComponentListModel().setComponents(components); + ExplorerSnapshot newSnapshot = + ExplorerSnapshot.of(components, plugInPort.getComponentGroups()); + // moving, rotating or nudging leaves everything the list shows untouched + if (!newSnapshot.equals(snapshot)) { + snapshot = newSnapshot; + this.getComponentListModel().setComponents(components); + } setSelection(new HashSet>(selectedComponents)); } + /** + * Called instead of {@link #setComponents} while the pane is not showing. When the project + * itself was replaced the list is emptied so it does not hold on to the old one. + */ + public void markStale(boolean projectReplaced) { + stale = true; + if (projectReplaced && snapshot != null) { + snapshot = null; + this.getComponentListModel().setComponents(new ArrayList>()); + } + } + + private void refreshIfStale() { + if (stale) { + stale = false; + setComponents(plugInPort.getCurrentProject().getComponents(), + plugInPort.getSelectedComponents()); + } + } + public void setSelection(Set> selectedComponents) { List selectedValuesList = componentList.getSelectedValuesList(); @@ -348,61 +392,16 @@ public class ExplorerPane extends JPanel { if (selectedComponents.size() == 0) { list.clearSelection(); } else { - // Build a map from component to its group (if any) - Set groups = plugInPort.getComponentGroups(); - Map, Set>> componentToGroup = new java.util.HashMap<>(); - if (groups != null && !groups.isEmpty()) { - // Build a map from UUID to component for quick lookup - List> allComponents = plugInPort.getCurrentProject().getComponents(); - Map> idToComponent = allComponents.stream() - .collect(Collectors.toMap(IDIYComponent::getId, comp -> comp)); - - for (ComponentGroup group : groups) { - // Convert ComponentGroup (with UUIDs) to Set> - Set> componentSet = group.getComponentIds().stream() - .map(idToComponent::get) - .filter(java.util.Objects::nonNull) - .collect(Collectors.toSet()); - - for (IDIYComponent component : componentSet) { - componentToGroup.put(component, componentSet); - } - } - } - - // Find indices of items to select - List components = this.getComponentListModel().getComponents(); + Map, Integer> rowIndex = this.getComponentListModel().getRowIndex(); Set selectedIndices = new HashSet<>(); - Set>> selectedGroups = new HashSet<>(); - for (IDIYComponent selected : selectedComponents) { - Set> group = componentToGroup.get(selected); - if (group != null) { - // Component is in a group - if (!selectedGroups.contains(group)) { - // Find the ComponentGroupItem for this group - for (int i = 0; i < components.size(); i++) { - ComponentGroupItem groupItem = components.get(i); - if (groupItem.getGroup().equals(group)) { - selectedIndices.add(i); - selectedGroups.add(group); - break; - } - } - } - } else { - // Component is not in any group, find the ComponentGroupItem that contains only this component - for (int i = 0; i < components.size(); i++) { - ComponentGroupItem groupItem = components.get(i); - if (groupItem.getGroup().size() == 1 && groupItem.getGroup().contains(selected)) { - selectedIndices.add(i); - break; - } - } + Integer index = rowIndex.get(selected); + if (index != null) { + selectedIndices.add(index); } } - int[] indices = selectedIndices.stream().mapToInt(i -> i).toArray(); + int[] indices = selectedIndices.stream().mapToInt(i -> i).sorted().toArray(); list.getSelectionModel().setValueIsAdjusting(true); list.setSelectedIndices(indices); @@ -428,6 +427,7 @@ public class ExplorerPane extends JPanel { private List> componentsRaw; private List components; + private Map, Integer> rowIndex = new HashMap<>(); private String searchText; private Sort sort = Sort.Z_INDEX; @@ -443,6 +443,10 @@ public class ExplorerPane extends JPanel { return components; } + public Map, Integer> getRowIndex() { + return rowIndex; + } + public Sort getSort() { return sort; } @@ -485,6 +489,13 @@ public class ExplorerPane extends JPanel { // Build list with groups and ungrouped components this.components = buildComponentListWithGroups(sortedComponents); + Map, Integer> newRowIndex = new HashMap<>(); + for (int i = 0; i < this.components.size(); i++) { + for (IDIYComponent component : this.components.get(i).getGroup()) { + newRowIndex.put(component, i); + } + } + this.rowIndex = newRowIndex; int newSize = this.components.size(); @@ -716,4 +727,30 @@ public class ExplorerPane extends JPanel { return group.hashCode(); } } + + /** + * Everything the list reads from the project, copied so that in-place edits of components + * and groups are detected. Components compare by identity, so an undo that swaps in clones + * still forces a rebuild. + */ + private record ExplorerSnapshot(List> components, List labels, + Map, String> groups) { + + static ExplorerSnapshot of(List> components, Set groups) { + List labels = new ArrayList<>(components.size() * 3); + for (IDIYComponent component : components) { + labels.add(component.getName()); + // toString() drives the name and type sorts + labels.add(component.toString()); + labels.add(component.getValueForDisplay()); + } + Map, String> groupNames = new HashMap<>(); + if (groups != null) { + for (ComponentGroup group : groups) { + groupNames.put(new HashSet<>(group.getComponentIds()), group.getName()); + } + } + return new ExplorerSnapshot(new ArrayList<>(components), labels, groupNames); + } + } } diff --git a/diylc/diylc-swing/src/main/java/org/diylc/swing/plugins/explorer/ExplorerPlugin.java b/diylc/diylc-swing/src/main/java/org/diylc/swing/plugins/explorer/ExplorerPlugin.java index 89948891..0abe4386 100644 --- a/diylc/diylc-swing/src/main/java/org/diylc/swing/plugins/explorer/ExplorerPlugin.java +++ b/diylc/diylc-swing/src/main/java/org/diylc/swing/plugins/explorer/ExplorerPlugin.java @@ -57,12 +57,19 @@ public class ExplorerPlugin implements IPlugIn { @Override public EnumSet getSubscribedEventTypes() { - return EnumSet.of(EventType.FILE_STATUS_CHANGED, EventType.PROJECT_LOADED, EventType.PROJECT_MODIFIED, EventType.SELECTION_CHANGED); + return EnumSet.of(EventType.PROJECT_LOADED, EventType.PROJECT_MODIFIED, + EventType.SELECTION_CHANGED); } @SuppressWarnings("unchecked") @Override public void processMessage(EventType eventType, Object... params) { + // the dispatcher offers no way to unsubscribe, so a collapsed pane only records that it + // has to catch up once it is shown again + if (!getExplorerPane().isShowing()) { + getExplorerPane().markStale(eventType == EventType.PROJECT_LOADED); + return; + } switch (eventType) { case PROJECT_MODIFIED: getExplorerPane().setComponents(((Project)params[1]).getComponents(), plugInPort.getSelectedComponents()); @@ -72,6 +79,7 @@ public class ExplorerPlugin implements IPlugIn { break; case SELECTION_CHANGED: getExplorerPane().setSelection((Set>)params[0]); + break; default: break; } diff --git a/diylc/docs/plans/explorer-refresh.md b/diylc/docs/plans/explorer-refresh.md new file mode 100644 index 00000000..f9eb38fc --- /dev/null +++ b/diylc/docs/plans/explorer-refresh.md @@ -0,0 +1,134 @@ +# Project Explorer Refresh Cost + +`ExplorerPane` (`diylc-swing/.../plugins/explorer`) rebuilds its whole list on every +`PROJECT_MODIFIED`, `PROJECT_LOADED` and `SELECTION_CHANGED`, even when the panel is collapsed +and even when the change cannot affect what it shows. This plan removes both costs without +touching `diylc-core` or the event contract. + +## 1. How the explorer is driven today + +- `Presenter` uses `MessageDispatcher(true)` — **synchronous**. Every + `processMessage` runs on the EDT inside the call that changed the project, so explorer work + adds directly to the latency of the edit that triggered it. +- The dispatcher reads `getSubscribedEventTypes()` **once, at registration**, and there is no + unregister path on `IPlugInPort`. "Disconnecting" therefore means returning early from + `processMessage`, which costs one method call per event and is equivalent in practice. +- `ExplorerPlugin.processMessage`: + - `PROJECT_MODIFIED` / `PROJECT_LOADED` → `ExplorerPane.setComponents(all, selection)`. + - `SELECTION_CHANGED` → `setSelection(...)`. (It also falls through to `default`; harmless.) + - `FILE_STATUS_CHANGED` is subscribed but unused. +- What `setComponents` costs, for *n* components: + 1. sort/filter/reverse copy of the component list; + 2. `buildComponentListWithGroups` — a UUID→component map over the whole project plus a + set per group; + 3. `fireContentsChanged(0, n)` — with no fixed cell height, `BasicListUI` re-measures + **every row** through the renderer, and most rows are `` labels, so each row is + an HTML parse. This is the dominant cost; + 4. `setSelection` rebuilds the group map again and does a linear scan per selected item. + +### Which interactions reach the explorer + +Read from `Presenter`; to be confirmed by measurement (step 0). + +| Interaction | Events reaching the explorer | Changes the list? | +| --- | --- | --- | +| Mouse drag of components / control points, per mouse move | none — `dragOver` fires only `REPAINT` | — | +| Drag start (`dragStarted`) | `SELECTION_CHANGED` | selection only | +| Drag end (`dragEnded`) | `SELECTION_CHANGED` + `PROJECT_MODIFIED("Drag")` | no | +| Arrow-key nudge (`nudgeSelection`), per key repeat | `PROJECT_MODIFIED("Move Selection")` | no | +| Rotate / mirror / align / distribute | `PROJECT_MODIFIED` | no | +| Click on canvas (even on the already-selected part) | `SELECTION_CHANGED` | selection only | +| Edit properties (name, value) | `PROJECT_MODIFIED("Edit Selection")` | yes | +| Add / delete / duplicate / paste, z-order, group / ungroup, undo / redo | `PROJECT_MODIFIED` or `PROJECT_LOADED` | yes | + +So the explorer does not rebuild on every mouse move; it rebuilds once per *gesture* and once +per *key repeat*, and each rebuild is O(n) HTML layouts. On a large project that is the stall +felt at the start and end of each drag and the lag when holding an arrow key. If profiling in +step 0 shows explorer work *during* a drag, something else is dispatching and needs finding +before this plan is enough. + +## 2. Decisions + +**Visibility is checked with `isShowing()` and a `HierarchyListener`, not the config key.** +`CollapsiblePanel` hides the pane with `setVisible(false)`; `isShowing()` also covers an +ancestor being hidden and the frame not yet being displayed. `HierarchyEvent.SHOWING_CHANGED` +fires for all of those, whereas listening to `ConfigPlugin.PROJECT_EXPLORER` would miss +everything except the menu toggle. + +**A hidden pane keeps no state in sync; it just remembers that it is stale.** On becoming +visible it pulls the current project and selection from `IPlugInPort` rather than replaying +events. On `PROJECT_LOADED` while hidden it also drops its model so it does not pin the +previous project's object graph. + +**"Does this change affect the list" is decided by comparing a snapshot of what the list +shows, not by the action string.** `PROJECT_MODIFIED` carries a description (`"Drag"`, +`"Move Selection"`, …) that could be whitelisted, but those strings are scattered through +`Presenter`, are not a contract, and a missed one shows stale data. A snapshot is robust: +the pane records exactly the inputs its model and renderer read, and skips the rebuild when +they are unchanged. Capturing it is O(n) plain field reads, far cheaper than O(n) HTML +layouts. + +The snapshot is a small private record (per `typed-records-over-string-keyed-maps`), holding +copies, not live references, because `Presenter` mutates components and groups in place: + +- the component list **by identity, in z-order**; +- per component: `getName()`, `toString()` (used by the NAME/TYPE sorts) and + `getValueForDisplay()`; +- per group: name and set of member UUIDs (`ComponentGroup` has no `equals`, and a rename + mutates the existing instance). + +The layer badge comes from `ComponentType.getZOrder()`, which is static per class, so it is +covered by identity. + +**Selection sync is O(selected), not O(n).** `refresh()` builds a +`Map, Integer>` from component to row index alongside the row list. +`setSelection` uses it instead of rebuilding the group map and scanning rows. The existing +"selection already matches" early-out stays. + +**Rows get a fixed height.** `setFixedCellHeight` from the font metrics stops `BasicListUI` +from measuring every row through the HTML renderer whenever contents change. Rows are a +single line, so nothing is lost. This also speeds up the rebuilds that remain necessary. + +**No debounce timer for now.** With the snapshot check, a nudge burst costs one O(n) +comparison per repeat and no layout. A `javax.swing.Timer` coalescer would add latency to +genuine edits for little gain; revisit only if step 0 shows the comparison itself matters. + +## 3. Changes + +All in `diylc-swing/src/main/java/org/diylc/swing/plugins/explorer/`. No core, library or +file-format changes; no regression-suite impact (the explorer is not rendered in exports). + +1. **`ExplorerPane`** + - Field `boolean stale`; method `refreshIfStale()` that, when showing, calls + `setComponents(plugInPort.getCurrentProject().getComponents(), + plugInPort.getSelectedComponents())`. + - `HierarchyListener` in the constructor: on `SHOWING_CHANGED` with `isShowing()`, call + `refreshIfStale()`. + - `setComponents`: build the snapshot; if equal to the previous one, skip + `ComponentListModel.setComponents` and go straight to `setSelection`. + - `ComponentListModel.refresh()`: also build the component→row map. + - `setSelection`: use the row map. + - `getComponentList()`: `setFixedCellHeight(...)`. + - Snapshot record declared at the bottom of the file with the other nested types. +2. **`ExplorerPlugin.processMessage`** + - First line: if `!explorerPane.isShowing()`, mark the pane stale (and on `PROJECT_LOADED` + clear its model), then return. + - Add the missing `break` after `SELECTION_CHANGED`. + - Drop `FILE_STATUS_CHANGED` from `getSubscribedEventTypes()`. + +Search and sort changes call `refresh()` directly and bypass the snapshot +check; the snapshot only gates the event-driven path. + +## 4. Status and next steps + +Everything in §3 is implemented in the working tree and `diylc-swing` compiles. Not yet done: + +1. **Measure.** On a large project (several hundred components), time `setComponents` and + `setSelection`, or run a profiler, while dragging, holding an arrow key and editing a + value. Compare against `master` to confirm the table in §1 and the gain. +2. **Manual check in the running app.** Collapse the explorer, edit, then expand: the list and + selection should be correct. Repeat with undo/redo and file open while it is collapsed. + Also check: rename and value edits in the property editor; group, rename the group, then + ungroup; z-order drag inside the explorer; search and sort while visible; row height + looks right under each look-and-feel/theme. +3. Add a debounce only if step 1 shows the snapshot comparison itself is significant.