Improve code of BibDataBaseDiff (#11355)

* Streamline code Co-authored-by: Carl Christian Snethlage <[email protected]> * Enable debug output * Even more debug * convert to rrecord, adjust hashcode with equals * fix record conversion * Discard changes to src/main/resources/tinylog.properties --------- Co-authored-by: Carl Christian Snethlage <[email protected]> Co-authored-by: Siedlerchr <[email protected]>
JabRef · Jun 17, 2024 · 5c8eb42 · 5c8eb42
1 parent 77c5188
commit 5c8eb42
Show file tree

Hide file tree

Showing 9 changed files with 83 additions and 49 deletions.
diff --git a/src/main/java/org/jabref/gui/collab/DatabaseChange.java b/src/main/java/org/jabref/gui/collab/DatabaseChange.java
@@ -50,7 +50,7 @@ public void setAccepted(boolean accepted) {
 
     /**
      * Convenience method for accepting changes
-     * */
+     */
     public void accept() {
         setAccepted(true);
     }

diff --git a/src/main/java/org/jabref/gui/collab/DatabaseChangeList.java b/src/main/java/org/jabref/gui/collab/DatabaseChangeList.java
@@ -65,14 +65,14 @@ private static DatabaseChange createBibStringDiff(BibDatabaseContext originalDat
     }
 
     private static DatabaseChange createBibEntryDiff(BibDatabaseContext originalDatabase, DatabaseChangeResolverFactory databaseChangeResolverFactory, BibEntryDiff diff) {
-        if (diff.getOriginalEntry() == null) {
-            return new EntryAdd(diff.getNewEntry(), originalDatabase, databaseChangeResolverFactory);
+        if (diff.originalEntry() == null) {
+            return new EntryAdd(diff.newEntry(), originalDatabase, databaseChangeResolverFactory);
         }
 
-        if (diff.getNewEntry() == null) {
-            return new EntryDelete(diff.getOriginalEntry(), originalDatabase, databaseChangeResolverFactory);
+        if (diff.newEntry() == null) {
+            return new EntryDelete(diff.originalEntry(), originalDatabase, databaseChangeResolverFactory);
         }
 
-        return new EntryChange(diff.getOriginalEntry(), diff.getNewEntry(), originalDatabase, databaseChangeResolverFactory);
+        return new EntryChange(diff.originalEntry(), diff.newEntry(), originalDatabase, databaseChangeResolverFactory);
     }
 }
diff --git a/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java b/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java
@@ -35,6 +35,8 @@ public class DatabaseChangeMonitor implements FileUpdateListener {
     private final TaskExecutor taskExecutor;
     private final DialogService dialogService;
     private final PreferencesService preferencesService;
+    private final LibraryTab.DatabaseNotification notificationPane;
+    private final StateManager stateManager;
     private LibraryTab saveState;
 
     public DatabaseChangeMonitor(BibDatabaseContext database,
@@ -49,6 +51,8 @@ public DatabaseChangeMonitor(BibDatabaseContext database,
         this.taskExecutor = taskExecutor;
         this.dialogService = dialogService;
         this.preferencesService = preferencesService;
+        this.notificationPane = notificationPane;
+        this.stateManager = stateManager;
 
         this.listeners = new ArrayList<>();
 
@@ -60,7 +64,12 @@ public DatabaseChangeMonitor(BibDatabaseContext database,
             }
         });
 
-        addListener(changes -> notificationPane.notify(
+        addListener(this::notifyOnChange);
+    }
+
+    private void notifyOnChange(List<DatabaseChange> changes) {
+        // The changes come from {@link org.jabref.gui.collab.DatabaseChangeList.compareAndGetChanges}
+        notificationPane.notify(
                 IconTheme.JabRefIcons.SAVE.getGraphicNode(),
                 Localization.lang("The library has been modified by another program."),
                 List.of(new Action(Localization.lang("Dismiss changes"), event -> notificationPane.hide()),
@@ -82,7 +91,7 @@ public DatabaseChangeMonitor(BibDatabaseContext database,
                             }
                             notificationPane.hide();
                         })),
-                Duration.ZERO));
+                Duration.ZERO);
     }
 
     @Override

diff --git a/src/main/java/org/jabref/logic/bibtex/comparator/BibDatabaseDiff.java b/src/main/java/org/jabref/logic/bibtex/comparator/BibDatabaseDiff.java
@@ -13,30 +13,50 @@
 import org.jabref.model.entry.BibEntryTypesManager;
 import org.jabref.model.entry.field.StandardField;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 public class BibDatabaseDiff {
 
+    private static final Logger LOGGER = LoggerFactory.getLogger(BibDatabaseDiff.class);
+
     private static final double MATCH_THRESHOLD = 0.4;
     private final Optional<MetaDataDiff> metaDataDiff;
     private final Optional<PreambleDiff> preambleDiff;
     private final List<BibStringDiff> bibStringDiffs;
     private final List<BibEntryDiff> entryDiffs;
 
-    private BibDatabaseDiff(BibDatabaseContext originalDatabase, BibDatabaseContext newDatabase, boolean includeEmptyEntries) {
+    private BibDatabaseDiff(BibDatabaseContext originalDatabase, BibDatabaseContext newDatabase) {
         metaDataDiff = MetaDataDiff.compare(originalDatabase.getMetaData(), newDatabase.getMetaData());
         preambleDiff = PreambleDiff.compare(originalDatabase, newDatabase);
         bibStringDiffs = BibStringDiff.compare(originalDatabase.getDatabase(), newDatabase.getDatabase());
+        entryDiffs = getBibEntryDiffs(originalDatabase, newDatabase);
+        if (LOGGER.isDebugEnabled() && !isEmpty()) {
+            LOGGER.debug("Differences detected");
+            metaDataDiff.ifPresent(diff -> LOGGER.debug("Metadata differences: {}", diff));
+            preambleDiff.ifPresent(diff -> LOGGER.debug("Premble differences: {}", diff));
+            LOGGER.debug("BibString differences: {}", bibStringDiffs);
+            LOGGER.debug("Entry differences: {}", entryDiffs);
+        }
+    }
 
+    private boolean isEmpty() {
+        return !metaDataDiff.isPresent() && !preambleDiff.isPresent() && bibStringDiffs.isEmpty() && entryDiffs.isEmpty();
+    }
+
+    private List<BibEntryDiff> getBibEntryDiffs(BibDatabaseContext originalDatabase, BibDatabaseContext newDatabase) {
+        final List<BibEntryDiff> entryDiffs;
         // Sort both databases according to a common sort key.
         EntryComparator comparator = getEntryComparator();
         List<BibEntry> originalEntriesSorted = originalDatabase.getDatabase().getEntriesSorted(comparator);
         List<BibEntry> newEntriesSorted = newDatabase.getDatabase().getEntriesSorted(comparator);
 
-        if (!includeEmptyEntries) {
-            originalEntriesSorted.removeIf(BibEntry::isEmpty);
-            newEntriesSorted.removeIf(BibEntry::isEmpty);
-        }
+        // Ignore empty entries
+        originalEntriesSorted.removeIf(BibEntry::isEmpty);
+        newEntriesSorted.removeIf(BibEntry::isEmpty);
 
         entryDiffs = compareEntries(originalEntriesSorted, newEntriesSorted, originalDatabase.getMode());
+        return entryDiffs;
     }
 
     private static EntryComparator getEntryComparator() {
@@ -56,7 +76,7 @@ private static List<BibEntryDiff> compareEntries(List<BibEntry> originalEntries,
 
         // Create a HashSet where we can put references to entries in the new
         // database that we have matched. This is to avoid matching them twice.
-        Set<Integer> used = new HashSet<>(newEntries.size());
+        Set<Integer> matchedEntries = new HashSet<>(newEntries.size());
         Set<BibEntry> notMatched = new HashSet<>(originalEntries.size());
 
         // Loop through the entries of the original database, looking for exact matches in the new one.
@@ -65,10 +85,10 @@ private static List<BibEntryDiff> compareEntries(List<BibEntry> originalEntries,
         mainLoop:
         for (BibEntry originalEntry : originalEntries) {
             for (int i = 0; i < newEntries.size(); i++) {
-                if (!used.contains(i)) {
+                if (!matchedEntries.contains(i)) {
                     double score = DuplicateCheck.compareEntriesStrictly(originalEntry, newEntries.get(i));
                     if (score > 1) {
-                        used.add(i);
+                        matchedEntries.add(i);
                         continue mainLoop;
                     }
                 }
@@ -85,7 +105,7 @@ private static List<BibEntryDiff> compareEntries(List<BibEntry> originalEntries,
             double bestMatch = 0;
             int bestMatchIndex = 0;
             for (int i = 0; i < newEntries.size(); i++) {
-                if (!used.contains(i)) {
+                if (!matchedEntries.contains(i)) {
                     double score = DuplicateCheck.compareEntriesStrictly(originalEntry, newEntries.get(i));
                     if (score > bestMatch) {
                         bestMatch = score;
@@ -97,7 +117,7 @@ private static List<BibEntryDiff> compareEntries(List<BibEntry> originalEntries,
             if (bestMatch > MATCH_THRESHOLD
                     || hasEqualCitationKey(originalEntry, bestEntry)
                     || duplicateCheck.isDuplicate(originalEntry, bestEntry, mode)) {
-                used.add(bestMatchIndex);
+                matchedEntries.add(bestMatchIndex);
                 differences.add(new BibEntryDiff(originalEntry, newEntries.get(bestMatchIndex)));
             } else {
                 differences.add(new BibEntryDiff(originalEntry, null));
@@ -106,7 +126,7 @@ private static List<BibEntryDiff> compareEntries(List<BibEntry> originalEntries,
 
         // Finally, look if there are still untouched entries in the new database. These may have been added.
         for (int i = 0; i < newEntries.size(); i++) {
-            if (!used.contains(i)) {
+            if (!matchedEntries.contains(i)) {
                 differences.add(new BibEntryDiff(null, newEntries.get(i)));
             }
         }
@@ -119,7 +139,7 @@ private static boolean hasEqualCitationKey(BibEntry oneEntry, BibEntry twoEntry)
     }
 
     public static BibDatabaseDiff compare(BibDatabaseContext base, BibDatabaseContext changed) {
-        return new BibDatabaseDiff(base, changed, false);
+        return new BibDatabaseDiff(base, changed);
     }
 
     public Optional<MetaDataDiff> getMetaDataDifferences() {

diff --git a/src/main/java/org/jabref/logic/bibtex/comparator/BibEntryDiff.java b/src/main/java/org/jabref/logic/bibtex/comparator/BibEntryDiff.java
@@ -1,21 +1,18 @@
 package org.jabref.logic.bibtex.comparator;
 
-import org.jabref.model.entry.BibEntry;
-
-public class BibEntryDiff {
-    private final BibEntry originalEntry;
-    private final BibEntry newEntry;
+import java.util.StringJoiner;
 
-    public BibEntryDiff(BibEntry originalEntry, BibEntry newEntry) {
-        this.originalEntry = originalEntry;
-        this.newEntry = newEntry;
-    }
+import org.jabref.model.entry.BibEntry;
 
-    public BibEntry getOriginalEntry() {
-        return originalEntry;
-    }
+public record BibEntryDiff(
+        BibEntry originalEntry,
+        BibEntry newEntry) {
 
-    public BibEntry getNewEntry() {
-        return newEntry;
+    @Override
+    public String toString() {
+        return new StringJoiner(",\n", BibEntryDiff.class.getSimpleName() + "[", "]")
+                .add("originalEntry=" + originalEntry)
+                .add("newEntry=" + newEntry)
+                .toString();
     }
 }
diff --git a/src/main/java/org/jabref/logic/bibtex/comparator/MetaDataDiff.java b/src/main/java/org/jabref/logic/bibtex/comparator/MetaDataDiff.java
@@ -75,8 +75,7 @@ private void addToListIfDiff(List<Difference> changes, DifferenceType difference
                 if (isDefaultContentSelectors(originalContentSelectors) && isDefaultContentSelectors(newContentSelectors)) {
                     return;
                 }
-            }
-            if (differenceType == DifferenceType.GROUPS) {
+            } else if (differenceType == DifferenceType.GROUPS) {
                 Optional<GroupTreeNode> originalGroups = (Optional<GroupTreeNode>) originalObject;
                 Optional<GroupTreeNode> newGroups = (Optional<GroupTreeNode>) newObject;
                 if (isDefaultGroup(originalGroups) && isDefaultGroup(newGroups)) {
@@ -137,6 +136,7 @@ public String toString() {
                 "groupDiff=" + groupDiff +
                 ", originalMetaData=" + originalMetaData +
                 ", newMetaData=" + getNewMetaData() +
+                ", getDifferences()=" + getDifferences(new GlobalCitationKeyPatterns(CitationKeyPattern.NULL_CITATION_KEY_PATTERN)) +
                 '}';
     }
 }
diff --git a/src/main/java/org/jabref/logic/bibtex/comparator/PreambleDiff.java b/src/main/java/org/jabref/logic/bibtex/comparator/PreambleDiff.java
@@ -50,4 +50,12 @@ public boolean equals(Object other) {
     public int hashCode() {
         return Objects.hash(originalPreamble, newPreamble);
     }
+
+    @Override
+    public String toString() {
+        return "PreambleDiff{" +
+                "originalPreamble='" + originalPreamble + '\'' +
+                ", newPreamble='" + newPreamble + '\'' +
+                '}';
+    }
 }
diff --git a/src/main/java/org/jabref/model/entry/BibEntry.java b/src/main/java/org/jabref/model/entry/BibEntry.java
@@ -942,7 +942,7 @@ public boolean equals(Object o) {
      */
     @Override
     public int hashCode() {
-        return Objects.hash(commentsBeforeEntry, type.getValue(), fields);
+        return Objects.hash(type.getValue(), fields, commentsBeforeEntry);
     }
 
     public void registerListener(Object object) {

diff --git a/src/test/java/org/jabref/logic/bibtex/comparator/BibDatabaseDiffTest.java b/src/test/java/org/jabref/logic/bibtex/comparator/BibDatabaseDiffTest.java
@@ -85,10 +85,10 @@ void compareOfTwoDifferentEntriesWithDifferentDataReportsDifferences() throws Ex
         // two different entries between the databases
         assertEquals(2, diff.getEntryDifferences().size(), "incorrect amount of different entries");
 
-        assertEquals(entryOne, diff.getEntryDifferences().getFirst().getOriginalEntry(), "there is another value as originalEntry");
-        assertNull(diff.getEntryDifferences().getFirst().getNewEntry(), "newEntry is not null");
-        assertEquals(entryTwo, diff.getEntryDifferences().get(1).getNewEntry(), "there is another value as newEntry");
-        assertNull(diff.getEntryDifferences().get(1).getOriginalEntry(), "originalEntry is not null");
+        assertEquals(entryOne, diff.getEntryDifferences().getFirst().originalEntry(), "there is another value as originalEntry");
+        assertNull(diff.getEntryDifferences().getFirst().newEntry(), "newEntry is not null");
+        assertEquals(entryTwo, diff.getEntryDifferences().get(1).newEntry(), "there is another value as newEntry");
+        assertNull(diff.getEntryDifferences().get(1).originalEntry(), "originalEntry is not null");
     }
 
     @Test
@@ -104,12 +104,12 @@ void compareOfThreeDifferentEntriesWithDifferentDataReportsDifferences() throws
         // three different entries between the databases
         assertEquals(3, diff.getEntryDifferences().size(), "incorrect amount of different entries");
 
-        assertEquals(entryOne, diff.getEntryDifferences().getFirst().getOriginalEntry(), "there is another value as originalEntry");
-        assertNull(diff.getEntryDifferences().getFirst().getNewEntry(), "newEntry is not null");
-        assertEquals(entryTwo, diff.getEntryDifferences().get(1).getNewEntry(), "there is another value as newEntry");
-        assertNull(diff.getEntryDifferences().get(1).getOriginalEntry(), "originalEntry is not null");
-        assertEquals(entryThree, diff.getEntryDifferences().get(2).getNewEntry(), "there is another value as newEntry [2]");
-        assertNull(diff.getEntryDifferences().get(2).getOriginalEntry(), "originalEntry is not null [2]");
+        assertEquals(entryOne, diff.getEntryDifferences().getFirst().originalEntry(), "there is another value as originalEntry");
+        assertNull(diff.getEntryDifferences().getFirst().newEntry(), "newEntry is not null");
+        assertEquals(entryTwo, diff.getEntryDifferences().get(1).newEntry(), "there is another value as newEntry");
+        assertNull(diff.getEntryDifferences().get(1).originalEntry(), "originalEntry is not null");
+        assertEquals(entryThree, diff.getEntryDifferences().get(2).newEntry(), "there is another value as newEntry [2]");
+        assertNull(diff.getEntryDifferences().get(2).originalEntry(), "originalEntry is not null [2]");
     }
 
     @Test
@@ -130,8 +130,8 @@ void compareOfTwoEntriesWithEqualCitationKeysShouldReportsOneDifference() {
         // two different entries between the databases
         assertEquals(1, diff.getEntryDifferences().size(), "incorrect amount of different entries");
 
-        assertEquals(entryOne, diff.getEntryDifferences().getFirst().getOriginalEntry(), "there is another value as originalEntry");
-        assertEquals(entryTwo, diff.getEntryDifferences().getFirst().getNewEntry(), "there is another value as newEntry");
+        assertEquals(entryOne, diff.getEntryDifferences().getFirst().originalEntry(), "there is another value as originalEntry");
+        assertEquals(entryTwo, diff.getEntryDifferences().getFirst().newEntry(), "there is another value as newEntry");
     }
 
     private BibDatabaseDiff compareEntries(BibEntry entryOne, BibEntry entryTwo) {
-Original file line number
+Diff line change
@@ Expand Up / @@ -50,7 +50,7 @@ public void setAccepted(boolean accepted) { @@
         /**
          * Convenience method for accepting changes
-         * */
+         */
         public void accept() {
             setAccepted(true);
         }
@@ Expand Down @@