Refactorings

CHANGELOG.md

@@ -36,7 +36,9 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `#
  - We fixed an issue where JabRef would freeze when trying to replace the original entry after a merge with new information from identifiers like DOI/ISBN etc. [3294](https://github.com/JabRef/jabref/issues/3294)
  - We fixed an issue where JabRef would not show the translated content at some points, although there existed a translation
  - We fixed an issue where editing in the source tab would override content of other entries [#3352](https://github.com/JabRef/jabref/issues/3352#issue-268580818)
-### Removed
+ - We fixed several issues with the automatic linking of files in the entry editor where files were not found or not correctly saved in the bibtex source [#3346](https://github.com/JabRef/jabref/issues/3346)
+
+ ### Removed
 
 
 

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

@@ -31,7 +31,6 @@
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Optional;
-import java.util.Vector;
 import java.util.concurrent.atomic.AtomicBoolean;
 
 import javax.swing.AbstractAction;
@@ -973,11 +972,7 @@ private void createFileTypesCombobox() {
 
         List<FileFilter> fileFilterList = creatorManager.getFileFilterList();
 
-        Vector<FileFilter> vector = new Vector<>();
-        for (FileFilter fileFilter : fileFilterList) {
-            vector.add(fileFilter);
-        }
-        comboBoxFileTypeSelection = new JComboBox<>(vector);
+        comboBoxFileTypeSelection = new JComboBox<>(fileFilterList.toArray(new FileFilter[fileFilterList.size()]));
 
         comboBoxFileTypeSelection.setRenderer(new DefaultListCellRenderer() {
 
@@ -1009,16 +1004,15 @@ private void createEntryTypesCombobox() {
 
         Iterator<EntryType> iterator = EntryTypes
                 .getAllValues(frame.getCurrentBasePanel().getBibDatabaseContext().getMode()).iterator();
-        Vector<BibtexEntryTypeWrapper> list = new Vector<>();
+        List<BibtexEntryTypeWrapper> list = new ArrayList<>();
         list.add(
                 new BibtexEntryTypeWrapper(null));
         while (iterator.hasNext()) {
             list.add(new BibtexEntryTypeWrapper(iterator.next()));
         }
-        comboBoxEntryTypeSelection = new JComboBox<>(list);
+        comboBoxEntryTypeSelection = new JComboBox<>(list.toArray(new BibtexEntryTypeWrapper[list.size()]));
     }
 
-
     /**
      * Wrapper for displaying the Type {@link BibtexEntryType} in a Combobox.
      *
@@ -1030,7 +1024,6 @@ private static class BibtexEntryTypeWrapper {
 
         private final EntryType entryType;
 
-
         BibtexEntryTypeWrapper(EntryType bibtexType) {
             this.entryType = bibtexType;
         }
@@ -1053,7 +1046,6 @@ public static class CheckableTreeNode extends DefaultMutableTreeNode {
         private boolean isSelected;
         private final JCheckBox checkbox;
 
-
         public CheckableTreeNode(Object userObject) {
             super(userObject);
             checkbox = new JCheckBox();
@@ -1134,7 +1126,6 @@ public static class FileNodeWrapper {
         public final File file;
         public final int fileCount;
 
-
         public FileNodeWrapper(File aFile) {
             this(aFile, 0);
         }

src/main/java/org/jabref/gui/actions/AutoLinkFilesAction.java

@@ -40,7 +40,7 @@ public void actionPerformed(ActionEvent event) {
         }
         JDialog diag = new JDialog(JabRefGUI.getMainFrame(), true);
         final NamedCompound nc = new NamedCompound(Localization.lang("Automatically set file links"));
-        Runnable runnable = AutoSetLinks.autoSetLinks(entries, nc, null, null,
+        Runnable runnable = AutoSetLinks.autoSetLinks(entries, nc, null,
                 JabRefGUI.getMainFrame().getCurrentBasePanel().getBibDatabaseContext(), e -> {
                     if (e.getID() > 0) {
                         // entry has been updated in Util.autoSetLinks, only treat nc and status message

src/main/java/org/jabref/gui/externalfiles/AutoSetLinks.java

@@ -3,8 +3,9 @@
 import java.awt.BorderLayout;
 import java.awt.event.ActionEvent;
 import java.awt.event.ActionListener;
+import java.io.IOException;
+import java.nio.file.Files;
 import java.nio.file.Path;
-import java.nio.file.Paths;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
@@ -25,21 +26,26 @@
 import org.jabref.gui.externalfiletype.ExternalFileType;
 import org.jabref.gui.externalfiletype.ExternalFileTypes;
 import org.jabref.gui.externalfiletype.UnknownExternalFileType;
-import org.jabref.gui.filelist.FileListEntry;
-import org.jabref.gui.filelist.FileListTableModel;
 import org.jabref.gui.undo.NamedCompound;
 import org.jabref.gui.undo.UndoableFieldChange;
+import org.jabref.gui.util.DefaultTaskExecutor;
 import org.jabref.logic.l10n.Localization;
 import org.jabref.logic.util.io.FileFinder;
 import org.jabref.logic.util.io.FileFinders;
-import org.jabref.logic.util.io.FileUtil;
 import org.jabref.model.database.BibDatabaseContext;
 import org.jabref.model.entry.BibEntry;
 import org.jabref.model.entry.FieldName;
+import org.jabref.model.entry.FileFieldWriter;
+import org.jabref.model.entry.LinkedFile;
 import org.jabref.model.util.FileHelper;
 
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+
 public class AutoSetLinks {
 
+    private static final Log LOGGER = LogFactory.getLog(AutoSetLinks.class);
+
     private AutoSetLinks() {
     }
 
@@ -50,7 +56,7 @@ private AutoSetLinks() {
      * @param databaseContext the database for which links are set
      */
     public static void autoSetLinks(List<BibEntry> entries, BibDatabaseContext databaseContext) {
-        autoSetLinks(entries, null, null, null, databaseContext, null, null);
+        autoSetLinks(entries, null, null, databaseContext, null, null);
     }
 
     /**
@@ -79,7 +85,7 @@ public static void autoSetLinks(List<BibEntry> entries, BibDatabaseContext datab
      * @return the thread performing the automatically setting
      */
     public static Runnable autoSetLinks(final List<BibEntry> entries, final NamedCompound ce,
-            final Set<BibEntry> changedEntries, final FileListTableModel singleTableModel,
+            final Set<BibEntry> changedEntries,
             final BibDatabaseContext databaseContext, final ActionListener callback, final JDialog diag) {
         final Collection<ExternalFileType> types = ExternalFileTypes.getInstance().getExternalFileTypeSelection();
         if (diag != null) {
@@ -95,97 +101,87 @@ public static Runnable autoSetLinks(final List<BibEntry> entries, final NamedCom
             diag.setLocationRelativeTo(diag.getParent());
         }
 
-        Runnable r = new Runnable() {
-
-            @Override
-            public void run() {
-                // determine directories to search in
-                final List<Path> dirs = databaseContext.getFileDirectoriesAsPaths(Globals.prefs.getFileDirectoryPreferences());
-
-                // determine extensions
-                final List<String> extensions = types.stream().map(ExternalFileType::getExtension).collect(Collectors.toList());
-
-                // Run the search operation:
-                FileFinder fileFinder = FileFinders.constructFromConfiguration(Globals.prefs.getAutoLinkPreferences());
-                Map<BibEntry, List<Path>> result = fileFinder.findAssociatedFiles(entries, dirs, extensions);
-
-                boolean foundAny = false;
-                // Iterate over the entries:
-                for (Entry<BibEntry, List<Path>> entryFilePair : result.entrySet()) {
-                    FileListTableModel tableModel;
-                    Optional<String> oldVal = entryFilePair.getKey().getField(FieldName.FILE);
-                    if (singleTableModel == null) {
-                        tableModel = new FileListTableModel();
-                        oldVal.ifPresent(tableModel::setContent);
-                    } else {
-                        assert entries.size() == 1;
-                        tableModel = singleTableModel;
-                    }
-                    List<Path> files = entryFilePair.getValue();
-                    for (Path file : files) {
-                        file = FileUtil.shortenFileName(file, dirs);
-                        boolean alreadyHas = false;
-
-                        for (int j = 0; j < tableModel.getRowCount(); j++) {
-                            FileListEntry existingEntry = tableModel.getEntry(j);
-                            if (Paths.get(existingEntry.getLink()).equals(file)) {
-                                alreadyHas = true;
-                                foundAny = true;
-                                break;
-                            }
+        Runnable r = () -> {
+
+            // determine directories to search in
+            final List<Path> dirs = databaseContext.getFileDirectoriesAsPaths(Globals.prefs.getFileDirectoryPreferences());
+
+            // determine extensions
+            final List<String> extensions = types.stream().map(ExternalFileType::getExtension).collect(Collectors.toList());
+
+            // Run the search operation:
+            FileFinder fileFinder = FileFinders.constructFromConfiguration(Globals.prefs.getAutoLinkPreferences());
+            Map<BibEntry, List<Path>> result = fileFinder.findAssociatedFiles(entries, dirs, extensions);
+
+            boolean foundAny = false;
+            // Iterate over the entries:
+            for (Entry<BibEntry, List<Path>> entryFilePair : result.entrySet()) {
+                Optional<String> oldVal = entryFilePair.getKey().getField(FieldName.FILE);
+
+                for (Path foundFile : entryFilePair.getValue()) {
+                    boolean existingSameFile = entryFilePair.getKey().getFiles().stream()
+                            .map(file -> file.findIn(dirs))
+                            .anyMatch(file -> {
+                                try {
+                                    return file.isPresent() && Files.isSameFile(file.get(), foundFile);
+                                } catch (IOException e) {
+                                    // TODO Auto-generated catch block
+                                    e.printStackTrace();
+                                }
+                                return false;
+                            });
+                    if (!existingSameFile) {
+
+                        foundAny = true;
+                        Optional<ExternalFileType> type = FileHelper.getFileExtension(foundFile)
+                                .map(extension -> ExternalFileTypes.getInstance().getExternalFileTypeByExt(extension))
+                                .orElse(Optional.of(new UnknownExternalFileType("")));
+
+                        String strType = type.isPresent() ? type.get().getName() : "";
+                        LinkedFile linkedFile = new LinkedFile("", foundFile.toString(), strType);
+
+                        DefaultTaskExecutor.runInJavaFXThread(() -> {
+                            entryFilePair.getKey().addFile(linkedFile);
+                        });
+
+                        String newVal = FileFieldWriter.getStringRepresentation(linkedFile);
+                        if (ce != null) {
+                            // store undo information
+                            UndoableFieldChange change = new UndoableFieldChange(entryFilePair.getKey(),
+                                    FieldName.FILE, oldVal.orElse(null), newVal);
+                            ce.addEdit(change);
                         }
-                        if (!alreadyHas) {
-                            foundAny = true;
-                            Optional<ExternalFileType> type = FileHelper.getFileExtension(file)
-                                    .map(extension -> ExternalFileTypes.getInstance().getExternalFileTypeByExt(extension))
-                                    .orElse(Optional.of(new UnknownExternalFileType("")));
-                            FileListEntry flEntry = new FileListEntry("", file.toString(), type);
-                            tableModel.addEntry(tableModel.getRowCount(), flEntry);
-
-                            String newVal = tableModel.getStringRepresentation();
-                            if (newVal.isEmpty()) {
-                                newVal = null;
-                            }
-                            if (ce != null) {
-                                // store undo information
-                                UndoableFieldChange change = new UndoableFieldChange(entryFilePair.getKey(),
-                                        FieldName.FILE, oldVal.orElse(null), newVal);
-                                ce.addEdit(change);
-                            }
-                            // hack: if table model is given, do NOT modify entry
-                            if (singleTableModel == null) {
-                                entryFilePair.getKey().setField(FieldName.FILE, newVal);
-                            }
-                            if (changedEntries != null) {
-                                changedEntries.add(entryFilePair.getKey());
-                            }
+
+                        if (changedEntries != null) {
+                            changedEntries.add(entryFilePair.getKey());
                         }
+                        break;
                     }
-                }
 
-                // handle callbacks and dialog
-                // FIXME: The ID signals if action was successful :/
-                final int id = foundAny ? 1 : 0;
-                SwingUtilities.invokeLater(new Runnable() {
+                }
 
-                    @Override
-                    public void run() {
-                        if (diag != null) {
-                            diag.dispose();
-                        }
-                        if (callback != null) {
-                            callback.actionPerformed(new ActionEvent(this, id, ""));
-                        }
-                    }
-                });
             }
+            final int id = foundAny ? 1 : 0;
+            SwingUtilities.invokeLater(() -> {
+
+                if (diag != null) {
+                    diag.dispose();
+                }
+                if (callback != null) {
+                    callback.actionPerformed(new ActionEvent(AutoSetLinks.class, id, ""));
+                }
+
+            });
+
         };
+
         SwingUtilities.invokeLater(() -> {
             // show dialog which will be hidden when the task is done
             if (diag != null) {
                 diag.setVisible(true);
             }
         });
+
         return r;
     }
 
@@ -206,9 +202,9 @@ public void run() {
      *                         parameter can be null, which means that no progress update will be shown.
      * @return the runnable able to perform the automatically setting
      */
-    public static Runnable autoSetLinks(final BibEntry entry, final FileListTableModel singleTableModel,
+    public static Runnable autoSetLinks(final BibEntry entry,
             final BibDatabaseContext databaseContext, final ActionListener callback, final JDialog diag) {
-        return autoSetLinks(Collections.singletonList(entry), null, null, singleTableModel, databaseContext, callback,
+        return autoSetLinks(Collections.singletonList(entry), null, null, databaseContext, callback,
                 diag);
     }
 

src/main/java/org/jabref/gui/externalfiles/SynchronizeFileField.java

@@ -120,7 +120,7 @@ public void run() {
             List<BibEntry> entries = new ArrayList<>(sel);
 
             // Start the automatically setting process:
-            Runnable r = AutoSetLinks.autoSetLinks(entries, ce, changedEntries, null, panel.getBibDatabaseContext(), null, null);
+            Runnable r = AutoSetLinks.autoSetLinks(entries, ce, changedEntries, panel.getBibDatabaseContext(), null, null);
             JabRefExecutorService.INSTANCE.executeAndWait(r);
         }
         progress += sel.size() * weightAutoSet;

src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditorViewModel.java

@@ -3,6 +3,7 @@
 import java.io.IOException;
 import java.net.MalformedURLException;
 import java.net.URL;
+import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.ArrayList;
@@ -165,14 +166,25 @@ private List<LinkedFileViewModel> findAssociatedNotLinkedFiles(BibEntry entry) {
         List<Path> newFiles = fileFinder.findAssociatedFiles(entry, dirs, extensions);
 
         List<LinkedFileViewModel> result = new ArrayList<>();
-        for (Path newFile : newFiles) {
-            boolean alreadyLinked = files.get().stream()
+        for (Path foundFile : newFiles) {
+
+            boolean existingSameFile = files.get().stream()
                     .map(file -> file.findIn(dirs))
-                    .anyMatch(file -> file.isPresent() && file.get().equals(newFile));
-            if (!alreadyLinked) {
-                LinkedFileViewModel newLinkedFile = new LinkedFileViewModel(fromFile(newFile, dirs), entry, databaseContext);
+                    .anyMatch(file -> {
+                        try {
+                            return file.isPresent() && Files.isSameFile(file.get(), foundFile);
+                        } catch (IOException e) {
+                            // TODO Auto-generated catch block
+                            e.printStackTrace();
+                        }
+                        return false;
+                    });
+
+            if (!existingSameFile) {
+                LinkedFileViewModel newLinkedFile = new LinkedFileViewModel(fromFile(foundFile, dirs), entry, databaseContext);
                 newLinkedFile.markAsAutomaticallyFound();
                 result.add(newLinkedFile);
+                break; //only add first file, if it exists multiple times
             }
         }
         return result;

src/main/java/org/jabref/gui/groups/UndoableModifySubtree.java

@@ -1,7 +1,7 @@
 package org.jabref.gui.groups;
 
+import java.util.ArrayList;
 import java.util.List;
-import java.util.Vector;
 
 import org.jabref.gui.undo.AbstractUndoableJabRefEdit;
 import org.jabref.model.groups.GroupTreeNode;
@@ -14,10 +14,9 @@ public class UndoableModifySubtree extends AbstractUndoableJabRefEdit {
     /** The path to the global groups root node */
     private final List<Integer> m_subtreeRootPath;
     /** This holds the new subtree (the root's modified children) to allow redo. */
-    private final List<GroupTreeNode> m_modifiedSubtree = new Vector<>();
+    private final List<GroupTreeNode> m_modifiedSubtree = new ArrayList<>();
     private final String m_name;
 
-
     /**
      *
      * @param subtree

src/main/java/org/jabref/gui/importer/ImportInspectionDialog.java

@@ -1294,7 +1294,7 @@ public void actionPerformed(ActionEvent actionEvent) {
             // links:
             JDialog diag = new JDialog(ImportInspectionDialog.this, true);
             JabRefExecutorService.INSTANCE
-                    .execute(AutoSetLinks.autoSetLinks(entry, localModel, bibDatabaseContext, e -> {
+                    .execute(AutoSetLinks.autoSetLinks(entry, bibDatabaseContext, e -> {
                         if (e.getID() > 0) {
 
                             entries.getReadWriteLock().writeLock().lock();