Fix alarm tree update issue

#2611

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 use a model item wrappers class as the cell value
+        // and replace it (still referencing 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,27 @@ 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 indicating a change
+    @Override
+    protected boolean isItemChanged(AlarmTreeItem<?> before, 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

@@ -7,12 +7,8 @@
  *******************************************************************************/
 package org.phoebus.applications.alarm;
 
-import java.time.Instant;
-import java.util.concurrent.TimeUnit;
-
 import org.phoebus.ui.javafx.ApplicationWrapper;
 
-import javafx.application.Platform;
 import javafx.scene.Scene;
 import javafx.scene.control.Button;
 import javafx.scene.control.TreeItem;
@@ -23,6 +19,11 @@
 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,
@@ -33,12 +34,14 @@
  *     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 hidden items.
+ *  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
@@ -61,6 +64,8 @@ public void start(final Stage stage)
             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);
 
@@ -70,7 +75,9 @@ public void start(final Stage stage)
         TreeItem<String> top = new TreeItem<>("Top");
         tree_view.setRoot(top);
 
-        top.getChildren().addAll(new TreeItem<>("One"), new TreeItem<>("Two"), new TreeItem<>("Three"));
+        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);