add dialog to Launcher#11800
Conversation
when user open two instances of JabRef, second one will open dialog saying Localization.lang("Another JabRef instance is already running. Please switch to that instance.")
| import javax.swing.JOptionPane; | ||
| import javax.swing.SwingUtilities; |
There was a problem hiding this comment.
I don't think we use swing in JabRef (we migrated long back). Can you find JavaFX equivalents and use them?
There was a problem hiding this comment.
Yes, but the javafx is not yet initialized only in JabrefGui.
There was a problem hiding this comment.
A workaround would be to add a consumer lambda as a parameter to the method handleMultipleAppInstances and then pass that down to JabRefGUI where you can use it to show a dialog
I did a similar thing for the FallbackExceptionHandler
There was a problem hiding this comment.
A workaround would be to add a consumer lambda as a parameter to the method handleMultipleAppInstances and then pass that down to JabRefGUI where you can use it to show a dialog I did a similar thing for the FallbackExceptionHandler
I have tried your solution yesterday, sorry I forget to include it into origin post:
4. Nothing happened; it seems the JavaFXThread wasn't created before the instance was terminated
FallbackExceptionHandler.installExceptionHandler((exception, thread) -> {
UiTaskExecutor.runInJavaFXThread(() -> {
DialogService dialogService = Injector.instantiateModelOrService(DialogService.class);
dialogService.showErrorDialogAndWait("Another JabRef instance is already running. Please switch to that instance.");
});
});
There was a problem hiding this comment.
Creating the javafx platform is costly and takes time. So displaying a message would require to wait for the platform to be initialized.
change it in JavaFX implement
|
Hi, thanks for your efforts.
My solution strategy for this would be either to put a message in the ui message list handle it in the GUI and then tear the GUI down again -or - to display the message in the already running instance. |
1. Refactor the message when user starts JabRef a second time. now it is"Another JabRef instance is already running. Please switch to that instance." not "Passing arguments passed on to running JabRef..." 2. send a message from second instance to first instance to open a dialog in exist instance
|
Adding label devcall since this is more controversial... |
|
DevCall decision: We prepared a sketch diff: diff --git a/src/main/java/org/jabref/Launcher.java b/src/main/java/org/jabref/Launcher.java
index 1d3cb9cb3e..e5a04684b7 100644
--- a/src/main/java/org/jabref/Launcher.java
+++ b/src/main/java/org/jabref/Launcher.java
@@ -109,6 +109,7 @@ public class Launcher {
}
List<UiCommand> uiCommands = new ArrayList<>(argumentProcessor.getUiCommands());
+ uiCommands.add(new UiCommand.InstanceAlreadyRunning());
JabRefGUI.setup(uiCommands, preferences, fileUpdateMonitor);
JabRefGUI.launch(JabRefGUI.class, args);
} catch (ParseException e) {
diff --git a/src/main/java/org/jabref/gui/JabRefGUI.java b/src/main/java/org/jabref/gui/JabRefGUI.java
index d6a80bf394..1ab30890a1 100644
--- a/src/main/java/org/jabref/gui/JabRefGUI.java
+++ b/src/main/java/org/jabref/gui/JabRefGUI.java
@@ -94,6 +94,12 @@ public class JabRefGUI extends Application {
});
});
+ if (uiCommands.stream().anyMatch(UiCommand.InstanceAlreadyRunning.class::isInstance)) {
+ // displayDialog
+ Platform.exit();
+ return;
+ }
+
initialize();
JabRefGUI.mainFrame = new JabRefFrame(
diff --git a/src/main/java/org/jabref/logic/UiCommand.java b/src/main/java/org/jabref/logic/UiCommand.java
index 1fc8a699ce..019d09983f 100644
--- a/src/main/java/org/jabref/logic/UiCommand.java
+++ b/src/main/java/org/jabref/logic/UiCommand.java
@@ -12,4 +12,6 @@ public sealed interface UiCommand {
record OpenDatabases(List<ParserResult> parserResults) implements UiCommand { }
record AutoSetFileLinks(List<ParserResult> parserResults) implements UiCommand { }
+
+ record InstanceAlreadyRunning() implements UiCommand { }
}Thanks for your patience! |
I tried your solution, got an error said It seems this solution add more unnecessary difficulty to this simple issue.If we don't like "magic strings", I think change the string to a formal command string would be better? |
|
Can you push your changes please? |
|
@leaf-soba, if you have some time, could you post the full stacktrace here? I thought this In any ways, quick win: before calling if (aiService != null) {
aiService.close();
} |
I know I should not close the first instance, I'm testing. I push this code since @calixtus ask me to push it.
done, sry I thought I should not push a test code to a branch in this project, so I just described my error which might be confusing. |
- a little difference between DevCall decision, since before `initialize()` the UiTaskExecutor and dialogService is null, I arrange the dialog code after `initialize()`
|
I never thought it would be that much effort to show a simple JavaFX window... Sorry for that! Do we really need to fire up the main window ( |
|
Extending Application here would probably cause more problems than it solves |
|
Extending application (in a separate class) is just the same effort as running JabRefGui. Most computing time is used by creating the ui thread by javafx, so there would be absolutely no benefit. |
|
Only adding complexity that needs to be maintained and a very weird two-javafx-application-setup in one app. |
If I can use swing like second solution it is simple, but we don't like swing. |
|
We decided against swing. Years ago. |
|
@leaf-soba Thank you for the efforts invested here. It turned out that the wish causes many code changes. @calixtus, @Siedlerchr and other devleopers in the devcall on Monday invested much time in discussing this. I think, at least two hours in total. Seeing the ongoing discussions and introduced architectural WTFs, it is simply not worth the current and future maintainence effort. We will come back to the issue if some more users complain. Internally, we will check our processes to ensure that issues are marked as "good first issues" after they were properly reviewed. Nevertheless, can you modify this PR to NOT do any command passing, but keeping the Java code around the check of // There is already a server out there, avoid showing log "Passing arguments" while no arguments are provided.
if (args.length == 0) {
LOGGER.debug("This JabRef instance is already running. Please switch to that instance.");
} else { |
OK, actually I do love doing a lot of changes based on comments and enjoy this times in this issue, since I have learned a lot from it, thank you very much. |
|
I think I should keep this code in |
1. only change debug log without dialog 2. add a null check before closing taskExecutor
I am unsure, but OK for me :) |
| return false; | ||
| // There is already a server out there, avoid showing log "Passing arguments" while no arguments are provided. | ||
| if (args.length == 0) { | ||
| LOGGER.debug("This JabRef instance is already running. Please switch to that instance."); |
There was a problem hiding this comment.
Higher log level to ensure that is really output on the console.
| LOGGER.debug("This JabRef instance is already running. Please switch to that instance."); | |
| LOGGER.warn("This JabRef instance is already running. Please switch to that instance."); |
| // Output to both to the log and the screen. Therefore, we do not have an additional System.out.println. | ||
| LOGGER.info("Arguments passed on to running JabRef instance. Shutting down."); | ||
| return false; | ||
| // There is already a server out there, avoid showing log "Passing arguments" while no arguments are provided. |
There was a problem hiding this comment.
Can you move that inside the if? (meaning one line down and indent)
fix comment position
| dialogService.showErrorDialogAndWait("Uncaught exception occurred in " + thread, exception); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
I think, this is not consistent with the other style in this method, where „everything“ is separated with an empty line. Thus, can you revert this please? 😅
fix revert missing empty line
koppor
left a comment
There was a problem hiding this comment.
Thank you for the follow-ups.
I found another thing which is strange ^^
"We do not launch a new instance in presence of an error" to "We do not launch a new instance in presence if there is another instance running"
DialogServicedidn't initialize at that life cycle, I tried some way to get around it but all failed, such as:java.lang.IllegalStateException: This operation is permitted on the event thread only; currentThread = mainJOptionPaneto open a dialog, but I think it may not the best practice in this situation.edit: I changed to JavaFX now:
edit:
edit: fix base on devcall
shutdownThreadPools()whentaskExecutoris null, it can't be shutdown.initialize()Mandatory checks
CHANGELOG.mddescribed in a way that is understandable for the average user (if applicable)