Try to fix writing of BibTeX @Strings (JabRef#11052)

* Try to fix writing of BibTeX @Strings * Add test cases * Some debug * Make setParsedSerialization private * l18n * Add JavaDoc * Fix test * Fix test
JabRef · Mar 19, 2024 · f8f5f4e · f8f5f4e
1 parent 0aaebad
commit f8f5f4e
Show file tree

Hide file tree

Showing 9 changed files with 70 additions and 83 deletions.
diff --git a/src/main/java/org/jabref/gui/libraryproperties/constants/ConstantsPropertiesViewModel.java b/src/main/java/org/jabref/gui/libraryproperties/constants/ConstantsPropertiesViewModel.java
@@ -18,7 +18,6 @@
 import org.jabref.gui.libraryproperties.PropertiesTabViewModel;
 import org.jabref.logic.bibtex.comparator.BibtexStringComparator;
 import org.jabref.logic.help.HelpFile;
-import org.jabref.logic.util.OS;
 import org.jabref.model.database.BibDatabaseContext;
 import org.jabref.model.entry.BibtexString;
 import org.jabref.preferences.FilePreferences;
@@ -90,8 +89,6 @@ public void storeSettings() {
         List<BibtexString> strings = stringsListProperty.stream()
                                                         .map(this::fromBibtexStringViewModel)
                                                         .toList();
-        strings.forEach(string -> string.setParsedSerialization("@String{" +
-                string.getName() + " = " + string.getContent() + "}" + OS.NEWLINE));
         databaseContext.getDatabase().setStrings(strings);
     }
 

diff --git a/src/main/java/org/jabref/logic/bibtex/FieldWriter.java b/src/main/java/org/jabref/logic/bibtex/FieldWriter.java
@@ -1,6 +1,7 @@
 package org.jabref.logic.bibtex;
 
 import org.jabref.model.entry.field.Field;
+import org.jabref.model.entry.field.InternalField;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -85,7 +86,7 @@ public String write(Field field, String content) throws InvalidFieldValueExcepti
             return FIELD_START + String.valueOf(FIELD_END);
         }
 
-        if (!shouldResolveStrings(field)) {
+        if (!shouldResolveStrings(field) || field.equals(InternalField.BIBTEX_STRING)) {
             return formatWithoutResolvingStrings(content, field);
         }
 

diff --git a/src/main/java/org/jabref/logic/exporter/BibtexDatabaseWriter.java b/src/main/java/org/jabref/logic/exporter/BibtexDatabaseWriter.java
@@ -21,6 +21,9 @@
 import org.jabref.model.metadata.MetaData;
 import org.jabref.model.strings.StringUtil;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 /**
  * Writes a .bib file following the BibTeX / BibLaTeX format using the provided {@link BibWriter}.
  * Reading is done by {@link org.jabref.logic.importer.fileformat.BibtexImporter}.
@@ -29,6 +32,8 @@ public class BibtexDatabaseWriter extends BibDatabaseWriter {
 
     public static final String DATABASE_ID_PREFIX = "DBID:";
 
+    private static final Logger LOGGER = LoggerFactory.getLogger(BibtexDatabaseWriter.class);
+
     private static final String COMMENT_PREFIX = "@Comment";
     private static final String PREAMBLE_PREFIX = "@Preamble";
     private static final String STRING_PREFIX = "@String";
@@ -95,6 +100,7 @@ protected void writePreamble(String preamble) throws IOException {
     protected void writeString(BibtexString bibtexString, int maxKeyLength) throws IOException {
         // If the string has not been modified, write it back as it was
         if (!saveConfiguration.shouldReformatFile() && !bibtexString.hasChanged()) {
+            LOGGER.debug("Writing parsed serialization {}.", bibtexString.getParsedSerialization());
             bibWriter.write(bibtexString.getParsedSerialization());
             return;
         }

diff --git a/src/main/java/org/jabref/logic/importer/fileformat/BibtexParser.java b/src/main/java/org/jabref/logic/importer/fileformat/BibtexParser.java
@@ -455,11 +455,10 @@ private void parseBibDeskComment(String comment, Map<String, String> meta) throw
 
     private void parseBibtexString() throws IOException {
         BibtexString bibtexString = parseString();
-        bibtexString.setParsedSerialization(dumpTextReadSoFarToString());
         try {
             database.addString(bibtexString);
         } catch (KeyCollisionException ex) {
-            parserResult.addWarning(Localization.lang("Duplicate string name") + ": " + bibtexString.getName());
+            parserResult.addWarning(Localization.lang("Duplicate string name: '%0'", bibtexString.getName()));
         }
     }
 
@@ -669,7 +668,7 @@ private BibtexString parseString() throws IOException {
         skipOneNewline();
         LOGGER.debug("Finished string parsing.");
 
-        return new BibtexString(name, content);
+        return new BibtexString(name, content, dumpTextReadSoFarToString());
     }
 
     private String parsePreamble() throws IOException {

diff --git a/src/main/java/org/jabref/model/entry/BibtexString.java b/src/main/java/org/jabref/model/entry/BibtexString.java
@@ -79,6 +79,11 @@ public static Type get(String key) {
     private String parsedSerialization;
     private boolean hasChanged;
 
+    /**
+     * Default constructor. Use this if in doubt.
+     *
+     * In case this constructor is used - and the library is eventually written, the serialization is generated from scratch (and not some null from parsedSerialization)
+     */
     public BibtexString(String name, String content) {
         this.id = IdGenerator.next();
         this.name = name;
@@ -87,6 +92,18 @@ public BibtexString(String name, String content) {
         type = Type.get(name);
     }
 
+    /**
+     * This is used to set the parsed serialization of the string. This is used when the string is read from a BibTeX file.
+     * Do not use if not working with reading BibTeX files (or similar actions).     *
+     *
+     * @param parsedSerialization The serialization read during parsing
+     */
+    public BibtexString(String name, String content, String parsedSerialization) {
+        this(name, content);
+        this.parsedSerialization = parsedSerialization;
+        hasChanged = false;
+    }
+
     public String getId() {
         return id;
     }
@@ -125,11 +142,6 @@ public Type getType() {
         return type;
     }
 
-    public void setParsedSerialization(String parsedSerialization) {
-        this.parsedSerialization = parsedSerialization;
-        hasChanged = false;
-    }
-
     public String getParsedSerialization() {
         return parsedSerialization;
     }
@@ -156,12 +168,13 @@ public String getUserComments() {
 
     @Override
     public Object clone() {
-        BibtexString clone = new BibtexString(name, content);
-        clone.setId(id);
-        if (parsedSerialization != null) {
-            clone.setParsedSerialization(parsedSerialization);
+        BibtexString clone;
+        if (parsedSerialization == null) {
+             clone = new BibtexString(name, content);
+        } else {
+            clone = new BibtexString(name, content, parsedSerialization);
         }
-
+        clone.setId(id);
         return clone;
     }
 

diff --git a/src/main/resources/l10n/JabRef_en.properties b/src/main/resources/l10n/JabRef_en.properties
@@ -61,7 +61,7 @@ Added\ group\ "%0".=Added group "%0".
 Added\ string\:\ '%0'=Added string: '%0'
 Added\ string=Added string
 Edit\ strings=Edit strings
-Duplicate\ string\ name=Duplicate string name
+Duplicate\ string\ name\:\ '%0'=Duplicate string name: '%0'
 Modified\ string=Modified string
 Modified\ string\:\ '%0' =Modified string: '%0'
 New\ string=New string

diff --git a/...est/java/org/jabref/gui/libraryproperties/constants/ConstantsPropertiesViewModelTest.java b/...est/java/org/jabref/gui/libraryproperties/constants/ConstantsPropertiesViewModelTest.java
@@ -1,7 +1,6 @@
 package org.jabref.gui.libraryproperties.constants;
 
 import java.util.List;
-import java.util.stream.Stream;
 
 import javafx.beans.property.StringProperty;
 
@@ -11,25 +10,19 @@
 import org.jabref.model.entry.BibtexString;
 import org.jabref.preferences.FilePreferences;
 
-import org.junit.jupiter.api.BeforeEach;
-import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Test;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.mockito.Mockito.mock;
 
 class ConstantsPropertiesViewModelTest {
 
-    private DialogService service;
-    private FilePreferences filePreferences;
+    private DialogService service = mock(DialogService.class);
+    private FilePreferences filePreferences = mock(FilePreferences.class);
 
-    @BeforeEach
-    void setUp() {
-        service = mock(DialogService.class);
-        filePreferences = mock(FilePreferences.class);
-    }
-
-    @DisplayName("Check that the list of strings is sorted according to their keys")
+    /**
+     * Check that the list of strings is sorted according to their keys
+     */
     @Test
     void stringsListPropertySorting() {
         BibtexString string1 = new BibtexString("TSE", "Transactions on Software Engineering");
@@ -50,7 +43,9 @@ void stringsListPropertySorting() {
         assertEquals(expected, actual);
     }
 
-    @DisplayName("Check that the list of strings is sorted after resorting it")
+    /**
+     * Check that the list of strings is sorted after resorting it
+     */
     @Test
     void stringsListPropertyResorting() {
         BibDatabase db = new BibDatabase();
@@ -72,64 +67,19 @@ void stringsListPropertyResorting() {
         assertEquals(expected, actual);
     }
 
-   @Test
-   @DisplayName("Check that the storeSettings method store settings on the model")
-   void storeSettingsTest() {
-       // Setup
-       // create a bibdatabse
-       BibDatabase db = new BibDatabase();
-       BibDatabaseContext context = new BibDatabaseContext(db);
-       List<String> expected = List.of("KTH", "Royal Institute of Technology");
-       // initialize a constantsPropertiesViewModel
-       ConstantsPropertiesViewModel model = new ConstantsPropertiesViewModel(context, service, filePreferences);
-
-       // construct value to store in model
-       var stringsList = model.stringsListProperty();
-       stringsList.add(new ConstantsItemModel("KTH", "Royal Institute of Technology"));
-
-       // Act
-       model.storeSettings();
-
-       // Assert
-       // get the names stored
-       List<String> names = context.getDatabase().getStringValues().stream()
-                                    .map(BibtexString::getName).toList();
-       // get the content stored
-       List<String> content = context.getDatabase().getStringValues().stream()
-                                          .map(BibtexString::getContent).toList();
-
-       List<String> actual = Stream.concat(names.stream(), content.stream()).toList();
-
-       assertEquals(expected, actual);
-   }
-
     @Test
-    @DisplayName("Check that the storeSettings method can identify string constants")
     void storeSettingsWithStringConstantTest() {
-        // Setup
-        // create a bibdatabse
         BibDatabase db = new BibDatabase();
         BibDatabaseContext context = new BibDatabaseContext(db);
-        List<String> expected = List.of("@String{KTH = Royal Institute of Technology}");
-        // initialize a constantsPropertiesViewModel
+
         ConstantsPropertiesViewModel model = new ConstantsPropertiesViewModel(context, service, filePreferences);
 
-        // construct value to store in model
         var stringsList = model.stringsListProperty();
         stringsList.add(new ConstantsItemModel("KTH", "Royal Institute of Technology"));
 
-        // Act
         model.storeSettings();
 
-        // Assert
-        // get string the constants through parsedSerialization() method
-        List<String> actual = context.getDatabase().getStringValues().stream()
-                                     .map(BibtexString::getParsedSerialization).toList();
-
-        // get the first value and clean strings
-        String actual_value = actual.getFirst().replaceAll("\\s+", " ").trim();
-        String expected_value = expected.getFirst().replaceAll("\\s+", " ").trim();
-
-        assertEquals(expected_value, actual_value);
+        List<BibtexString> actual = context.getDatabase().getStringValues().stream().toList();
+        assertEquals(List.of(new BibtexString("KTH", "Royal Institute of Technology")), actual);
     }
 }
diff --git a/src/test/java/org/jabref/logic/exporter/BibtexDatabaseWriterTest.java b/src/test/java/org/jabref/logic/exporter/BibtexDatabaseWriterTest.java
@@ -324,6 +324,17 @@ void writeString() throws Exception {
         assertEquals("@String{name = {content}}" + OS.NEWLINE, stringWriter.toString());
     }
 
+    @Test
+    void writeStringWithQuotes() throws Exception {
+        String parsedSerialization = "@String{name = \"content\"}";
+        BibtexString bibtexString = new BibtexString("name", "content", parsedSerialization);
+        database.addString(bibtexString);
+
+        databaseWriter.savePartOfDatabase(bibtexContext, Collections.emptyList());
+
+        assertEquals(parsedSerialization + OS.NEWLINE, stringWriter.toString());
+    }
+
     @Test
     void writeStringAndEncoding() throws Exception {
         metaData.setEncoding(StandardCharsets.US_ASCII);
@@ -691,8 +702,7 @@ void reformatEntryIfAskedToDoSo() throws Exception {
 
     @Test
     void writeSavedSerializationOfStringIfUnchanged() throws Exception {
-        BibtexString string = new BibtexString("name", "content");
-        string.setParsedSerialization("serialization");
+        BibtexString string = new BibtexString("name", "content", "serialization");
         database.addString(string);
 
         databaseWriter.savePartOfDatabase(bibtexContext, Collections.emptyList());
@@ -702,8 +712,7 @@ void writeSavedSerializationOfStringIfUnchanged() throws Exception {
 
     @Test
     void reformatStringIfAskedToDoSo() throws Exception {
-        BibtexString string = new BibtexString("name", "content");
-        string.setParsedSerialization("wrong serialization");
+        BibtexString string = new BibtexString("name", "content", "wrong serialization");
         database.addString(string);
 
         saveConfiguration = new SelfContainedSaveConfiguration(SaveOrder.getDefaultSaveOrder(), false, BibDatabaseWriter.SaveType.WITH_JABREF_META_DATA, true);

diff --git a/src/test/java/org/jabref/logic/importer/fileformat/BibtexParserTest.java b/src/test/java/org/jabref/logic/importer/fileformat/BibtexParserTest.java
@@ -780,6 +780,18 @@ void parseRecognizesString() throws IOException {
         assertEquals("Bourdieu, Pierre", string.getContent());
     }
 
+    @Test
+    void parseRecognizesStringWithQuotes() throws IOException {
+        ParserResult result = parser
+                .parse(new StringReader("@string{bourdieu = \"Bourdieu, Pierre\"}"));
+
+        BibtexString string = result.getDatabase().getStringValues().iterator().next();
+
+        assertEquals(1, result.getDatabase().getStringCount());
+        assertEquals("bourdieu", string.getName());
+        assertEquals("Bourdieu, Pierre", string.getContent());
+    }
+
     @Test
     void parseSavesOneNewlineAfterStringInParsedSerialization() throws IOException {
         String string = "@string{bourdieu = {Bourdieu, Pierre}}" + OS.NEWLINE;