improvements from review

- remove abstraction of AutogramBatchStartCallback - real class now - remve updateBatch from autogram and UI - it's implementation detail of GUI - add static asserts for GUI/work threads
slovensko-digital · Jul 31, 2023 · dc4dc93 · dc4dc93
1 parent df5b6cb
commit dc4dc93
Show file tree

Hide file tree

Showing 12 changed files with 80 additions and 84 deletions.
diff --git a/src/main/java/digital/slovensko/autogram/core/Autogram.java b/src/main/java/digital/slovensko/autogram/core/Autogram.java
@@ -88,32 +88,7 @@ public void batchStart(int totalNumberOfDocuments, BatchResponder responder) {
             throw new BatchConflictException("Iné hromadné podpisovanie už prebieha");
         batch = new Batch(totalNumberOfDocuments);
 
-        var startBatchTask = new AutogramBatchStartCallback() {
-            public void accept(SigningKey key) {
-                try {
-                    Logging.log("Starting batch");
-                    batch.start(key);
-                    handleSuccess();
-                } catch (Exception e) {
-                    handleException(e);
-                }                
-            }
-
-            private void handleException(Exception e) {
-                if (e instanceof AutogramException)
-                    responder.onBatchStartFailure((AutogramException) e);
-                else {
-                    Logging.log("Batch start failed with exception: " + e);
-                    responder.onBatchStartFailure(
-                            new AutogramException("Unkown error occured while starting batch", "",
-                                    "Batch start failed with exception: " + e, e));
-                }
-            }
-
-            private void handleSuccess() {
-                ui.onWorkThreadDo(() -> responder.onBatchStartSuccess(batch));
-            }
-        };
+        var startBatchTask = new AutogramBatchStartCallback(batch, responder);
 
         ui.onUIThreadDo(() -> {
             ui.startBatch(batch, this, startBatchTask);
@@ -141,12 +116,6 @@ public void batchSign(SigningJob job, String batchId) {
         }
     }
 
-    public void updateBatch() {
-        ui.onUIThreadDo(() -> {
-            ui.updateBatch();
-        });
-    }
-
     /**
      * End the batch
      * 

diff --git a/src/main/java/digital/slovensko/autogram/core/AutogramBatchStartCallback.java b/src/main/java/digital/slovensko/autogram/core/AutogramBatchStartCallback.java
@@ -2,5 +2,43 @@
 
 import java.util.function.Consumer;
 
-public abstract class AutogramBatchStartCallback implements Consumer<SigningKey> {
-}
+import digital.slovensko.autogram.core.errors.AutogramException;
+import digital.slovensko.autogram.ui.gui.GUI;
+import digital.slovensko.autogram.util.Logging;
+
+public class AutogramBatchStartCallback implements Consumer<SigningKey> {
+
+    private final Batch batch;
+    private final BatchResponder responder;
+
+    public AutogramBatchStartCallback(Batch batch, BatchResponder responder) {
+        this.batch = batch;
+        this.responder = responder;
+    }
+
+    public void accept(SigningKey key) {
+        GUI.assertOnWorkThread();
+        try {
+            Logging.log("Starting batch");
+            batch.start(key);
+            handleSuccess();
+        } catch (Exception e) {
+            handleException(e);
+        }
+    }
+
+    private void handleException(Exception e) {
+        if (e instanceof AutogramException)
+            responder.onBatchStartFailure((AutogramException) e);
+        else {
+            Logging.log("Batch start failed with exception: " + e);
+            responder.onBatchStartFailure(
+                    new AutogramException("Unkown error occured while starting batch", "",
+                            "Batch start failed with exception: " + e, e));
+        }
+    }
+
+    private void handleSuccess() {
+        responder.onBatchStartSuccess(batch);
+    }
+};
diff --git a/src/main/java/digital/slovensko/autogram/core/SigningJob.java b/src/main/java/digital/slovensko/autogram/core/SigningJob.java
@@ -16,7 +16,6 @@
 import eu.europa.esig.dss.pades.signature.PAdESService;
 import eu.europa.esig.dss.validation.CommonCertificateVerifier;
 import eu.europa.esig.dss.xades.signature.XAdESService;
-import javafx.application.Platform;
 
 public class SigningJob {
     private final Responder responder;

diff --git a/src/main/java/digital/slovensko/autogram/ui/SaveFileFromBatchResponder.java b/src/main/java/digital/slovensko/autogram/ui/SaveFileFromBatchResponder.java
@@ -1,6 +1,5 @@
 package digital.slovensko.autogram.ui;
 
-import digital.slovensko.autogram.core.Autogram;
 import digital.slovensko.autogram.core.Responder;
 import digital.slovensko.autogram.core.SignedDocument;
 import digital.slovensko.autogram.core.TargetPath;
@@ -13,16 +12,13 @@
 
 public class SaveFileFromBatchResponder extends Responder {
     private final File file;
-    private final Autogram autogram;
-    // private final File targetDirectory;
     private final Consumer<File> callbackSuccess;
     private final Consumer<AutogramException> callbackError;
     private final TargetPath targetPath;
 
-    public SaveFileFromBatchResponder(File file, Autogram autogram, TargetPath targetPath,
+    public SaveFileFromBatchResponder(File file, TargetPath targetPath,
             Consumer<File> callbackSuccess, Consumer<AutogramException> callbackError) {
         this.file = file;
-        this.autogram = autogram;
         this.targetPath = targetPath;
         this.callbackSuccess = callbackSuccess;
         this.callbackError = callbackError;
@@ -33,7 +29,6 @@ public void onDocumentSigned(SignedDocument signedDocument) {
             var targetFile = targetPath.getSaveFilePath(file.toPath());
             signedDocument.getDocument().save(targetFile.toString());
             Logging.log("Saved file " + targetFile.toString());
-            autogram.updateBatch();
             callbackSuccess.accept(targetFile.toFile());
         } catch (IOException e) {
             throw new RuntimeException(e);

diff --git a/src/main/java/digital/slovensko/autogram/ui/UI.java b/src/main/java/digital/slovensko/autogram/ui/UI.java
@@ -14,11 +14,9 @@
 public interface UI {
     void startSigning(SigningJob job, Autogram autogram);
 
-    void startBatch(Batch batch, Autogram autogram, AutogramBatchStartCallback callback);
+    void startBatch(Batch batch, Autogram autogram, Consumer<SigningKey> callback);
 
     void signBatch(SigningJob job, SigningKey key);
-
-    public void updateBatch();
 
     void cancelBatch(Batch batch);
 

diff --git a/src/main/java/digital/slovensko/autogram/ui/cli/CliUI.java b/src/main/java/digital/slovensko/autogram/ui/cli/CliUI.java
@@ -9,7 +9,6 @@
 
 import digital.slovensko.autogram.Main;
 import digital.slovensko.autogram.core.Autogram;
-import digital.slovensko.autogram.core.AutogramBatchStartCallback;
 import digital.slovensko.autogram.core.Batch;
 import digital.slovensko.autogram.core.SigningJob;
 import digital.slovensko.autogram.core.SigningKey;
@@ -69,7 +68,7 @@ public void setJobsCount(int nJobsTotal) {
     }
 
     @Override
-    public void startBatch(Batch batch, Autogram autogram, AutogramBatchStartCallback callback) {
+    public void startBatch(Batch batch, Autogram autogram, Consumer<SigningKey> callback) {
         // TODO Auto-generated method stub
     }
 
@@ -83,11 +82,6 @@ public void cancelBatch(Batch batch) {
         // TODO Auto-generated method stub
     }
 
-    @Override
-    public void updateBatch() {
-        // TODO Auto-generated method stub
-    }
-
     @Override
     public void pickTokenDriverAndThen(List<TokenDriver> drivers, Consumer<TokenDriver> callback) {
         TokenDriver pickedDriver;

diff --git a/src/main/java/digital/slovensko/autogram/ui/gui/BatchDialogController.java b/src/main/java/digital/slovensko/autogram/ui/gui/BatchDialogController.java
@@ -1,7 +1,8 @@
 package digital.slovensko.autogram.ui.gui;
 
+import java.util.function.Consumer;
+
 import digital.slovensko.autogram.core.Autogram;
-import digital.slovensko.autogram.core.AutogramBatchStartCallback;
 import digital.slovensko.autogram.core.Batch;
 import digital.slovensko.autogram.core.SigningKey;
 import digital.slovensko.autogram.util.DSSUtils;
@@ -21,7 +22,7 @@ public class BatchDialogController implements SuppressedFocusController {
     private final GUI gui;
     private final Batch batch;
     private final Autogram autogram;
-    private final AutogramBatchStartCallback startBatchCallback;
+    private final Consumer<SigningKey> startBatchCallback;
 
     @FXML
     VBox mainBox;
@@ -51,7 +52,7 @@ public class BatchDialogController implements SuppressedFocusController {
     @FXML
     public Button cancelBatchButton;
 
-    public BatchDialogController(Batch batch, AutogramBatchStartCallback startBatchCallback,
+    public BatchDialogController(Batch batch, Consumer<SigningKey> startBatchCallback,
             Autogram autogram, GUI gui) {
 
         this.gui = gui;

diff --git a/src/main/java/digital/slovensko/autogram/ui/gui/GUI.java b/src/main/java/digital/slovensko/autogram/ui/gui/GUI.java
@@ -6,8 +6,10 @@
 import java.util.WeakHashMap;
 import java.util.function.Consumer;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 import digital.slovensko.autogram.core.Autogram;
-import digital.slovensko.autogram.core.AutogramBatchStartCallback;
 import digital.slovensko.autogram.core.Batch;
 import digital.slovensko.autogram.core.SigningJob;
 import digital.slovensko.autogram.core.SigningKey;
@@ -36,7 +38,8 @@ public class GUI implements UI {
     private SigningKey activeKey;
     private final HostServices hostServices;
     private BatchDialogController batchController;
-    private final boolean DEBUG = false;
+    private static final boolean DEBUG = true;
+    private static Logger logger = LoggerFactory.getLogger(GUI.class);
 
     public GUI(HostServices hostServices) {
         this.hostServices = hostServices;
@@ -48,7 +51,7 @@ public void startSigning(SigningJob job, Autogram autogram) {
     }
 
     @Override
-    public void startBatch(Batch batch, Autogram autogram, AutogramBatchStartCallback callback) {
+    public void startBatch(Batch batch, Autogram autogram, Consumer<SigningKey> callback) {
         batchController = new BatchDialogController(batch, callback, autogram, this);
         var root = GUIUtils.loadFXML(batchController, "batch-dialog.fxml");
 
@@ -72,6 +75,7 @@ public void cancelBatch(Batch batch) {
 
     @Override
     public void signBatch(SigningJob job, SigningKey key) {
+        assertOnWorkThread();
         try {
             job.signWithKeyAndRespond(key);
             Logging.log("GUI: Signing batch job: " + job.hashCode() + " file " + job.getDocument().getName());
@@ -83,18 +87,16 @@ public void signBatch(SigningJob job, SigningKey key) {
             AutogramException autogramException = new AutogramException("Document signing has failed", "", "", e);
             job.onDocumentSignFailed(autogramException);
         }
-        updateBatch();
+        onUIThreadDo(() -> {
+            updateBatch();
+        });
     }
 
-    @Override
-    public void updateBatch() {
-        Logging.log("GUI: updateBatch " + Platform.isFxApplicationThread());
-        if (Platform.isFxApplicationThread()) {
-            batchController.update();
-        } else {
-            onWorkThreadDo(() -> onUIThreadDo(batchController::update));
-        }
-        Logging.log("GUI: updateBatch2");
+    private void updateBatch() {
+        if (batchController == null)
+            return;
+        assertOnUIThread();
+        batchController.update();
     }
 
     @Override
@@ -173,6 +175,7 @@ private void refreshKeyOnAllJobs() {
     }
 
     private void showError(AutogramException e) {
+        logger.debug("GUI showing error", e);
         var controller = new ErrorController(e);
         var root = GUIUtils.loadFXML(controller, "error-dialog.fxml");
 
@@ -280,6 +283,7 @@ public void onPickSigningKeyFailed(AutogramException e) {
     public void onSigningSuccess(SigningJob job) {
         jobControllers.get(job).close();
         refreshKeyOnAllJobs();
+        updateBatch();
     }
 
     @Override
@@ -328,9 +332,6 @@ public void onDocumentBatchSaved(BatchUiResult result) {
 
     @Override
     public void onWorkThreadDo(Runnable callback) {
-        if (DEBUG && !Platform.isFxApplicationThread())
-            throw new RuntimeException("Can be run only on UI thread");
-
         if (Platform.isFxApplicationThread()) {
             new Thread(callback).start();
         } else {
@@ -340,22 +341,29 @@ public void onWorkThreadDo(Runnable callback) {
 
     @Override
     public void onUIThreadDo(Runnable callback) {
-        if (DEBUG && Platform.isFxApplicationThread())
-            throw new RuntimeException("Can be run only on work thread");
-
         if (Platform.isFxApplicationThread()) {
             callback.run();
         } else {
             Platform.runLater(callback);
         }
     }
 
+    public static void assertOnUIThread() {
+        if (DEBUG && !Platform.isFxApplicationThread())
+            throw new RuntimeException("Can be run only on UI thread");
+    }
+
+    public static void assertOnWorkThread() {
+        if (DEBUG && Platform.isFxApplicationThread())
+            throw new RuntimeException("Can be run only on work thread");
+    }
+
     public SigningKey getActiveSigningKey() {
         return activeKey;
     }
 
     public void setActiveSigningKey(SigningKey newKey) {
-        if (!isActiveSigningKeyChangeAllowed()){
+        if (!isActiveSigningKeyChangeAllowed()) {
             throw new RuntimeException("Signing key change is not allowed");
         }
         if (activeKey != null)
@@ -364,7 +372,7 @@ public void setActiveSigningKey(SigningKey newKey) {
         refreshKeyOnAllJobs();
     }
 
-    public boolean isActiveSigningKeyChangeAllowed(){
+    public boolean isActiveSigningKeyChangeAllowed() {
         return true;
     }
 

diff --git a/src/main/java/digital/slovensko/autogram/ui/gui/GUIApp.java b/src/main/java/digital/slovensko/autogram/ui/gui/GUIApp.java
@@ -19,7 +19,7 @@ public void start(Stage windowStage) throws Exception {
 
         setUserAgentStylesheet(getClass().getResource("idsk.css").toExternalForm());
 
-        var controller = new MainMenuController(autogram);
+        var controller = new MainMenuController(autogram, ui);
         var root = GUIUtils.loadFXML(controller, "main-menu.fxml");
 
         var scene = new Scene(root);

diff --git a/src/main/java/digital/slovensko/autogram/util/Logging.java b/src/main/java/digital/slovensko/autogram/util/Logging.java
@@ -1,6 +1,5 @@
 package digital.slovensko.autogram.util;
 
-import java.io.PrintStream;
 import java.text.SimpleDateFormat;
 import java.util.Date;
 

diff --git a/src/main/resources/simplelogger.properties b/src/main/resources/simplelogger.properties
@@ -1,3 +1,3 @@
 org.slf4j.simpleLogger.defaultLogLevel=INFO
 org.slf4j.simpleLogger.log.eu.europa.esig.dss=ERROR
-org.slf4j.simpleLogger.log.digital.slovensko.autogram.util=INFO
+org.slf4j.simpleLogger.log.digital.slovensko.autogram=INFO
diff --git a/src/test/java/digital/slovensko/autogram/AutogramTests.java b/src/test/java/digital/slovensko/autogram/AutogramTests.java
@@ -115,7 +115,7 @@ public void startSigning(SigningJob signingJob, Autogram autogram) {
         }
 
         @Override
-        public void startBatch(Batch batch, Autogram autogram, AutogramBatchStartCallback callback) {
+        public void startBatch(Batch batch, Autogram autogram, Consumer<SigningKey> callback) {
         }
 
         @Override
@@ -126,11 +126,6 @@ public void cancelBatch(Batch batch) {
         public void signBatch(SigningJob job, SigningKey key) {
         }
 
-        @Override
-        public void updateBatch() {
-            // TODO Auto-generated method stub
-        }
-
         @Override
         public void pickTokenDriverAndThen(List<TokenDriver> drivers, Consumer<TokenDriver> callback) {
             callback.accept(drivers.get(0));