Push to external application config (#10303)

* Introduce SelfContainedSaveConfiguration

* prepare adding cite key delimiter and start end chars

* Fix checkstyle

* Fix NPE

* add preferences and text fields

* fix sublime escaping

* fix emacs

* add

* fix checkstyle

* fix imports

* checkstyle

* checkstyle the xt

* Update JabRef_en.properties

* Introduced dissectCiteCommand

* Fixed compile

* fix cite key and add error logger

* fix npe

* fix

* fix index of

* Update JabRef_en.properties

fix l10n

* fix texstudio insert

* Shorten localization strings for emacs (and group the pushing localization)

* Rename "couldNotConnect" to "couldNotPush"

* Some debug

* New variable pushToApplicationPreferences

* Works on Windows

* Use "replace" instead of "replaceAll"

* Shorten messages for Vim

* Add logger to Vim execution, too

* Style: LOGGER first

* Modified test for linux

* Make emacs test "portable"

* Fix localization language

---------

Co-authored-by: Oliver Kopp <kopp.dev@gmail.com>
Co-authored-by: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com>

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

@@ -123,9 +123,9 @@ public JabRefFrame(Stage mainStage) {
         this.fileUpdateMonitor = Globals.getFileUpdateMonitor();
         this.entryTypesManager = Globals.entryTypesManager;
         this.globalSearchBar = new GlobalSearchBar(this, stateManager, prefs, undoManager, dialogService);
-        this.pushToApplicationCommand = new PushToApplicationCommand(stateManager, dialogService, prefs);
-        this.fileHistory = new FileHistoryMenu(prefs.getGuiPreferences().getFileHistory(), dialogService, getOpenDatabaseAction());
         this.taskExecutor = Globals.TASK_EXECUTOR;
+        this.pushToApplicationCommand = new PushToApplicationCommand(stateManager, dialogService, prefs, taskExecutor);
+        this.fileHistory = new FileHistoryMenu(prefs.getGuiPreferences().getFileHistory(), dialogService, getOpenDatabaseAction());
         this.setOnKeyTyped(key -> {
             if (this.fileHistory.isShowing()) {
                 if (this.fileHistory.openFileByKey(key)) {

src/main/java/org/jabref/gui/preferences/external/ExternalTab.fxml

@@ -55,7 +55,7 @@
 
             <Label text="%Cite command" GridPane.rowIndex="2"/>
             <TextField fx:id="citeCommand"
-                       prefWidth="200.0" GridPane.columnIndex="1" GridPane.rowIndex="2"/>
+                       prefWidth="300.0" GridPane.columnIndex="1" GridPane.rowIndex="2"/>
         </GridPane>
     </HBox>
 

src/main/java/org/jabref/gui/preferences/external/ExternalTab.java

@@ -23,6 +23,7 @@ public class ExternalTab extends AbstractPreferenceTabView<ExternalTabViewModel>
     @FXML private CheckBox autoOpenAttachedFolders;
     @FXML private ComboBox<PushToApplication> pushToApplicationCombo;
     @FXML private TextField citeCommand;
+
     @FXML private CheckBox useCustomTerminal;
     @FXML private TextField customTerminalCommand;
     @FXML private Button customTerminalBrowse;

src/main/java/org/jabref/gui/preferences/external/ExternalTabViewModel.java

@@ -210,8 +210,6 @@ public StringProperty citeCommandProperty() {
         return this.citeCommandProperty;
     }
 
-    // Open console
-
     public BooleanProperty useCustomTerminalProperty() {
         return this.useCustomTerminalProperty;
     }

src/main/java/org/jabref/gui/push/AbstractPushToApplication.java

@@ -26,21 +26,48 @@
 public abstract class AbstractPushToApplication implements PushToApplication {
 
     private static final Logger LOGGER = LoggerFactory.getLogger(AbstractPushToApplication.class);
+    private static final String CITE_KEY1 = "key1";
+    private static final String CITE_KEY2 = "key2";
 
     protected boolean couldNotCall; // Set to true in case the command could not be executed, e.g., if the file is not found
-    protected boolean couldNotConnect; // Set to true in case the tunnel to the program (if one is used) does not operate
+    protected boolean couldNotPush; // Set to true in case the tunnel to the program (if one is used) does not operate
     protected boolean notDefined; // Set to true if the corresponding path is not defined in the preferences
 
     protected String commandPath;
 
     protected final DialogService dialogService;
     protected final PreferencesService preferencesService;
 
+    private String cachedCiteCommand;
+    private String cachedCitePrefix;
+    private String cachedCiteSuffix;
+    private String cachedCiteDelimiter;
+
     public AbstractPushToApplication(DialogService dialogService, PreferencesService preferencesService) {
         this.dialogService = dialogService;
         this.preferencesService = preferencesService;
     }
 
+    private void dissectCiteCommand() {
+        String preferencesCiteCommand = preferencesService.getExternalApplicationsPreferences().getCiteCommand();
+
+        if (preferencesCiteCommand != null && preferencesCiteCommand.equals(cachedCiteCommand)) {
+            return;
+        }
+
+        cachedCiteCommand = preferencesCiteCommand;
+
+        int indexKey1 = cachedCiteCommand.indexOf(CITE_KEY1);
+        int indexKey2 = cachedCiteCommand.indexOf(CITE_KEY2);
+        if (indexKey1 < 0 || indexKey2 < 0 || indexKey2 < (indexKey1 + CITE_KEY1.length())) {
+            return;
+        }
+
+        cachedCitePrefix = preferencesCiteCommand.substring(0, indexKey1);
+        cachedCiteDelimiter = preferencesCiteCommand.substring(preferencesCiteCommand.lastIndexOf(CITE_KEY1) + CITE_KEY1.length(), indexKey2);
+        cachedCiteSuffix = preferencesCiteCommand.substring(preferencesCiteCommand.lastIndexOf(CITE_KEY2) + CITE_KEY2.length());
+    }
+
     @Override
     public JabRefIcon getApplicationIcon() {
         return IconTheme.JabRefIcons.APPLICATION_GENERIC;
@@ -58,7 +85,7 @@ public Action getAction() {
 
     @Override
     public void pushEntries(BibDatabaseContext database, List<BibEntry> entries, String keyString) {
-        couldNotConnect = false;
+        couldNotPush = false;
         couldNotCall = false;
         notDefined = false;
 
@@ -108,7 +135,7 @@ public void onOperationCompleted() {
             dialogService.showErrorDialogAndWait(
                     Localization.lang("Error pushing entries"),
                     Localization.lang("Could not call executable") + " '" + commandPath + "'.");
-        } else if (couldNotConnect) {
+        } else if (couldNotPush) {
             dialogService.showErrorDialogAndWait(
                     Localization.lang("Error pushing entries"),
                     Localization.lang("Could not connect to %0", getDisplayName()) + ".");
@@ -142,8 +169,19 @@ protected String getCommandName() {
         return null;
     }
 
-    protected String getCiteCommand() {
-        return preferencesService.getExternalApplicationsPreferences().getCiteCommand();
+    protected String getCitePrefix() {
+        dissectCiteCommand();
+        return cachedCitePrefix;
+    }
+
+    public String getDelimiter() {
+        dissectCiteCommand();
+        return cachedCiteDelimiter;
+    }
+
+    protected String getCiteSuffix() {
+        dissectCiteCommand();
+        return cachedCiteSuffix;
     }
 
     @Override

src/main/java/org/jabref/gui/push/PushToApplication.java

@@ -41,4 +41,6 @@ public interface PushToApplication {
     Action getAction();
 
     PushToApplicationSettings getSettings(PushToApplication application, PushToApplicationPreferences pushToApplicationPreferences);
+
+    String getDelimiter();
 }

src/main/java/org/jabref/gui/push/PushToApplicationCommand.java

@@ -9,13 +9,13 @@
 import javafx.scene.control.MenuItem;
 
 import org.jabref.gui.DialogService;
-import org.jabref.gui.Globals;
 import org.jabref.gui.StateManager;
 import org.jabref.gui.actions.Action;
 import org.jabref.gui.actions.ActionFactory;
 import org.jabref.gui.actions.SimpleCommand;
 import org.jabref.gui.util.BackgroundTask;
 import org.jabref.gui.util.BindingsHelper;
+import org.jabref.gui.util.TaskExecutor;
 import org.jabref.logic.l10n.Localization;
 import org.jabref.model.database.BibDatabaseContext;
 import org.jabref.model.entry.BibEntry;
@@ -41,13 +41,15 @@ public class PushToApplicationCommand extends SimpleCommand {
     private final PreferencesService preferencesService;
 
     private final List<Object> reconfigurableControls = new ArrayList<>();
+    private final TaskExecutor taskExecutor;
 
     private PushToApplication application;
 
-    public PushToApplicationCommand(StateManager stateManager, DialogService dialogService, PreferencesService preferencesService) {
+    public PushToApplicationCommand(StateManager stateManager, DialogService dialogService, PreferencesService preferencesService, TaskExecutor taskExecutor) {
         this.stateManager = stateManager;
         this.dialogService = dialogService;
         this.preferencesService = preferencesService;
+        this.taskExecutor = taskExecutor;
 
         setApplication(preferencesService.getPushToApplicationPreferences()
                                                             .getActiveApplicationName());
@@ -95,7 +97,7 @@ public Action getAction() {
         return application.getAction();
     }
 
-    private static String getKeyString(List<BibEntry> entries) {
+    private static String getKeyString(List<BibEntry> entries, String delimiter) {
         StringBuilder result = new StringBuilder();
         Optional<String> citeKey;
         boolean first = true;
@@ -109,7 +111,7 @@ private static String getKeyString(List<BibEntry> entries) {
                 result.append(citeKey.get());
                 first = false;
             } else {
-                result.append(',').append(citeKey.get());
+                result.append(delimiter).append(citeKey.get());
             }
         }
         return result.toString();
@@ -132,11 +134,12 @@ public void execute() {
         // All set, call the operation in a new thread:
         BackgroundTask.wrap(this::pushEntries)
                       .onSuccess(s -> application.onOperationCompleted())
-                      .executeWith(Globals.TASK_EXECUTOR);
+                      .onFailure(ex -> LOGGER.error("Error pushing citation", ex))
+                      .executeWith(taskExecutor);
     }
 
     private void pushEntries() {
         BibDatabaseContext database = stateManager.getActiveDatabase().orElseThrow(() -> new NullPointerException("Database null"));
-        application.pushEntries(database, stateManager.getSelectedEntries(), getKeyString(stateManager.getSelectedEntries()));
+        application.pushEntries(database, stateManager.getSelectedEntries(), getKeyString(stateManager.getSelectedEntries(), application.getDelimiter()));
     }
 }

src/main/java/org/jabref/gui/push/PushToEmacs.java

@@ -2,6 +2,7 @@
 
 import java.io.IOException;
 import java.io.InputStream;
+import java.util.Arrays;
 import java.util.List;
 
 import org.jabref.gui.DialogService;
@@ -24,6 +25,9 @@ public class PushToEmacs extends AbstractPushToApplication {
 
     private static final Logger LOGGER = LoggerFactory.getLogger(PushToEmacs.class);
 
+    /**
+     * @param preferencesService getPushToApplicationPreferences(), getExternalApplicationsPreferences(), and getFilePreferences() are used
+     */
     public PushToEmacs(DialogService dialogService, PreferencesService preferencesService) {
         super(dialogService, preferencesService);
     }
@@ -40,43 +44,60 @@ public JabRefIcon getApplicationIcon() {
 
     @Override
     public void pushEntries(BibDatabaseContext database, List<BibEntry> entries, String keys) {
-        couldNotConnect = false;
+        couldNotPush = false;
         couldNotCall = false;
         notDefined = false;
 
-        commandPath = preferencesService.getPushToApplicationPreferences().getCommandPaths().get(this.getDisplayName());
+        PushToApplicationPreferences pushToApplicationPreferences = preferencesService.getPushToApplicationPreferences();
+
+        commandPath = pushToApplicationPreferences.getCommandPaths().get(this.getDisplayName());
 
         if ((commandPath == null) || commandPath.trim().isEmpty()) {
             notDefined = true;
             return;
         }
 
-        commandPath = preferencesService.getPushToApplicationPreferences().getCommandPaths().get(this.getDisplayName());
+        commandPath = pushToApplicationPreferences.getCommandPaths().get(this.getDisplayName());
 
-        String[] addParams = preferencesService.getPushToApplicationPreferences().getEmacsArguments().split(" ");
+        String[] addParams = pushToApplicationPreferences.getEmacsArguments().split(" ");
         try {
             String[] com = new String[addParams.length + 2];
             com[0] = commandPath;
             System.arraycopy(addParams, 0, com, 1, addParams.length);
-            String prefix;
-            String suffix;
-            prefix = "(with-current-buffer (window-buffer) (insert ";
-            suffix = "))";
+
+            // Surrounding with is handled below
+            String prefix = "(with-current-buffer (window-buffer (selected-window)) (insert ";
+            String suffix = "))";
 
             if (OS.WINDOWS) {
                 // Windows gnuclient/emacsclient escaping:
                 // java string: "(insert \\\"\\\\cite{Blah2001}\\\")";
                 // so cmd receives: (insert \"\\cite{Blah2001}\")
                 // so emacs receives: (insert "\cite{Blah2001}")
-                com[com.length - 1] = prefix.concat("\\\"\\" + getCiteCommand().replaceAll("\\\\", "\\\\\\\\") + "{" + keys + "}\\\"").concat(suffix);
+
+                com[com.length - 1] = prefix.concat("\""
+                                                    + getCitePrefix().replace("\\", "\\\\")
+                                                    + keys
+                                                    + getCiteSuffix().replace("\\", "\\\\")
+                                                    + "\"").concat(suffix)
+                                            .replace("\"", "\\\"");
             } else {
                 // Linux gnuclient/emacslient escaping:
                 // java string: "(insert \"\\\\cite{Blah2001}\")"
                 // so sh receives: (insert "\\cite{Blah2001}")
                 // so emacs receives: (insert "\cite{Blah2001}")
-                com[com.length - 1] = prefix.concat("\"" + getCiteCommand().replaceAll("\\\\", "\\\\\\\\") + "{" + keys + "}\"").concat(suffix);
+                com[com.length - 1] = prefix.concat("\""
+                                                    + getCitePrefix().replace("\\", "\\\\")
+                                                    + keys
+                                                    + getCiteSuffix().replace("\\", "\\\\")
+                                                    + "\"").concat(suffix);
             }
 
+            LOGGER.atDebug()
+                  .setMessage("Executing command {}")
+                  .addArgument(() -> Arrays.toString(com))
+                  .log();
+
             final Process p = Runtime.getRuntime().exec(com);
 
             JabRefExecutorService.INSTANCE.executeAndWait(() -> {
@@ -92,30 +113,27 @@ public void pushEntries(BibDatabaseContext database, List<BibEntry> entries, Str
                     }
                     // Error stream has been closed. See if there were any errors:
                     if (!sb.toString().trim().isEmpty()) {
-                        LOGGER.warn("Push to Emacs error: " + sb);
-                        couldNotConnect = true;
+                        LOGGER.warn("Push to Emacs error: {}", sb);
+                        couldNotPush = true;
                     }
                 } catch (IOException e) {
-                    LOGGER.warn("File problem.", e);
+                    LOGGER.warn("Error handling std streams", e);
                 }
             });
         } catch (IOException excep) {
-            couldNotCall = true;
             LOGGER.warn("Problem pushing to Emacs.", excep);
+            couldNotCall = true;
         }
     }
 
     @Override
     public void onOperationCompleted() {
-        if (couldNotConnect) {
+        if (couldNotPush) {
             dialogService.showErrorDialogAndWait(Localization.lang("Error pushing entries"),
-                    Localization.lang("Could not connect to a running gnuserv process. Make sure that "
-                            + "Emacs or XEmacs is running, and that the server has been started "
-                            + "(by running the command 'server-start'/'gnuserv-start')."));
+                    Localization.lang("Could not push to a running emacs daemon."));
         } else if (couldNotCall) {
             dialogService.showErrorDialogAndWait(Localization.lang("Error pushing entries"),
-                    Localization.lang("Could not run the gnuclient/emacsclient program. Make sure you have "
-                            + "the emacsclient/gnuclient program installed and available in the PATH."));
+                    Localization.lang("Could not run the emacs client."));
         } else {
             super.onOperationCompleted();
         }

src/main/java/org/jabref/gui/push/PushToLyx.java

@@ -42,7 +42,7 @@ public JabRefIcon getApplicationIcon() {
 
     @Override
     public void onOperationCompleted() {
-        if (couldNotConnect) {
+        if (couldNotPush) {
             dialogService.showErrorDialogAndWait(Localization.lang("Error pushing entries"),
                     Localization.lang("Verify that LyX is running and that the lyxpipe is valid.")
                             + "[" + commandPath + "]");
@@ -60,7 +60,7 @@ public PushToApplicationSettings getSettings(PushToApplication application, Push
 
     @Override
     public void pushEntries(BibDatabaseContext database, final List<BibEntry> entries, final String keyString) {
-        couldNotConnect = false;
+        couldNotPush = false;
         couldNotCall = false;
         notDefined = false;
 
@@ -79,7 +79,7 @@ public void pushEntries(BibDatabaseContext database, final List<BibEntry> entrie
             // See if it helps to append ".in":
             lp = new File(commandPath + ".in");
             if (!lp.exists() || !lp.canWrite()) {
-                couldNotConnect = true;
+                couldNotPush = true;
                 return;
             }
         }

src/main/java/org/jabref/gui/push/PushToSublimeText.java

@@ -41,7 +41,7 @@ public JabRefIcon getApplicationIcon() {
 
     @Override
     public void pushEntries(BibDatabaseContext database, List<BibEntry> entries, String keyString) {
-        couldNotConnect = false;
+        couldNotPush = false;
         couldNotCall = false;
         notDefined = false;
 
@@ -74,11 +74,17 @@ public void pushEntries(BibDatabaseContext database, List<BibEntry> entries, Str
 
     @Override
     protected String[] getCommandLine(String keyString) {
+        String citeCommand = getCitePrefix();
+        // we need to escape the extra slashses
+        if (getCitePrefix().contains("\\")) {
+            citeCommand = "\"\\" + getCitePrefix();
+        }
+
         if (OS.WINDOWS) {
             // TODO we might need to escape the inner double quotes with """ """
-            return new String[] {"cmd.exe", "/c", "\"" + commandPath + "\"" + "--command \"insert {\\\"characters\\\": \"\\" + getCiteCommand() + "{" + keyString + "}\"}\""};
+            return new String[] {"cmd.exe", "/c", "\"" + commandPath + "\"" + "--command \"insert {\\\"characters\\\": \"\\" + getCitePrefix() + keyString + getCiteSuffix() + "\"}\""};
         } else {
-            return new String[] {"sh", "-c", "\"" + commandPath + "\"" + " --command 'insert {\"characters\": \"\\" + getCiteCommand() + "{" + keyString + "}\"}'"};
+            return new String[] {"sh", "-c", "\"" + commandPath + "\"" + " --command 'insert {\"characters\": \"" + citeCommand + keyString + getCiteSuffix() + "\"}'"};
         }
     }
 }

src/main/java/org/jabref/gui/push/PushToTeXstudio.java

@@ -25,6 +25,6 @@ public JabRefIcon getApplicationIcon() {
 
     @Override
     protected String[] getCommandLine(String keyString) {
-        return new String[] {commandPath, "--insert-cite", String.format("%s{%s}", getCiteCommand(), keyString)};
+        return new String[] {commandPath, "--insert-cite", String.format("%s%s%s", getCitePrefix(), keyString, getCiteSuffix())};
     }
 }

src/main/java/org/jabref/gui/push/PushToTexmaker.java

@@ -28,6 +28,6 @@ public JabRefIcon getApplicationIcon() {
 
     @Override
     protected String[] getCommandLine(String keyString) {
-        return new String[] {commandPath, "-insert", getCiteCommand() + "{" + keyString + "}"};
+        return new String[] {commandPath, "-insert", getCitePrefix() + keyString + getCiteSuffix()};
     }
 }