From 04e5170403a86cd5be2c1f4b48e08317b460b4d2 Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Fri, 21 May 2021 17:47:42 +0200 Subject: [PATCH] * Made DiagnosticResultAction a "pseudo-singleton" that consumes diagnostic results * renamed FxmlFile.HEALTH_CHECK to match fxml file name * added missing DI scopes * simplified cell layout --- .../org/cryptomator/ui/common/FxmlFile.java | 2 +- .../ui/health/CheckDetailController.java | 8 ++-- .../cryptomator/ui/health/CheckListCell.java | 24 +++++++---- .../ui/health/HealthCheckModule.java | 7 ++-- .../ui/health/HealthCheckTask.java | 9 ++--- .../cryptomator/ui/health/ReportWriter.java | 2 +- ...esultAction.java => ResultFixApplier.java} | 40 +++++++++---------- .../ui/health/ResultListCellController.java | 26 ++++++------ .../ui/health/ResultListCellFactory.java | 10 +++-- .../ui/health/StartController.java | 2 +- 10 files changed, 71 insertions(+), 59 deletions(-) rename main/ui/src/main/java/org/cryptomator/ui/health/{DiagnosticResultAction.java => ResultFixApplier.java} (50%) diff --git a/main/ui/src/main/java/org/cryptomator/ui/common/FxmlFile.java b/main/ui/src/main/java/org/cryptomator/ui/common/FxmlFile.java index 32c142762..b8d5bbff0 100644 --- a/main/ui/src/main/java/org/cryptomator/ui/common/FxmlFile.java +++ b/main/ui/src/main/java/org/cryptomator/ui/common/FxmlFile.java @@ -12,7 +12,7 @@ public enum FxmlFile { ERROR("/fxml/error.fxml"), // FORGET_PASSWORD("/fxml/forget_password.fxml"), // HEALTH_START("/fxml/health_start.fxml"), // - HEALTH_CHECK("/fxml/health_check_list.fxml"), // + HEALTH_CHECK_LIST("/fxml/health_check_list.fxml"), // LOCK_FORCED("/fxml/lock_forced.fxml"), // LOCK_FAILED("/fxml/lock_failed.fxml"), // MAIN_WINDOW("/fxml/main_window.fxml"), // diff --git a/main/ui/src/main/java/org/cryptomator/ui/health/CheckDetailController.java b/main/ui/src/main/java/org/cryptomator/ui/health/CheckDetailController.java index 64f7cd6f3..12f6338e3 100644 --- a/main/ui/src/main/java/org/cryptomator/ui/health/CheckDetailController.java +++ b/main/ui/src/main/java/org/cryptomator/ui/health/CheckDetailController.java @@ -2,6 +2,7 @@ package org.cryptomator.ui.health; import com.tobiasdiez.easybind.EasyBind; import com.tobiasdiez.easybind.optional.OptionalBinding; +import org.cryptomator.cryptofs.health.api.DiagnosticResult; import org.cryptomator.ui.common.FxController; import javax.inject.Inject; @@ -14,15 +15,16 @@ import javafx.concurrent.Worker; import javafx.fxml.FXML; import javafx.scene.control.ListView; +@HealthCheckScoped public class CheckDetailController implements FxController { - private final Binding> results; + private final Binding> results; private final OptionalBinding taskState; private final Binding taskName; private final ResultListCellFactory resultListCellFactory; private final BooleanBinding producingResults; - public ListView resultsListView; + public ListView resultsListView; @Inject public CheckDetailController(ObjectProperty selectedTask, ResultListCellFactory resultListCellFactory) { @@ -45,8 +47,8 @@ public class CheckDetailController implements FxController { resultsListView.itemsProperty().bind(results); resultsListView.setCellFactory(resultListCellFactory); } - /* Getter/Setter */ + /* Getter/Setter */ public String getTaskName() { return taskName.getValue(); diff --git a/main/ui/src/main/java/org/cryptomator/ui/health/CheckListCell.java b/main/ui/src/main/java/org/cryptomator/ui/health/CheckListCell.java index f9e1beda9..19dd40051 100644 --- a/main/ui/src/main/java/org/cryptomator/ui/health/CheckListCell.java +++ b/main/ui/src/main/java/org/cryptomator/ui/health/CheckListCell.java @@ -6,6 +6,8 @@ import org.cryptomator.ui.controls.FontAwesome5IconView; import javafx.beans.value.ObservableValue; import javafx.concurrent.Worker; import javafx.geometry.Insets; +import javafx.geometry.Pos; +import javafx.scene.Node; import javafx.scene.control.ContentDisplay; import javafx.scene.control.ListCell; @@ -13,25 +15,24 @@ class CheckListCell extends ListCell { private final FontAwesome5IconView stateIcon = new FontAwesome5IconView(); - CheckListCell(){ - paddingProperty().set(new Insets(6)); + CheckListCell() { + setPadding(new Insets(6)); + setAlignment(Pos.CENTER_LEFT); + setContentDisplay(ContentDisplay.LEFT); } @Override protected void updateItem(HealthCheckTask item, boolean empty) { super.updateItem(item, empty); + if (item != null) { setText(item.getTitle()); item.stateProperty().addListener(this::stateChanged); - setGraphic(stateIcon); + setGraphic(graphicForState(item.getState())); stateIcon.setGlyph(glyphForState(item.getState())); - if (item.getState() == Worker.State.READY) { - stateIcon.setVisible(false); - } - setContentDisplay(ContentDisplay.LEFT); } else { + setGraphic(null); setText(null); - setContentDisplay(ContentDisplay.TEXT_ONLY); } } @@ -40,6 +41,13 @@ class CheckListCell extends ListCell { stateIcon.setVisible(true); } + private Node graphicForState(Worker.State state) { + return switch (state) { + case READY -> null; + case SCHEDULED, RUNNING, FAILED, CANCELLED, SUCCEEDED -> stateIcon; + }; + } + private FontAwesome5Icon glyphForState(Worker.State state) { return switch (state) { case READY -> FontAwesome5Icon.COG; //just a placeholder diff --git a/main/ui/src/main/java/org/cryptomator/ui/health/HealthCheckModule.java b/main/ui/src/main/java/org/cryptomator/ui/health/HealthCheckModule.java index 7ca3cbd05..0afb3c8bf 100644 --- a/main/ui/src/main/java/org/cryptomator/ui/health/HealthCheckModule.java +++ b/main/ui/src/main/java/org/cryptomator/ui/health/HealthCheckModule.java @@ -27,7 +27,6 @@ import javafx.scene.Scene; import javafx.stage.Modality; import javafx.stage.Stage; import java.security.SecureRandom; -import java.util.ArrayList; import java.util.Collection; import java.util.Map; import java.util.Optional; @@ -113,10 +112,10 @@ abstract class HealthCheckModule { } @Provides - @FxmlScene(FxmlFile.HEALTH_CHECK) + @FxmlScene(FxmlFile.HEALTH_CHECK_LIST) @HealthCheckScoped - static Scene provideHealthCheckScene(@HealthCheckWindow FxmlLoaderFactory fxmlLoaders) { - return fxmlLoaders.createScene(FxmlFile.HEALTH_CHECK); + static Scene provideHealthCheckListScene(@HealthCheckWindow FxmlLoaderFactory fxmlLoaders) { + return fxmlLoaders.createScene(FxmlFile.HEALTH_CHECK_LIST); } @Binds diff --git a/main/ui/src/main/java/org/cryptomator/ui/health/HealthCheckTask.java b/main/ui/src/main/java/org/cryptomator/ui/health/HealthCheckTask.java index db4fb71cb..e7aec4f7a 100644 --- a/main/ui/src/main/java/org/cryptomator/ui/health/HealthCheckTask.java +++ b/main/ui/src/main/java/org/cryptomator/ui/health/HealthCheckTask.java @@ -26,7 +26,7 @@ class HealthCheckTask extends Task { private final Masterkey masterkey; private final SecureRandom csprng; private final HealthCheck check; - private final ObservableList results; + private final ObservableList results; public HealthCheckTask(Path vaultPath, VaultConfig vaultConfig, Masterkey masterkey, SecureRandom csprng, HealthCheck check) { this.vaultPath = Objects.requireNonNull(vaultPath); @@ -35,7 +35,6 @@ class HealthCheckTask extends Task { this.csprng = Objects.requireNonNull(csprng); this.check = Objects.requireNonNull(check); this.results = FXCollections.observableArrayList(); - updateTitle(getDisplayNameOf(check)); } @@ -58,7 +57,7 @@ class HealthCheckTask extends Task { throw new RuntimeException(e); } } - Platform.runLater(() -> results.add(new DiagnosticResultAction(result,vaultPath,vaultConfig, masterkey,csprng))); //FIXME: there can be a lotta results, each with a reference to the master key -> differentiate with severity! + Platform.runLater(() -> results.add(result)); }); } return null; @@ -76,7 +75,7 @@ class HealthCheckTask extends Task { /* Getter */ - public ObservableList results() { + public ObservableList results() { return results; } @@ -85,7 +84,7 @@ class HealthCheckTask extends Task { } static String getDisplayNameOf(HealthCheck check) { - if( check instanceof DirIdCheck) { //TODO: discuss if this should be localized + if (check instanceof DirIdCheck) { //TODO: discuss if this should be localized return "DirectoryCheck"; } else { return check.identifier(); diff --git a/main/ui/src/main/java/org/cryptomator/ui/health/ReportWriter.java b/main/ui/src/main/java/org/cryptomator/ui/health/ReportWriter.java index 9590626cd..fb74cbd51 100644 --- a/main/ui/src/main/java/org/cryptomator/ui/health/ReportWriter.java +++ b/main/ui/src/main/java/org/cryptomator/ui/health/ReportWriter.java @@ -72,7 +72,7 @@ public class ReportWriter { case SUCCEEDED -> { writer.write("STATUS: SUCCESS\nRESULTS:\n"); for (var result : task.results()) { - writer.write(REPORT_CHECK_RESULT.formatted(result.getSeverity(), result.getDescription())); + writer.write(REPORT_CHECK_RESULT.formatted(result.getServerity(), result.toString())); } } case CANCELLED -> writer.write("STATUS: CANCELED\n"); diff --git a/main/ui/src/main/java/org/cryptomator/ui/health/DiagnosticResultAction.java b/main/ui/src/main/java/org/cryptomator/ui/health/ResultFixApplier.java similarity index 50% rename from main/ui/src/main/java/org/cryptomator/ui/health/DiagnosticResultAction.java rename to main/ui/src/main/java/org/cryptomator/ui/health/ResultFixApplier.java index 057acbb13..9a639e8a0 100644 --- a/main/ui/src/main/java/org/cryptomator/ui/health/DiagnosticResultAction.java +++ b/main/ui/src/main/java/org/cryptomator/ui/health/ResultFixApplier.java @@ -1,47 +1,47 @@ package org.cryptomator.ui.health; +import com.google.common.base.Preconditions; +import org.cryptomator.common.vaults.Vault; import org.cryptomator.cryptofs.VaultConfig; import org.cryptomator.cryptofs.health.api.DiagnosticResult; import org.cryptomator.cryptolib.api.Masterkey; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import javax.inject.Inject; import javafx.scene.control.Alert; import java.nio.file.Path; import java.security.SecureRandom; +import java.util.concurrent.atomic.AtomicReference; -class DiagnosticResultAction implements Runnable { +@HealthCheckScoped +class ResultFixApplier { + + private static final Logger LOG = LoggerFactory.getLogger(ResultFixApplier.class); - private final DiagnosticResult result; private final Path vaultPath; - private final VaultConfig vaultConfig; - private final Masterkey masterkey; private final SecureRandom csprng; + private final Masterkey masterkey; + private final VaultConfig vaultConfig; - DiagnosticResultAction(DiagnosticResult result, Path vaultPath, VaultConfig vaultConfig, Masterkey masterkey, SecureRandom csprng) { - this.result = result; - this.vaultPath = vaultPath; - this.vaultConfig = vaultConfig; - this.masterkey = masterkey; + @Inject + public ResultFixApplier(@HealthCheckWindow Vault vault, AtomicReference masterkeyRef, AtomicReference vaultConfigRef, SecureRandom csprng) { + this.vaultPath = vault.getPath(); + this.masterkey = masterkeyRef.get(); + this.vaultConfig = vaultConfigRef.get(); this.csprng = csprng; } - public void run() { + public void fix(DiagnosticResult result) { + Preconditions.checkArgument(result.getServerity() == DiagnosticResult.Severity.WARN, "Unfixable result"); try (var masterkeyClone = masterkey.clone(); // var cryptor = vaultConfig.getCipherCombo().getCryptorProvider(csprng).withKey(masterkeyClone)) { result.fix(vaultPath, vaultConfig, masterkeyClone, cryptor); } catch (Exception e) { - e.printStackTrace(); + LOG.error("Failed to apply fix", e); Alert alert = new Alert(Alert.AlertType.ERROR, e.getMessage()); alert.showAndWait(); //TODO: real error/not supported handling } } - - public DiagnosticResult.Severity getSeverity() { - return result.getServerity(); //TODO: fix spelling with updated cryptofs release - } - - public String getDescription() { - return result.toString(); - } - } diff --git a/main/ui/src/main/java/org/cryptomator/ui/health/ResultListCellController.java b/main/ui/src/main/java/org/cryptomator/ui/health/ResultListCellController.java index 0051265dd..a8720165c 100644 --- a/main/ui/src/main/java/org/cryptomator/ui/health/ResultListCellController.java +++ b/main/ui/src/main/java/org/cryptomator/ui/health/ResultListCellController.java @@ -1,6 +1,7 @@ package org.cryptomator.ui.health; import com.tobiasdiez.easybind.EasyBind; +import org.cryptomator.cryptofs.health.api.DiagnosticResult; import org.cryptomator.ui.common.FxController; import org.cryptomator.ui.controls.FontAwesome5Icon; import org.cryptomator.ui.controls.FontAwesome5IconView; @@ -13,27 +14,28 @@ import javafx.beans.value.ObservableValue; import javafx.fxml.FXML; import javafx.scene.control.Button; +// unscoped because each cell needs its own controller public class ResultListCellController implements FxController { - private final ObjectProperty result; + private final ResultFixApplier fixApplier; + private final ObjectProperty result; private final Binding description; - @FXML public FontAwesome5IconView iconView; - @FXML public Button actionButton; @Inject - public ResultListCellController() { + public ResultListCellController(ResultFixApplier fixApplier) { this.result = new SimpleObjectProperty<>(null); - this.description = EasyBind.wrapNullable(result).map(DiagnosticResultAction::getDescription).orElse(""); + this.description = EasyBind.wrapNullable(result).map(DiagnosticResult::toString).orElse(""); + this.fixApplier = fixApplier; result.addListener(this::updateCellContent); } - private void updateCellContent(ObservableValue observable, DiagnosticResultAction oldVal, DiagnosticResultAction newVal) { + private void updateCellContent(ObservableValue observable, DiagnosticResult oldVal, DiagnosticResult newVal) { iconView.getStyleClass().clear(); actionButton.setVisible(false); - switch (newVal.getSeverity()) { + switch (newVal.getServerity()) { case INFO -> { iconView.setGlyph(FontAwesome5Icon.INFO_CIRCLE); iconView.getStyleClass().add("glyph-icon-muted"); @@ -58,21 +60,21 @@ public class ResultListCellController implements FxController { public void runResultAction() { final var realResult = result.get(); if (realResult != null) { - realResult.run(); //TODO: this hogs currently the JAVAFX thread + fixApplier.fix(realResult); } } - /* Getter & Setter */ - public DiagnosticResultAction getResult() { + + public DiagnosticResult getResult() { return result.get(); } - public void setResult(DiagnosticResultAction result) { + public void setResult(DiagnosticResult result) { this.result.set(result); } - public ObjectProperty resultProperty() { + public ObjectProperty resultProperty() { return result; } diff --git a/main/ui/src/main/java/org/cryptomator/ui/health/ResultListCellFactory.java b/main/ui/src/main/java/org/cryptomator/ui/health/ResultListCellFactory.java index 11f0e0f6b..7acada487 100644 --- a/main/ui/src/main/java/org/cryptomator/ui/health/ResultListCellFactory.java +++ b/main/ui/src/main/java/org/cryptomator/ui/health/ResultListCellFactory.java @@ -1,6 +1,7 @@ package org.cryptomator.ui.health; +import org.cryptomator.cryptofs.health.api.DiagnosticResult; import org.cryptomator.ui.common.FxmlLoaderFactory; import javax.inject.Inject; @@ -13,7 +14,8 @@ import javafx.util.Callback; import java.io.IOException; import java.io.UncheckedIOException; -public class ResultListCellFactory implements Callback, ListCell> { +@HealthCheckScoped +public class ResultListCellFactory implements Callback, ListCell> { private final FxmlLoaderFactory fxmlLoaders; @@ -23,7 +25,7 @@ public class ResultListCellFactory implements Callback call(ListView param) { + public ListCell call(ListView param) { try { FXMLLoader fxmlLoader = fxmlLoaders.load("/fxml/health_result_listcell.fxml"); return new ResultListCellFactory.Cell(fxmlLoader.getRoot(), fxmlLoader.getController()); @@ -32,7 +34,7 @@ public class ResultListCellFactory implements Callback { + private static class Cell extends ListCell { private final Parent node; private final ResultListCellController controller; @@ -43,7 +45,7 @@ public class ResultListCellFactory implements Callback masterkeyRef, AtomicReference vaultConfigRef, @FxmlScene(FxmlFile.HEALTH_CHECK) Lazy checkScene, Lazy errorComponent) { + public StartController(@HealthCheckWindow Vault vault, @HealthCheckWindow Stage window, @HealthCheckWindow KeyLoadingStrategy keyLoadingStrategy, ExecutorService executor, AtomicReference masterkeyRef, AtomicReference vaultConfigRef, @FxmlScene(FxmlFile.HEALTH_CHECK_LIST) Lazy checkScene, Lazy errorComponent) { this.window = window; this.unverifiedVaultConfig = vault.getUnverifiedVaultConfig(); //TODO: prevent workflow at all, if the vault config is emtpy this.keyLoadingStrategy = keyLoadingStrategy;