Fix removal of special fields when syncing them via keywords (#3438)

* Move replaceKeywords to the other keyword methods When reading the code, one would expect to find all methods concerned with keywords next to each other. * Fix the removal of special fields when using keyword sync Fixes #3432 * Update Authors and Changelog * Add missing whitespace for checkstyle * Make requested changes for #3438 * Remove optional wrapper on replaceKeywords Requested change for #3438
JabRef · Nov 20, 2017 · fe07fcd · fe07fcd
1 parent 98d980e
commit fe07fcd
Show file tree

Hide file tree

Showing 6 changed files with 77 additions and 17 deletions.
diff --git a/AUTHORS b/AUTHORS
@@ -182,6 +182,7 @@ Thomas Arildsen
 Thomas Ilsche
 Thorsten Dahlheimer
 Tim van Rossum
+Tim Würtele
 Tobias Boceck
 Tobias Denkinger
 Tobias Diez

diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -48,6 +48,7 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `#
  - We fixed an issue where fetching entries from crossref that had no titles caused an error [#3376](https://github.com/JabRef/jabref/issues/3376)
  - We fixed an issue where the same Java Look and Feel would be listed more than once in the Preferences. [#3391](https://github.com/JabRef/jabref/issues/3391)
  - We fixed an issue where errors in citation styles triggered an exception when opening the preferences dialog [#3389](https://github.com/JabRef/jabref/issues/3389)
+ - We fixed an issue where special fields (such as `printed`) could not be cleared when syncing special fields via the keywords [#3432](https://github.com/JabRef/jabref/issues/3432)
 
 
  ### Removed

diff --git a/src/main/java/org/jabref/logic/specialfields/SpecialFieldsUtils.java b/src/main/java/org/jabref/logic/specialfields/SpecialFieldsUtils.java
@@ -44,11 +44,11 @@ public static List<FieldChange> updateField(SpecialField field, String value, Bi
     private static List<FieldChange> exportFieldToKeywords(SpecialField specialField, BibEntry entry, Character keywordDelimiter) {
         List<FieldChange> fieldChanges = new ArrayList<>();
 
-        Optional<Keyword> newValue = entry.getField(specialField.getFieldName()).map(Keyword::new);
         KeywordList keyWords = specialField.getKeyWords();
-
-        Optional<FieldChange> change = entry.replaceKeywords(keyWords, newValue, keywordDelimiter);
-        change.ifPresent(changeValue -> fieldChanges.add(changeValue));
+        Optional<Keyword> newValue = entry.getField(specialField.getFieldName()).map(Keyword::new);
+        newValue.map(value -> entry.replaceKeywords(keyWords, newValue.get(), keywordDelimiter))
+                .orElseGet(() -> entry.removeKeywords(keyWords, keywordDelimiter))
+                .ifPresent(changeValue -> fieldChanges.add(changeValue));
 
         return fieldChanges;
     }
@@ -59,7 +59,7 @@ private static List<FieldChange> exportFieldToKeywords(SpecialField specialField
     public static List<FieldChange> syncKeywordsFromSpecialFields(BibEntry entry, Character keywordDelimiter) {
         List<FieldChange> fieldChanges = new ArrayList<>();
 
-        for (SpecialField field: SpecialField.values()) {
+        for (SpecialField field : SpecialField.values()) {
             fieldChanges.addAll(SpecialFieldsUtils.exportFieldToKeywords(field, entry, keywordDelimiter));
         }
 
@@ -96,7 +96,7 @@ public static List<FieldChange> syncSpecialFieldsFromKeywords(BibEntry entry, Ch
 
         KeywordList keywordList = entry.getKeywords(keywordDelimiter);
 
-        for (SpecialField field: SpecialField.values()) {
+        for (SpecialField field : SpecialField.values()) {
             fieldChanges.addAll(SpecialFieldsUtils.importKeywordsForField(keywordList, field, entry));
         }
 
@@ -113,7 +113,7 @@ public static void synchronizeSpecialFields(KeywordList keywordsToAdd, KeywordLi
 
         // Priority
         clone = keywordsToAdd.createClone();
-        for (SpecialField field: SpecialField.values()) {
+        for (SpecialField field : SpecialField.values()) {
             clone.retainAll(field.getKeyWords());
             if (!clone.isEmpty()) {
                 keywordsToRemove.addAll(field.getKeyWords());

diff --git a/src/main/java/org/jabref/model/entry/BibEntry.java b/src/main/java/org/jabref/model/entry/BibEntry.java
@@ -105,16 +105,6 @@ public Optional<FieldChange> setMonth(Month parsedMonth) {
         return setField(FieldName.MONTH, parsedMonth.getJabRefFormat());
     }
 
-    public Optional<FieldChange> replaceKeywords(KeywordList keywordsToReplace, Optional<Keyword> newValue,
-            Character keywordDelimiter) {
-        KeywordList keywordList = getKeywords(keywordDelimiter);
-        if (newValue.isPresent()) {
-            keywordList.replaceAll(keywordsToReplace, newValue.get());
-        }
-
-        return putKeywords(keywordList, keywordDelimiter);
-    }
-
     /**
      * Returns the text stored in the given field of the given bibtex entry
      * which belongs to the given database.
@@ -688,6 +678,20 @@ public KeywordList getResolvedKeywords(Character delimiter, BibDatabase database
         return KeywordList.parse(keywordsContent, delimiter);
     }
 
+    public Optional<FieldChange> removeKeywords(KeywordList keywordsToRemove, Character keywordDelimiter) {
+        KeywordList keywordList = getKeywords(keywordDelimiter);
+        keywordList.removeAll(keywordsToRemove);
+        return putKeywords(keywordList, keywordDelimiter);
+    }
+
+    public Optional<FieldChange> replaceKeywords(KeywordList keywordsToReplace, Keyword newValue,
+                                                 Character keywordDelimiter) {
+        KeywordList keywordList = getKeywords(keywordDelimiter);
+        keywordList.replaceAll(keywordsToReplace, newValue);
+
+        return putKeywords(keywordList, keywordDelimiter);
+    }
+
     public Collection<String> getFieldValues() {
         return fields.values();
     }

diff --git a/src/test/java/org/jabref/logic/specialfields/SpecialFieldsUtilsTest.java b/src/test/java/org/jabref/logic/specialfields/SpecialFieldsUtilsTest.java
@@ -1,11 +1,16 @@
 package org.jabref.logic.specialfields;
 
+import java.util.Arrays;
 import java.util.List;
 import java.util.Optional;
 
 import org.jabref.model.FieldChange;
 import org.jabref.model.entry.BibEntry;
 
+import org.jabref.model.entry.FieldName;
+import org.jabref.model.entry.Keyword;
+import org.jabref.model.entry.KeywordList;
+import org.jabref.model.entry.specialfields.SpecialField;
 import org.junit.Test;
 
 import static org.junit.Assert.assertEquals;
@@ -69,4 +74,19 @@ public void syncSpecialFieldsFromKeywordCausesNoChangeWhenKeywordsAreEmpty() {
         List<FieldChange> changes = SpecialFieldsUtils.syncSpecialFieldsFromKeywords(entry, ',');
         assertFalse(changes.size() > 0);
     }
+
+    @Test
+    public void updateFieldRemovesSpecialFieldKeywordWhenKeywordSyncIsUsed() {
+        BibEntry entry = new BibEntry();
+        SpecialField specialField = SpecialField.PRINTED;
+        Keyword specialFieldKeyword = specialField.getKeyWords().get(0);
+        // Add the special field
+        SpecialFieldsUtils.updateField(specialField, specialFieldKeyword.get(), entry, true, true, ',');
+        // Remove it
+        List<FieldChange> changes = SpecialFieldsUtils.updateField(specialField, specialFieldKeyword.get(), entry, true, true, ',');
+        assertEquals(Arrays.asList(new FieldChange(entry, specialField.getFieldName(), specialFieldKeyword.get(), null),
+                new FieldChange(entry, FieldName.KEYWORDS, specialFieldKeyword.get(), null)), changes);
+        KeywordList remainingKeywords = entry.getKeywords(',');
+        assertFalse(remainingKeywords.contains(specialFieldKeyword));
+    }
 }
diff --git a/src/test/java/org/jabref/model/entry/BibEntryTests.java b/src/test/java/org/jabref/model/entry/BibEntryTests.java
@@ -6,13 +6,17 @@
 import java.util.List;
 import java.util.Optional;
 
+import com.google.common.collect.Sets;
 import org.jabref.model.FieldChange;
 
+import org.jabref.model.entry.specialfields.SpecialField;
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
 
 public class BibEntryTests {
 
@@ -369,6 +373,36 @@ public void getKeywordsReturnsParsedKeywordListFromKeywordsField() {
         assertEquals(new KeywordList("w1", "w2a w2b", "w3"), entry.getKeywords(','));
     }
 
+    @Test
+    public void removeKeywordsOnEntryWithoutKeywordsDoesNothing() {
+        BibEntry entry = new BibEntry();
+        Optional<FieldChange> change = entry.removeKeywords(SpecialField.RANKING.getKeyWords(), ',');
+        assertEquals(Optional.empty(), change);
+    }
+
+    @Test
+    public void removeKeywordsWithEmptyListDoesNothing() {
+        keywordEntry.putKeywords(Arrays.asList("kw1", "kw2"), ',');
+        Optional<FieldChange> change = keywordEntry.removeKeywords(new KeywordList(), ',');
+        assertEquals(Optional.empty(), change);
+    }
+
+    @Test
+    public void removeKeywordsWithNonExistingKeywordsDoesNothing() {
+        keywordEntry.putKeywords(Arrays.asList("kw1", "kw2"), ',');
+        Optional<FieldChange> change = keywordEntry.removeKeywords(KeywordList.parse("kw3, kw4", ','), ',');
+        assertEquals(Optional.empty(), change);
+        assertEquals(Sets.newHashSet("kw1", "kw2"), keywordEntry.getKeywords(',').toStringList());
+    }
+
+    @Test
+    public void removeKeywordsWithExistingKeywordsRemovesThem() {
+        keywordEntry.putKeywords(Arrays.asList("kw1", "kw2", "kw3"), ',');
+        Optional<FieldChange> change = keywordEntry.removeKeywords(KeywordList.parse("kw1, kw2", ','), ',');
+        assertTrue(change.isPresent());
+        assertEquals(KeywordList.parse("kw3", ','), keywordEntry.getKeywords(','));
+    }
+
     @Test
     public void testGroupAndSearchHits() {
         BibEntry be = new BibEntry();