From 5c8eb42237dedc5afe5c50cedb28113de3c17dae Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Mon, 17 Jun 2024 22:27:37 +0200 Subject: [PATCH] Improve code of BibDataBaseDiff (#11355) * Streamline code Co-authored-by: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com> * 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 <50491877+calixtus@users.noreply.github.com> Co-authored-by: Siedlerchr --- .../org/jabref/gui/collab/DatabaseChange.java | 2 +- .../jabref/gui/collab/DatabaseChangeList.java | 10 ++--- .../gui/collab/DatabaseChangeMonitor.java | 13 +++++- .../bibtex/comparator/BibDatabaseDiff.java | 44 ++++++++++++++----- .../logic/bibtex/comparator/BibEntryDiff.java | 25 +++++------ .../logic/bibtex/comparator/MetaDataDiff.java | 4 +- .../logic/bibtex/comparator/PreambleDiff.java | 8 ++++ .../java/org/jabref/model/entry/BibEntry.java | 2 +- .../comparator/BibDatabaseDiffTest.java | 24 +++++----- 9 files changed, 83 insertions(+), 49 deletions(-) diff --git a/src/main/java/org/jabref/gui/collab/DatabaseChange.java b/src/main/java/org/jabref/gui/collab/DatabaseChange.java index ec28509b864..0ef63184c37 100644 --- 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 index de4f9423303..be4969d1e52 100644 --- 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 index 258ac7be392..3282e010970 100644 --- 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 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 index 70d479d45c3..9e7b5939372 100644 --- 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; private final Optional preambleDiff; private final List bibStringDiffs; private final List 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 getBibEntryDiffs(BibDatabaseContext originalDatabase, BibDatabaseContext newDatabase) { + final List entryDiffs; // Sort both databases according to a common sort key. EntryComparator comparator = getEntryComparator(); List originalEntriesSorted = originalDatabase.getDatabase().getEntriesSorted(comparator); List 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 compareEntries(List 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 used = new HashSet<>(newEntries.size()); + Set matchedEntries = new HashSet<>(newEntries.size()); Set 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 compareEntries(List 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 compareEntries(List 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 compareEntries(List 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 compareEntries(List 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 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 index bcd098a3850..0d79d4dfd97 100644 --- 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 index 2748be2ec00..be9a0fd8c0d 100644 --- 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 changes, DifferenceType difference if (isDefaultContentSelectors(originalContentSelectors) && isDefaultContentSelectors(newContentSelectors)) { return; } - } - if (differenceType == DifferenceType.GROUPS) { + } else if (differenceType == DifferenceType.GROUPS) { Optional originalGroups = (Optional) originalObject; Optional newGroups = (Optional) 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 index 2308a8d80c6..a89ab3c88ef 100644 --- 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 index 04566a98c5e..91fb8d01f51 100644 --- 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 index 8c788f5f666..2146d4247b1 100644 --- 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) {