add dialog to Launcher #11800

leaf-soba · 2024-09-20T12:44:05Z

Closes Add a visible notice when user opens more than one JabRef instance #11783
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.")
since DialogService didn't initialize at that life cycle, I tried some way to get around it but all failed, such as:

NPE

dialogService.showInformationDialogAndWait(Localization.lang("Opening multiple JabRef instance without arguments"), Localization.lang("Another JabRef instance is already running. Please switch to that instance."));

java.lang.IllegalStateException: This operation is permitted on the event thread only; currentThread = main

JabRefDialogService dialogService = new JabRefDialogService(new BaseWindow());
dialogService.showInformationDialogAndWait(Localization.lang("Opening multiple JabRef instance without arguments"), Localization.lang("Another JabRef instance is already running. Please switch to that instance."));

Unexpected exception: java.lang.IllegalStateException: Toolkit not initialized

UiTaskExecutor.runInJavaFXThread(() -> {
    DialogService dialogService = Injector.instantiateModelOrService(DialogService.class);
    dialogService.showErrorDialogAndWait(Localization.lang("Another JabRef instance is already running. Please switch to that instance."));
}));

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.");
                        });
                    });

so I add JOptionPane to open a dialog, but I think it may not the best practice in this situation.

edit: I changed to JavaFX now:

edit:

removed swing based on comment
open a dialog in exist instance

edit: fix base on devcall

fix a little bug in shutdownThreadPools() when taskExecutor is null, it can't be shutdown.
open a dialog after initialize()

Mandatory checks

Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

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.")

subhramit · 2024-09-20T19:24:26Z

src/main/java/org/jabref/Launcher.java

+import javax.swing.JOptionPane;
+import javax.swing.SwingUtilities;


I don't think we use swing in JabRef (we migrated long back). Can you find JavaFX equivalents and use them?

Yes, but the javafx is not yet initialized only in JabrefGui.

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

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."); }); });

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

calixtus · 2024-09-21T05:55:53Z

Hi, thanks for your efforts.
There are several constraints to keep in mind:

Use of Swing is not allowed, except for undo-redo. And we are even thinking of moving away to are more modern framework working with the command design pattern.
The GUI is constructed in JabRefGui, the Launcher is just there to do the most basic app setup and to call the argument processor, then the GUI.
there is no proper di framework, only afterburner.fx . Afterburner does not understand a Singleton and cannot just inject without the programmer initializing/running the GUI first and creating the instance for the injection map.

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

koppor · 2024-09-21T14:04:24Z

Adding label devcall since this is more controversial...

calixtus · 2024-09-23T19:19:50Z

DevCall decision:
Hi leaf-soba, we discussed your approach, that looked certainly interesting, but decided against it, because it would be using magic strings to be send to the remote client. We thought about a different approach:
You could check in the Launcher for a second instance and if it exists, add an uiCommand to the list sent to JabRefGUI::setup.
This can be intercepted in JabRefGUI::start before running JabRefGUI::initialize a dialog be shown and then javafx torne down.

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!

leaf-soba · 2024-09-24T12:17:34Z

DevCall decision: Hi leaf-soba, we discussed your approach, that looked certainly interesting, but decided against it, because it would be using magic strings to be send to the remote client. We thought about a different approach: You could check in the Launcher for a second instance and if it exists, add an uiCommand to the list sent to JabRefGUI::setup. This can be intercepted in JabRefGUI::start before running JabRefGUI::initialize a dialog be shown and then javafx torne down.

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 ERROR: Unable to close AI service: java.lang.NullPointerException: Cannot invoke "org.jabref.logic.ai.AiService.close()" because "org.jabref.gui.JabRefGUI.aiService" is null
at

Platform.exit()

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?

calixtus · 2024-09-24T14:20:14Z

Can you push your changes please?
And @InAnYan can you take a look? Seems like the aiService is not shutting down properly at this stage...

InAnYan · 2024-09-24T14:40:06Z

@leaf-soba, if you have some time, could you post the full stacktrace here? I thought this aiService.close() isn't called...

In any ways, quick win: before calling aiService.close(), check if it's not null. Something like this:

if (aiService != null) {
	aiService.close();
}

@calixtus

I know I should not close the first instance, I'm testing. I push this code since @calixtus ask me to push it.

leaf-soba · 2024-09-25T01:15:48Z

Can you push your changes please? And @InAnYan can you take a look? Seems like the aiService is not shutting down properly at this stage...

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()`

koppor · 2024-09-26T08:52:47Z

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 ( openWindow())? -- Isn't it possible to just show the dialog? "dialog" interpreted in a more broader sence: A small window showing something... -- I would be fine in having another class extends Application showing only that. But @Siedlerchr was agagainst it?!

Siedlerchr · 2024-09-26T09:34:27Z

Extending Application here would probably cause more problems than it solves

calixtus · 2024-09-26T10:05:05Z

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.

calixtus · 2024-09-26T10:06:48Z

Only adding complexity that needs to be maintained and a very weird two-javafx-application-setup in one app.

leaf-soba · 2024-09-26T11:14:24Z

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 ( openWindow())? -- Isn't it possible to just show the dialog? "dialog" interpreted in a more broader sence: A small window showing something... -- I would be fine in having another class extends Application showing only that. But @Siedlerchr was agagainst it?!

If I can use swing like second solution it is simple, but we don't like swing.

calixtus · 2024-09-26T12:51:41Z

We decided against swing. Years ago.
In theory everything is simple. Yet it would overturn and ridicule decisions made and ourselves.
Also the Launcher is not part of GUI, but of headless CLI package. If we plan to move forward into multi module build, this would be a major step back to include ui stuff in the Launcher.

koppor · 2024-09-26T16:43:41Z

@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 args.lgenth == 0?

                // 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 {

leaf-soba · 2024-09-27T00:37:59Z

@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 args.lgenth == 0?
                // 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.

leaf-soba · 2024-09-27T00:42:12Z

I think I should keep this code in org.jabref.gui.JabRefGUI#shutdownThreadPools?

if (taskExecutor != null) {
    taskExecutor.shutdown();
}

1. only change debug log without dialog 2. add a null check before closing taskExecutor

koppor · 2024-09-27T06:36:39Z

I think I should keep this code in org.jabref.gui.JabRefGUI#shutdownThreadPools?
if (taskExecutor != null) {
    taskExecutor.shutdown();
}

I am unsure, but OK for me :)

koppor · 2024-09-27T06:37:23Z

src/main/java/org/jabref/Launcher.java

-                    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.");


Higher log level to ensure that is really output on the console.

Suggested change

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.");

koppor · 2024-09-27T06:37:38Z

src/main/java/org/jabref/Launcher.java

-                    // 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.


Can you move that inside the if? (meaning one line down and indent)

fix comment position

koppor · 2024-09-27T08:38:10Z

src/main/java/org/jabref/gui/JabRefGUI.java

@@ -93,7 +93,6 @@ public void start(Stage stage) {
                dialogService.showErrorDialogAndWait("Uncaught exception occurred in " + thread, exception);
            });
        });
-


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

Thank you for the follow-ups.

I found another thing which is strange ^^

src/main/java/org/jabref/Launcher.java

"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"

add dialog to Launcher

e8532c1

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.")

subhramit reviewed Sep 20, 2024

View reviewed changes

change it in JavaFX implement

9e2fab2

change it in JavaFX implement

koppor added the status: devcall label Sep 21, 2024

calixtus removed the status: devcall label Sep 23, 2024

push the test code don't merge

1b18953

I know I should not close the first instance, I'm testing. I push this code since @calixtus ask me to push it.

update based on DevCall decision

fc58453

- a little difference between DevCall decision, since before `initialize()` the UiTaskExecutor and dialogService is null, I arrange the dialog code after `initialize()`

leaf-soba requested review from Siedlerchr, calixtus and subhramit September 26, 2024 06:24

only change debug log without dialog

f852466

1. only change debug log without dialog 2. add a null check before closing taskExecutor

koppor reviewed Sep 27, 2024

View reviewed changes

fix a logger level to warn

1921411

fix comment position

koppor reviewed Sep 27, 2024

View reviewed changes

fix revert missing empty line

17d416f

fix revert missing empty line

leaf-soba requested a review from koppor September 29, 2024 01:05

koppor requested changes Sep 29, 2024

View reviewed changes

src/main/java/org/jabref/Launcher.java Outdated Show resolved Hide resolved

fix the comment

0319891

"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"

leaf-soba requested a review from koppor September 29, 2024 13:49

koppor approved these changes Sep 29, 2024

View reviewed changes

koppor added this pull request to the merge queue Sep 29, 2024

Merged via the queue into JabRef:main with commit e395853 Sep 29, 2024
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add dialog to Launcher #11800

add dialog to Launcher #11800

leaf-soba commented Sep 20, 2024 •

edited

Loading

subhramit Sep 20, 2024 •

edited

Loading

Siedlerchr Sep 20, 2024

Siedlerchr Sep 20, 2024

leaf-soba Sep 21, 2024 •

edited

Loading

calixtus Sep 21, 2024

calixtus commented Sep 21, 2024

koppor commented Sep 21, 2024

calixtus commented Sep 23, 2024 •

edited

Loading

leaf-soba commented Sep 24, 2024

calixtus commented Sep 24, 2024

InAnYan commented Sep 24, 2024

leaf-soba commented Sep 25, 2024

koppor commented Sep 26, 2024

Siedlerchr commented Sep 26, 2024

calixtus commented Sep 26, 2024

calixtus commented Sep 26, 2024

leaf-soba commented Sep 26, 2024

calixtus commented Sep 26, 2024

koppor commented Sep 26, 2024

leaf-soba commented Sep 27, 2024

leaf-soba commented Sep 27, 2024

koppor commented Sep 27, 2024

koppor Sep 27, 2024

koppor Sep 27, 2024

koppor Sep 27, 2024

koppor left a comment

		import javax.swing.JOptionPane;
		import javax.swing.SwingUtilities;

	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.");

add dialog to Launcher #11800

add dialog to Launcher #11800

Conversation

leaf-soba commented Sep 20, 2024 • edited Loading

Mandatory checks

subhramit Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Siedlerchr Sep 20, 2024

Choose a reason for hiding this comment

Siedlerchr Sep 20, 2024

Choose a reason for hiding this comment

leaf-soba Sep 21, 2024 • edited Loading

Choose a reason for hiding this comment

calixtus Sep 21, 2024

Choose a reason for hiding this comment

calixtus commented Sep 21, 2024

koppor commented Sep 21, 2024

calixtus commented Sep 23, 2024 • edited Loading

leaf-soba commented Sep 24, 2024

calixtus commented Sep 24, 2024

InAnYan commented Sep 24, 2024

leaf-soba commented Sep 25, 2024

koppor commented Sep 26, 2024

Siedlerchr commented Sep 26, 2024

calixtus commented Sep 26, 2024

calixtus commented Sep 26, 2024

leaf-soba commented Sep 26, 2024

calixtus commented Sep 26, 2024

koppor commented Sep 26, 2024

leaf-soba commented Sep 27, 2024

leaf-soba commented Sep 27, 2024

koppor commented Sep 27, 2024

koppor Sep 27, 2024

Choose a reason for hiding this comment

koppor Sep 27, 2024

Choose a reason for hiding this comment

koppor Sep 27, 2024

Choose a reason for hiding this comment

koppor left a comment

Choose a reason for hiding this comment

leaf-soba commented Sep 20, 2024 •

edited

Loading

subhramit Sep 20, 2024 •

edited

Loading

leaf-soba Sep 21, 2024 •

edited

Loading

calixtus commented Sep 23, 2024 •

edited

Loading