From dc4dc93bfa63300c0753d9ad62b01edef296a793 Mon Sep 17 00:00:00 2001 From: pom Date: Mon, 31 Jul 2023 12:12:13 +0200 Subject: [PATCH] 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/autogram/core/Autogram.java | 33 +----------- .../core/AutogramBatchStartCallback.java | 42 +++++++++++++++- .../slovensko/autogram/core/SigningJob.java | 1 - .../ui/SaveFileFromBatchResponder.java | 7 +-- .../digital/slovensko/autogram/ui/UI.java | 4 +- .../slovensko/autogram/ui/cli/CliUI.java | 8 +-- .../ui/gui/BatchDialogController.java | 7 +-- .../slovensko/autogram/ui/gui/GUI.java | 50 +++++++++++-------- .../slovensko/autogram/ui/gui/GUIApp.java | 2 +- .../slovensko/autogram/util/Logging.java | 1 - src/main/resources/simplelogger.properties | 2 +- .../slovensko/autogram/AutogramTests.java | 7 +-- 12 files changed, 80 insertions(+), 84 deletions(-) diff --git a/src/main/java/digital/slovensko/autogram/core/Autogram.java b/src/main/java/digital/slovensko/autogram/core/Autogram.java index 1f378aa5..6c8478b7 100644 --- 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 index 6ee2f410..e2dba752 100644 --- 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 { -} +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 { + + 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 index 1b858d52..7e530c39 100644 --- 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 index f87b2ce0..256dcfcb 100644 --- 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 callbackSuccess; private final Consumer callbackError; private final TargetPath targetPath; - public SaveFileFromBatchResponder(File file, Autogram autogram, TargetPath targetPath, + public SaveFileFromBatchResponder(File file, TargetPath targetPath, Consumer callbackSuccess, Consumer 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 index cea5ca8c..d596f16b 100644 --- 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 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 index c9bbbf61..4d84e3f9 100644 --- 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 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 drivers, Consumer 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 index d168a9ab..c47c3a40 100644 --- 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 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 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 index 4dc6e72b..e87c4c57 100644 --- 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 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,9 +341,6 @@ 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 { @@ -350,12 +348,22 @@ public void onUIThreadDo(Runnable 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 index 2bcc3a21..21d443b0 100644 --- 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 index 638fab76..544e5ac2 100644 --- 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 index 8ab0f193..6e5d994e 100644 --- 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 \ No newline at end of file +org.slf4j.simpleLogger.log.digital.slovensko.autogram=INFO \ No newline at end of file diff --git a/src/test/java/digital/slovensko/autogram/AutogramTests.java b/src/test/java/digital/slovensko/autogram/AutogramTests.java index 3bed4b75..dc060f8c 100644 --- 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 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 drivers, Consumer callback) { callback.accept(drivers.get(0));