diff --git a/app/alarm/ui/src/main/java/org/phoebus/applications/alarm/ui/area/AlarmAreaView.java b/app/alarm/ui/src/main/java/org/phoebus/applications/alarm/ui/area/AlarmAreaView.java index 63f5762453..809ad3d2e1 100644 --- a/app/alarm/ui/src/main/java/org/phoebus/applications/alarm/ui/area/AlarmAreaView.java +++ b/app/alarm/ui/src/main/java/org/phoebus/applications/alarm/ui/area/AlarmAreaView.java @@ -75,8 +75,11 @@ public class AlarmAreaView extends StackPane implements AlarmClientListener */ private final AtomicReference> item_changes = new AtomicReference<>(); - /** Set of items that have updated, with iteration order based on when they updated */ - private final Set items_to_update = new LinkedHashSet<>(); + /** Set of items that have updated, with iteration order based on when they updated + * + * SYNC on access + */ + private final Set item_paths_to_update = new LinkedHashSet<>(); /** Throttle [5Hz] used for updates of existing items */ private final UpdateThrottle update_throttle = new UpdateThrottle(200, TimeUnit.MILLISECONDS, this::updateItems); @@ -165,9 +168,9 @@ public void itemUpdated(final AlarmTreeItem item) if (null == item_name) return; - synchronized (items_to_update) + synchronized (item_paths_to_update) { - items_to_update.add(item_name); + item_paths_to_update.add(item_name); } update_throttle.trigger(); } @@ -178,10 +181,10 @@ private void updateItems() final List changed_items = item_changes.getAndSet(null); final String[] update_array; - synchronized (items_to_update) + synchronized (item_paths_to_update) { - update_array = items_to_update.toArray(new String[items_to_update.size()]); - items_to_update.clear(); + update_array = item_paths_to_update.toArray(new String[item_paths_to_update.size()]); + item_paths_to_update.clear(); } Platform.runLater(() -> { diff --git a/app/alarm/ui/src/main/java/org/phoebus/applications/alarm/ui/tree/AlarmTreeView.java b/app/alarm/ui/src/main/java/org/phoebus/applications/alarm/ui/tree/AlarmTreeView.java index f3d65c9801..3671f35bed 100644 --- a/app/alarm/ui/src/main/java/org/phoebus/applications/alarm/ui/tree/AlarmTreeView.java +++ b/app/alarm/ui/src/main/java/org/phoebus/applications/alarm/ui/tree/AlarmTreeView.java @@ -12,10 +12,11 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.HashMap; import java.util.LinkedHashSet; import java.util.List; +import java.util.Map; import java.util.Set; -import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CountDownLatch; import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; @@ -80,6 +81,14 @@ * as generic JavaFX Node, only calling public methods * defined on this class. * + *

The model (AlarmClient) and the AlarmTreeView are kept in + * sync by a lockstep between model and view updates. When the + * model is changed, it calls item[removed|added]. The AlarmClient + * then blocks until the method returns (i.e. until the UI has + * been updated) and then can move on to the next update. The + * exception to this is itemUpdated, which adds to a queue of + * items to be updated which is then regularly processed. + * * @author Kay Kasemir */ @SuppressWarnings("nls") @@ -112,13 +121,13 @@ public class AlarmTreeView extends BorderPane implements AlarmClientListener private final CountDownLatch block_item_changes = new CountDownLatch(1); /** Map from alarm tree path to view's TreeItem */ - private final ConcurrentHashMap>> path2view = new ConcurrentHashMap<>(); + private final Map>> path2view = new HashMap<>(); /** Items to update, ordered by time of original update request * - * SYNC on access + * SYNC on access */ - private final Set>> items_to_update = new LinkedHashSet<>(); + private final Set item_paths_to_update = new LinkedHashSet<>(); /** Throttle [5Hz] used for updates of existing items */ private final UpdateThrottle throttle = new UpdateThrottle(200, TimeUnit.MILLISECONDS, this::performUpdates); @@ -161,7 +170,7 @@ public AlarmTreeView(final AlarmClient model) { * @param model Model to represent. Must not be running, yet * @param itemName item name that may be expanded or given focus */ - public AlarmTreeView(final AlarmClient model, String itemName) + public AlarmTreeView(final AlarmClient model, final String itemName) { if (model.isRunning()) throw new IllegalStateException(); @@ -387,37 +396,34 @@ private void blockItemChanges() public void itemAdded(final AlarmTreeItem item) { blockItemChanges(); - // System.out.println(Thread.currentThread() + " Add " + item.getPathName()); - - // Parent must already exist - final AlarmTreeItem model_parent = item.getParent(); - final TreeItem> view_parent = path2view.get(model_parent.getPathName()); - if (view_parent == null) - { - dumpTree(tree_view.getRoot()); - throw new IllegalStateException("Missing parent view item for " + item.getPathName()); - } - // Create view item ASAP so that following updates will find it.. - final TreeItem> view_item = createViewItem(item); - - // .. but defer showing it on screen to UI thread final CountDownLatch done = new CountDownLatch(1); + Platform.runLater(() -> { indicateChange(); + + // Parent must already exist + final AlarmTreeItem model_parent = item.getParent(); + final TreeItem> view_parent = path2view.get(model_parent.getPathName()); + + if (view_parent == null) { + dumpTree(tree_view.getRoot()); + throw new IllegalStateException("Missing parent view item for " + item.getPathName()); + } + final TreeItem> view_item = createViewItem(item); + // Keep sorted by inserting at appropriate index final List>> items = view_parent.getChildren(); final int index = Collections.binarySearch(items, view_item, - (a, b) -> CompareNatural.compareTo(a.getValue().getName(), - b.getValue().getName())); + (a, b) -> CompareNatural.compareTo(a.getValue().getName(), + b.getValue().getName())); if (index < 0) - items.add(-index-1, view_item); + items.add(-index - 1, view_item); else items.add(index, view_item); done.countDown(); }); - updateStats(); // Waiting on the UI thread throttles the model's updates // to a rate that the UI can handle. @@ -438,18 +444,19 @@ public void itemAdded(final AlarmTreeItem item) public void itemRemoved(final AlarmTreeItem item) { blockItemChanges(); - // System.out.println(Thread.currentThread() + " Removed " + item.getPathName()); - // Remove item and all sub-items from model2ui - final TreeItem> view_item = removeViewItems(item); - if (view_item == null) - throw new IllegalStateException("No view item for " + item.getPathName()); - - // Remove the corresponding view final CountDownLatch done = new CountDownLatch(1); Platform.runLater(() -> { indicateChange(); + + // Remove item and all sub-items from model2ui + final TreeItem> view_item = removeViewItems(item); + if (view_item == null) + throw new IllegalStateException("No view item for " + item.getPathName()); + + // Remove the corresponding view + // // Can only locate the parent view item on UI thread, // because item might just have been created by itemAdded() event // and won't be on the screen until UI thread runs. @@ -459,7 +466,6 @@ public void itemRemoved(final AlarmTreeItem item) view_parent.getChildren().remove(view_item); done.countDown(); }); - updateStats(); // Waiting on the UI thread throttles the model's updates // to a rate that the UI can handle. @@ -493,87 +499,85 @@ private TreeItem> removeViewItems(final AlarmTreeItem item) public void itemUpdated(final AlarmTreeItem item) { blockItemChanges(); - // System.out.println(Thread.currentThread() + " Updated " + item.getPathName()); - final TreeItem> view_item = path2view.get(item.getPathName()); - if (view_item == null) - { - System.out.println("Unknown view for " + item.getPathName()); - path2view.keySet().stream().forEach(System.out::println); - throw new IllegalStateException("No view item for " + item.getPathName()); - } // UI update of existing item, i.e. // Platform.runLater(() -> TreeHelper.triggerTreeItemRefresh(view_item)); // is throttled. // If several items update, they're all redrawn in one Platform call, // and rapid updates of the same item are merged into just one final update - synchronized (items_to_update) + // We push the path instead of the view item in case the view item is removed + // between now and the update being handled. + synchronized (item_paths_to_update) { - items_to_update.add(view_item); + item_paths_to_update.add(item.getPathName()); } throttle.trigger(); - updateStats(); } /** Called by throttle to perform accumulated updates */ - @SuppressWarnings("unchecked") private void performUpdates() { - final TreeItem>[] view_items; - synchronized (items_to_update) + + // Drain items to update in one go; we will process it later on the main thread + final String[] view_item_paths; + synchronized (item_paths_to_update) { - // Creating a direct copy, i.e. another new LinkedHashSet<>(items_to_update), - // would be expensive, since we only need a _list_ of what's to update. - // Could use type-safe - // new ArrayList>>(items_to_update) - // but that calls toArray() internally, so doing that directly - view_items = items_to_update.toArray(new TreeItem[items_to_update.size()]); - items_to_update.clear(); + view_item_paths = item_paths_to_update.toArray(new String[item_paths_to_update.size()]); + item_paths_to_update.clear(); } - // Remember selection - final ObservableList>> updatedSelectedItems = - FXCollections.observableArrayList(tree_view.getSelectionModel().getSelectedItems()); - - // How to update alarm tree cells when data changed? - // `setValue()` with a truly new value (not 'equal') should suffice, - // but there are two problems: - // Since we're currently using the alarm tree model item as a value, - // the value as seen by the TreeView remains the same. - // We could use a model item wrapper class as the cell value - // and replace it (while still holding the same model item!) - // for the TreeView to see a different wrapper value, but - // as shown in org.phoebus.applications.alarm.TreeItemUpdateDemo, - // replacing a tree cell value fails to trigger refreshes - // for certain hidden items. - // Only replacing the TreeItem gives reliable refreshes. - for (final TreeItem> view_item : view_items) - // Top-level item has no parent, and is not visible, so we keep it - if (view_item.getParent() != null) - { - // Locate item in tree parent - final TreeItem> parent = view_item.getParent(); - final int index = parent.getChildren().indexOf(view_item); - - // Create new TreeItem for that value - final AlarmTreeItem value = view_item.getValue(); - final TreeItem> update = new TreeItem<>(value); - if (updatedSelectedItems.contains(view_item)) { - updatedSelectedItems.remove(view_item); - updatedSelectedItems.add(update); + Platform.runLater(() -> { + // Remember selection + final ObservableList>> updatedSelectedItems = + FXCollections.observableArrayList(tree_view.getSelectionModel().getSelectedItems()); + + // How to update alarm tree cells when data changed? + // `setValue()` with a truly new value (not 'equal') should suffice, + // but there are two problems: + // Since we're currently using the alarm tree model item as a value, + // the value as seen by the TreeView remains the same. + // We could use a model item wrapper class as the cell value + // and replace it (while still holding the same model item!) + // for the TreeView to see a different wrapper value, but + // as shown in org.phoebus.applications.alarm.TreeItemUpdateDemo, + // replacing a tree cell value fails to trigger refreshes + // for certain hidden items. + // Only replacing the TreeItem gives reliable refreshes. + for (final String view_item_path : view_item_paths) { + final TreeItem> view_item = path2view.get(view_item_path); + if (view_item == null) { + // Possible if the item has been removed since it was added to the update queue + continue; + } + + // Top-level item has no parent, and is not visible, so we keep it + if (view_item.getParent() != null) { + // Locate item in tree parent + final TreeItem> parent = view_item.getParent(); + final int index = parent.getChildren().indexOf(view_item); + + // Create new TreeItem for that value + final AlarmTreeItem value = view_item.getValue(); + final TreeItem> update = new TreeItem<>(value); + if (updatedSelectedItems.contains(view_item)) { + updatedSelectedItems.remove(view_item); + updatedSelectedItems.add(update); + } + // Move child links to new item + final ArrayList>> children = new ArrayList<>(view_item.getChildren()); + view_item.getChildren().clear(); + update.getChildren().addAll(children); + update.setExpanded(view_item.isExpanded()); + + path2view.put(value.getPathName(), update); + parent.getChildren().set(index, update); } - // Move child links to new item - final ArrayList>> children = new ArrayList<>(view_item.getChildren()); - view_item.getChildren().clear(); - update.getChildren().addAll(children); - update.setExpanded(view_item.isExpanded()); - - path2view.put(value.getPathName(), update); - parent.getChildren().set(index, update); } - // Restore selection - tree_view.getSelectionModel().clearSelection(); - updatedSelectedItems.forEach(item -> tree_view.getSelectionModel().select(item)); + + // Restore selection + tree_view.getSelectionModel().clearSelection(); + updatedSelectedItems.forEach(item -> tree_view.getSelectionModel().select(item)); + }); } /** Context menu, details depend on selected items */ @@ -689,25 +693,7 @@ private void addDragSupport() }); } -// private long next_stats = 0; -// private final AtomicInteger update_count = new AtomicInteger(); -// private volatile double updates_per_sec = 0.0; - - private void updateStats() - { -// final long time = System.currentTimeMillis(); -// if (time > next_stats) -// { -// final int updates = update_count.getAndSet(0); -// updates_per_sec = updates_per_sec * 0.9 + updates * 0.1; -// next_stats = time + 1000; -// System.out.format("%.2f updates/sec\n", updates_per_sec); -// } -// else -// update_count.incrementAndGet(); - } - - private void dumpTree(TreeItem> item) + private static void dumpTree(final TreeItem> item) { final ObservableList>> children = item.getChildren(); System.out.printf("item: %s , has %d children.\n", item.getValue().getName(), children.size());