From 4178789ce3add3f8437151a5561365641f13eff3 Mon Sep 17 00:00:00 2001 From: Christoph Date: Sun, 29 Oct 2017 19:20:33 +0100 Subject: [PATCH] Refactorings (#3370) * Rework AutoSetFileLinks * Only add first found file * Improve magic AutoFileLinking in entry editor by using same logic as manually triggering it * Don't use paths.get * add changelog entry * replcaed some occurrences of vector with arrayList * fix checkstyle --- CHANGELOG.md | 4 +- .../jabref/gui/FindUnlinkedFilesDialog.java | 15 +- .../gui/actions/AutoLinkFilesAction.java | 2 +- .../gui/externalfiles/AutoSetLinks.java | 168 +++++++++--------- .../externalfiles/SynchronizeFileField.java | 2 +- .../LinkedFilesEditorViewModel.java | 22 ++- .../gui/groups/UndoableModifySubtree.java | 5 +- .../gui/importer/ImportInspectionDialog.java | 2 +- .../model/search/matchers/MatcherSet.java | 10 +- 9 files changed, 115 insertions(+), 115 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e43533d6ef..bf91c2e88ff 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/main/java/org/jabref/gui/FindUnlinkedFilesDialog.java b/src/main/java/org/jabref/gui/FindUnlinkedFilesDialog.java index 7a30dd412a9..fbf049a942d 100644 --- a/src/main/java/org/jabref/gui/FindUnlinkedFilesDialog.java +++ b/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 fileFilterList = creatorManager.getFileFilterList(); - Vector 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 iterator = EntryTypes .getAllValues(frame.getCurrentBasePanel().getBibDatabaseContext().getMode()).iterator(); - Vector list = new Vector<>(); + List 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); } diff --git a/src/main/java/org/jabref/gui/actions/AutoLinkFilesAction.java b/src/main/java/org/jabref/gui/actions/AutoLinkFilesAction.java index 3f946a096a2..db9931a8f00 100644 --- a/src/main/java/org/jabref/gui/actions/AutoLinkFilesAction.java +++ b/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 diff --git a/src/main/java/org/jabref/gui/externalfiles/AutoSetLinks.java b/src/main/java/org/jabref/gui/externalfiles/AutoSetLinks.java index 4e63fffcb00..eb218f7621d 100644 --- a/src/main/java/org/jabref/gui/externalfiles/AutoSetLinks.java +++ b/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 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 entries, BibDatabaseContext datab * @return the thread performing the automatically setting */ public static Runnable autoSetLinks(final List entries, final NamedCompound ce, - final Set changedEntries, final FileListTableModel singleTableModel, + final Set changedEntries, final BibDatabaseContext databaseContext, final ActionListener callback, final JDialog diag) { final Collection types = ExternalFileTypes.getInstance().getExternalFileTypeSelection(); if (diag != null) { @@ -95,97 +101,87 @@ public static Runnable autoSetLinks(final List entries, final NamedCom diag.setLocationRelativeTo(diag.getParent()); } - Runnable r = new Runnable() { - - @Override - public void run() { - // determine directories to search in - final List dirs = databaseContext.getFileDirectoriesAsPaths(Globals.prefs.getFileDirectoryPreferences()); - - // determine extensions - final List extensions = types.stream().map(ExternalFileType::getExtension).collect(Collectors.toList()); - - // Run the search operation: - FileFinder fileFinder = FileFinders.constructFromConfiguration(Globals.prefs.getAutoLinkPreferences()); - Map> result = fileFinder.findAssociatedFiles(entries, dirs, extensions); - - boolean foundAny = false; - // Iterate over the entries: - for (Entry> entryFilePair : result.entrySet()) { - FileListTableModel tableModel; - Optional oldVal = entryFilePair.getKey().getField(FieldName.FILE); - if (singleTableModel == null) { - tableModel = new FileListTableModel(); - oldVal.ifPresent(tableModel::setContent); - } else { - assert entries.size() == 1; - tableModel = singleTableModel; - } - List 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 dirs = databaseContext.getFileDirectoriesAsPaths(Globals.prefs.getFileDirectoryPreferences()); + + // determine extensions + final List extensions = types.stream().map(ExternalFileType::getExtension).collect(Collectors.toList()); + + // Run the search operation: + FileFinder fileFinder = FileFinders.constructFromConfiguration(Globals.prefs.getAutoLinkPreferences()); + Map> result = fileFinder.findAssociatedFiles(entries, dirs, extensions); + + boolean foundAny = false; + // Iterate over the entries: + for (Entry> entryFilePair : result.entrySet()) { + Optional 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 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 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); } diff --git a/src/main/java/org/jabref/gui/externalfiles/SynchronizeFileField.java b/src/main/java/org/jabref/gui/externalfiles/SynchronizeFileField.java index 2cbda14fcea..7f8a5011fa7 100644 --- a/src/main/java/org/jabref/gui/externalfiles/SynchronizeFileField.java +++ b/src/main/java/org/jabref/gui/externalfiles/SynchronizeFileField.java @@ -120,7 +120,7 @@ public void run() { List 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; diff --git a/src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditorViewModel.java b/src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditorViewModel.java index 4ce91d23bf7..5ac697e8c24 100644 --- a/src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditorViewModel.java +++ b/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 findAssociatedNotLinkedFiles(BibEntry entry) { List newFiles = fileFinder.findAssociatedFiles(entry, dirs, extensions); List 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; diff --git a/src/main/java/org/jabref/gui/groups/UndoableModifySubtree.java b/src/main/java/org/jabref/gui/groups/UndoableModifySubtree.java index 88da8d43396..f8d83ed5c5a 100644 --- a/src/main/java/org/jabref/gui/groups/UndoableModifySubtree.java +++ b/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 m_subtreeRootPath; /** This holds the new subtree (the root's modified children) to allow redo. */ - private final List m_modifiedSubtree = new Vector<>(); + private final List m_modifiedSubtree = new ArrayList<>(); private final String m_name; - /** * * @param subtree diff --git a/src/main/java/org/jabref/gui/importer/ImportInspectionDialog.java b/src/main/java/org/jabref/gui/importer/ImportInspectionDialog.java index f17cb19f4e2..cdf00d5d921 100644 --- a/src/main/java/org/jabref/gui/importer/ImportInspectionDialog.java +++ b/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(); diff --git a/src/main/java/org/jabref/model/search/matchers/MatcherSet.java b/src/main/java/org/jabref/model/search/matchers/MatcherSet.java index 934ea16b598..92e6e7974f3 100644 --- a/src/main/java/org/jabref/model/search/matchers/MatcherSet.java +++ b/src/main/java/org/jabref/model/search/matchers/MatcherSet.java @@ -1,33 +1,33 @@ package org.jabref.model.search.matchers; +import java.util.ArrayList; import java.util.List; import java.util.Objects; -import java.util.Vector; import org.jabref.model.search.SearchMatcher; public abstract class MatcherSet implements SearchMatcher { - protected final List matchers = new Vector<>(); + protected final List matchers = new ArrayList<>(); @Override public boolean equals(Object o) { if (this == o) { return true; } - if (o == null || getClass() != o.getClass()) { + if ((o == null) || (getClass() != o.getClass())) { return false; } MatcherSet that = (MatcherSet) o; - return matchers.equals(that.matchers); + return Objects.equals(matchers, that.matchers); } @Override public int hashCode() { - return matchers.hashCode(); + return Objects.hash(matchers); } public void addRule(SearchMatcher newRule) {