From f827b1fc89dc70a43ed543aaef4bd438e3298e43 Mon Sep 17 00:00:00 2001 From: Armin Schrenk Date: Thu, 29 Apr 2021 16:31:05 +0200 Subject: [PATCH] Refactor HealthCheckTask execution: * new class HealthCheckSupervisor which encapsulates execution of all selected health checks * checkControllor only users supervisor --- .../ui/controls/FontAwesome5Icon.java | 1 + .../ui/health/CheckController.java | 96 +++++++++---------- .../cryptomator/ui/health/CheckListCell.java | 3 +- .../ui/health/HealthCheckModule.java | 8 -- .../ui/health/HealthCheckSupervisor.java | 80 ++++++++++++++++ .../ui/health/HealthCheckTask.java | 32 ++++++- .../ui/src/main/resources/css/light_theme.css | 48 ++++++++++ .../src/main/resources/fxml/health_check.fxml | 29 ++++-- 8 files changed, 224 insertions(+), 73 deletions(-) create mode 100644 main/ui/src/main/java/org/cryptomator/ui/health/HealthCheckSupervisor.java diff --git a/main/ui/src/main/java/org/cryptomator/ui/controls/FontAwesome5Icon.java b/main/ui/src/main/java/org/cryptomator/ui/controls/FontAwesome5Icon.java index f4d0d058a..e48818c1e 100644 --- a/main/ui/src/main/java/org/cryptomator/ui/controls/FontAwesome5Icon.java +++ b/main/ui/src/main/java/org/cryptomator/ui/controls/FontAwesome5Icon.java @@ -6,6 +6,7 @@ package org.cryptomator.ui.controls; public enum FontAwesome5Icon { ANCHOR("\uF13D"), // ARROW_UP("\uF062"), // + BAN("\uF05E"), // BUG("\uF188"), // CHECK("\uF00C"), // COG("\uF013"), // diff --git a/main/ui/src/main/java/org/cryptomator/ui/health/CheckController.java b/main/ui/src/main/java/org/cryptomator/ui/health/CheckController.java index 40287a6ca..571a447b5 100644 --- a/main/ui/src/main/java/org/cryptomator/ui/health/CheckController.java +++ b/main/ui/src/main/java/org/cryptomator/ui/health/CheckController.java @@ -2,106 +2,98 @@ package org.cryptomator.ui.health; import com.tobiasdiez.easybind.EasyBind; import com.tobiasdiez.easybind.optional.OptionalBinding; -import dagger.Lazy; -import org.cryptomator.cryptofs.VaultConfig; import org.cryptomator.cryptofs.health.api.DiagnosticResult; import org.cryptomator.ui.common.FxController; import javax.inject.Inject; -import javafx.beans.binding.Bindings; -import javafx.beans.binding.BooleanExpression; -import javafx.beans.binding.ObjectBinding; +import javafx.beans.binding.Binding; import javafx.beans.property.ObjectProperty; +import javafx.beans.property.ReadOnlyBooleanProperty; +import javafx.beans.property.SimpleObjectProperty; +import javafx.beans.property.SimpleStringProperty; import javafx.collections.FXCollections; import javafx.collections.ObservableList; import javafx.concurrent.Worker; import javafx.fxml.FXML; import javafx.scene.control.ListView; -import java.util.Collection; -import java.util.Objects; +import javafx.scene.control.TableColumn; +import javafx.scene.control.TableView; import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; -import java.util.concurrent.atomic.AtomicReference; @HealthCheckScoped public class CheckController implements FxController { - private final VaultConfig vaultConfig; - private final Runnable masterkeyDestructor; - private final ExecutorService executor; + private final HealthCheckSupervisor supervisor; + private final ExecutorService executorService; private final ObjectProperty selectedTask; - private final ObservableList tasks; - private final ObjectBinding> selectedResults; + private final Binding> selectedResults; private final OptionalBinding selectedTaskState; - private final BooleanExpression ready; - private final BooleanExpression running; + private final Binding selectedTaskName; + private final Binding selectedTaskDescription; + private final ReadOnlyBooleanProperty running; + /* FXML */ public ListView checksListView; - public ListView resultsListView; + public TableView resultsTableView; + public TableColumn resultDescriptionColumn; + public TableColumn resultSeverityColumn; @Inject - public CheckController(AtomicReference vaultConfigRef, Runnable masterkeyDestructor, Lazy> tasks, ObjectProperty selectedTask) { - this.vaultConfig = Objects.requireNonNull(vaultConfigRef.get()); - this.masterkeyDestructor = masterkeyDestructor; - this.executor = Executors.newSingleThreadExecutor(); - this.selectedTask = selectedTask; - this.selectedResults = Bindings.createObjectBinding(this::getSelectedResults, selectedTask); + public CheckController(HealthCheckSupervisor supervisor, ExecutorService executorService) { + this.supervisor = supervisor; + this.executorService = executorService; + this.selectedTask = new SimpleObjectProperty<>(); + this.selectedResults = EasyBind.wrapNullable(selectedTask).map(HealthCheckTask::results).orElse(FXCollections.emptyObservableList()); this.selectedTaskState = EasyBind.wrapNullable(selectedTask).mapObservable(HealthCheckTask::stateProperty); - this.ready = BooleanExpression.booleanExpression(selectedTaskState.map(Worker.State.READY::equals).orElse(false)); - this.running = BooleanExpression.booleanExpression(selectedTaskState.map(Worker.State.RUNNING::equals).orElse(false)); - this.tasks = FXCollections.observableArrayList(tasks.get()); + this.selectedTaskName = EasyBind.wrapNullable(selectedTask).map(HealthCheckTask::getTitle).orElse(""); + this.selectedTaskDescription = EasyBind.wrapNullable(selectedTask).map(task -> task.getCheck().toString()).orElse(""); + this.running = supervisor.runningProperty(); } @FXML public void initialize() { - checksListView.setItems(tasks); + checksListView.setItems(FXCollections.observableArrayList(supervisor.getTasks())); checksListView.setCellFactory(ignored -> new CheckListCell()); - resultsListView.itemsProperty().bind(selectedResults); - resultsListView.setCellFactory(ignored -> new ResultListCell()); - selectedTask.bind(checksListView.getSelectionModel().selectedItemProperty()); checksListView.getSelectionModel().select(0); + selectedTask.bind(checksListView.getSelectionModel().selectedItemProperty()); - tasks.forEach(task -> executor.execute(task)); - - //ensure that at the end the masterkey is destroyed - executor.submit(masterkeyDestructor); + resultsTableView.itemsProperty().bind(selectedResults); + resultDescriptionColumn.setCellValueFactory(cellFeatures -> new SimpleStringProperty(cellFeatures.getValue().toString())); + resultSeverityColumn.setCellValueFactory(cellFeatures -> new SimpleStringProperty(cellFeatures.getValue().getServerity().name())); + executorService.execute(supervisor); } @FXML public void cancelCheck() { - //assert selectedTask.get().isRunning(); Replace with something like executor.isRunning() - executor.shutdownNow(); - masterkeyDestructor.run(); + assert running.get(); + supervisor.cancel(true); } - /* Getter/Setter */ - public VaultConfig getVaultConfig() { - return vaultConfig; - } + /* Getter&Setter */ public boolean isRunning() { return running.get(); } - public BooleanExpression runningProperty() { + public ReadOnlyBooleanProperty runningProperty() { return running; } - public boolean isReady() { - return ready.get(); + public Binding selectedTaskNameProperty() { + return selectedTaskName; } - public BooleanExpression readyProperty() { - return ready; + public String isSelectedTaskName() { + return selectedTaskName.getValue(); } - private ObservableList getSelectedResults() { - if (selectedTask.get() == null) { - return FXCollections.emptyObservableList(); - } else { - return selectedTask.get().results(); - } + public Binding selectedTaskDescriptionProperty() { + return selectedTaskDescription; + } + + public String isSelectedTaskDescription() { + return selectedTaskDescription.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 a7da1c32e..d3f6712a4 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 @@ -36,7 +36,8 @@ class CheckListCell extends ListCell { return switch (state) { case READY, SCHEDULED -> FontAwesome5Icon.ANCHOR; case RUNNING -> FontAwesome5Icon.SPINNER; - case FAILED, CANCELLED -> FontAwesome5Icon.EXCLAMATION_TRIANGLE; + case FAILED -> FontAwesome5Icon.EXCLAMATION_TRIANGLE; + case CANCELLED -> FontAwesome5Icon.BAN; case SUCCEEDED -> FontAwesome5Icon.CHECK; }; } 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 dc587fdbe..281af947d 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 @@ -20,8 +20,6 @@ import org.cryptomator.ui.keyloading.KeyLoadingStrategy; import org.cryptomator.ui.mainwindow.MainWindow; import javax.inject.Provider; -import javafx.beans.property.ObjectProperty; -import javafx.beans.property.SimpleObjectProperty; import javafx.scene.Scene; import javafx.stage.Modality; import javafx.stage.Stage; @@ -36,12 +34,6 @@ import java.util.concurrent.atomic.AtomicReference; @Module(subcomponents = {KeyLoadingComponent.class}) abstract class HealthCheckModule { - @Provides - @HealthCheckScoped - static ObjectProperty selectedHealthCheckTask() { - return new SimpleObjectProperty<>(); - } - @Provides @HealthCheckScoped static AtomicReference provideMasterkeyRef() { diff --git a/main/ui/src/main/java/org/cryptomator/ui/health/HealthCheckSupervisor.java b/main/ui/src/main/java/org/cryptomator/ui/health/HealthCheckSupervisor.java new file mode 100644 index 000000000..5b42429fd --- /dev/null +++ b/main/ui/src/main/java/org/cryptomator/ui/health/HealthCheckSupervisor.java @@ -0,0 +1,80 @@ +package org.cryptomator.ui.health; + +import dagger.Lazy; + +import javax.inject.Inject; +import javafx.concurrent.Task; +import java.util.Collection; +import java.util.concurrent.CancellationException; +import java.util.concurrent.ConcurrentLinkedQueue; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.atomic.AtomicReference; + +@HealthCheckScoped +public class HealthCheckSupervisor extends Task { + + private final ExecutorService executor; + private final Lazy> lazyTasks; + private final Runnable masterkeyDestructor; + private final AtomicReference currentTask; + private final ConcurrentLinkedQueue remainingTasks; + + @Inject + public HealthCheckSupervisor(Lazy> lazyTasks, Runnable masterkeyDestructor) { + this.lazyTasks = lazyTasks; + this.masterkeyDestructor = masterkeyDestructor; + this.currentTask = new AtomicReference<>(null); + this.executor = Executors.newSingleThreadExecutor(); + this.remainingTasks = new ConcurrentLinkedQueue<>(); + } + + public Void call() { + remainingTasks.addAll(lazyTasks.get()); + while (!remainingTasks.isEmpty()) { + final var task = remainingTasks.remove(); + currentTask.set(task); + executor.execute(task); //TODO: think about if we set the scheduled property for all tasks? + try { + task.get(); + } catch (InterruptedException e) { + executor.shutdownNow(); + cleanup(); + ; + Thread.currentThread().interrupt(); //TODO: do we need this? + } catch (ExecutionException e) { + e.printStackTrace(); + } catch (CancellationException e) { + // ok + } + } + return null; + } + + @Override + public boolean cancel(boolean mayInterruptIfRunning) { + cleanup(); + currentTask.get().cancel(mayInterruptIfRunning); + if (mayInterruptIfRunning) { + executor.shutdownNow(); + } else { + executor.shutdown(); + } + return super.cancel(mayInterruptIfRunning); + } + + private void cleanup() { + remainingTasks.forEach(task -> task.cancel(false)); + remainingTasks.clear(); + } + + @Override + protected void done() { + masterkeyDestructor.run(); //TODO: if we destroy it, no check can rerun + } + + public Collection getTasks() { + return lazyTasks.get(); + } +} 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 686054205..56bcffc17 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 @@ -14,7 +14,9 @@ import javafx.concurrent.Task; import java.nio.file.Path; import java.security.SecureRandom; import java.util.Objects; +import java.util.Optional; import java.util.concurrent.CancellationException; +import java.util.concurrent.atomic.AtomicReference; class HealthCheckTask extends Task { @@ -27,6 +29,9 @@ class HealthCheckTask extends Task { private final HealthCheck check; private final ObservableList results; + private final AtomicReference endState; + private final AtomicReference exceptionOnDone; + public HealthCheckTask(Path vaultPath, VaultConfig vaultConfig, Masterkey masterkey, SecureRandom csprng, HealthCheck check) { this.vaultPath = Objects.requireNonNull(vaultPath); this.vaultConfig = Objects.requireNonNull(vaultConfig); @@ -34,6 +39,11 @@ class HealthCheckTask extends Task { this.csprng = Objects.requireNonNull(csprng); this.check = Objects.requireNonNull(check); this.results = FXCollections.observableArrayList(); + this.endState = new AtomicReference<>(null); + this.exceptionOnDone = new AtomicReference<>(); + + var tmp = check.identifier(); + updateTitle(tmp.substring(tmp.length() - 10)); //TODO: new method with reliable logic } @Override @@ -46,9 +56,14 @@ class HealthCheckTask extends Task { } // FIXME: slowdown for demonstration purposes only: try { - Thread.sleep(200); + Thread.sleep(2000); } catch (InterruptedException e) { - e.printStackTrace(); + if(isCancelled()) { + return; + } else { + Thread.currentThread().interrupt(); + throw new RuntimeException(e); + } } Platform.runLater(() -> results.add(result)); }); @@ -64,6 +79,12 @@ class HealthCheckTask extends Task { @Override protected void done() { LOG.info("finished {}", check.identifier()); + Platform.runLater(() -> endState.set(getState())); + } + + @Override + protected void failed() { + Platform.runLater(() -> exceptionOnDone.set(getException())); } /* Getter */ @@ -76,4 +97,11 @@ class HealthCheckTask extends Task { return check; } + public State getEndState() { + return endState.get(); + } + + public Optional getExceptionOnDone() { + return Optional.ofNullable(exceptionOnDone.get()); + } } diff --git a/main/ui/src/main/resources/css/light_theme.css b/main/ui/src/main/resources/css/light_theme.css index b0ba8ac8c..34b388c3f 100644 --- a/main/ui/src/main/resources/css/light_theme.css +++ b/main/ui/src/main/resources/css/light_theme.css @@ -947,3 +947,51 @@ -fx-fill: linear-gradient(to bottom, PRIMARY, transparent); -fx-stroke: transparent; } + +/***** + Sanitizer Results + TODO: make it pretty and copy it to dark theme +****/ + +.table-view{ + -fx-background-color: transparent; +} + +.table-view:focused{ + -fx-background-color: transparent; +} + +.table-view .column-header-background{ + -fx-background-color: linear-gradient(#131313 0%, #424141 100%); +} + +.table-view .column-header-background .label{ + -fx-background-color: transparent; + -fx-text-fill: white; +} + +.table-view .column-header { + -fx-background-color: transparent; +} + +.table-view .table-cell{ + -fx-text-fill: white; +} + +.table-row-cell{ + -fx-background-color: #616161; + -fx-background-insets: 0, 0 0 1 0; + -fx-padding: 0.0em; +} + +.table-row-cell:odd{ + -fx-background-color: #424242; + -fx-background-insets: 0, 0 0 1 0; + -fx-padding: 0.0em; +} + +.table-row-cell:selected { + -fx-background-color: #005797; + -fx-background-insets: 0; + -fx-background-radius: 1; +} diff --git a/main/ui/src/main/resources/fxml/health_check.fxml b/main/ui/src/main/resources/fxml/health_check.fxml index 786df27e7..131d025ab 100644 --- a/main/ui/src/main/resources/fxml/health_check.fxml +++ b/main/ui/src/main/resources/fxml/health_check.fxml @@ -2,10 +2,13 @@ - + + + + - - - - - - -