Browse Source

Refactor HealthCheckTask execution:
* new class HealthCheckSupervisor which encapsulates execution of all selected health checks
* checkControllor only users supervisor

Armin Schrenk 4 years ago
parent
commit
f827b1fc89

+ 1 - 0
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"), //

+ 44 - 52
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<HealthCheckTask> selectedTask;
-	private final ObservableList<HealthCheckTask> tasks;
-	private final ObjectBinding<ObservableList<DiagnosticResult>> selectedResults;
+	private final Binding<ObservableList<DiagnosticResult>> selectedResults;
 	private final OptionalBinding<Worker.State> selectedTaskState;
-	private final BooleanExpression ready;
-	private final BooleanExpression running;
+	private final Binding<String> selectedTaskName;
+	private final Binding<String> selectedTaskDescription;
+	private final ReadOnlyBooleanProperty running;
 
+	/* FXML */
 	public ListView<HealthCheckTask> checksListView;
-	public ListView<DiagnosticResult> resultsListView;
+	public TableView<DiagnosticResult> resultsTableView;
+	public TableColumn<DiagnosticResult, String> resultDescriptionColumn;
+	public TableColumn<DiagnosticResult, String> resultSeverityColumn;
 
 
 	@Inject
-	public CheckController(AtomicReference<VaultConfig> vaultConfigRef, Runnable masterkeyDestructor, Lazy<Collection<HealthCheckTask>> tasks, ObjectProperty<HealthCheckTask> 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<String> selectedTaskNameProperty() {
+		return selectedTaskName;
+	}
+
+	public String isSelectedTaskName() {
+		return selectedTaskName.getValue();
 	}
 
-	public BooleanExpression readyProperty() {
-		return ready;
+	public Binding<String> selectedTaskDescriptionProperty() {
+		return selectedTaskDescription;
 	}
 
-	private ObservableList<DiagnosticResult> getSelectedResults() {
-		if (selectedTask.get() == null) {
-			return FXCollections.emptyObservableList();
-		} else {
-			return selectedTask.get().results();
-		}
+	public String isSelectedTaskDescription() {
+		return selectedTaskDescription.getValue();
 	}
 }

+ 2 - 1
main/ui/src/main/java/org/cryptomator/ui/health/CheckListCell.java

@@ -36,7 +36,8 @@ class CheckListCell extends ListCell<HealthCheckTask> {
 		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;
 		};
 	}

+ 0 - 8
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<HealthCheckTask> selectedHealthCheckTask() {
-		return new SimpleObjectProperty<>();
-	}
-
 	@Provides
 	@HealthCheckScoped
 	static AtomicReference<Masterkey> provideMasterkeyRef() {

+ 80 - 0
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<Void> {
+
+	private final ExecutorService executor;
+	private final Lazy<Collection<HealthCheckTask>> lazyTasks;
+	private final Runnable masterkeyDestructor;
+	private final AtomicReference<HealthCheckTask> currentTask;
+	private final ConcurrentLinkedQueue<HealthCheckTask> remainingTasks;
+
+	@Inject
+	public HealthCheckSupervisor(Lazy<Collection<HealthCheckTask>> 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<HealthCheckTask> getTasks() {
+		return lazyTasks.get();
+	}
+}

+ 30 - 2
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<Void> {
 
@@ -27,6 +29,9 @@ class HealthCheckTask extends Task<Void> {
 	private final HealthCheck check;
 	private final ObservableList<DiagnosticResult> results;
 
+	private final AtomicReference<State> endState;
+	private final AtomicReference<Throwable> 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<Void> {
 		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<Void> {
 				}
 				// 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<Void> {
 	@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<Void> {
 		return check;
 	}
 
+	public State getEndState() {
+		return endState.get();
+	}
+
+	public Optional<Throwable> getExceptionOnDone() {
+		return Optional.ofNullable(exceptionOnDone.get());
+	}
 }

+ 48 - 0
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;
+}

+ 19 - 10
main/ui/src/main/resources/fxml/health_check.fxml

@@ -2,10 +2,13 @@
 
 <?import javafx.geometry.Insets?>
 <?import javafx.scene.control.Button?>
-<?import javafx.scene.control.ButtonBar?>
+<?import javafx.scene.control.Label?>
 <?import javafx.scene.control.ListView?>
+<?import javafx.scene.control.TableColumn?>
+<?import javafx.scene.control.TableView?>
 <?import javafx.scene.layout.HBox?>
 <?import javafx.scene.layout.VBox?>
+<?import javafx.scene.text.Text?>
 <HBox xmlns:fx="http://javafx.com/fxml"
 	  xmlns="http://javafx.com/javafx"
 	  fx:controller="org.cryptomator.ui.health.CheckController"
@@ -17,15 +20,21 @@
 		<Insets topRightBottomLeft="12"/>
 	</padding>
 	<children>
-		<ListView fx:id="checksListView"/>
-		<VBox>
-			<ListView fx:id="resultsListView"/>
-
-			<ButtonBar buttonMinWidth="120" buttonOrder="+CA">
-				<buttons>
-					<Button text="%generic.button.cancel" ButtonBar.buttonData="CANCEL_CLOSE" onAction="#cancelCheck" disable="${!controller.running}"/>
-				</buttons>
-			</ButtonBar>
+		<VBox minWidth="80">
+			<Label fx:id="listHeading" text="Health checks"/>
+			<ListView fx:id="checksListView"/>
+			<Button text="%generic.button.cancel" onAction="#cancelCheck" disable="${!controller.running}" maxWidth="Infinity"/>
+		</VBox>
+		<VBox spacing="6">
+			<Label fx:id="checkTitle" styleClass="label-large" text="${controller.selectedTaskName}" />
+			<Text fx:id="checkDescription" styleClass="label" text="${controller.selectedTaskDescription}" />
+			<TableView fx:id="resultsTableView" >
+				<columns>
+					<TableColumn fx:id="resultDescriptionColumn" text="Info" editable="false"/>
+					<TableColumn fx:id="resultSeverityColumn" text="Severity" editable="false"/>
+					<!--TableColumn fx:id="resultAction" text="Action" /-->
+				</columns>
+			</TableView>
 		</VBox>
 	</children>
 </HBox>