Use Set instead of List for custom journal abbreviations

CHANGELOG.md

@@ -21,8 +21,8 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
 
 - We changed database structure: in MySQL/MariaDB we renamed tables by adding a `JABREF_` prefix, and in PGSQL we moved tables in `jabref` schema. We added `VersionDBStructure` variable in `METADATA` table to indicate current version of structure, this variable is needed for automatic migration. [#9312](https://github.com/JabRef/jabref/issues/9312)
 - We moved some preferences options to a new tab in the preferences dialog. [#9442](https://github.com/JabRef/jabref/pull/9308)
-- We renamed "Medline abbrevation" to "dotless abbreviation". [#9504](https://github.com/JabRef/jabref/pull/9504)
-- We now have more "dots" in the offered journal abbrevations. [#9504](https://github.com/JabRef/jabref/pull/9504)
+- We renamed "Medline abbreviation" to "dotless abbreviation". [#9504](https://github.com/JabRef/jabref/pull/9504)
+- We now have more "dots" in the offered journal abbreviations. [#9504](https://github.com/JabRef/jabref/pull/9504)
 
 
 
@@ -32,7 +32,8 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
 
 - The tab "deprecated fields" is shown in biblatex-mode only. [#7757](https://github.com/JabRef/jabref/issues/7757)
 - In case a journal name of an IEEE journal is abbreviated, the "normal" abbreviation is used - and not the one of the IEEE BibTeX strings. [abbrv#91](https://github.com/JabRef/abbrv.jabref.org/issues/91)
-- We fixed an issue where the last opened libraries were not remembered when a new unsaved libray was open as well. [#9190](https://github.com/JabRef/jabref/issues/9190)
+- We fixed a performance issue when loading large lists of custom journal abbreviations. [#8928](https://github.com/JabRef/jabref/issues/8928)
+- We fixed an issue where the last opened libraries were not remembered when a new unsaved library was open as well. [#9190](https://github.com/JabRef/jabref/issues/9190)
 - We fixed an issue where no context menu for the group "All entries" was present. [forum#3682](https://discourse.jabref.org/t/how-sort-groups-a-z-not-subgroups/3682)
 - We fixed an issue where extra curly braces in some fields would trigger an exception when selecting the entry or doing an integrity check [#9475](https://github.com/JabRef/jabref/issues/9475), [#9503](https://github.com/JabRef/jabref/issues/9503)
 - We fixed an issue where entering a date in the format "YYYY/MM" in the entry editor date field caused an exception. [#9492](https://github.com/JabRef/jabref/issues/9492)

src/main/java/org/jabref/gui/preferences/journals/AbbreviationViewModel.java

@@ -19,14 +19,24 @@ public class AbbreviationViewModel {
     private final StringProperty name = new SimpleStringProperty("");
     private final StringProperty abbreviation = new SimpleStringProperty("");
     private final StringProperty shortestUniqueAbbreviation = new SimpleStringProperty("");
+
+    // Used when a "null" abbreviation object is added
     private final BooleanProperty pseudoAbbreviation = new SimpleBooleanProperty();
 
     public AbbreviationViewModel(Abbreviation abbreviationObject) {
         this.pseudoAbbreviation.set(abbreviationObject == null);
         if (abbreviationObject != null) {
             this.name.setValue(abbreviationObject.getName());
             this.abbreviation.setValue(abbreviationObject.getAbbreviation());
-            this.shortestUniqueAbbreviation.setValue(abbreviationObject.getShortestUniqueAbbreviation());
+
+            // the view model stores the "real" values, not the default fallback
+            String shortestUniqueAbbreviationOfAbbreviation = abbreviationObject.getShortestUniqueAbbreviation();
+            boolean shortestUniqueAbbreviationIsDefaultValue = shortestUniqueAbbreviationOfAbbreviation.equals(abbreviationObject.getAbbreviation());
+            if (shortestUniqueAbbreviationIsDefaultValue) {
+                this.shortestUniqueAbbreviation.set("");
+            } else {
+                this.shortestUniqueAbbreviation.setValue(shortestUniqueAbbreviationOfAbbreviation);
+            }
         }
     }
 
@@ -87,13 +97,15 @@ public boolean equals(Object o) {
             return false;
         }
         AbbreviationViewModel that = (AbbreviationViewModel) o;
-        return Objects.equals(getName(), that.getName()) &&
-                isPseudoAbbreviation() == that.isPseudoAbbreviation();
+        return getName().equals(that.getName())
+                && getAbbreviation().equals(that.getAbbreviation())
+                && getShortestUniqueAbbreviation().equals(that.getShortestUniqueAbbreviation())
+                && (isPseudoAbbreviation() == that.isPseudoAbbreviation());
     }
 
     @Override
     public int hashCode() {
-        return Objects.hash(getName(), isPseudoAbbreviation());
+        return Objects.hash(getName(), getAbbreviation(), getShortestUniqueAbbreviation(), isPseudoAbbreviation());
     }
 
     public boolean containsCaseIndependent(String searchTerm) {

src/main/java/org/jabref/gui/preferences/journals/AbbreviationsFileViewModel.java

@@ -2,9 +2,9 @@
 
 import java.io.FileNotFoundException;
 import java.io.IOException;
-import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.util.Collection;
 import java.util.List;
 import java.util.Objects;
 import java.util.Optional;
@@ -39,7 +39,7 @@ public AbbreviationsFileViewModel(Path filePath) {
 
     /**
      * This constructor should only be called to create a pseudo abbreviation file for built in lists. This means it is
-     * a placeholder and it's path will be null meaning it has no place on the filesystem. It's isPseudoFile property
+     * a placeholder and its path will be null meaning it has no place on the filesystem. Its isPseudoFile property
      * will therefore be set to true.
      */
     public AbbreviationsFileViewModel(List<AbbreviationViewModel> abbreviations, String name) {
@@ -51,7 +51,7 @@ public AbbreviationsFileViewModel(List<AbbreviationViewModel> abbreviations, Str
 
     public void readAbbreviations() throws IOException {
         if (path.isPresent()) {
-            List<Abbreviation> abbreviationList = JournalAbbreviationLoader.readJournalListFromFile(path.get());
+            Collection<Abbreviation> abbreviationList = JournalAbbreviationLoader.readJournalListFromFile(path.get());
             abbreviationList.forEach(abbreviation -> abbreviations.addAll(new AbbreviationViewModel(abbreviation)));
         } else {
             throw new FileNotFoundException();
@@ -60,15 +60,16 @@ public void readAbbreviations() throws IOException {
 
     /**
      * This method will write all abbreviations of this abbreviation file to the file on the file system.
-     * It essentially will check if the current file is a built in list and if not it will call
+     * It essentially will check if the current file is a builtin list and if not it will call
      * {@link AbbreviationWriter#writeOrCreate}.
      */
     public void writeOrCreate() throws IOException {
         if (!isBuiltInList.get()) {
             List<Abbreviation> actualAbbreviations =
                     abbreviations.stream().filter(abb -> !abb.isPseudoAbbreviation())
-                                 .map(AbbreviationViewModel::getAbbreviationObject).collect(Collectors.toList());
-            AbbreviationWriter.writeOrCreate(path.get(), actualAbbreviations, StandardCharsets.UTF_8);
+                                 .map(AbbreviationViewModel::getAbbreviationObject)
+                                 .collect(Collectors.toList());
+            AbbreviationWriter.writeOrCreate(path.get(), actualAbbreviations);
         }
     }
 

src/main/java/org/jabref/gui/preferences/journals/JournalAbbreviationsTabViewModel.java

@@ -185,7 +185,7 @@ private void openFile(Path filePath) {
             try {
                 abbreviationsFile.readAbbreviations();
             } catch (IOException e) {
-                LOGGER.debug(e.getLocalizedMessage());
+                LOGGER.debug("Could not read abbreviations file", e);
             }
         }
         journalFiles.add(abbreviationsFile);
@@ -217,83 +217,62 @@ public void removeCurrentFile() {
     /**
      * Method to add a new abbreviation to the abbreviations list property. It also sets the currentAbbreviation
      * property to the new abbreviation.
-     *
-     * @param name                       name of the abbreviation
-     * @param abbreviation               default abbreviation of the abbreviation
-     * @param shortestUniqueAbbreviation shortest unique abbreviation of the abbreviation
      */
-    public void addAbbreviation(String name, String abbreviation, String shortestUniqueAbbreviation) {
-        Abbreviation abbreviationObject = new Abbreviation(name, abbreviation, shortestUniqueAbbreviation);
+    public void addAbbreviation(Abbreviation abbreviationObject) {
         AbbreviationViewModel abbreviationViewModel = new AbbreviationViewModel(abbreviationObject);
         if (abbreviations.contains(abbreviationViewModel)) {
             dialogService.showErrorDialogAndWait(Localization.lang("Duplicated Journal Abbreviation"),
-                    Localization.lang("Abbreviation '%0' for journal '%1' already defined.", abbreviation, name));
+                    Localization.lang("Abbreviation '%0' for journal '%1' already defined.", abbreviationObject.getAbbreviation(), abbreviationObject.getName()));
         } else {
             abbreviations.add(abbreviationViewModel);
             currentAbbreviation.set(abbreviationViewModel);
             shouldWriteLists = true;
         }
     }
 
-    public void addAbbreviation(String name, String abbreviation) {
-        addAbbreviation(name, abbreviation, "");
-    }
-
     public void addAbbreviation() {
-        addAbbreviation(
+        addAbbreviation(new Abbreviation(
                 Localization.lang("Name"),
                 Localization.lang("Abbreviation"),
-                Localization.lang("Shortest unique abbreviation"));
+                Localization.lang("Shortest unique abbreviation")));
     }
 
     /**
      * Method to change the currentAbbreviation property to a new abbreviation.
-     *
-     * @param name                       name of the abbreviation
-     * @param abbreviation               default abbreviation of the abbreviation
-     * @param shortestUniqueAbbreviation shortest unique abbreviation of the abbreviation
      */
-    public void editAbbreviation(String name, String abbreviation, String shortestUniqueAbbreviation) {
+    void editAbbreviation(Abbreviation abbreviationObject) {
         if (isEditableAndRemovable.get()) {
-            Abbreviation abbreviationObject = new Abbreviation(name, abbreviation, shortestUniqueAbbreviation);
             AbbreviationViewModel abbViewModel = new AbbreviationViewModel(abbreviationObject);
             if (abbreviations.contains(abbViewModel)) {
                 if (abbViewModel.equals(currentAbbreviation.get())) {
-                    setCurrentAbbreviationNameAndAbbreviationIfValid(name, abbreviation, shortestUniqueAbbreviation);
+                    setCurrentAbbreviationNameAndAbbreviationIfValid(abbreviationObject);
                 } else {
                     dialogService.showErrorDialogAndWait(Localization.lang("Duplicated Journal Abbreviation"),
-                            Localization.lang("Abbreviation '%0' for journal '%1' already defined.", abbreviation, name));
+                            Localization.lang("Abbreviation '%0' for journal '%1' already defined.", abbreviationObject.getAbbreviation(), abbreviationObject.getName()));
                 }
             } else {
-                setCurrentAbbreviationNameAndAbbreviationIfValid(name, abbreviation, shortestUniqueAbbreviation);
+                setCurrentAbbreviationNameAndAbbreviationIfValid(abbreviationObject);
             }
         }
     }
 
-    public void editAbbreviation(String name, String abbreviation) {
-        editAbbreviation(name, abbreviation, "");
-    }
-
-    /**
-     * Sets the name and the abbreviation of the {@code currentAbbreviation} property to the values of the {@code name},
-     * {@code abbreviation}, and {@code shortestUniqueAbbreviation} properties.
-     *
-     * @param name                       name of the abbreviation
-     * @param abbreviation               default abbreviation of the abbreviation
-     * @param shortestUniqueAbbreviation shortest unique abbreviation of the abbreviation
-     */
-    private void setCurrentAbbreviationNameAndAbbreviationIfValid(String name, String abbreviation, String shortestUniqueAbbreviation) {
-        if (name.trim().isEmpty()) {
+    private void setCurrentAbbreviationNameAndAbbreviationIfValid(Abbreviation abbreviationObject) {
+        if (abbreviationObject.getName().trim().isEmpty()) {
             dialogService.showErrorDialogAndWait(Localization.lang("Name cannot be empty"));
             return;
         }
-        if (abbreviation.trim().isEmpty()) {
+        if (abbreviationObject.getAbbreviation().trim().isEmpty()) {
             dialogService.showErrorDialogAndWait(Localization.lang("Abbreviation cannot be empty"));
             return;
         }
-        currentAbbreviation.get().setName(name);
-        currentAbbreviation.get().setAbbreviation(abbreviation);
-        currentAbbreviation.get().setShortestUniqueAbbreviation(shortestUniqueAbbreviation);
+        AbbreviationViewModel abbreviationViewModel = currentAbbreviation.get();
+        abbreviationViewModel.setName(abbreviationObject.getName());
+        abbreviationViewModel.setAbbreviation(abbreviationObject.getAbbreviation());
+        if (abbreviationObject.isDefaultShortestUniqueAbbreviation()) {
+            abbreviationViewModel.setShortestUniqueAbbreviation("");
+        } else {
+            abbreviationViewModel.setShortestUniqueAbbreviation(abbreviationObject.getShortestUniqueAbbreviation());
+        }
         shouldWriteLists = true;
     }
 
@@ -331,14 +310,14 @@ public void removeAbbreviation(AbbreviationViewModel abbreviation) {
     /**
      * Calls the {@link AbbreviationsFileViewModel#writeOrCreate()} method for each file in the journalFiles property
      * which will overwrite the existing files with the content of the abbreviations property of the AbbreviationsFile.
-     * Non existing files will be created.
+     * Non-existing files will be created.
      */
     public void saveJournalAbbreviationFiles() {
         journalFiles.forEach(file -> {
             try {
                 file.writeOrCreate();
             } catch (IOException e) {
-                LOGGER.debug(e.getLocalizedMessage());
+                LOGGER.debug("Error during writing journal CSV", e);
             }
         });
     }

src/main/java/org/jabref/logic/journals/Abbreviation.java

@@ -10,7 +10,9 @@ public class Abbreviation implements Comparable<Abbreviation>, Serializable {
     private transient String name;
     private final String abbreviation;
     private transient String dotlessAbbreviation;
-    private final String shortestUniqueAbbreviation;
+
+    // Is the empty string if not available
+    private String shortestUniqueAbbreviation;
 
     public Abbreviation(String name, String abbreviation) {
         this(name, abbreviation, "");
@@ -24,7 +26,7 @@ public Abbreviation(String name, String abbreviation, String shortestUniqueAbbre
                 shortestUniqueAbbreviation.trim());
     }
 
-    public Abbreviation(String name, String abbreviation, String dotlessAbbreviation, String shortestUniqueAbbreviation) {
+    private Abbreviation(String name, String abbreviation, String dotlessAbbreviation, String shortestUniqueAbbreviation) {
         this.name = name.intern();
         this.abbreviation = abbreviation.intern();
         this.dotlessAbbreviation = dotlessAbbreviation.intern();
@@ -40,11 +42,14 @@ public String getAbbreviation() {
     }
 
     public String getShortestUniqueAbbreviation() {
-        String result = shortestUniqueAbbreviation;
-        if (result.isEmpty()) {
-            return getAbbreviation();
+        if (shortestUniqueAbbreviation.isEmpty()) {
+            shortestUniqueAbbreviation = getAbbreviation();
         }
-        return result;
+        return shortestUniqueAbbreviation;
+    }
+
+    public boolean isDefaultShortestUniqueAbbreviation() {
+        return (shortestUniqueAbbreviation.isEmpty()) || this.shortestUniqueAbbreviation.equals(this.abbreviation);
     }
 
     public String getDotlessAbbreviation() {
@@ -53,7 +58,17 @@ public String getDotlessAbbreviation() {
 
     @Override
     public int compareTo(Abbreviation toCompare) {
-        return getName().compareTo(toCompare.getName());
+        int nameComparison = getName().compareTo(toCompare.getName());
+        if (nameComparison != 0) {
+            return nameComparison;
+        }
+
+        int abbreviationComparison = getAbbreviation().compareTo(toCompare.getAbbreviation());
+        if (abbreviationComparison != 0) {
+            return abbreviationComparison;
+        }
+
+        return getShortestUniqueAbbreviation().compareTo(toCompare.getShortestUniqueAbbreviation());
     }
 
     public String getNext(String current) {
@@ -80,22 +95,19 @@ public String toString() {
     }
 
     @Override
-    public boolean equals(Object obj) {
-        if (this == obj) {
+    public boolean equals(Object o) {
+        if (this == o) {
             return true;
         }
-
-        if ((obj == null) || (getClass() != obj.getClass())) {
+        if (o == null || getClass() != o.getClass()) {
             return false;
         }
-
-        Abbreviation that = (Abbreviation) obj;
-
-        return Objects.equals(getName(), that.getName());
+        Abbreviation that = (Abbreviation) o;
+        return getName().equals(that.getName()) && getAbbreviation().equals(that.getAbbreviation()) && getShortestUniqueAbbreviation().equals(that.getShortestUniqueAbbreviation());
     }
 
     @Override
     public int hashCode() {
-        return Objects.hash(getName());
+        return Objects.hash(getName(), getAbbreviation(), getShortestUniqueAbbreviation());
     }
 }

src/main/java/org/jabref/logic/journals/AbbreviationParser.java

@@ -9,10 +9,8 @@
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
-import java.util.HashSet;
-import java.util.LinkedList;
-import java.util.List;
-import java.util.Set;
+import java.util.Collection;
+import java.util.LinkedHashSet;
 
 import org.apache.commons.csv.CSVParser;
 import org.apache.commons.csv.CSVRecord;
@@ -26,9 +24,10 @@ public class AbbreviationParser {
 
     private static final Logger LOGGER = LoggerFactory.getLogger(AbbreviationParser.class);
 
-    private final Set<Abbreviation> abbreviations = new HashSet<>(5000);
+    // Ensures ordering while preventing duplicates
+    private final LinkedHashSet<Abbreviation> abbreviations = new LinkedHashSet<>();
 
-    public void readJournalListFromResource(String resourceFileName) {
+    void readJournalListFromResource(String resourceFileName) {
         try (InputStream stream = JournalAbbreviationRepository.class.getResourceAsStream(resourceFileName);
              BufferedReader reader = new BufferedReader(new InputStreamReader(stream, StandardCharsets.UTF_8))) {
             readJournalList(reader);
@@ -65,12 +64,13 @@ private void readJournalList(Reader reader) throws IOException {
                     return;
                 }
 
-                abbreviations.add(new Abbreviation(name, abbreviation, shortestUniqueAbbreviation));
+                Abbreviation abbreviationToAdd = new Abbreviation(name, abbreviation, shortestUniqueAbbreviation);
+                abbreviations.add(abbreviationToAdd);
             }
         }
     }
 
-    public List<Abbreviation> getAbbreviations() {
-        return new LinkedList<>(abbreviations);
+    public Collection<Abbreviation> getAbbreviations() {
+        return abbreviations;
     }
 }