Bug fix logbook attachments handling

Author: georgweiss

app/logbook/olog/ui/src/main/java/org/phoebus/logbook/olog/ui/AttachmentsViewController.java

@@ -21,6 +21,7 @@
 import javafx.application.Platform;
 import javafx.beans.binding.Bindings;
 import javafx.beans.property.ReadOnlyBooleanProperty;
+import javafx.beans.property.SimpleListProperty;
 import javafx.beans.property.SimpleObjectProperty;
 import javafx.collections.FXCollections;
 import javafx.collections.ListChangeListener;
@@ -47,7 +48,6 @@
 import org.phoebus.framework.workbench.ApplicationService;
 import org.phoebus.logbook.Attachment;
 import org.phoebus.logbook.LogEntry;
-import org.phoebus.logbook.olog.ui.write.AttachmentsEditorController;
 import org.phoebus.ui.application.ApplicationLauncherService;
 import org.phoebus.ui.application.PhoebusApplication;
 import org.phoebus.ui.dialog.ExceptionDetailsErrorDialog;
@@ -61,7 +61,6 @@
 import java.net.URI;
 import java.nio.file.Files;
 import java.nio.file.StandardCopyOption;
-import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
@@ -75,49 +74,65 @@
 public class AttachmentsViewController {
 
     @FXML
+    @SuppressWarnings("unused")
     private SplitPane splitPane;
 
     @FXML
+    @SuppressWarnings("unused")
     private StackPane previewPane;
 
     @FXML
+    @SuppressWarnings("unused")
     private ImageView imagePreview;
 
     @FXML
+    @SuppressWarnings("unused")
     private GridPane noPreviewPane;
 
     @FXML
+    @SuppressWarnings("unused")
     private ListView<Attachment> attachmentListView;
 
     @FXML
+    @SuppressWarnings("unused")
     private Label placeholderLabel;
 
     /**
-     * List of attachments selected by user in the preview's {@link ListView}.
+     * List of attachments selected by user in the {@link ListView}.
      */
     private final ObservableList<Attachment> selectedAttachments = FXCollections.observableArrayList();
 
+    /**
+     * Keeps track of the {@link Attachment} currently in the {@link ImageView}, if any.
+     */
     private final SimpleObjectProperty<Attachment> selectedAttachment = new SimpleObjectProperty<>();
 
     /**
-     * List of listeners that will be notified when user has selected one or multiple attachments in
-     * the {@link ListView}.
+     * List of all {@link Attachment}s currently in view.
      */
-    private final List<ListChangeListener<Attachment>> listSelectionChangeListeners = new ArrayList<>();
-
     private final ObservableList<Attachment> attachments = FXCollections.observableArrayList();
 
-    public AttachmentsViewController(){
+    public AttachmentsViewController() {
     }
 
     @FXML
     public void initialize() {
 
         attachmentListView.getSelectionModel().setSelectionMode(SelectionMode.MULTIPLE);
         attachmentListView.setCellFactory(view -> new AttachmentRow());
-        attachmentListView.getSelectionModel().selectedItemProperty().addListener((observable, oldValue, newValue) -> {
-            selectedAttachment.set(newValue);
-            showPreview();
+
+        // Show preview when single selection property changes
+        selectedAttachment.addListener((obs, o, n) -> showPreview(n));
+
+        attachmentListView.getSelectionModel().getSelectedItems().addListener((ListChangeListener<Attachment>) change -> {
+            selectedAttachments.setAll(change.getList());
+            if (change.getList().size() == 1) {
+                // Set single selection property only if selection contains a single element
+                selectedAttachment.set(change.getList().get(0));
+            }
+            else if(attachments.isEmpty()){
+                showPreview(null);
+            }
         });
 
         attachmentListView.setOnMouseClicked(me -> {
@@ -145,17 +160,12 @@ public void initialize() {
                             ExceptionDetailsErrorDialog.openError(Messages.PreviewOpenErrorTitle, Messages.PreviewOpenErrorBody, null);
                         }
                     }
-                }
-                else {
-                   showImageAttachment();
+                } else {
+                    showImageAttachment();
                 }
             }
         });
 
-        attachmentListView.getSelectionModel().getSelectedItems().addListener((ListChangeListener<Attachment>) change -> {
-            selectedAttachments.setAll(change.getList());
-            listSelectionChangeListeners.forEach(l -> l.onChanged(change));
-        });
 
         attachmentListView.setOnContextMenuRequested((e) -> {
             ContextMenu contextMenu = new ContextMenu();
@@ -195,12 +205,14 @@ public void initialize() {
             }
             event.consume();
         });
+
+        attachmentListView.itemsProperty().bindBidirectional(new SimpleListProperty<>(attachments));
     }
 
     /**
      * Launches the Image Viewer application to show the selected image attachment with a watermark.
      */
-    private void showImageAttachment(){
+    private void showImageAttachment() {
         URI uri = selectedAttachment.get().getFile().toURI();
         URI withWatermark = UriBuilder.fromUri(uri).queryParam("watermark", "true").build();
         ApplicationLauncherService.openResource(withWatermark,
@@ -217,13 +229,13 @@ public void invalidateAttachmentList(LogEntry logEntry) {
         } else {
             placeholderLabel.setText(Messages.DownloadingAttachments);
         }
-        setAttachments(Collections.emptyList());
+        attachments.setAll(Collections.emptyList());
     }
 
-    public void setAttachments(Collection<Attachment> attachments) {
+    public void setAttachments(Collection<Attachment> attachmentsList) {
         Platform.runLater(() -> {
-            this.attachments.setAll(attachments);
-            attachmentListView.setItems(this.attachments);
+            this.attachments.setAll(attachmentsList);
+            //attachmentListView.setItems(this.attachments);
             // Update UI
             if (!this.attachments.isEmpty()) {
                 attachmentListView.getSelectionModel().select(this.attachments.get(0));
@@ -246,13 +258,15 @@ public void updateItem(Attachment attachment, boolean empty) {
     /**
      * Shows selected attachment in preview pane.
      */
-    private void showPreview() {
-        if (selectedAttachment.get() == null) {
+    private void showPreview(Attachment attachment) {
+        if (attachment == null) {
             imagePreview.visibleProperty().setValue(false);
             return;
         }
-        if (selectedAttachment.get().getContentType().startsWith("image")) {
-            showImagePreview(selectedAttachment.get());
+        if (attachment.getContentType().startsWith("image")) {
+            imagePreview.visibleProperty().setValue(true);
+            noPreviewPane.visibleProperty().setValue(false);
+            showImagePreview(attachment);
         } else {
             imagePreview.visibleProperty().setValue(false);
             noPreviewPane.visibleProperty().setValue(true);
@@ -279,9 +293,9 @@ private void showImagePreview(Attachment attachment) {
                         Image image = SwingFXUtils.toFXImage(bufferedImage, null);
                         imagePreview.visibleProperty().setValue(true);
                         imagePreview.setImage(image);
-                   });
+                    });
                 } catch (IOException ex) {
-                    Logger.getLogger(AttachmentsEditorController.class.getName())
+                    Logger.getLogger(AttachmentsViewController.class.getName())
                             .log(Level.SEVERE, "Unable to load image file " + attachment.getFile().getAbsolutePath(), ex);
                 }
             });
@@ -313,26 +327,24 @@ private void copyAttachment(File targetFolder, Attachment attachment) {
         }
     }
 
-    public void addListSelectionChangeListener(ListChangeListener<Attachment> changeListener) {
-        listSelectionChangeListeners.add(changeListener);
-    }
-
-    public void removeAttachments(List<Attachment> attachmentsToRemove) {
-        attachments.removeAll(attachmentsToRemove);
-    }
-
-    public List<Attachment> getAttachments() {
+    /**
+     * @return The {@link ObservableList} of attachments.
+     */
+    public ObservableList<Attachment> getAttachments() {
         return attachments;
     }
 
     /**
-     * Adds an {@link Attachment} to the list of {@link Attachment}s and selects it in order
-     * to show a preview (image files only).
+     * Adds {@link Attachment}s to the list of {@link Attachment}s. If only one
+     * {@link Attachment} is added it is also selected.
      *
-     * @param attachment The new {@link Attachment}
+     * @param attachmentsList The new {@link Attachment}s
      */
-    public void addAttachment(Attachment attachment) {
-        attachments.add(attachment);
-        attachmentListView.getSelectionModel().select(attachment);
+    public void addAttachments(List<Attachment> attachmentsList) {
+        Platform.runLater(() -> {
+            attachmentListView.getSelectionModel().clearSelection();
+            attachments.addAll(attachmentsList);
+            attachmentListView.getSelectionModel().select(attachmentsList.get(0));
+        });
     }
 }

app/logbook/olog/ui/src/main/java/org/phoebus/logbook/olog/ui/LogEntryDisplayController.java

@@ -160,35 +160,30 @@ public void goForward() {
         }
     }
 
+    /**
+     * Sets/renders the {@link LogEntry} in the view unless already rendered or unedited.
+     * @param logEntry A non-null {@link LogEntry}
+     */
     public void setLogEntry(LogEntry logEntry) {
-        if(logEntry == null){
-            currentViewProperty.set(EMPTY);
-        }
-        else{
-            logEntryProperty.set(logEntry);
-            singleLogEntryDisplayController.setLogEntry(logEntry);
-            currentViewProperty.set(SINGLE);
-            showHideLogEntryGroupButton.selectedProperty().set(false);
-            hasLinkedEntriesProperty.set(logEntry.getProperties()
-                    .stream().anyMatch(p -> p.getName().equals(LogGroupProperty.NAME)));
+        if(logEntryProperty.get() == null ||
+                !logEntryProperty.get().getId().equals(logEntry.getId()) ||
+                (logEntry.getModifiedDate() != null &&
+                        !logEntry.getModifiedDate().equals(logEntryProperty.get().getModifiedDate()))) {
+            Platform.runLater(() -> {
+                logEntryProperty.set(logEntry);
+                singleLogEntryDisplayController.setLogEntry(logEntry);
+                currentViewProperty.set(SINGLE);
+                showHideLogEntryGroupButton.selectedProperty().set(false);
+                hasLinkedEntriesProperty.set(logEntry.getProperties()
+                        .stream().anyMatch(p -> p.getName().equals(LogGroupProperty.NAME)));;
+            });
         }
     }
 
     public LogEntry getLogEntry() {
         return logEntryProperty.get();
     }
 
-    /**
-     * Updates the current {@link LogEntry} if it matches the passed argument.
-     * @param logEntry A log entry that has been updated by user and saved by service.
-     */
-    public void updateLogEntry(LogEntry logEntry){
-        // Log entry display may be "empty", i.e. logEntryProperty not set yet
-        if(!logEntryProperty.isNull().get() && logEntryProperty.get().getId().equals(logEntry.getId())){
-            setLogEntry(logEntry);
-        }
-    }
-
     public void setLogEntryTableViewController(LogEntryTableViewController logEntryTableViewController){
         this.logEntryTableViewController = logEntryTableViewController;
         if (logEntryTableViewController.goBackAndGoForwardActions.isPresent()) {