From 9c2f360a3325b3eef3f0c1dab9910f82ab5883ae Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Mon, 2 Jan 2023 22:38:12 +0100 Subject: [PATCH 01/10] Use Set instead of List for custom journal abbreviations Co-authored-by: Christoph Co-authored-by: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com> Co-authored-by: ThiloteE <73715071+ThiloteE@users.noreply.github.com> --- CHANGELOG.md | 7 ++-- .../journals/AbbreviationsFileViewModel.java | 5 ++- .../jabref/logic/journals/Abbreviation.java | 40 +++++++++++-------- .../logic/journals/AbbreviationParser.java | 4 +- .../journals/JournalAbbreviationLoader.java | 3 +- .../JournalAbbreviationRepository.java | 15 ++++--- .../logic/journals/AbbreviationTest.java | 14 +++++-- .../JournalAbbreviationRepositoryTest.java | 34 +++++++++++----- 8 files changed, 75 insertions(+), 47 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 66000c5103f..cc6704ed2bf 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/src/main/java/org/jabref/gui/preferences/journals/AbbreviationsFileViewModel.java b/src/main/java/org/jabref/gui/preferences/journals/AbbreviationsFileViewModel.java index 4c6961cdf0b..dfba0025452 100644 --- a/src/main/java/org/jabref/gui/preferences/journals/AbbreviationsFileViewModel.java +++ b/src/main/java/org/jabref/gui/preferences/journals/AbbreviationsFileViewModel.java @@ -5,6 +5,7 @@ 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 +40,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. It's isPseudoFile property * will therefore be set to true. */ public AbbreviationsFileViewModel(List abbreviations, String name) { @@ -51,7 +52,7 @@ public AbbreviationsFileViewModel(List abbreviations, Str public void readAbbreviations() throws IOException { if (path.isPresent()) { - List abbreviationList = JournalAbbreviationLoader.readJournalListFromFile(path.get()); + Collection abbreviationList = JournalAbbreviationLoader.readJournalListFromFile(path.get()); abbreviationList.forEach(abbreviation -> abbreviations.addAll(new AbbreviationViewModel(abbreviation))); } else { throw new FileNotFoundException(); diff --git a/src/main/java/org/jabref/logic/journals/Abbreviation.java b/src/main/java/org/jabref/logic/journals/Abbreviation.java index c3c18ff02bf..e9ec79d7a52 100644 --- a/src/main/java/org/jabref/logic/journals/Abbreviation.java +++ b/src/main/java/org/jabref/logic/journals/Abbreviation.java @@ -10,7 +10,9 @@ public class Abbreviation implements Comparable, 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,10 @@ public String getAbbreviation() { } public String getShortestUniqueAbbreviation() { - String result = shortestUniqueAbbreviation; - if (result.isEmpty()) { - return getAbbreviation(); + if (shortestUniqueAbbreviation.isEmpty()) { + shortestUniqueAbbreviation = getAbbreviation(); } - return result; + return shortestUniqueAbbreviation; } public String getDotlessAbbreviation() { @@ -53,7 +54,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 +91,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()); } } diff --git a/src/main/java/org/jabref/logic/journals/AbbreviationParser.java b/src/main/java/org/jabref/logic/journals/AbbreviationParser.java index 6506469d7da..377174739ee 100644 --- a/src/main/java/org/jabref/logic/journals/AbbreviationParser.java +++ b/src/main/java/org/jabref/logic/journals/AbbreviationParser.java @@ -9,9 +9,9 @@ import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; +import java.util.Collection; import java.util.HashSet; import java.util.LinkedList; -import java.util.List; import java.util.Set; import org.apache.commons.csv.CSVParser; @@ -70,7 +70,7 @@ private void readJournalList(Reader reader) throws IOException { } } - public List getAbbreviations() { + public Collection getAbbreviations() { return new LinkedList<>(abbreviations); } } diff --git a/src/main/java/org/jabref/logic/journals/JournalAbbreviationLoader.java b/src/main/java/org/jabref/logic/journals/JournalAbbreviationLoader.java index d74b6a9fc9d..2c67a734a8b 100644 --- a/src/main/java/org/jabref/logic/journals/JournalAbbreviationLoader.java +++ b/src/main/java/org/jabref/logic/journals/JournalAbbreviationLoader.java @@ -4,6 +4,7 @@ import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; +import java.util.Collection; import java.util.Collections; import java.util.List; @@ -22,7 +23,7 @@ public class JournalAbbreviationLoader { private static final Logger LOGGER = LoggerFactory.getLogger(JournalAbbreviationLoader.class); - public static List readJournalListFromFile(Path file) throws IOException { + public static Collection readJournalListFromFile(Path file) throws IOException { LOGGER.debug(String.format("Reading journal list from file %s", file)); AbbreviationParser parser = new AbbreviationParser(); parser.readJournalListFromFile(file); diff --git a/src/main/java/org/jabref/logic/journals/JournalAbbreviationRepository.java b/src/main/java/org/jabref/logic/journals/JournalAbbreviationRepository.java index 29b61f0d960..ca0db044368 100644 --- a/src/main/java/org/jabref/logic/journals/JournalAbbreviationRepository.java +++ b/src/main/java/org/jabref/logic/journals/JournalAbbreviationRepository.java @@ -1,14 +1,13 @@ package org.jabref.logic.journals; import java.nio.file.Path; -import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; -import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Optional; import java.util.Set; +import java.util.TreeSet; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -25,7 +24,7 @@ public class JournalAbbreviationRepository { private final Map abbreviationToAbbreviationObject = new HashMap<>(); private final Map dotlessToAbbreviationObject = new HashMap<>(); private final Map shortestUniqueToAbbreviationObject = new HashMap<>(); - private final List customAbbreviations = new ArrayList<>(); + private final TreeSet customAbbreviations = new TreeSet<>(); public JournalAbbreviationRepository(Path journalList) { MVStore store = new MVStore.Builder().readOnly().fileName(journalList.toAbsolutePath().toString()).open(); @@ -106,7 +105,7 @@ public Optional get(String input) { Optional customAbbreviation = customAbbreviations.stream() .filter(abbreviation -> isMatched(journal, abbreviation)) - .findAny(); + .findFirst(); if (customAbbreviation.isPresent()) { return customAbbreviation; } @@ -120,13 +119,13 @@ public Optional get(String input) { public void addCustomAbbreviation(Abbreviation abbreviation) { Objects.requireNonNull(abbreviation); - // We do not want to keep duplicates, thus remove the old abbreviation - // (abbreviation equality is tested on name only, so we cannot use a Set instead) - customAbbreviations.remove(abbreviation); + // We do NOT want to keep duplicates + // The set automatically "removes" duplicates + // What is a duplicate? An abbreviation is NOT the same if any field is NOT equal (e.g., if the shortest unique differs, the abbreviation is NOT the same) customAbbreviations.add(abbreviation); } - public List getCustomAbbreviations() { + public Collection getCustomAbbreviations() { return customAbbreviations; } diff --git a/src/test/java/org/jabref/logic/journals/AbbreviationTest.java b/src/test/java/org/jabref/logic/journals/AbbreviationTest.java index 38a0d7ee132..1f3ba7a8eab 100644 --- a/src/test/java/org/jabref/logic/journals/AbbreviationTest.java +++ b/src/test/java/org/jabref/logic/journals/AbbreviationTest.java @@ -3,8 +3,7 @@ import org.junit.jupiter.api.Test; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.assertNotEquals; class AbbreviationTest { @@ -108,7 +107,14 @@ void testDefaultAndShortestUniqueAbbreviationsAreSame() { void testEquals() { Abbreviation abbreviation = new Abbreviation("Long Name", "L N", "LN"); Abbreviation otherAbbreviation = new Abbreviation("Long Name", "L N", "LN"); - assertTrue(abbreviation.equals(otherAbbreviation)); - assertFalse(abbreviation.equals("String")); + assertEquals(abbreviation, otherAbbreviation); + assertNotEquals(abbreviation, "String"); + } + + @Test + void equalAbbrevationsWithFourComponentsAreAlsoCompareZero() { + Abbreviation abbreviation1 = new Abbreviation("Long Name", "L. N.", "LN"); + Abbreviation abbreviation2 = new Abbreviation("Long Name", "L. N.", "LN"); + assertEquals(0, abbreviation1.compareTo(abbreviation2)); } } diff --git a/src/test/java/org/jabref/logic/journals/JournalAbbreviationRepositoryTest.java b/src/test/java/org/jabref/logic/journals/JournalAbbreviationRepositoryTest.java index 42194347d92..a0e8465c542 100644 --- a/src/test/java/org/jabref/logic/journals/JournalAbbreviationRepositoryTest.java +++ b/src/test/java/org/jabref/logic/journals/JournalAbbreviationRepositoryTest.java @@ -1,5 +1,7 @@ package org.jabref.logic.journals; +import java.util.Set; + import javax.swing.undo.CompoundEdit; import org.jabref.architecture.AllowedToUseSwing; @@ -120,26 +122,36 @@ void testDuplicatesIsoOnlyWithShortestUniqueAbbreviation() { @Test void testDuplicateKeys() { - repository.addCustomAbbreviation(new Abbreviation("Long Name", "L. N.")); - assertEquals(1, repository.getCustomAbbreviations().size()); + Abbreviation abbreviationOne = new Abbreviation("Long Name", "L. N."); + repository.addCustomAbbreviation(abbreviationOne); + assertEquals(Set.of(abbreviationOne), repository.getCustomAbbreviations()); assertEquals("L. N.", repository.getDefaultAbbreviation("Long Name").orElse("WRONG")); - repository.addCustomAbbreviation(new Abbreviation("Long Name", "LA. N.")); - assertEquals(1, repository.getCustomAbbreviations().size()); - assertEquals("LA. N.", repository.getDefaultAbbreviation("Long Name").orElse("WRONG")); + Abbreviation abbreviationTwo = new Abbreviation("Long Name", "LA. N."); + repository.addCustomAbbreviation(abbreviationTwo); + assertEquals(Set.of(abbreviationOne, abbreviationTwo), repository.getCustomAbbreviations()); + + // Both abbreviations are kept in the repository + // "L. N." is smaller than "LA. N.", therefore "L. N." is returned + assertEquals("L. N.", repository.getDefaultAbbreviation("Long Name").orElse("WRONG")); } @Test void testDuplicateKeysWithShortestUniqueAbbreviation() { - repository.addCustomAbbreviation(new Abbreviation("Long Name", "L. N.", "LN")); - assertEquals(1, repository.getCustomAbbreviations().size()); + Abbreviation abbreviationOne = new Abbreviation("Long Name", "L. N.", "LN"); + repository.addCustomAbbreviation(abbreviationOne); + assertEquals(Set.of(abbreviationOne), repository.getCustomAbbreviations()); assertEquals("L. N.", repository.getDefaultAbbreviation("Long Name").orElse("WRONG")); assertEquals("LN", repository.getShortestUniqueAbbreviation("Long Name").orElse("WRONG")); - repository.addCustomAbbreviation(new Abbreviation("Long Name", "LA. N.", "LAN")); - assertEquals(1, repository.getCustomAbbreviations().size()); - assertEquals("LA. N.", repository.getDefaultAbbreviation("Long Name").orElse("WRONG")); - assertEquals("LAN", repository.getShortestUniqueAbbreviation("Long Name").orElse("WRONG")); + Abbreviation abbreviationTwo = new Abbreviation("Long Name", "LA. N.", "LAN"); + repository.addCustomAbbreviation(abbreviationTwo); + assertEquals(Set.of(abbreviationOne, abbreviationTwo), repository.getCustomAbbreviations()); + + // Both abbreviations are kept in the repository + // "L. N." is smaller than "LA. N.", therefore "L. N." is returned + assertEquals("L. N.", repository.getDefaultAbbreviation("Long Name").orElse("WRONG")); + assertEquals("LN", repository.getShortestUniqueAbbreviation("Long Name").orElse("WRONG")); } @Test From 3644d7e01961e715ae6f04d48352e726b53bf6c2 Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Tue, 3 Jan 2023 00:10:47 +0100 Subject: [PATCH 02/10] Try to fix tests Co-authored-by: Christoph --- .../journals/AbbreviationViewModel.java | 10 +++-- .../journals/AbbreviationsFileViewModel.java | 5 ++- .../JournalAbbreviationsTabViewModel.java | 4 +- .../JournalAbbreviationsViewModelTabTest.java | 37 +++++++++++-------- 4 files changed, 33 insertions(+), 23 deletions(-) diff --git a/src/main/java/org/jabref/gui/preferences/journals/AbbreviationViewModel.java b/src/main/java/org/jabref/gui/preferences/journals/AbbreviationViewModel.java index b8f294fb06a..dc652bc2369 100644 --- a/src/main/java/org/jabref/gui/preferences/journals/AbbreviationViewModel.java +++ b/src/main/java/org/jabref/gui/preferences/journals/AbbreviationViewModel.java @@ -19,6 +19,8 @@ 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) { @@ -87,13 +89,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) { diff --git a/src/main/java/org/jabref/gui/preferences/journals/AbbreviationsFileViewModel.java b/src/main/java/org/jabref/gui/preferences/journals/AbbreviationsFileViewModel.java index dfba0025452..42c3c28a493 100644 --- a/src/main/java/org/jabref/gui/preferences/journals/AbbreviationsFileViewModel.java +++ b/src/main/java/org/jabref/gui/preferences/journals/AbbreviationsFileViewModel.java @@ -61,14 +61,15 @@ 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 actualAbbreviations = abbreviations.stream().filter(abb -> !abb.isPseudoAbbreviation()) - .map(AbbreviationViewModel::getAbbreviationObject).collect(Collectors.toList()); + .map(AbbreviationViewModel::getAbbreviationObject) + .collect(Collectors.toList()); AbbreviationWriter.writeOrCreate(path.get(), actualAbbreviations, StandardCharsets.UTF_8); } } diff --git a/src/main/java/org/jabref/gui/preferences/journals/JournalAbbreviationsTabViewModel.java b/src/main/java/org/jabref/gui/preferences/journals/JournalAbbreviationsTabViewModel.java index 43d5cf4b18e..762bbf5f842 100644 --- a/src/main/java/org/jabref/gui/preferences/journals/JournalAbbreviationsTabViewModel.java +++ b/src/main/java/org/jabref/gui/preferences/journals/JournalAbbreviationsTabViewModel.java @@ -331,14 +331,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); } }); } diff --git a/src/test/java/org/jabref/gui/preferences/journals/JournalAbbreviationsViewModelTabTest.java b/src/test/java/org/jabref/gui/preferences/journals/JournalAbbreviationsViewModelTabTest.java index c7db04c42e4..4f77704f808 100644 --- a/src/test/java/org/jabref/gui/preferences/journals/JournalAbbreviationsViewModelTabTest.java +++ b/src/test/java/org/jabref/gui/preferences/journals/JournalAbbreviationsViewModelTabTest.java @@ -28,7 +28,6 @@ import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; -import static org.jabref.logic.util.OS.NEWLINE; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNull; @@ -52,30 +51,35 @@ public static Stream provideTestFiles() { return Stream.of( // Mixed abbreviations Arguments.of( - List.of(List.of("testFile1Entries.csv", "Test Entry;TE" + NEWLINE + ""), - List.of("testFile3Entries.csv", "Abbreviations;Abb;A" + NEWLINE + "Test Entry;TE" + NEWLINE + "MoreEntries;ME;M" + NEWLINE + ""), - List.of("testFile4Entries.csv", "Abbreviations;Abb" + NEWLINE + "Test Entry;TE;T" + NEWLINE + "MoreEntries;ME;M" + NEWLINE + "Entry;E" + NEWLINE + ""), - List.of("testFile5Entries.csv", "Abbreviations;Abb" + NEWLINE + "Test Entry;TE;T" + NEWLINE + "Test Entry;TE" + NEWLINE + "MoreEntries;ME;M" + NEWLINE + "EntryEntry;EE" + NEWLINE + "")), + List.of(List.of("testFile1Entries.csv", "Test Entry;TE\n"), + List.of("testFile3Entries.csv", "Abbreviations;Abb;A\nTest Entry;TE\nMoreEntries;ME;M\n"), + List.of("testFile4Entries.csv", "Abbreviations;Abb\nTest Entry;TE;T\nMoreEntries;ME;M\nEntry;E\n"), + // contains similar entry "Test Entry" + List.of("testFile5Entries.csv", "Abbreviations;Abb\nTest Entry;TE;T\nTest Entry;TE\nMoreEntries;ME;M\nEntryEntry;EE\n")), List.of( + // Entries of testFile4 List.of("Abbreviations;Abb;Abb", "Test Entry;TE;T", "MoreEntries;ME;M", "JabRefTestEntry;JTE;JTE"), - List.of("EntryEntry;EE;EE", "Abbreviations;Abb;Abb", "Test Entry;TE;T", "SomeOtherEntry;SOE;SOE"))), + // Entries of testFile5 + List.of("EntryEntry;EE;EE", "Abbreviations;Abb;Abb", "Test Entry;TE;T", "Test Entry;TE", "SomeOtherEntry;SOE;SOE"))), // No shortest unique abbreviations Arguments.of( - List.of(List.of("testFile1Entries.csv", "Test Entry;TE" + NEWLINE + ""), - List.of("testFile3Entries.csv", "Abbreviations;Abb" + NEWLINE + "Test Entry;TE" + NEWLINE + "MoreEntries;ME" + NEWLINE + ""), - List.of("testFile4Entries.csv", "Abbreviations;Abb" + NEWLINE + "Test Entry;TE" + NEWLINE + "MoreEntries;ME" + NEWLINE + "Entry;E" + NEWLINE + ""), - List.of("testFile5Entries.csv", "Abbreviations;Abb" + NEWLINE + "Test Entry;TE" + NEWLINE + "Test Entry;TE" + NEWLINE + "MoreEntries;ME" + NEWLINE + "EntryEntry;EE" + NEWLINE + "")), + List.of(List.of("testFile1Entries.csv", "Test Entry;TE\n"), + List.of("testFile3Entries.csv", "Abbreviations;Abb\nTest Entry;TE\nMoreEntries;ME\n"), + List.of("testFile4Entries.csv", "Abbreviations;Abb\nTest Entry;TE\nMoreEntries;ME\nEntry;E\n"), + // contains duplicate entry "Test Entry" + List.of("testFile5Entries.csv", "Abbreviations;Abb\nTest Entry;TE\nTest Entry;TE\nMoreEntries;ME\nEntryEntry;EE\n")), List.of( List.of("Abbreviations;Abb;Abb", "Test Entry;TE;TE", "MoreEntries;ME;ME", "JabRefTestEntry;JTE;JTE"), List.of("EntryEntry;EE;EE", "Abbreviations;Abb;Abb", "Test Entry;TE;TE", "SomeOtherEntry;SOE;SOE"))), // Shortest unique abbreviations Arguments.of( - List.of(List.of("testFile1Entries.csv", "Test Entry;TE;T" + NEWLINE + ""), - List.of("testFile3Entries.csv", "Abbreviations;Abb;A" + NEWLINE + "Test Entry;TE;T" + NEWLINE + "MoreEntries;ME;M" + NEWLINE + ""), - List.of("testFile4Entries.csv", "Abbreviations;Abb;A" + NEWLINE + "Test Entry;TE;T" + NEWLINE + "MoreEntries;ME;M" + NEWLINE + "Entry;En;E" + NEWLINE + ""), - List.of("testFile5Entries.csv", "Abbreviations;Abb;A" + NEWLINE + "Test Entry;TE;T" + NEWLINE + "Test Entry;TE;T" + NEWLINE + "MoreEntries;ME;M" + NEWLINE + "EntryEntry;EE" + NEWLINE + "")), + List.of(List.of("testFile1Entries.csv", "Test Entry;TE;T\n"), + List.of("testFile3Entries.csv", "Abbreviations;Abb;A\nTest Entry;TE;T\nMoreEntries;ME;M\n"), + List.of("testFile4Entries.csv", "Abbreviations;Abb;A\nTest Entry;TE;T\nMoreEntries;ME;M\nEntry;En;E\n"), + // contains duplicate entry "Test Entry" + List.of("testFile5Entries.csv", "Abbreviations;Abb;A\nTest Entry;TE;T\nTest Entry;TE;T\nMoreEntries;ME;M\nEntryEntry;EE\n")), List.of( List.of("Abbreviations;Abb;A", "Test Entry;TE;T", "MoreEntries;ME;M", "JabRefTestEntry;JTE;JTE"), List.of("EntryEntry;EE;EE", "Abbreviations;Abb;A", "Test Entry;TE;T", "SomeOtherEntry;SOE;SOE"))) @@ -226,6 +230,7 @@ void testMixedFileUsage(List> testFiles) throws IOException { assertEquals(4, viewModel.journalFilesProperty().size()); assertEquals(4, viewModel.abbreviationsProperty().size()); + // check some abbreviation assertTrue(viewModel.abbreviationsProperty().contains(new AbbreviationViewModel(testAbbreviation2))); } @@ -426,11 +431,11 @@ void testSaveExternalFilesListToPreferences(List> testFiles) throws } private void addAbbreviation(Abbreviation testAbbreviation) { - viewModel.addAbbreviation(testAbbreviation.getName(), testAbbreviation.getAbbreviation()); + viewModel.addAbbreviation(testAbbreviation.getName(), testAbbreviation.getAbbreviation(), testAbbreviation.getShortestUniqueAbbreviation()); } private void editAbbreviation(Abbreviation testAbbreviation) { - viewModel.editAbbreviation(testAbbreviation.getName(), testAbbreviation.getAbbreviation()); + viewModel.editAbbreviation(testAbbreviation.getName(), testAbbreviation.getAbbreviation(), testAbbreviation.getShortestUniqueAbbreviation()); } /** From e42dd26928d5b3a50b21ca53f6513eedbfd83c47 Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Tue, 3 Jan 2023 13:43:28 +0100 Subject: [PATCH 03/10] Prepare parallel test execution --- src/test/resources/junit-platform.properties | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 src/test/resources/junit-platform.properties diff --git a/src/test/resources/junit-platform.properties b/src/test/resources/junit-platform.properties new file mode 100644 index 00000000000..4f6f62fc89c --- /dev/null +++ b/src/test/resources/junit-platform.properties @@ -0,0 +1,4 @@ +# See https://www.baeldung.com/junit-5-parallel-tests for details +junit.jupiter.execution.parallel.enabled = true + +# junit.jupiter.execution.parallel.mode.default = concurrent From 6c9084cd4c7afea56439bad0fea078853eba99f6 Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Tue, 3 Jan 2023 14:26:00 +0100 Subject: [PATCH 04/10] Refine tests - Sort test cases - More clear file names - enable concurrency - introduce "containsDuplicate" - introduce "abbreviationToContain" --- .../JournalAbbreviationsViewModelTabTest.java | 99 +++++++++++-------- 1 file changed, 56 insertions(+), 43 deletions(-) diff --git a/src/test/java/org/jabref/gui/preferences/journals/JournalAbbreviationsViewModelTabTest.java b/src/test/java/org/jabref/gui/preferences/journals/JournalAbbreviationsViewModelTabTest.java index 4f77704f808..daa6c753df7 100644 --- a/src/test/java/org/jabref/gui/preferences/journals/JournalAbbreviationsViewModelTabTest.java +++ b/src/test/java/org/jabref/gui/preferences/journals/JournalAbbreviationsViewModelTabTest.java @@ -24,6 +24,8 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; +import org.junit.jupiter.api.parallel.Execution; +import org.junit.jupiter.api.parallel.ExecutionMode; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; @@ -38,6 +40,7 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +@Execution(ExecutionMode.CONCURRENT) class JournalAbbreviationsViewModelTabTest { private JournalAbbreviationsTabViewModel viewModel; @@ -49,41 +52,50 @@ class JournalAbbreviationsViewModelTabTest { public static Stream provideTestFiles() { return Stream.of( - // Mixed abbreviations + // No shortest unique abbreviations in files + // Shortest unique abbreviations in entries (being the same then the abbreviation) Arguments.of( - List.of(List.of("testFile1Entries.csv", "Test Entry;TE\n"), - List.of("testFile3Entries.csv", "Abbreviations;Abb;A\nTest Entry;TE\nMoreEntries;ME;M\n"), - List.of("testFile4Entries.csv", "Abbreviations;Abb\nTest Entry;TE;T\nMoreEntries;ME;M\nEntry;E\n"), - // contains similar entry "Test Entry" - List.of("testFile5Entries.csv", "Abbreviations;Abb\nTest Entry;TE;T\nTest Entry;TE\nMoreEntries;ME;M\nEntryEntry;EE\n")), - List.of( - // Entries of testFile4 - List.of("Abbreviations;Abb;Abb", "Test Entry;TE;T", "MoreEntries;ME;M", "JabRefTestEntry;JTE;JTE"), - // Entries of testFile5 - List.of("EntryEntry;EE;EE", "Abbreviations;Abb;Abb", "Test Entry;TE;T", "Test Entry;TE", "SomeOtherEntry;SOE;SOE"))), - - // No shortest unique abbreviations - Arguments.of( - List.of(List.of("testFile1Entries.csv", "Test Entry;TE\n"), - List.of("testFile3Entries.csv", "Abbreviations;Abb\nTest Entry;TE\nMoreEntries;ME\n"), - List.of("testFile4Entries.csv", "Abbreviations;Abb\nTest Entry;TE\nMoreEntries;ME\nEntry;E\n"), - // contains duplicate entry "Test Entry" - List.of("testFile5Entries.csv", "Abbreviations;Abb\nTest Entry;TE\nTest Entry;TE\nMoreEntries;ME\nEntryEntry;EE\n")), + List.of(List.of("testFile0_1Entries.csv", "Test Entry;TE\n"), + List.of("testFile1_3Entries.csv", "Abbreviations;Abb\nTest Entry;TE\nMoreEntries;ME\n"), + List.of("testFile2_4Entries.csv", "Abbreviations;Abb\nTest Entry;TE\nMoreEntries;ME\nEntry;E\n"), + // contains duplicate entry "Test Entry" (without shortest unique "T") + List.of("testFile3_5Entries.csv", "Abbreviations;Abb\nTest Entry;TE\nTest Entry;TE\nMoreEntries;ME\nEntryEntry;EE\n")), + true, List.of( List.of("Abbreviations;Abb;Abb", "Test Entry;TE;TE", "MoreEntries;ME;ME", "JabRefTestEntry;JTE;JTE"), - List.of("EntryEntry;EE;EE", "Abbreviations;Abb;Abb", "Test Entry;TE;TE", "SomeOtherEntry;SOE;SOE"))), + List.of("EntryEntry;EE;EE", "Abbreviations;Abb;Abb", "Test Entry;TE;TE", "SomeOtherEntry;SOE;SOE")), + new Abbreviation("Test Entry", "TE", "TE")), // Shortest unique abbreviations Arguments.of( - List.of(List.of("testFile1Entries.csv", "Test Entry;TE;T\n"), - List.of("testFile3Entries.csv", "Abbreviations;Abb;A\nTest Entry;TE;T\nMoreEntries;ME;M\n"), - List.of("testFile4Entries.csv", "Abbreviations;Abb;A\nTest Entry;TE;T\nMoreEntries;ME;M\nEntry;En;E\n"), - // contains duplicate entry "Test Entry" - List.of("testFile5Entries.csv", "Abbreviations;Abb;A\nTest Entry;TE;T\nTest Entry;TE;T\nMoreEntries;ME;M\nEntryEntry;EE\n")), + List.of(List.of("testFile0_1Entries.csv", "Test Entry;TE;T\n"), + List.of("testFile1_3Entries.csv", "Abbreviations;Abb;A\nTest Entry;TE;T\nMoreEntries;ME;M\n"), + List.of("testFile2_4Entries.csv", "Abbreviations;Abb;A\nTest Entry;TE;T\nMoreEntries;ME;M\nEntry;En;E\n"), + // contains duplicate entry "Test Entry" (with shortest unique "T") + List.of("testFile3_5Entries.csv", "Abbreviations;Abb;A\nTest Entry;TE;T\nTest Entry;TE;T\nMoreEntries;ME;M\nEntryEntry;EE\n")), + true, List.of( List.of("Abbreviations;Abb;A", "Test Entry;TE;T", "MoreEntries;ME;M", "JabRefTestEntry;JTE;JTE"), - List.of("EntryEntry;EE;EE", "Abbreviations;Abb;A", "Test Entry;TE;T", "SomeOtherEntry;SOE;SOE"))) - ); + List.of("EntryEntry;EE;EE", "Abbreviations;Abb;A", "Test Entry;TE;T", "SomeOtherEntry;SOE;SOE")), + new Abbreviation("Test Entry", "TE", "T")), + + // Mixed abbreviations (some have shortest unique, some have not) + Arguments.of( + List.of(List.of("testFile0_1Entries.csv", "Test Entry;TE\n"), + List.of("testFile1_3Entries.csv", "Abbreviations;Abb;A\nTest Entry;TE\nMoreEntries;ME;M\n"), + // Here, "Test Entry" has "T" as shortest unique abbreviation (and not "TE" anymore) + List.of("testFile2_4Entries.csv", "Abbreviations;Abb\nTest Entry;TE;T\nMoreEntries;ME;M\nEntry;E\n"), + // contains "Test Entry" in two variants + List.of("testFile3_5Entries.csv", "Abbreviations;Abb\nTest Entry;TE;T\nTest Entry;TE\nMoreEntries;ME;M\nEntryEntry;EE\n")), + false, + List.of( + // Entries of testFile4 plus "JabRefTestEntry" + List.of("Abbreviations;Abb;Abb", "Test Entry;TE;T", "MoreEntries;ME;M", "JabRefTestEntry;JTE;JTE"), + // Entries of testFile5 plus "SomeOtherEntry" minus MoreEntries minus EntryEntry + List.of("EntryEntry;EE;EE", "Abbreviations;Abb;Abb", "Test Entry;TE;T", "Test Entry;TE", "SomeOtherEntry;SOE;SOE")), + new Abbreviation("Test Entry", "TE", "T")) + + ); } @BeforeEach @@ -167,15 +179,15 @@ void testSelectLastJournalFileSwitchesFilesAndTheirAbbreviations(List> testFiles) throws IOException { - Abbreviation testAbbreviation = new Abbreviation("Test Entry", "TE"); - when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testFiles.get(1)))); + void testOpenValidFileContainsTheSpecificEntryAndEnoughAbbreviations(List> testFiles, boolean containsDuplicate, List> entries, Abbreviation abbreviationToContain) throws IOException { + when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testFiles.get(2)))); viewModel.addNewFile(); viewModel.selectLastJournalFile(); assertEquals(1, viewModel.journalFilesProperty().size()); - assertEquals(3, viewModel.abbreviationsProperty().size()); - assertTrue(viewModel.abbreviationsProperty().contains(new AbbreviationViewModel(testAbbreviation))); + assertEquals(4, viewModel.abbreviationsProperty().size()); + + assertTrue(viewModel.abbreviationsProperty().contains(new AbbreviationViewModel(abbreviationToContain))); } @ParameterizedTest @@ -193,10 +205,7 @@ void testRemoveLastListSetsCurrentFileAndCurrentAbbreviationToNull(List> testFiles) throws IOException { - Abbreviation testAbbreviation = new Abbreviation("Entry", "E"); - Abbreviation testAbbreviation2 = new Abbreviation("EntryEntry", "EE"); - + void testMixedFileUsage(List> testFiles, boolean containsDuplicate, List> entries, Abbreviation abbreviationToContain) throws IOException { // simulate open file button twice when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testFiles.get(1)))); viewModel.addNewFile(); @@ -206,10 +215,12 @@ void testMixedFileUsage(List> testFiles) throws IOException { // size of the list of journal files should be incremented by two assertEquals(2, viewModel.journalFilesProperty().size()); - // our second test file has 4 abbreviations + + // our third test file has 4 abbreviations assertEquals(4, viewModel.abbreviationsProperty().size()); - // check some abbreviation - assertTrue(viewModel.abbreviationsProperty().contains(new AbbreviationViewModel(testAbbreviation))); + + // check "arbitrary" abbreviation + assertTrue(viewModel.abbreviationsProperty().contains(new AbbreviationViewModel(abbreviationToContain))); // simulate add new file button when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(emptyTestFile)); @@ -218,6 +229,7 @@ void testMixedFileUsage(List> testFiles) throws IOException { // size of the list of journal files should be incremented by one assertEquals(3, viewModel.journalFilesProperty().size()); + // a new file has zero abbreviations assertEquals(0, viewModel.abbreviationsProperty().size()); @@ -229,10 +241,11 @@ void testMixedFileUsage(List> testFiles) throws IOException { // size of the list of journal files should be incremented by one assertEquals(4, viewModel.journalFilesProperty().size()); - assertEquals(4, viewModel.abbreviationsProperty().size()); + // Fourth file has 5 entries, but "sometimes" has a duplicate entry + assertEquals(containsDuplicate ? 4 : 5, viewModel.abbreviationsProperty().size()); - // check some abbreviation - assertTrue(viewModel.abbreviationsProperty().contains(new AbbreviationViewModel(testAbbreviation2))); + // check "arbitrary" abbreviation + assertTrue(viewModel.abbreviationsProperty().contains(new AbbreviationViewModel(abbreviationToContain))); } @Test @@ -389,7 +402,7 @@ void testEditAbbreviationToEmptyAbbreviationResultsInException(List @ParameterizedTest @MethodSource("provideTestFiles") - void testSaveAbbreviationsToFilesCreatesNewFilesWithWrittenAbbreviations(List> testFiles, List> testEntries) throws Exception { + void testSaveAbbreviationsToFilesCreatesNewFilesWithWrittenAbbreviations(List> testFiles, boolean containsDuplicate, List> testEntries) throws Exception { Path testFile4Entries = createTestFile(testFiles.get(2)); Path testFile5EntriesWithDuplicate = createTestFile(testFiles.get(3)); From 5493e7a4ed296283bf7550a99ed1622d99645b00 Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Tue, 3 Jan 2023 16:29:10 +0100 Subject: [PATCH 05/10] More OO in JournalAbstractionsTabViewModel --- .../journals/AbbreviationViewModel.java | 10 ++- .../JournalAbbreviationsTabViewModel.java | 61 ++++++------------- .../jabref/logic/journals/Abbreviation.java | 4 ++ .../JournalAbbreviationsViewModelTabTest.java | 32 ++++------ 4 files changed, 45 insertions(+), 62 deletions(-) diff --git a/src/main/java/org/jabref/gui/preferences/journals/AbbreviationViewModel.java b/src/main/java/org/jabref/gui/preferences/journals/AbbreviationViewModel.java index dc652bc2369..1c13439cf6b 100644 --- a/src/main/java/org/jabref/gui/preferences/journals/AbbreviationViewModel.java +++ b/src/main/java/org/jabref/gui/preferences/journals/AbbreviationViewModel.java @@ -28,7 +28,15 @@ public AbbreviationViewModel(Abbreviation abbreviationObject) { 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); + } } } diff --git a/src/main/java/org/jabref/gui/preferences/journals/JournalAbbreviationsTabViewModel.java b/src/main/java/org/jabref/gui/preferences/journals/JournalAbbreviationsTabViewModel.java index 762bbf5f842..3dc07113edf 100644 --- a/src/main/java/org/jabref/gui/preferences/journals/JournalAbbreviationsTabViewModel.java +++ b/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,17 +217,12 @@ 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); @@ -235,65 +230,49 @@ public void addAbbreviation(String name, String abbreviation, String shortestUni } } - 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; } diff --git a/src/main/java/org/jabref/logic/journals/Abbreviation.java b/src/main/java/org/jabref/logic/journals/Abbreviation.java index e9ec79d7a52..71fc20060e3 100644 --- a/src/main/java/org/jabref/logic/journals/Abbreviation.java +++ b/src/main/java/org/jabref/logic/journals/Abbreviation.java @@ -48,6 +48,10 @@ public String getShortestUniqueAbbreviation() { return shortestUniqueAbbreviation; } + public boolean isDefaultShortestUniqueAbbreviation() { + return this.shortestUniqueAbbreviation.equals(this.abbreviation); + } + public String getDotlessAbbreviation() { return this.dotlessAbbreviation; } diff --git a/src/test/java/org/jabref/gui/preferences/journals/JournalAbbreviationsViewModelTabTest.java b/src/test/java/org/jabref/gui/preferences/journals/JournalAbbreviationsViewModelTabTest.java index daa6c753df7..d8dd2f0578c 100644 --- a/src/test/java/org/jabref/gui/preferences/journals/JournalAbbreviationsViewModelTabTest.java +++ b/src/test/java/org/jabref/gui/preferences/journals/JournalAbbreviationsViewModelTabTest.java @@ -303,7 +303,7 @@ void testAddAbbreviationIncludesAbbreviationsInAbbreviationList(List> testFile when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testFiles.get(1)))); viewModel.addNewFile(); viewModel.selectLastJournalFile(); - viewModel.addAbbreviation("YetAnotherEntry", "YAE"); - viewModel.addAbbreviation("YetAnotherEntry", "YAE"); + viewModel.addAbbreviation(new Abbreviation("YetAnotherEntry", "YAE")); + viewModel.addAbbreviation(new Abbreviation("YetAnotherEntry", "YAE")); verify(dialogService).showErrorDialogAndWait(anyString(), anyString()); } @@ -326,8 +326,8 @@ void testEditSameAbbreviationWithNoChangeDoesNotResultInException() { viewModel.addNewFile(); viewModel.selectLastJournalFile(); Abbreviation testAbbreviation = new Abbreviation("YetAnotherEntry", "YAE"); - addAbbreviation(testAbbreviation); - editAbbreviation(testAbbreviation); + viewModel.addAbbreviation(testAbbreviation); + viewModel.editAbbreviation(testAbbreviation); assertTrue(viewModel.abbreviationsProperty().contains(new AbbreviationViewModel(testAbbreviation))); } @@ -342,7 +342,7 @@ void testEditAbbreviationIncludesNewAbbreviationInAbbreviationsList(List> test assertEquals(3, viewModel.abbreviationsProperty().size()); - viewModel.editAbbreviation("YetAnotherEntry", "YAE"); + viewModel.editAbbreviation(new Abbreviation("YetAnotherEntry", "YAE")); viewModel.currentAbbreviationProperty().set(viewModel.abbreviationsProperty().get(1)); - viewModel.editAbbreviation("YetAnotherEntry", "YAE"); + viewModel.editAbbreviation(new Abbreviation("YetAnotherEntry", "YAE")); verify(dialogService).showErrorDialogAndWait(anyString(), anyString()); } @@ -382,7 +382,7 @@ void testEditAbbreviationToEmptyNameResultsInException(List> testFi assertEquals(3, viewModel.abbreviationsProperty().size()); - viewModel.editAbbreviation("", "YAE"); + viewModel.editAbbreviation(new Abbreviation("", "YAE")); verify(dialogService).showErrorDialogAndWait(anyString()); } @@ -396,7 +396,7 @@ void testEditAbbreviationToEmptyAbbreviationResultsInException(List assertEquals(3, viewModel.abbreviationsProperty().size()); - viewModel.editAbbreviation("YetAnotherEntry", ""); + viewModel.editAbbreviation(new Abbreviation("YetAnotherEntry", "")); verify(dialogService).showErrorDialogAndWait(anyString()); } @@ -411,7 +411,7 @@ void testSaveAbbreviationsToFilesCreatesNewFilesWithWrittenAbbreviations(List
  • > testFiles) throws verify(preferencesService).storeJournalAbbreviationPreferences(any()); } - private void addAbbreviation(Abbreviation testAbbreviation) { - viewModel.addAbbreviation(testAbbreviation.getName(), testAbbreviation.getAbbreviation(), testAbbreviation.getShortestUniqueAbbreviation()); - } - - private void editAbbreviation(Abbreviation testAbbreviation) { - viewModel.editAbbreviation(testAbbreviation.getName(), testAbbreviation.getAbbreviation(), testAbbreviation.getShortestUniqueAbbreviation()); - } - /** * Select the last abbreviation in the list of abbreviations */ From d8dc0e4c2871a791ff46139d27b30b5bb900c092 Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Tue, 3 Jan 2023 16:29:36 +0100 Subject: [PATCH 06/10] Switch from HashSet to List, refactor test --- .../journals/AbbreviationsFileViewModel.java | 3 +- .../jabref/logic/journals/Abbreviation.java | 2 +- .../logic/journals/AbbreviationParser.java | 17 +- .../logic/journals/AbbreviationWriter.java | 8 +- .../JournalAbbreviationsViewModelTabTest.java | 316 ++++++++++++------ .../journals/AbbreviationWriterTest.java | 36 ++ 6 files changed, 260 insertions(+), 122 deletions(-) create mode 100644 src/test/java/org/jabref/logic/journals/AbbreviationWriterTest.java diff --git a/src/main/java/org/jabref/gui/preferences/journals/AbbreviationsFileViewModel.java b/src/main/java/org/jabref/gui/preferences/journals/AbbreviationsFileViewModel.java index 42c3c28a493..a38e02d6c36 100644 --- a/src/main/java/org/jabref/gui/preferences/journals/AbbreviationsFileViewModel.java +++ b/src/main/java/org/jabref/gui/preferences/journals/AbbreviationsFileViewModel.java @@ -2,7 +2,6 @@ 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; @@ -70,7 +69,7 @@ public void writeOrCreate() throws IOException { abbreviations.stream().filter(abb -> !abb.isPseudoAbbreviation()) .map(AbbreviationViewModel::getAbbreviationObject) .collect(Collectors.toList()); - AbbreviationWriter.writeOrCreate(path.get(), actualAbbreviations, StandardCharsets.UTF_8); + AbbreviationWriter.writeOrCreate(path.get(), actualAbbreviations); } } diff --git a/src/main/java/org/jabref/logic/journals/Abbreviation.java b/src/main/java/org/jabref/logic/journals/Abbreviation.java index 71fc20060e3..a207cc20431 100644 --- a/src/main/java/org/jabref/logic/journals/Abbreviation.java +++ b/src/main/java/org/jabref/logic/journals/Abbreviation.java @@ -49,7 +49,7 @@ public String getShortestUniqueAbbreviation() { } public boolean isDefaultShortestUniqueAbbreviation() { - return this.shortestUniqueAbbreviation.equals(this.abbreviation); + return (shortestUniqueAbbreviation.isEmpty()) || this.shortestUniqueAbbreviation.equals(this.abbreviation); } public String getDotlessAbbreviation() { diff --git a/src/main/java/org/jabref/logic/journals/AbbreviationParser.java b/src/main/java/org/jabref/logic/journals/AbbreviationParser.java index 377174739ee..22665018a4b 100644 --- a/src/main/java/org/jabref/logic/journals/AbbreviationParser.java +++ b/src/main/java/org/jabref/logic/journals/AbbreviationParser.java @@ -12,6 +12,7 @@ import java.util.Collection; import java.util.HashSet; import java.util.LinkedList; +import java.util.List; import java.util.Set; import org.apache.commons.csv.CSVParser; @@ -26,9 +27,13 @@ public class AbbreviationParser { private static final Logger LOGGER = LoggerFactory.getLogger(AbbreviationParser.class); - private final Set abbreviations = new HashSet<>(5000); + // List to ensure ordering + private final List abbreviations = new LinkedList<>(); - public void readJournalListFromResource(String resourceFileName) { + // We need to check duplicates fast (!) + private final Set readAbbreviations = new HashSet<>(); + + 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 +70,16 @@ private void readJournalList(Reader reader) throws IOException { return; } - abbreviations.add(new Abbreviation(name, abbreviation, shortestUniqueAbbreviation)); + Abbreviation abbreviationToAdd = new Abbreviation(name, abbreviation, shortestUniqueAbbreviation); + if (!readAbbreviations.contains(abbreviationToAdd)) { + readAbbreviations.add(abbreviationToAdd); + abbreviations.add(abbreviationToAdd); + } } } } public Collection getAbbreviations() { - return new LinkedList<>(abbreviations); + return abbreviations; } } diff --git a/src/main/java/org/jabref/logic/journals/AbbreviationWriter.java b/src/main/java/org/jabref/logic/journals/AbbreviationWriter.java index d922ddd488d..4a3054f7634 100644 --- a/src/main/java/org/jabref/logic/journals/AbbreviationWriter.java +++ b/src/main/java/org/jabref/logic/journals/AbbreviationWriter.java @@ -2,7 +2,7 @@ import java.io.IOException; import java.io.OutputStreamWriter; -import java.nio.charset.Charset; +import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; import java.util.List; @@ -24,11 +24,11 @@ private AbbreviationWriter() { * @param path to a file (doesn't have to exist just yet) * @param abbreviations as a list specifying which entries should be written */ - public static void writeOrCreate(Path path, List abbreviations, Charset encoding) throws IOException { - try (OutputStreamWriter writer = new OutputStreamWriter(Files.newOutputStream(path), encoding); + public static void writeOrCreate(Path path, List abbreviations) throws IOException { + try (OutputStreamWriter writer = new OutputStreamWriter(Files.newOutputStream(path), StandardCharsets.UTF_8); CSVPrinter csvPrinter = new CSVPrinter(writer, AbbreviationFormat.getCSVFormat())) { for (Abbreviation entry : abbreviations) { - if (entry.getShortestUniqueAbbreviation().isEmpty()) { + if (entry.isDefaultShortestUniqueAbbreviation()) { csvPrinter.printRecord(entry.getName(), entry.getAbbreviation()); } else { csvPrinter.printRecord(entry.getName(), entry.getAbbreviation(), entry.getShortestUniqueAbbreviation()); diff --git a/src/test/java/org/jabref/gui/preferences/journals/JournalAbbreviationsViewModelTabTest.java b/src/test/java/org/jabref/gui/preferences/journals/JournalAbbreviationsViewModelTabTest.java index d8dd2f0578c..750a94b4688 100644 --- a/src/test/java/org/jabref/gui/preferences/journals/JournalAbbreviationsViewModelTabTest.java +++ b/src/test/java/org/jabref/gui/preferences/journals/JournalAbbreviationsViewModelTabTest.java @@ -4,6 +4,7 @@ import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; +import java.util.Arrays; import java.util.List; import java.util.Optional; import java.util.stream.Collectors; @@ -27,7 +28,6 @@ import org.junit.jupiter.api.parallel.Execution; import org.junit.jupiter.api.parallel.ExecutionMode; import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -50,52 +50,149 @@ class JournalAbbreviationsViewModelTabTest { private final JournalAbbreviationRepository repository = JournalAbbreviationLoader.loadBuiltInRepository(); private DialogService dialogService; - public static Stream provideTestFiles() { + static class TestAbbreviation extends Abbreviation { + + private final boolean showShortestUniqueAbbreviation; + + public TestAbbreviation(String name, String abbreviation) { + this(name, abbreviation, false); + } + + public TestAbbreviation(String name, String abbreviation, boolean showShortestUniqueAbbreviation) { + super(name, abbreviation); + this.showShortestUniqueAbbreviation = showShortestUniqueAbbreviation; + } + + public TestAbbreviation(String name, String abbreviation, String shortestUniqueAbbreviation) { + super(name, abbreviation, shortestUniqueAbbreviation); + this.showShortestUniqueAbbreviation = true; + } + + @Override + public String toString() { + if (showShortestUniqueAbbreviation) { + return this.getName() + ";" + this.getAbbreviation() + ";" + this.getShortestUniqueAbbreviation(); + } + return this.getName() + ";" + this.getAbbreviation(); + } + } + + private final static TestAbbreviation ABBREVIATION_0 = new TestAbbreviation("Full0", "Abb0"); + private final static TestAbbreviation ABBREVIATION_0_SHOW = new TestAbbreviation("Full0", "Abb0", true); + private final static TestAbbreviation ABBREVIATION_0_OTHER_SHORT_UNIQUE = new TestAbbreviation("Full0", "Abb0", "A0"); + + private final static TestAbbreviation ABBREVIATION_1 = new TestAbbreviation("Full1", "Abb1"); + private final static TestAbbreviation ABBREVIATION_1_SHOW = new TestAbbreviation("Full1", "Abb1", true); + private final static TestAbbreviation ABBREVIATION_1_OTHER_SHORT_UNIQUE = new TestAbbreviation("Full1", "Abb1", "A1"); + + private final static TestAbbreviation ABBREVIATION_2 = new TestAbbreviation("Full2", "Abb2"); + private final static TestAbbreviation ABBREVIATION_2_SHOW = new TestAbbreviation("Full2", "Abb2", true); + private final static TestAbbreviation ABBREVIATION_2_OTHER_SHORT_UNIQUE = new TestAbbreviation("Full2", "Abb2", "A2"); + + // Entry + private final static TestAbbreviation ABBREVIATION_3 = new TestAbbreviation("Full3", "Abb3"); + private final static TestAbbreviation ABBREVIATION_3_OTHER_SHORT_UNIQUE = new TestAbbreviation("Full3", "Abb3", "A3"); + + // EntryEntry + private final static TestAbbreviation ABBREVIATION_4 = new TestAbbreviation("Full4", "Abb4"); + private final static TestAbbreviation ABBREVIATION_4_SHOW = new TestAbbreviation("Full4", "Abb4", true); + + // JAbRefTstEntry + private final static TestAbbreviation ABBREVIATION_5 = new TestAbbreviation("Full5", "Abb5"); + private final static TestAbbreviation ABBREVIATION_5_SHOW = new TestAbbreviation("Full5", "Abb5", true); + + // SomeOtherEntry + private final static TestAbbreviation ABBREVIATION_6 = new TestAbbreviation("Full6", "Abb6"); + private final static TestAbbreviation ABBREVIATION_6_SHOW = new TestAbbreviation("Full6", "Abb6", true); + + private static String csvListOfAbbreviations(List testAbbreviations) { + return testAbbreviations.stream() + .map(TestAbbreviation::toString) + .collect(Collectors.joining("\n")); + } + + private static String csvListOfAbbreviations(TestAbbreviation... testAbbreviations) { + return csvListOfAbbreviations(Arrays.stream(testAbbreviations).toList()); + } + + private record CsvFileNameAndContent(String fileName, String content) { + CsvFileNameAndContent(String fileName, TestAbbreviation... testAbbreviations) { + this(fileName, csvListOfAbbreviations(testAbbreviations)); + } + } + + private record TestData( + List csvFiles, + boolean containsDuplicates, + List finalContentsOfFile2, + List finalContentsOfFile3, + TestAbbreviation abbreviationToCheck + ) { + /** + * Note that we have a **different** ordering at the constructor, because Java generics have "type erasure" + */ + public TestData( + boolean containsDuplicates, + List csvFiles, + List finalContentsOfFile2, + List finalContentsOfFile3, + TestAbbreviation abbreviationToCheck + ) { + this(csvFiles, + containsDuplicates, + finalContentsOfFile2.stream().map(TestAbbreviation::toString).toList(), + finalContentsOfFile3.stream().map(TestAbbreviation::toString).toList(), + abbreviationToCheck + ); + } + } + + public static Stream provideTestFiles() { + // filenameing: testfileXY, where X is the number of test (count starts at 1), and Y is the number of the file in the test (count starts at 0) + // testfile_3 has 5 entries after de-duplication return Stream.of( // No shortest unique abbreviations in files // Shortest unique abbreviations in entries (being the same then the abbreviation) - Arguments.of( - List.of(List.of("testFile0_1Entries.csv", "Test Entry;TE\n"), - List.of("testFile1_3Entries.csv", "Abbreviations;Abb\nTest Entry;TE\nMoreEntries;ME\n"), - List.of("testFile2_4Entries.csv", "Abbreviations;Abb\nTest Entry;TE\nMoreEntries;ME\nEntry;E\n"), - // contains duplicate entry "Test Entry" (without shortest unique "T") - List.of("testFile3_5Entries.csv", "Abbreviations;Abb\nTest Entry;TE\nTest Entry;TE\nMoreEntries;ME\nEntryEntry;EE\n")), + new TestData( true, List.of( - List.of("Abbreviations;Abb;Abb", "Test Entry;TE;TE", "MoreEntries;ME;ME", "JabRefTestEntry;JTE;JTE"), - List.of("EntryEntry;EE;EE", "Abbreviations;Abb;Abb", "Test Entry;TE;TE", "SomeOtherEntry;SOE;SOE")), - new Abbreviation("Test Entry", "TE", "TE")), + new CsvFileNameAndContent("testFile10.csv", ABBREVIATION_1), + new CsvFileNameAndContent("testFile11.csv", ABBREVIATION_0, ABBREVIATION_1, ABBREVIATION_2), + new CsvFileNameAndContent("testFile12.csv", ABBREVIATION_0, ABBREVIATION_1, ABBREVIATION_2, ABBREVIATION_3), + new CsvFileNameAndContent("testFile13.csv", ABBREVIATION_0, ABBREVIATION_1, ABBREVIATION_1, ABBREVIATION_2, ABBREVIATION_3, ABBREVIATION_4)), + List.of(ABBREVIATION_0, ABBREVIATION_1, ABBREVIATION_2, ABBREVIATION_5), + List.of(ABBREVIATION_0, ABBREVIATION_1, ABBREVIATION_2, ABBREVIATION_3, ABBREVIATION_6), + ABBREVIATION_1_SHOW), // Shortest unique abbreviations - Arguments.of( - List.of(List.of("testFile0_1Entries.csv", "Test Entry;TE;T\n"), - List.of("testFile1_3Entries.csv", "Abbreviations;Abb;A\nTest Entry;TE;T\nMoreEntries;ME;M\n"), - List.of("testFile2_4Entries.csv", "Abbreviations;Abb;A\nTest Entry;TE;T\nMoreEntries;ME;M\nEntry;En;E\n"), - // contains duplicate entry "Test Entry" (with shortest unique "T") - List.of("testFile3_5Entries.csv", "Abbreviations;Abb;A\nTest Entry;TE;T\nTest Entry;TE;T\nMoreEntries;ME;M\nEntryEntry;EE\n")), + new TestData( true, List.of( - List.of("Abbreviations;Abb;A", "Test Entry;TE;T", "MoreEntries;ME;M", "JabRefTestEntry;JTE;JTE"), - List.of("EntryEntry;EE;EE", "Abbreviations;Abb;A", "Test Entry;TE;T", "SomeOtherEntry;SOE;SOE")), - new Abbreviation("Test Entry", "TE", "T")), + new CsvFileNameAndContent("testFile20.csv", ABBREVIATION_1_OTHER_SHORT_UNIQUE), + new CsvFileNameAndContent("testFile21.csv", ABBREVIATION_0_OTHER_SHORT_UNIQUE, ABBREVIATION_1_OTHER_SHORT_UNIQUE, ABBREVIATION_2_OTHER_SHORT_UNIQUE), + new CsvFileNameAndContent("testFile22.csv", ABBREVIATION_0_OTHER_SHORT_UNIQUE, ABBREVIATION_1_OTHER_SHORT_UNIQUE, ABBREVIATION_2_OTHER_SHORT_UNIQUE, ABBREVIATION_3_OTHER_SHORT_UNIQUE), + // contains duplicate entry ABBREVIATION_1_OTHER_SHORT_UNIQUE, therefore 6 entries + new CsvFileNameAndContent("testFile23.csv", ABBREVIATION_0_OTHER_SHORT_UNIQUE, ABBREVIATION_1_OTHER_SHORT_UNIQUE, ABBREVIATION_1_OTHER_SHORT_UNIQUE, ABBREVIATION_2_OTHER_SHORT_UNIQUE, ABBREVIATION_3, ABBREVIATION_4)), + List.of(ABBREVIATION_0_OTHER_SHORT_UNIQUE, ABBREVIATION_1_OTHER_SHORT_UNIQUE, ABBREVIATION_2_OTHER_SHORT_UNIQUE, ABBREVIATION_5), + // without duplicates + List.of(ABBREVIATION_0_OTHER_SHORT_UNIQUE, ABBREVIATION_1_OTHER_SHORT_UNIQUE, ABBREVIATION_2_OTHER_SHORT_UNIQUE, ABBREVIATION_3, ABBREVIATION_6), + ABBREVIATION_1_OTHER_SHORT_UNIQUE), // Mixed abbreviations (some have shortest unique, some have not) - Arguments.of( - List.of(List.of("testFile0_1Entries.csv", "Test Entry;TE\n"), - List.of("testFile1_3Entries.csv", "Abbreviations;Abb;A\nTest Entry;TE\nMoreEntries;ME;M\n"), - // Here, "Test Entry" has "T" as shortest unique abbreviation (and not "TE" anymore) - List.of("testFile2_4Entries.csv", "Abbreviations;Abb\nTest Entry;TE;T\nMoreEntries;ME;M\nEntry;E\n"), - // contains "Test Entry" in two variants - List.of("testFile3_5Entries.csv", "Abbreviations;Abb\nTest Entry;TE;T\nTest Entry;TE\nMoreEntries;ME;M\nEntryEntry;EE\n")), + new TestData( false, List.of( - // Entries of testFile4 plus "JabRefTestEntry" - List.of("Abbreviations;Abb;Abb", "Test Entry;TE;T", "MoreEntries;ME;M", "JabRefTestEntry;JTE;JTE"), - // Entries of testFile5 plus "SomeOtherEntry" minus MoreEntries minus EntryEntry - List.of("EntryEntry;EE;EE", "Abbreviations;Abb;Abb", "Test Entry;TE;T", "Test Entry;TE", "SomeOtherEntry;SOE;SOE")), - new Abbreviation("Test Entry", "TE", "T")) - - ); + new CsvFileNameAndContent("testFile30.csv", ABBREVIATION_1), + new CsvFileNameAndContent("testFile31.csv", ABBREVIATION_0_OTHER_SHORT_UNIQUE, ABBREVIATION_1, ABBREVIATION_2_OTHER_SHORT_UNIQUE), + new CsvFileNameAndContent("testFile32.csv", ABBREVIATION_0, ABBREVIATION_1_OTHER_SHORT_UNIQUE, ABBREVIATION_2_OTHER_SHORT_UNIQUE, ABBREVIATION_3), + // contains ABBREVIATION_1 in two variants, therefore 5 in total + new CsvFileNameAndContent("testFile33.csv", ABBREVIATION_0, ABBREVIATION_1_OTHER_SHORT_UNIQUE, ABBREVIATION_1, ABBREVIATION_2_OTHER_SHORT_UNIQUE, ABBREVIATION_4)), + // Entries of testFile2 plus ABBREVIATION_5_SHOW minus ABBREVIATION_3 + List.of(ABBREVIATION_0, ABBREVIATION_1_OTHER_SHORT_UNIQUE, ABBREVIATION_2_OTHER_SHORT_UNIQUE, ABBREVIATION_5), + // Entries of testFile3 plus ABBREVIATION_6_SHOW minus ABBREVIATION_4 + List.of(ABBREVIATION_0, ABBREVIATION_1_OTHER_SHORT_UNIQUE, ABBREVIATION_1, ABBREVIATION_2_OTHER_SHORT_UNIQUE, ABBREVIATION_6), + ABBREVIATION_1_OTHER_SHORT_UNIQUE) + ); } @BeforeEach @@ -110,7 +207,7 @@ void setUpViewModel(@TempDir Path tempFolder) throws Exception { TaskExecutor taskExecutor = new CurrentThreadTaskExecutor(); viewModel = new JournalAbbreviationsTabViewModel(preferencesService, dialogService, taskExecutor, repository); - emptyTestFile = createTestFile("emptyTestFile.csv", ""); + emptyTestFile = createTestFile(new CsvFileNameAndContent("emptyTestFile.csv", "")); } @Test @@ -121,19 +218,19 @@ void testInitialHasNoFilesAndNoAbbreviations() { @ParameterizedTest @MethodSource("provideTestFiles") - void testInitialWithSavedFilesIncrementsFilesCounter(List> testFiles) throws IOException { - addFourTestFileToViewModelAndPreferences(testFiles); + void testInitialWithSavedFilesIncrementsFilesCounter(TestData testData) throws IOException { + addFourTestFileToViewModelAndPreferences(testData); assertEquals(4, viewModel.journalFilesProperty().size()); } @ParameterizedTest @MethodSource("provideTestFiles") - void testRemoveDuplicatesWhenReadingFiles(List> testFiles) throws IOException { - addFourTestFileToViewModelAndPreferences(testFiles); + void testRemoveDuplicatesWhenReadingFiles(TestData testData) throws IOException { + addFourTestFileToViewModelAndPreferences(testData); viewModel.selectLastJournalFile(); assertEquals(4, viewModel.journalFilesProperty().size()); - assertEquals(4, viewModel.abbreviationsProperty().size()); + assertEquals(5, viewModel.abbreviationsProperty().size()); } @Test @@ -147,8 +244,8 @@ void addFileIncreasesCounterOfOpenFilesAndHasNoAbbreviations() { @ParameterizedTest @MethodSource("provideTestFiles") - void addDuplicatedFileResultsInErrorDialog(List> testFiles) throws IOException { - when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testFiles.get(0)))); + void addDuplicatedFileResultsInErrorDialog(TestData testData) throws IOException { + when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testData.csvFiles.get(0)))); viewModel.addNewFile(); viewModel.addNewFile(); verify(dialogService).showErrorDialogAndWait(anyString(), anyString()); @@ -156,8 +253,8 @@ void addDuplicatedFileResultsInErrorDialog(List> testFiles) throws @ParameterizedTest @MethodSource("provideTestFiles") - void testOpenDuplicatedFileResultsInAnException(List> testFiles) throws IOException { - when(dialogService.showFileOpenDialog(any())).thenReturn(Optional.of(createTestFile(testFiles.get(0)))); + void testOpenDuplicatedFileResultsInAnException(TestData testData) throws IOException { + when(dialogService.showFileOpenDialog(any())).thenReturn(Optional.of(createTestFile(testData.csvFiles.get(0)))); viewModel.openFile(); viewModel.openFile(); verify(dialogService).showErrorDialogAndWait(anyString(), anyString()); @@ -165,13 +262,13 @@ void testOpenDuplicatedFileResultsInAnException(List> testFiles) th @ParameterizedTest @MethodSource("provideTestFiles") - void testSelectLastJournalFileSwitchesFilesAndTheirAbbreviations(List> testFiles) throws IOException { + void testSelectLastJournalFileSwitchesFilesAndTheirAbbreviations(TestData testData) throws IOException { when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(emptyTestFile)); viewModel.addNewFile(); viewModel.selectLastJournalFile(); assertEquals(0, viewModel.abbreviationsCountProperty().get()); - when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testFiles.get(0)))); + when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testData.csvFiles.get(0)))); viewModel.addNewFile(); viewModel.selectLastJournalFile(); assertEquals(1, viewModel.abbreviationsCountProperty().get()); @@ -179,21 +276,21 @@ void testSelectLastJournalFileSwitchesFilesAndTheirAbbreviations(List> testFiles, boolean containsDuplicate, List> entries, Abbreviation abbreviationToContain) throws IOException { - when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testFiles.get(2)))); + void testOpenValidFileContainsTheSpecificEntryAndEnoughAbbreviations(TestData testData) throws IOException { + when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testData.csvFiles.get(2)))); viewModel.addNewFile(); viewModel.selectLastJournalFile(); assertEquals(1, viewModel.journalFilesProperty().size()); assertEquals(4, viewModel.abbreviationsProperty().size()); - assertTrue(viewModel.abbreviationsProperty().contains(new AbbreviationViewModel(abbreviationToContain))); + assertTrue(viewModel.abbreviationsProperty().contains(new AbbreviationViewModel(testData.abbreviationToCheck))); } @ParameterizedTest @MethodSource("provideTestFiles") - void testRemoveLastListSetsCurrentFileAndCurrentAbbreviationToNull(List> testFiles) throws IOException { - when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testFiles.get(0)))); + void testRemoveLastListSetsCurrentFileAndCurrentAbbreviationToNull(TestData testData) throws IOException { + when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testData.csvFiles.get(0)))); viewModel.addNewFile(); viewModel.removeCurrentFile(); @@ -205,11 +302,11 @@ void testRemoveLastListSetsCurrentFileAndCurrentAbbreviationToNull(List> testFiles, boolean containsDuplicate, List> entries, Abbreviation abbreviationToContain) throws IOException { + void testMixedFileUsage(TestData testData) throws IOException { // simulate open file button twice - when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testFiles.get(1)))); + when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testData.csvFiles.get(1)))); viewModel.addNewFile(); - when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testFiles.get(2)))); + when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testData.csvFiles.get(2)))); viewModel.addNewFile(); viewModel.currentFileProperty().set(viewModel.journalFilesProperty().get(1)); @@ -220,7 +317,7 @@ void testMixedFileUsage(List> testFiles, boolean containsDuplicate, assertEquals(4, viewModel.abbreviationsProperty().size()); // check "arbitrary" abbreviation - assertTrue(viewModel.abbreviationsProperty().contains(new AbbreviationViewModel(abbreviationToContain))); + assertTrue(viewModel.abbreviationsProperty().contains(new AbbreviationViewModel(testData.abbreviationToCheck))); // simulate add new file button when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(emptyTestFile)); @@ -233,19 +330,19 @@ void testMixedFileUsage(List> testFiles, boolean containsDuplicate, // a new file has zero abbreviations assertEquals(0, viewModel.abbreviationsProperty().size()); - // simulate open file button - when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testFiles.get(3)))); + // simulate opening of testFile_3 + when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testData.csvFiles.get(3)))); viewModel.addNewFile(); viewModel.currentFileProperty().set(viewModel.journalFilesProperty().get(3)); - // size of the list of journal files should be incremented by one + // size of the list of journal files should have been incremented by one assertEquals(4, viewModel.journalFilesProperty().size()); - // Fourth file has 5 entries, but "sometimes" has a duplicate entry - assertEquals(containsDuplicate ? 4 : 5, viewModel.abbreviationsProperty().size()); + // after de-duplication + assertEquals(5, viewModel.abbreviationsProperty().size()); // check "arbitrary" abbreviation - assertTrue(viewModel.abbreviationsProperty().contains(new AbbreviationViewModel(abbreviationToContain))); + assertTrue(viewModel.abbreviationsProperty().contains(new AbbreviationViewModel(testData.abbreviationToCheck))); } @Test @@ -264,8 +361,8 @@ void testBuiltInListsIncludeAllBuiltInAbbreviations() { @ParameterizedTest @MethodSource("provideTestFiles") - void testCurrentFilePropertyChangeActiveFile(List> testFiles) throws IOException { - for (List testFile : testFiles) { + void testCurrentFilePropertyChangeActiveFile(TestData testData) throws IOException { + for (CsvFileNameAndContent testFile : testData.csvFiles) { when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testFile))); viewModel.addNewFile(); } @@ -277,7 +374,7 @@ void testCurrentFilePropertyChangeActiveFile(List> testFiles) throw AbbreviationsFileViewModel test5 = viewModel.journalFilesProperty().get(3); // test if the last opened file is active, but duplicated entry has been removed - assertEquals(4, viewModel.abbreviationsProperty().size()); + assertEquals(5, viewModel.abbreviationsProperty().size()); viewModel.currentFileProperty().set(test1); @@ -291,28 +388,28 @@ void testCurrentFilePropertyChangeActiveFile(List> testFiles) throw viewModel.currentFileProperty().set(test4); assertEquals(4, viewModel.abbreviationsProperty().size()); viewModel.currentFileProperty().set(test5); - assertEquals(4, viewModel.abbreviationsProperty().size()); + assertEquals(5, viewModel.abbreviationsProperty().size()); } @ParameterizedTest @MethodSource("provideTestFiles") - void testAddAbbreviationIncludesAbbreviationsInAbbreviationList(List> testFiles) throws IOException { - when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testFiles.get(2)))); + void testAddAbbreviationIncludesAbbreviationsInAbbreviationList(TestData testData) throws IOException { + when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testData.csvFiles.get(2)))); viewModel.addNewFile(); - when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testFiles.get(3)))); + when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testData.csvFiles.get(3)))); viewModel.addNewFile(); viewModel.selectLastJournalFile(); Abbreviation testAbbreviation = new Abbreviation("YetAnotherEntry", "YAE"); viewModel.addAbbreviation(testAbbreviation); - assertEquals(5, viewModel.abbreviationsProperty().size()); + assertEquals(6, viewModel.abbreviationsProperty().size()); assertTrue(viewModel.abbreviationsProperty().contains(new AbbreviationViewModel(testAbbreviation))); } @ParameterizedTest @MethodSource("provideTestFiles") - void testAddDuplicatedAbbreviationResultsInException(List> testFiles) throws IOException { - when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testFiles.get(1)))); + void testAddDuplicatedAbbreviationResultsInException(TestData testData) throws IOException { + when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testData.csvFiles.get(1)))); viewModel.addNewFile(); viewModel.selectLastJournalFile(); viewModel.addAbbreviation(new Abbreviation("YetAnotherEntry", "YAE")); @@ -334,17 +431,17 @@ void testEditSameAbbreviationWithNoChangeDoesNotResultInException() { @ParameterizedTest @MethodSource("provideTestFiles") - void testEditAbbreviationIncludesNewAbbreviationInAbbreviationsList(List> testFiles) throws IOException { - when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testFiles.get(2)))); + void testEditAbbreviationIncludesNewAbbreviationInAbbreviationsList(TestData testData) throws IOException { + when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testData.csvFiles.get(2)))); viewModel.addNewFile(); - when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testFiles.get(3)))); + when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testData.csvFiles.get(3)))); viewModel.addNewFile(); viewModel.selectLastJournalFile(); selectLastAbbreviation(); Abbreviation testAbbreviation = new Abbreviation("YetAnotherEntry", "YAE"); viewModel.addAbbreviation(testAbbreviation); - assertEquals(5, viewModel.abbreviationsProperty().size()); + assertEquals(6, viewModel.abbreviationsProperty().size()); assertTrue(viewModel.abbreviationsProperty().contains(new AbbreviationViewModel(testAbbreviation))); when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(emptyTestFile)); @@ -358,8 +455,8 @@ void testEditAbbreviationIncludesNewAbbreviationInAbbreviationsList(List> testFiles) throws IOException { - when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testFiles.get(1)))); + void testEditAbbreviationToExistingOneResultsInException(TestData testData) throws IOException { + when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testData.csvFiles.get(1)))); viewModel.addNewFile(); viewModel.selectLastJournalFile(); selectLastAbbreviation(); @@ -374,8 +471,8 @@ void testEditAbbreviationToExistingOneResultsInException(List> test @ParameterizedTest @MethodSource("provideTestFiles") - void testEditAbbreviationToEmptyNameResultsInException(List> testFiles) throws IOException { - when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testFiles.get(1)))); + void testEditAbbreviationToEmptyNameResultsInException(TestData testData) throws IOException { + when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testData.csvFiles.get(1)))); viewModel.addNewFile(); viewModel.selectLastJournalFile(); selectLastAbbreviation(); @@ -388,8 +485,8 @@ void testEditAbbreviationToEmptyNameResultsInException(List> testFi @ParameterizedTest @MethodSource("provideTestFiles") - void testEditAbbreviationToEmptyAbbreviationResultsInException(List> testFiles) throws IOException { - when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testFiles.get(1)))); + void testEditAbbreviationToEmptyAbbreviationResultsInException(TestData testData) throws IOException { + when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testData.csvFiles.get(1)))); viewModel.addNewFile(); viewModel.selectLastJournalFile(); selectLastAbbreviation(); @@ -402,44 +499,45 @@ void testEditAbbreviationToEmptyAbbreviationResultsInException(List @ParameterizedTest @MethodSource("provideTestFiles") - void testSaveAbbreviationsToFilesCreatesNewFilesWithWrittenAbbreviations(List> testFiles, boolean containsDuplicate, List> testEntries) throws Exception { - Path testFile4Entries = createTestFile(testFiles.get(2)); - Path testFile5EntriesWithDuplicate = createTestFile(testFiles.get(3)); - - when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(testFile4Entries)); + void testSaveAbbreviationsToFilesCreatesNewFilesWithWrittenAbbreviations(TestData testData) throws Exception { + Path testFile2 = createTestFile(testData.csvFiles.get(2)); + when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(testFile2)); viewModel.addNewFile(); viewModel.selectLastJournalFile(); selectLastAbbreviation(); - Abbreviation testAbbreviation = new Abbreviation("JabRefTestEntry", "JTE"); - viewModel.editAbbreviation(testAbbreviation); - + viewModel.editAbbreviation(ABBREVIATION_5); assertEquals(4, viewModel.abbreviationsProperty().size()); - assertTrue(viewModel.abbreviationsProperty().contains(new AbbreviationViewModel(testAbbreviation))); + assertTrue(viewModel.abbreviationsProperty().contains(new AbbreviationViewModel(ABBREVIATION_5))); - when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(testFile5EntriesWithDuplicate)); + Path testFile3 = createTestFile(testData.csvFiles.get(3)); + when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(testFile3)); viewModel.addNewFile(); + assertEquals(5, viewModel.abbreviationsProperty().size()); + viewModel.selectLastJournalFile(); + assertTrue(viewModel.currentFileProperty().get().getAbsolutePath().get().getFileName().toString().endsWith("3.csv")); selectLastAbbreviation(); viewModel.deleteAbbreviation(); - Abbreviation testAbbreviation1 = new Abbreviation("SomeOtherEntry", "SOE"); - viewModel.addAbbreviation(testAbbreviation1); + viewModel.addAbbreviation(ABBREVIATION_6); - assertEquals(4, viewModel.abbreviationsProperty().size()); - assertTrue(viewModel.abbreviationsProperty().contains(new AbbreviationViewModel(testAbbreviation1))); + // deletion and addition of an entry leads to same size + assertEquals(5, viewModel.abbreviationsProperty().size()); + assertTrue(viewModel.abbreviationsProperty().contains(new AbbreviationViewModel(ABBREVIATION_6))); + // overwrite all files viewModel.saveJournalAbbreviationFiles(); - List actual = Files.readAllLines(testFile4Entries, StandardCharsets.UTF_8); - assertEquals(testEntries.get(0), actual); + List actual = Files.readAllLines(testFile2, StandardCharsets.UTF_8); + assertEquals(testData.finalContentsOfFile2, actual); - actual = Files.readAllLines(testFile5EntriesWithDuplicate, StandardCharsets.UTF_8); - assertEquals(testEntries.get(1), actual); + actual = Files.readAllLines(testFile3, StandardCharsets.UTF_8); + assertEquals(testData.finalContentsOfFile3, actual); } @ParameterizedTest @MethodSource("provideTestFiles") - void testSaveExternalFilesListToPreferences(List> testFiles) throws IOException { - addFourTestFileToViewModelAndPreferences(testFiles); + void testSaveExternalFilesListToPreferences(TestData testData) throws IOException { + addFourTestFileToViewModelAndPreferences(testData); verify(preferencesService).storeJournalAbbreviationPreferences(any()); } @@ -451,21 +549,17 @@ private void selectLastAbbreviation() { .set(viewModel.abbreviationsProperty().get(viewModel.abbreviationsCountProperty().get() - 1)); } - private void addFourTestFileToViewModelAndPreferences(List> testFiles) throws IOException { - for (List testFile : testFiles) { - when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(testFile.get(0), testFile.get(1)))); + private void addFourTestFileToViewModelAndPreferences(TestData testData) throws IOException { + for (CsvFileNameAndContent csvFile : testData.csvFiles) { + when(dialogService.showFileSaveDialog(any())).thenReturn(Optional.of(createTestFile(csvFile))); viewModel.addNewFile(); } viewModel.storeSettings(); } - private Path createTestFile(String name, String content) throws IOException { - Path file = this.tempFolder.resolve(name); - Files.writeString(file, content); + private Path createTestFile(CsvFileNameAndContent testFile) throws IOException { + Path file = this.tempFolder.resolve(testFile.fileName); + Files.writeString(file, testFile.content); return file; } - - private Path createTestFile(List testFile) throws IOException { - return createTestFile(testFile.get(0), testFile.get(1)); - } } diff --git a/src/test/java/org/jabref/logic/journals/AbbreviationWriterTest.java b/src/test/java/org/jabref/logic/journals/AbbreviationWriterTest.java new file mode 100644 index 00000000000..c16e24b23ab --- /dev/null +++ b/src/test/java/org/jabref/logic/journals/AbbreviationWriterTest.java @@ -0,0 +1,36 @@ +package org.jabref.logic.journals; + +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import org.junit.jupiter.api.parallel.Execution; +import org.junit.jupiter.api.parallel.ExecutionMode; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +@Execution(ExecutionMode.CONCURRENT) +class AbbreviationWriterTest { + + @Test + void shortestUniqueAbbreviationWrittenIfItDiffers(@TempDir Path tempDir) throws Exception { + Abbreviation abbreviation = new Abbreviation("Full", "Abbr", "A"); + Path csvFile = tempDir.resolve("test.csv"); + AbbreviationWriter.writeOrCreate( + csvFile, + List.of(abbreviation)); + assertEquals(List.of("Full;Abbr;A"), Files.readAllLines(csvFile)); + } + + @Test + void doNotWriteShortestUniqueAbbreviationWrittenIfItDiffers(@TempDir Path tempDir) throws Exception { + Abbreviation abbreviation = new Abbreviation("Full", "Abbr"); + Path csvFile = tempDir.resolve("test.csv"); + AbbreviationWriter.writeOrCreate( + csvFile, + List.of(abbreviation)); + assertEquals(List.of("Full;Abbr"), Files.readAllLines(csvFile)); + } +} From 3007daf9d95c108612c06d22e34d45fae653e858 Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Tue, 3 Jan 2023 23:53:39 +0100 Subject: [PATCH 07/10] Fix checkstyle (and remove non-used variables) --- .../JournalAbbreviationsViewModelTabTest.java | 47 ++++++++----------- 1 file changed, 19 insertions(+), 28 deletions(-) diff --git a/src/test/java/org/jabref/gui/preferences/journals/JournalAbbreviationsViewModelTabTest.java b/src/test/java/org/jabref/gui/preferences/journals/JournalAbbreviationsViewModelTabTest.java index 750a94b4688..5f52159559a 100644 --- a/src/test/java/org/jabref/gui/preferences/journals/JournalAbbreviationsViewModelTabTest.java +++ b/src/test/java/org/jabref/gui/preferences/journals/JournalAbbreviationsViewModelTabTest.java @@ -43,6 +43,25 @@ @Execution(ExecutionMode.CONCURRENT) class JournalAbbreviationsViewModelTabTest { + private final static TestAbbreviation ABBREVIATION_0 = new TestAbbreviation("Full0", "Abb0"); + private final static TestAbbreviation ABBREVIATION_0_OTHER_SHORT_UNIQUE = new TestAbbreviation("Full0", "Abb0", "A0"); + + private final static TestAbbreviation ABBREVIATION_1 = new TestAbbreviation("Full1", "Abb1"); + private final static TestAbbreviation ABBREVIATION_1_SHOW = new TestAbbreviation("Full1", "Abb1", true); + private final static TestAbbreviation ABBREVIATION_1_OTHER_SHORT_UNIQUE = new TestAbbreviation("Full1", "Abb1", "A1"); + + private final static TestAbbreviation ABBREVIATION_2 = new TestAbbreviation("Full2", "Abb2"); + private final static TestAbbreviation ABBREVIATION_2_OTHER_SHORT_UNIQUE = new TestAbbreviation("Full2", "Abb2", "A2"); + + private final static TestAbbreviation ABBREVIATION_3 = new TestAbbreviation("Full3", "Abb3"); + private final static TestAbbreviation ABBREVIATION_3_OTHER_SHORT_UNIQUE = new TestAbbreviation("Full3", "Abb3", "A3"); + + private final static TestAbbreviation ABBREVIATION_4 = new TestAbbreviation("Full4", "Abb4"); + + private final static TestAbbreviation ABBREVIATION_5 = new TestAbbreviation("Full5", "Abb5"); + + private final static TestAbbreviation ABBREVIATION_6 = new TestAbbreviation("Full6", "Abb6"); + private JournalAbbreviationsTabViewModel viewModel; private Path emptyTestFile; private Path tempFolder; @@ -77,34 +96,6 @@ public String toString() { } } - private final static TestAbbreviation ABBREVIATION_0 = new TestAbbreviation("Full0", "Abb0"); - private final static TestAbbreviation ABBREVIATION_0_SHOW = new TestAbbreviation("Full0", "Abb0", true); - private final static TestAbbreviation ABBREVIATION_0_OTHER_SHORT_UNIQUE = new TestAbbreviation("Full0", "Abb0", "A0"); - - private final static TestAbbreviation ABBREVIATION_1 = new TestAbbreviation("Full1", "Abb1"); - private final static TestAbbreviation ABBREVIATION_1_SHOW = new TestAbbreviation("Full1", "Abb1", true); - private final static TestAbbreviation ABBREVIATION_1_OTHER_SHORT_UNIQUE = new TestAbbreviation("Full1", "Abb1", "A1"); - - private final static TestAbbreviation ABBREVIATION_2 = new TestAbbreviation("Full2", "Abb2"); - private final static TestAbbreviation ABBREVIATION_2_SHOW = new TestAbbreviation("Full2", "Abb2", true); - private final static TestAbbreviation ABBREVIATION_2_OTHER_SHORT_UNIQUE = new TestAbbreviation("Full2", "Abb2", "A2"); - - // Entry - private final static TestAbbreviation ABBREVIATION_3 = new TestAbbreviation("Full3", "Abb3"); - private final static TestAbbreviation ABBREVIATION_3_OTHER_SHORT_UNIQUE = new TestAbbreviation("Full3", "Abb3", "A3"); - - // EntryEntry - private final static TestAbbreviation ABBREVIATION_4 = new TestAbbreviation("Full4", "Abb4"); - private final static TestAbbreviation ABBREVIATION_4_SHOW = new TestAbbreviation("Full4", "Abb4", true); - - // JAbRefTstEntry - private final static TestAbbreviation ABBREVIATION_5 = new TestAbbreviation("Full5", "Abb5"); - private final static TestAbbreviation ABBREVIATION_5_SHOW = new TestAbbreviation("Full5", "Abb5", true); - - // SomeOtherEntry - private final static TestAbbreviation ABBREVIATION_6 = new TestAbbreviation("Full6", "Abb6"); - private final static TestAbbreviation ABBREVIATION_6_SHOW = new TestAbbreviation("Full6", "Abb6", true); - private static String csvListOfAbbreviations(List testAbbreviations) { return testAbbreviations.stream() .map(TestAbbreviation::toString) From 3cb1cc2f281f62ebbbd35d30c8b422ec22ffed53 Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Wed, 4 Jan 2023 00:05:13 +0100 Subject: [PATCH 08/10] Fix typo --- .../gui/preferences/journals/AbbreviationsFileViewModel.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/jabref/gui/preferences/journals/AbbreviationsFileViewModel.java b/src/main/java/org/jabref/gui/preferences/journals/AbbreviationsFileViewModel.java index a38e02d6c36..c1e8424d45c 100644 --- a/src/main/java/org/jabref/gui/preferences/journals/AbbreviationsFileViewModel.java +++ b/src/main/java/org/jabref/gui/preferences/journals/AbbreviationsFileViewModel.java @@ -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 its 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 abbreviations, String name) { From 4379530344876938a139e91cc9b2155ae814dc56 Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Wed, 4 Jan 2023 01:13:55 +0100 Subject: [PATCH 09/10] Remove obsolete variable "containsDuplicates" --- .../JournalAbbreviationsViewModelTabTest.java | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/src/test/java/org/jabref/gui/preferences/journals/JournalAbbreviationsViewModelTabTest.java b/src/test/java/org/jabref/gui/preferences/journals/JournalAbbreviationsViewModelTabTest.java index 5f52159559a..10e87331d04 100644 --- a/src/test/java/org/jabref/gui/preferences/journals/JournalAbbreviationsViewModelTabTest.java +++ b/src/test/java/org/jabref/gui/preferences/journals/JournalAbbreviationsViewModelTabTest.java @@ -114,27 +114,23 @@ private record CsvFileNameAndContent(String fileName, String content) { private record TestData( List csvFiles, - boolean containsDuplicates, + TestAbbreviation abbreviationToCheck, List finalContentsOfFile2, - List finalContentsOfFile3, - TestAbbreviation abbreviationToCheck + List finalContentsOfFile3 ) { /** * Note that we have a **different** ordering at the constructor, because Java generics have "type erasure" */ public TestData( - boolean containsDuplicates, List csvFiles, List finalContentsOfFile2, List finalContentsOfFile3, TestAbbreviation abbreviationToCheck ) { this(csvFiles, - containsDuplicates, + abbreviationToCheck, finalContentsOfFile2.stream().map(TestAbbreviation::toString).toList(), - finalContentsOfFile3.stream().map(TestAbbreviation::toString).toList(), - abbreviationToCheck - ); + finalContentsOfFile3.stream().map(TestAbbreviation::toString).toList()); } } @@ -145,7 +141,6 @@ public static Stream provideTestFiles() { // No shortest unique abbreviations in files // Shortest unique abbreviations in entries (being the same then the abbreviation) new TestData( - true, List.of( new CsvFileNameAndContent("testFile10.csv", ABBREVIATION_1), new CsvFileNameAndContent("testFile11.csv", ABBREVIATION_0, ABBREVIATION_1, ABBREVIATION_2), @@ -157,7 +152,6 @@ public static Stream provideTestFiles() { // Shortest unique abbreviations new TestData( - true, List.of( new CsvFileNameAndContent("testFile20.csv", ABBREVIATION_1_OTHER_SHORT_UNIQUE), new CsvFileNameAndContent("testFile21.csv", ABBREVIATION_0_OTHER_SHORT_UNIQUE, ABBREVIATION_1_OTHER_SHORT_UNIQUE, ABBREVIATION_2_OTHER_SHORT_UNIQUE), @@ -171,7 +165,6 @@ public static Stream provideTestFiles() { // Mixed abbreviations (some have shortest unique, some have not) new TestData( - false, List.of( new CsvFileNameAndContent("testFile30.csv", ABBREVIATION_1), new CsvFileNameAndContent("testFile31.csv", ABBREVIATION_0_OTHER_SHORT_UNIQUE, ABBREVIATION_1, ABBREVIATION_2_OTHER_SHORT_UNIQUE), From b993c4a0990a36016010102ca4a52b657198b9d5 Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Wed, 4 Jan 2023 14:54:18 +0100 Subject: [PATCH 10/10] Switch to LinkedHashSet --- .../logic/journals/AbbreviationParser.java | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/src/main/java/org/jabref/logic/journals/AbbreviationParser.java b/src/main/java/org/jabref/logic/journals/AbbreviationParser.java index 22665018a4b..9a9f67f0bfe 100644 --- a/src/main/java/org/jabref/logic/journals/AbbreviationParser.java +++ b/src/main/java/org/jabref/logic/journals/AbbreviationParser.java @@ -10,10 +10,7 @@ import java.nio.file.Files; import java.nio.file.Path; import java.util.Collection; -import java.util.HashSet; -import java.util.LinkedList; -import java.util.List; -import java.util.Set; +import java.util.LinkedHashSet; import org.apache.commons.csv.CSVParser; import org.apache.commons.csv.CSVRecord; @@ -27,11 +24,8 @@ public class AbbreviationParser { private static final Logger LOGGER = LoggerFactory.getLogger(AbbreviationParser.class); - // List to ensure ordering - private final List abbreviations = new LinkedList<>(); - - // We need to check duplicates fast (!) - private final Set readAbbreviations = new HashSet<>(); + // Ensures ordering while preventing duplicates + private final LinkedHashSet abbreviations = new LinkedHashSet<>(); void readJournalListFromResource(String resourceFileName) { try (InputStream stream = JournalAbbreviationRepository.class.getResourceAsStream(resourceFileName); @@ -71,10 +65,7 @@ private void readJournalList(Reader reader) throws IOException { } Abbreviation abbreviationToAdd = new Abbreviation(name, abbreviation, shortestUniqueAbbreviation); - if (!readAbbreviations.contains(abbreviationToAdd)) { - readAbbreviations.add(abbreviationToAdd); - abbreviations.add(abbreviationToAdd); - } + abbreviations.add(abbreviationToAdd); } } }