Merge pull request #2615 from ControlSystemStudio/alarm_tree_fixes

Alarm tree cell refresh fix

Author: kasemir

app/alarm/ui/src/main/java/org/phoebus/applications/alarm/ui/tree/AlarmTreeView.java

@@ -9,6 +9,7 @@
 
 import static org.phoebus.applications.alarm.AlarmSystem.logger;
 
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.LinkedHashSet;
@@ -41,7 +42,6 @@
 import org.phoebus.ui.javafx.PrintAction;
 import org.phoebus.ui.javafx.Screenshot;
 import org.phoebus.ui.javafx.ToolbarHelper;
-import org.phoebus.ui.javafx.TreeHelper;
 import org.phoebus.ui.javafx.UpdateThrottle;
 import org.phoebus.ui.selection.AppSelection;
 import org.phoebus.ui.spi.ContextMenuEntry;
@@ -466,9 +466,10 @@ public void itemUpdated(final AlarmTreeItem<?> item)
     }
 
     /** Called by throttle to perform accumulated updates */
+    @SuppressWarnings("unchecked")
     private void performUpdates()
     {
-        final TreeItem<?>[] view_items;
+        final TreeItem<AlarmTreeItem<?>>[] view_items;
         synchronized (items_to_update)
         {
             // Creating a direct copy, i.e. another new LinkedHashSet<>(items_to_update),
@@ -480,8 +481,38 @@ private void performUpdates()
             items_to_update.clear();
         }
 
-        for (final TreeItem<?> view_item : view_items)
-            TreeHelper.triggerTreeItemRefresh(view_item);
+        // 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<AlarmTreeItem<?>> 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<AlarmTreeItem<?>> 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<AlarmTreeItem<?>> update = new TreeItem<>(value);
+                // Move child links to new item
+                final ArrayList<TreeItem<AlarmTreeItem<?>>> 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);
+            }
     }
 
     /** Context menu, details depend on selected items */

app/alarm/ui/src/main/java/org/phoebus/applications/alarm/ui/tree/AlarmTreeViewCell.java

@@ -38,12 +38,31 @@ class AlarmTreeViewCell extends TreeCell<AlarmTreeItem<?>>
     private final Label label = new Label();
     private final ImageView image = new ImageView();
     private final HBox content = new HBox(image, label);
-            
+
+    // TreeCell optimizes redraws by suppressing updates
+    // when old and new values match.
+    // Since we use the model item as a value,
+    // the cell sees no change, in fact an identical reference.
+    // In the fullness of time, a complete redesign might be useful
+    // to present changing values to the TreeCell, but also note
+    // the issue shown in org.phoebus.applications.alarm.TreeItemUpdateDemo
+    //
+    // So for now we simply force redraws by always pretending a change.
+    // This seems bad for performance, but profiling the alarm GUI for
+    // a large configuration like org.phoebus.applications.alarm.AlarmConfigProducerDemo
+    // with 1000 'sections' of 10 subsections of 10 PVs,
+    // the time spent in updateItem is negligible.
+    @Override
+    protected boolean isItemChanged(final AlarmTreeItem<?> before, final AlarmTreeItem<?> after)
+    {
+        return true;
+    }
+
     @Override
     protected void updateItem(final AlarmTreeItem<?> item, final boolean empty)
     {
         super.updateItem(item, empty);
-        
+
         if (empty  ||  item == null)
             setGraphic(null);
         else

app/alarm/ui/src/test/java/org/phoebus/applications/alarm/TreeItemUpdateDemo.java

@@ -0,0 +1,92 @@
+/*******************************************************************************
+ * Copyright (c) 2023 Oak Ridge National Laboratory.
+ * All rights reserved. This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License v1.0
+ * which accompanies this distribution, and is available at
+ * http://www.eclipse.org/legal/epl-v10.html
+ *******************************************************************************/
+package org.phoebus.applications.alarm;
+
+import org.phoebus.ui.javafx.ApplicationWrapper;
+
+import javafx.scene.Scene;
+import javafx.scene.control.Button;
+import javafx.scene.control.TreeItem;
+import javafx.scene.control.TreeView;
+import javafx.scene.layout.HBox;
+import javafx.scene.layout.Priority;
+import javafx.scene.layout.VBox;
+import javafx.stage.Stage;
+
+/** Demo of alarm tree cell update
+ *
+ *  Demonstrates refresh issue for tree cells that are
+ *  not visible because they scrolled below the window bottom.
+ *
+ *  Only replacing the TreeItem results in reliable tree updates.
+ *
+ *  1) Start this program, click the "On" and "Off" buttons, and observe how item "Two" is updated
+ *  2) Click "Off", reduce the window height so that the items under "Top" are hidden,
+ *     click "On", increase window height to again show all items.
+ *     Would expect to see "On", but tree still shows "Off"
+ *     until the tree is collapsed and again expanded.
+ *  3) Click "On", reduce the window height so that the items under "Top" are hidden,
+ *     click "Off", increase window height to again show all items.
+ *     Tree should indeed show "Off".
+ *
+ *  Conclusion is that TreeItem.setValue() does not trigger updates of certain hidden items.
+ *  Basic google search suggests that the value must change, but "On" and "Off"
+ *  are different object references and they are not 'equal'.
+ *
+ *  When instead replacing the complete TreeItem, the tree is always updated.
+ *
+ *  Tested with JavaFX 15, 19, 20
+ *
+ *  @author Kay Kasemir
+ */
+public class TreeItemUpdateDemo extends ApplicationWrapper
+{
+    private TreeView<String> tree_view = new TreeView<>();
+
+    @Override
+    public void start(final Stage stage)
+    {
+        final Button on = new Button("On");
+        on.setOnAction(event ->
+        {   // Just updating the value of TreeItem is ignored for hidden items?
+            tree_view.getRoot().getChildren().get(1).setValue("On");
+        });
+        final Button off = new Button("Off");
+        off.setOnAction(event ->
+        {   // Replacing the TreeItem always "works"?
+            // (Note this would be more work for intermediate items
+            //  that have child nodes, since child nodes need to be moved/copied)
+            tree_view.getRoot().getChildren().set(1, new TreeItem<>("Off"));
+        });
+        final HBox bottom = new HBox(on, off);
+        on.setMaxWidth(1000);
+        off.setMaxWidth(1000);
+        HBox.setHgrow(on, Priority.ALWAYS);
+        HBox.setHgrow(off, Priority.ALWAYS);
+
+        final VBox root = new VBox(tree_view, bottom);
+        VBox.setVgrow(tree_view, Priority.ALWAYS);
+
+        TreeItem<String> top = new TreeItem<>("Top");
+        tree_view.setRoot(top);
+
+        top.getChildren().add(new TreeItem<>("One"));
+        top.getChildren().add(new TreeItem<>("Two"));
+        top.getChildren().add(new TreeItem<>("Three"));
+        top.setExpanded(true);
+
+        final Scene scene = new Scene(root, 300, 300);
+        stage.setScene(scene);
+        stage.show();
+    }
+
+    public static void main(final String[] args)
+    {
+        launch(TreeItemUpdateDemo.class, args);
+    }
+}