From b39eb17aea69ffcb28d2323d8ddec70743bcb4db Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Tue, 1 Sep 2026 22:32:56 +0200 Subject: [PATCH 01/27] Synchronize local libraries with their files External changes to a library file are merged into the open library without asking; the review dialog only appears for an item that was changed differently in memory and on disk. The autosave preference is relabeled accordingly, since both directions together keep library and file the same. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01GWmXyN3GUNhfF9fSo1UNrt --- docs/requirements/ux.md | 12 + .../org/jabref/gui/collab/ChangeScanner.java | 5 + .../gui/collab/DatabaseChangeMonitor.java | 52 +++- .../jabref/gui/collab/LibraryBaseline.java | 267 ++++++++++++++++++ .../gui/collab/entrychange/EntryChange.java | 25 +- .../gui/preferences/general/GeneralTab.java | 2 +- .../gui/collab/LibraryBaselineTest.java | 199 +++++++++++++ jablib/src/main/java/module-info.java | 1 + .../main/resources/l10n/JabRef_en.properties | 3 +- 9 files changed, 559 insertions(+), 7 deletions(-) create mode 100644 jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java create mode 100644 jabgui/src/test/java/org/jabref/gui/collab/LibraryBaselineTest.java diff --git a/docs/requirements/ux.md b/docs/requirements/ux.md index 949c4d5ec8ac..646cbe24ccb3 100644 --- a/docs/requirements/ux.md +++ b/docs/requirements/ux.md @@ -118,7 +118,19 @@ Since inspecting a library file means parsing it completely, the inspection is s Needs: impl +## Synchronizing a library with its file +`req~ux.external-library-changes.synchronize~1` + +While a local library is set to be synchronized with its file (the former "autosave" preference), external changes to the file must be merged into the in-memory library without asking, and unsaved in-memory changes must not be reported as external changes. +A review is only required for an item (entry field, entry type, metadata, preamble, string) that was changed differently in memory and in the file, or that was deleted on one side and changed on the other. +Fields changed only in the file are taken over even when other fields of the same entry were changed in memory. + +Needs: impl + +## Committing a library that is not under version control ## Committing a library that is not under version control +||||||| parent of 9b7de4f9fc (Synchronize local libraries with their files) +### Committing a library that is not under version control `req~ux.git-commit.initialize-repository~1` When a user commits a library that is not inside a Git repository, JabRef must offer to initialize a repository in the library's directory and commit the library file there. diff --git a/jabgui/src/main/java/org/jabref/gui/collab/ChangeScanner.java b/jabgui/src/main/java/org/jabref/gui/collab/ChangeScanner.java index f93f146b9da4..3a77904cafbb 100644 --- a/jabgui/src/main/java/org/jabref/gui/collab/ChangeScanner.java +++ b/jabgui/src/main/java/org/jabref/gui/collab/ChangeScanner.java @@ -46,6 +46,11 @@ public List scanForChanges() { } } + /// @return the external changes sorted by the side they happened on, see [LibraryBaseline#triage] + public LibraryBaseline.Triage scanForChanges(LibraryBaseline baseline) { + return baseline.triage(scanForChanges(), database, databaseChangeResolverFactory); + } + public List getDatabaseChanges(Path fileToCompare) throws IOException { ImportFormatPreferences importFormatPreferences = preferences.getImportFormatPreferences(); ParserResult result = OpenDatabase.loadDatabase(fileToCompare, importFormatPreferences, new DummyFileUpdateMonitor()); diff --git a/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java b/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java index 0a512d999eb6..5dc240fa4f10 100644 --- a/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java +++ b/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java @@ -12,6 +12,7 @@ import org.jabref.gui.StateManager; import org.jabref.gui.preferences.GuiPreferences; import org.jabref.logic.l10n.Localization; +import org.jabref.logic.shared.DatabaseLocation; import org.jabref.logic.undo.UndoManager; import org.jabref.logic.util.BackgroundTask; import org.jabref.logic.util.TaskExecutor; @@ -47,6 +48,11 @@ public class DatabaseChangeMonitor implements FileUpdateListener { /// `synchronized (database)`; `null` when unknown, in which case the next event triggers a full scan. @Nullable private FileSnapshot knownDiskState; + /// The library as of the last point where it was known to match the disk, for telling external changes apart + /// from unsaved in-memory edits. Guarded like [#knownDiskState]; `null` while synchronizing is off, in which case + /// every external change is offered for review. + @Nullable private LibraryBaseline baseline; + public DatabaseChangeMonitor(BibDatabaseContext database, FileUpdateMonitor fileMonitor, TaskExecutor taskExecutor, @@ -69,6 +75,7 @@ public DatabaseChangeMonitor(BibDatabaseContext database, monitoredPath.ifPresent(path -> { knownDiskState = FileSnapshot.read(path); + baseline = captureBaseline(); try { fileMonitor.addListenerForFile(path, this); } catch (IOException e) { @@ -167,7 +174,21 @@ public void markConsistentWithDisk(@Nullable FileSnapshot diskState) { } else { monitoredPath.ifPresent(path -> knownDiskState = FileSnapshot.read(path)); } + baseline = captureBaseline(); + } + } + + /// Synchronizing (silently merging external changes) is tied to the autosave preference: both together keep a + /// local library and its file the same in both directions. + private boolean isSynchronizing() { + return database.getLocation() == DatabaseLocation.LOCAL && preferences.getLibraryPreferences().shouldAutoSave(); + } + + private @Nullable LibraryBaseline captureBaseline() { + if (!isSynchronizing()) { + return null; } + return LibraryBaseline.of(database, preferences.getCitationKeyPatternPreferences().getKeyPatterns()); } /// A full scan parses the whole library file, so it is skipped when size and modification time show that the file @@ -185,7 +206,16 @@ private void scanIfFileChanged() { /// there are such changes. private void scanForChanges() { ChangeScanner scanner = new ChangeScanner(database, dialogService, preferences, stateManager); - BackgroundTask.wrap(scanner::scanForChanges) + LibraryBaseline scannedBaseline = baseline; + if (scannedBaseline != null && isSynchronizing()) { + // [impl->req~ux.external-library-changes.synchronize~1] + BackgroundTask.wrap(() -> scanner.scanForChanges(scannedBaseline)) + .onSuccess(triage -> synchronize(scannedBaseline, triage)) + .onFailure(e -> LOGGER.error("Error while synchronizing with the library file", e)) + .executeWith(taskExecutor); + return; + } + BackgroundTask.wrap(() -> scanner.scanForChanges()) .onSuccess(changes -> { if (!changes.isEmpty()) { listeners.forEach(listener -> listener.databaseChanged(changes)); @@ -195,6 +225,26 @@ private void scanForChanges() { .executeWith(taskExecutor); } + /// Applies what changed on disk only, and offers the review for what changed on both sides. + private void synchronize(LibraryBaseline scannedBaseline, LibraryBaseline.Triage triage) { + List unresolved = new ArrayList<>(triage.bothSides()); + unresolved.addAll(triage.memoryOnly()); + if (!triage.diskOnly().isEmpty()) { + applyResolvedChanges(triage.diskOnly(), unresolved.isEmpty() && !libraryTab.isModified()); + dialogService.notify(Localization.lang("Merged %0 change(s) from the library file", String.valueOf(triage.diskOnly().size()))); + } + synchronized (database) { + LibraryBaseline updated = captureBaseline(); + if (updated != null) { + updated.keepUnresolved(scannedBaseline, unresolved); + } + baseline = updated; + } + if (!triage.bothSides().isEmpty()) { + listeners.forEach(listener -> listener.databaseChanged(triage.bothSides())); + } + } + /// Applies the accepted external changes and updates the library's dirty state. /// /// @param resolvedChanges the externally resolved changes to apply to the in-memory database diff --git a/jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java b/jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java new file mode 100644 index 000000000000..8de73db1039f --- /dev/null +++ b/jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java @@ -0,0 +1,267 @@ +package org.jabref.gui.collab; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; +import java.util.function.BiPredicate; + +import org.jabref.gui.collab.entryadd.EntryAdd; +import org.jabref.gui.collab.entrychange.EntryChange; +import org.jabref.gui.collab.entrydelete.EntryDelete; +import org.jabref.gui.collab.groupchange.GroupChange; +import org.jabref.gui.collab.metedatachange.MetadataChange; +import org.jabref.gui.collab.preamblechange.PreambleChange; +import org.jabref.gui.collab.stringadd.BibTexStringAdd; +import org.jabref.gui.collab.stringchange.BibTexStringChange; +import org.jabref.gui.collab.stringdelete.BibTexStringDelete; +import org.jabref.gui.collab.stringrename.BibTexStringRename; +import org.jabref.logic.citationkeypattern.GlobalCitationKeyPatterns; +import org.jabref.logic.exporter.MetaDataSerializer; +import org.jabref.logic.git.merge.planning.util.ConflictRules; +import org.jabref.logic.git.merge.planning.util.FieldPatchComputer; +import org.jabref.model.database.BibDatabaseContext; +import org.jabref.model.entry.BibEntry; +import org.jabref.model.entry.BibtexString; +import org.jabref.model.metadata.MetaData; + +import org.jspecify.annotations.Nullable; + +/// The library as it last matched its file on disk. Comparing both the in-memory library and the file against it tells +/// for each external change whether the same item was also modified in memory, which is the only case that needs a +/// review; everything else is merged silently (see [#triage]). +/// +/// Entries are keyed by the id of the in-memory entry they were copied from, so that a citation key change in memory +/// does not break the association. The metadata is kept in its serialized form, which is the only way to snapshot it. +public final class LibraryBaseline { + + /// Outcome of comparing external changes against the baseline. + /// + /// @param diskOnly changes to items untouched in memory; already accepted, to be applied without asking + /// @param bothSides changes to items that were modified in memory as well, in a way that cannot be merged automatically; need a review + /// @param memoryOnly not external changes at all: differences caused by unsaved in-memory edits; to be dropped + public record Triage(List diskOnly, List bothSides, List memoryOnly) { + } + + private enum Side { DISK, MEMORY, BOTH } + + private final Map entriesById; + private final Map metaData; + private @Nullable String preamble; + private final Map strings; + private final GlobalCitationKeyPatterns citationKeyPatterns; + + private LibraryBaseline(Map entriesById, Map metaData, @Nullable String preamble, Map strings, GlobalCitationKeyPatterns citationKeyPatterns) { + this.entriesById = entriesById; + this.metaData = metaData; + this.preamble = preamble; + this.strings = strings; + this.citationKeyPatterns = citationKeyPatterns; + } + + public static LibraryBaseline of(BibDatabaseContext context, GlobalCitationKeyPatterns citationKeyPatterns) { + Map entries = new HashMap<>(); + for (BibEntry entry : context.getDatabase().getEntries()) { + entries.put(entry.getId(), new BibEntry(entry)); + } + Map strings = new HashMap<>(); + for (BibtexString string : context.getDatabase().getStringValues()) { + strings.put(string.getName(), string.getContent()); + } + return new LibraryBaseline(entries, + serialize(context.getMetaData(), citationKeyPatterns), + context.getDatabase().getPreamble().orElse(null), + strings, + citationKeyPatterns); + } + + /// Sorts the differences between the in-memory library and its file, as computed by + /// [DatabaseChangeList#compareAndGetChanges], by which side changed. Entries modified on both sides in different + /// fields are merged field by field (with the Git merge rules) into a new, accepted [EntryChange]. + public Triage triage(List changes, BibDatabaseContext local, @Nullable DatabaseChangeResolverFactory resolverFactory) { + Triage triage = new Triage(new ArrayList<>(), new ArrayList<>(), new ArrayList<>()); + // A group change is always accompanied by the metadata change it is part of, which precedes it in the list + Side metaDataSide = Side.BOTH; + for (DatabaseChange change : pairSplitEntries(changes, local, resolverFactory)) { + Side side = switch (change) { + case EntryChange entryChange -> { + BibEntry base = entriesById.get(entryChange.getOldEntry().getId()); + Side entrySide = sideOf(base, entryChange.getOldEntry(), entryChange.getNewEntry(), LibraryBaseline::sameContent); + if (entrySide == Side.BOTH) { + Optional merged = mergeFields(base, entryChange.getOldEntry(), entryChange.getNewEntry()); + if (merged.isPresent()) { + change = new EntryChange(entryChange.getOldEntry(), merged.get(), local, resolverFactory); + entrySide = Side.DISK; + } + } + yield entrySide; + } + // No in-memory counterpart: either new on disk, or deleted in memory (and possibly modified on disk) + case EntryAdd entryAdd -> + find(entryAdd.getAddedEntry()).map(base -> sameContent(base, entryAdd.getAddedEntry()) ? Side.MEMORY : Side.BOTH) + .orElse(Side.DISK); + // No counterpart on disk: either deleted on disk, or added in memory (and possibly modified on disk) + case EntryDelete entryDelete -> { + BibEntry base = entriesById.get(entryDelete.getDeletedEntry().getId()); + yield base == null ? Side.MEMORY : sameContent(base, entryDelete.getDeletedEntry()) ? Side.DISK : Side.BOTH; + } + case MetadataChange metadataChange -> { + metaDataSide = sideOf(metaData, serialize(local.getMetaData(), citationKeyPatterns), serialize(metadataChange.getMetaDataDiff().getNewMetaData(), citationKeyPatterns), Objects::equals); + yield metaDataSide; + } + case GroupChange _ -> metaDataSide; + case PreambleChange preambleChange -> + sideOf(preamble, local.getDatabase().getPreamble().orElse(null), preambleChange.getPreambleDiff().getNewPreamble(), Objects::equals); + case BibTexStringAdd stringAdd -> + sideOf(strings.get(stringAdd.getAddedString().getName()), null, stringAdd.getAddedString().getContent(), Objects::equals); + case BibTexStringDelete stringDelete -> + sideOf(strings.get(stringDelete.getDeletedString().getName()), stringDelete.getDeletedString().getContent(), null, Objects::equals); + case BibTexStringChange stringChange -> + sideOf(strings.get(stringChange.getOldString().getName()), stringChange.getOldString().getContent(), stringChange.getNewString().getContent(), Objects::equals); + case BibTexStringRename stringRename -> { + boolean oldUntouchedInMemory = Objects.equals(strings.get(stringRename.getOldString().getName()), stringRename.getOldString().getContent()); + yield oldUntouchedInMemory && !strings.containsKey(stringRename.getNewString().getName()) ? Side.DISK : Side.BOTH; + } + }; + switch (side) { + case DISK -> { + change.accept(); + triage.diskOnly().add(change); + } + case BOTH -> triage.bothSides().add(change); + case MEMORY -> triage.memoryOnly().add(change); + } + } + return triage; + } + + /// Carries over the baseline of every item whose external change was not applied, so that the next scan sees + /// the same divergence again instead of mistaking the current in-memory version for the common ancestor. + public void keepUnresolved(LibraryBaseline previous, List unresolved) { + for (DatabaseChange change : unresolved) { + switch (change) { + case EntryChange entryChange -> keepEntry(previous, entryChange.getOldEntry().getId()); + case EntryDelete entryDelete -> keepEntry(previous, entryDelete.getDeletedEntry().getId()); + case EntryAdd entryAdd -> previous.find(entryAdd.getAddedEntry()).ifPresent(base -> entriesById.put(base.getId(), base)); + case MetadataChange _, GroupChange _ -> { + metaData.clear(); + metaData.putAll(previous.metaData); + } + case PreambleChange _ -> preamble = previous.preamble; + case BibTexStringAdd stringAdd -> keepString(previous, stringAdd.getAddedString().getName()); + case BibTexStringDelete stringDelete -> keepString(previous, stringDelete.getDeletedString().getName()); + case BibTexStringChange stringChange -> keepString(previous, stringChange.getOldString().getName()); + case BibTexStringRename stringRename -> { + keepString(previous, stringRename.getOldString().getName()); + keepString(previous, stringRename.getNewString().getName()); + } + } + } + } + + /// The two-way diff pairs entries by similarity, so an entry whose citation key changed on one side while fields + /// changed on the other can fall below the similarity threshold and show up as a deletion plus an addition. The + /// baseline knows both belong to the same entry. + private List pairSplitEntries(List changes, BibDatabaseContext local, @Nullable DatabaseChangeResolverFactory resolverFactory) { + Map deletesByBaseId = new HashMap<>(); + for (DatabaseChange change : changes) { + if (change instanceof EntryDelete entryDelete) { + BibEntry base = entriesById.get(entryDelete.getDeletedEntry().getId()); + if (base != null) { + deletesByBaseId.put(base.getId(), entryDelete); + } + } + } + if (deletesByBaseId.isEmpty()) { + return changes; + } + List paired = new ArrayList<>(changes); + for (DatabaseChange change : changes) { + if (change instanceof EntryAdd entryAdd) { + find(entryAdd.getAddedEntry()).map(base -> deletesByBaseId.remove(base.getId())).ifPresent(entryDelete -> { + paired.remove(entryDelete); + paired.set(paired.indexOf(entryAdd), new EntryChange(entryDelete.getDeletedEntry(), entryAdd.getAddedEntry(), local, resolverFactory)); + }); + } + } + return paired; + } + + /// Which side diverged from the baseline. Only called for items that differ between memory and disk, so at least + /// one side changed. `null` stands for "absent" on that side. + private static Side sideOf(@Nullable T base, @Nullable T local, @Nullable T remote, BiPredicate same) { + boolean localChanged = !same(base, local, same); + boolean remoteChanged = !same(base, remote, same); + if (localChanged && remoteChanged) { + return Side.BOTH; + } + return remoteChanged ? Side.DISK : Side.MEMORY; + } + + private static boolean same(@Nullable T one, @Nullable T other, BiPredicate same) { + if (one == null || other == null) { + return one == other; + } + return same.test(one, other); + } + + /// Field-level merge of an entry modified on both sides: disk wins for every field memory left alone, memory wins + /// where disk left the field alone. Empty if the same field was changed differently on both sides. + private static Optional mergeFields(@Nullable BibEntry base, BibEntry local, BibEntry remote) { + // Without a common ancestor (entry new on both sides), an empty entry of the in-memory type makes every field + // an addition, so only fields set differently on both sides count as conflicts and the in-memory type is kept + BibEntry ancestor = base == null ? new BibEntry(local.getType()) : base; + if (ConflictRules.hasConflictingFields(ancestor, local, remote)) { + return Optional.empty(); + } + BibEntry merged = new BibEntry(local); + if (!ancestor.getType().equals(remote.getType())) { + merged.setType(remote.getType()); + } + FieldPatchComputer.compute(ancestor, local, remote).forEach((field, value) -> { + if (value == null) { + merged.clearField(field); + } else { + merged.setField(field, value); + } + }); + return Optional.of(merged); + } + + private static boolean sameContent(BibEntry one, BibEntry other) { + return one.getType().equals(other.getType()) && one.getFieldMap().equals(other.getFieldMap()); + } + + /// Looks up the baseline of a disk entry without in-memory counterpart: by citation key, or by content for entries without one. + private Optional find(BibEntry remote) { + return entriesById.values().stream() + .filter(base -> remote.getCitationKey() + .map(key -> base.getCitationKey().filter(key::equals).isPresent()) + .orElseGet(() -> sameContent(base, remote))) + .findFirst(); + } + + private void keepEntry(LibraryBaseline previous, String entryId) { + BibEntry base = previous.entriesById.get(entryId); + if (base == null) { + entriesById.remove(entryId); + } else { + entriesById.put(entryId, base); + } + } + + private void keepString(LibraryBaseline previous, String name) { + String base = previous.strings.get(name); + if (base == null) { + strings.remove(name); + } else { + strings.put(name, base); + } + } + + private static Map serialize(MetaData metaData, GlobalCitationKeyPatterns citationKeyPatterns) { + return new HashMap<>(MetaDataSerializer.getSerializedStringMap(metaData, citationKeyPatterns)); + } +} diff --git a/jabgui/src/main/java/org/jabref/gui/collab/entrychange/EntryChange.java b/jabgui/src/main/java/org/jabref/gui/collab/entrychange/EntryChange.java index 512326589b2b..6526440fb697 100644 --- a/jabgui/src/main/java/org/jabref/gui/collab/entrychange/EntryChange.java +++ b/jabgui/src/main/java/org/jabref/gui/collab/entrychange/EntryChange.java @@ -1,13 +1,18 @@ package org.jabref.gui.collab.entrychange; +import java.util.LinkedHashSet; +import java.util.Objects; +import java.util.Set; + import org.jabref.gui.collab.DatabaseChange; import org.jabref.gui.collab.DatabaseChangeResolverFactory; import org.jabref.logic.l10n.Localization; import org.jabref.model.database.BibDatabaseContext; import org.jabref.model.entry.BibEntry; +import org.jabref.model.entry.field.Field; import org.jabref.model.undo.CompoundEdit; -import org.jabref.model.undo.UndoableInsertEntries; -import org.jabref.model.undo.UndoableRemoveEntries; +import org.jabref.model.undo.UndoableChangeType; +import org.jabref.model.undo.UndoableFieldChange; public final class EntryChange extends DatabaseChange { private final BibEntry oldEntry; @@ -33,9 +38,21 @@ public BibEntry getNewEntry() { return newEntry; } + /// Changes the entry in place rather than replacing it, so it keeps its identity: table position, selection, and + /// an open entry editor stay as they are. @Override public void applyChange(CompoundEdit undoEdit) { - undoEdit.applyEdit(new UndoableRemoveEntries(databaseContext.getDatabase(), oldEntry)); - undoEdit.applyEdit(new UndoableInsertEntries(databaseContext.getDatabase(), newEntry)); + if (!oldEntry.getType().equals(newEntry.getType())) { + undoEdit.applyEdit(new UndoableChangeType(oldEntry, oldEntry.getType(), newEntry.getType())); + } + Set fields = new LinkedHashSet<>(oldEntry.getFields()); + fields.addAll(newEntry.getFields()); + for (Field field : fields) { + String before = oldEntry.getField(field).orElse(null); + String after = newEntry.getField(field).orElse(null); + if (!Objects.equals(before, after)) { + undoEdit.applyEdit(new UndoableFieldChange(oldEntry, field, before, after)); + } + } } } diff --git a/jabgui/src/main/java/org/jabref/gui/preferences/general/GeneralTab.java b/jabgui/src/main/java/org/jabref/gui/preferences/general/GeneralTab.java index e534be0e364a..4c592597a2b2 100644 --- a/jabgui/src/main/java/org/jabref/gui/preferences/general/GeneralTab.java +++ b/jabgui/src/main/java/org/jabref/gui/preferences/general/GeneralTab.java @@ -113,7 +113,7 @@ private void buildView() { .section(Localization.lang("Saving"), saving -> saving .checkbox(Localization.lang("Always reformat library on save and export"), viewModel.alwaysReformatBibProperty()) - .checkbox(Localization.lang("Autosave local libraries"), viewModel.autosaveLocalLibrariesProperty(), + .checkbox(Localization.lang("Synchronize local libraries with their files"), viewModel.autosaveLocalLibrariesProperty(), autosave -> autosave.help(HelpFile.AUTOSAVE)) .checkbox(Localization.lang("Create backup"), viewModel.createBackupProperty()) .stringField(null, viewModel.backupDirectoryProperty(), diff --git a/jabgui/src/test/java/org/jabref/gui/collab/LibraryBaselineTest.java b/jabgui/src/test/java/org/jabref/gui/collab/LibraryBaselineTest.java new file mode 100644 index 000000000000..c684dcff0afb --- /dev/null +++ b/jabgui/src/test/java/org/jabref/gui/collab/LibraryBaselineTest.java @@ -0,0 +1,199 @@ +package org.jabref.gui.collab; + +import java.util.List; + +import org.jabref.gui.collab.entryadd.EntryAdd; +import org.jabref.gui.collab.entrychange.EntryChange; +import org.jabref.gui.collab.entrydelete.EntryDelete; +import org.jabref.gui.collab.stringadd.BibTexStringAdd; +import org.jabref.logic.citationkeypattern.GlobalCitationKeyPatterns; +import org.jabref.model.database.BibDatabase; +import org.jabref.model.database.BibDatabaseContext; +import org.jabref.model.entry.BibEntry; +import org.jabref.model.entry.BibtexString; +import org.jabref.model.entry.field.StandardField; +import org.jabref.model.entry.types.StandardEntryType; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertTrue; + +class LibraryBaselineTest { + + private static final GlobalCitationKeyPatterns PATTERNS = GlobalCitationKeyPatterns.fromPattern("[auth][year]"); + + private BibEntry local; + private BibDatabaseContext localContext; + private BibEntry disk; + private BibDatabaseContext diskContext; + private LibraryBaseline baseline; + + @BeforeEach + void setUp() { + local = new BibEntry(StandardEntryType.Article).withCitationKey("Key") + .withField(StandardField.TITLE, "Title") + .withField(StandardField.YEAR, "2020"); + // A second, never touched entry keeps the file from becoming empty, which the two-way diff cannot handle + BibEntry other = new BibEntry(StandardEntryType.Book).withCitationKey("Other").withField(StandardField.TITLE, "Other"); + localContext = new BibDatabaseContext(new BibDatabase(List.of(local, other))); + baseline = LibraryBaseline.of(localContext, PATTERNS); + // the file as it would be parsed: same content, different entry objects + disk = new BibEntry(local); + diskContext = new BibDatabaseContext(new BibDatabase(List.of(disk, new BibEntry(other)))); + } + + private LibraryBaseline.Triage triage() { + return baseline.triage(DatabaseChangeList.compareAndGetChanges(localContext, diskContext, null), localContext, null); + } + + @Test + void changeOnDiskOnlyIsAccepted() { + disk.setField(StandardField.TITLE, "Disk title"); + + LibraryBaseline.Triage triage = triage(); + + assertEquals(1, triage.diskOnly().size()); + assertTrue(triage.diskOnly().getFirst().isAccepted()); + assertTrue(triage.bothSides().isEmpty()); + assertTrue(triage.memoryOnly().isEmpty()); + } + + @Test + void unsavedChangeInMemoryIsNotAnExternalChange() { + local.setField(StandardField.TITLE, "Memory title"); + + LibraryBaseline.Triage triage = triage(); + + assertEquals(1, triage.memoryOnly().size()); + assertTrue(triage.diskOnly().isEmpty()); + assertTrue(triage.bothSides().isEmpty()); + } + + @Test + void differentFieldsChangedOnBothSidesAreMerged() { + local.setField(StandardField.TITLE, "Memory title"); + disk.setField(StandardField.YEAR, "2021"); + disk.setField(StandardField.AUTHOR, "Disk author"); + + LibraryBaseline.Triage triage = triage(); + + assertTrue(triage.bothSides().isEmpty()); + EntryChange merged = assertInstanceOf(EntryChange.class, triage.diskOnly().getFirst()); + assertTrue(merged.isAccepted()); + assertEquals(new BibEntry(StandardEntryType.Article).withCitationKey("Key") + .withField(StandardField.TITLE, "Memory title") + .withField(StandardField.YEAR, "2021") + .withField(StandardField.AUTHOR, "Disk author"), + merged.getNewEntry()); + } + + @Test + void sameFieldChangedDifferentlyNeedsReview() { + local.setField(StandardField.TITLE, "Memory title"); + disk.setField(StandardField.TITLE, "Disk title"); + + LibraryBaseline.Triage triage = triage(); + + assertEquals(1, triage.bothSides().size()); + assertTrue(triage.diskOnly().isEmpty()); + } + + @Test + void citationKeyChangedInMemoryStillMergesDiskChange() { + local.setCitationKey("NewKey"); + disk.setField(StandardField.TITLE, "Disk title"); + + LibraryBaseline.Triage triage = triage(); + + EntryChange merged = assertInstanceOf(EntryChange.class, triage.diskOnly().getFirst()); + assertEquals("NewKey", merged.getNewEntry().getCitationKey().orElseThrow()); + assertEquals("Disk title", merged.getNewEntry().getField(StandardField.TITLE).orElseThrow()); + } + + @Test + void entryAddedOnDiskIsAccepted() { + diskContext.getDatabase().insertEntry(new BibEntry().withCitationKey("New").withField(StandardField.TITLE, "New on disk")); + + LibraryBaseline.Triage triage = triage(); + + assertInstanceOf(EntryAdd.class, triage.diskOnly().getFirst()); + assertTrue(triage.bothSides().isEmpty()); + } + + @Test + void entryDeletedInMemoryStaysDeletedWhenUnchangedOnDisk() { + localContext.getDatabase().removeEntry(local); + + LibraryBaseline.Triage triage = triage(); + + assertInstanceOf(EntryAdd.class, triage.memoryOnly().getFirst()); + assertTrue(triage.diskOnly().isEmpty()); + } + + @Test + void entryDeletedInMemoryButChangedOnDiskNeedsReview() { + localContext.getDatabase().removeEntry(local); + disk.setField(StandardField.TITLE, "Disk title"); + + LibraryBaseline.Triage triage = triage(); + + assertInstanceOf(EntryAdd.class, triage.bothSides().getFirst()); + } + + @Test + void entryDeletedOnDiskIsAcceptedWhenUnchangedInMemory() { + diskContext.getDatabase().removeEntry(disk); + + LibraryBaseline.Triage triage = triage(); + + assertInstanceOf(EntryDelete.class, triage.diskOnly().getFirst()); + } + + @Test + void entryDeletedOnDiskButChangedInMemoryNeedsReview() { + diskContext.getDatabase().removeEntry(disk); + local.setField(StandardField.TITLE, "Memory title"); + + LibraryBaseline.Triage triage = triage(); + + assertInstanceOf(EntryDelete.class, triage.bothSides().getFirst()); + } + + @Test + void entryAddedInMemoryIsNotAnExternalChange() { + localContext.getDatabase().insertEntry(new BibEntry().withCitationKey("New").withField(StandardField.TITLE, "New in memory")); + + LibraryBaseline.Triage triage = triage(); + + assertInstanceOf(EntryDelete.class, triage.memoryOnly().getFirst()); + assertTrue(triage.diskOnly().isEmpty()); + } + + @Test + void stringAddedOnDiskIsAccepted() { + diskContext.getDatabase().addString(new BibtexString("jan", "January")); + + LibraryBaseline.Triage triage = triage(); + + assertInstanceOf(BibTexStringAdd.class, triage.diskOnly().getFirst()); + } + + @Test + void keepUnresolvedPreservesBaselineOfUnsavedMemoryChange() { + local.setField(StandardField.TITLE, "Memory title"); + LibraryBaseline.Triage first = triage(); + LibraryBaseline updated = LibraryBaseline.of(localContext, PATTERNS); + updated.keepUnresolved(baseline, first.memoryOnly()); + baseline = updated; + + // The file now changes the same field: this must be reported as a conflict, not taken over silently + disk.setField(StandardField.TITLE, "Disk title"); + LibraryBaseline.Triage second = triage(); + + assertEquals(1, second.bothSides().size()); + assertTrue(second.diskOnly().isEmpty()); + } +} diff --git a/jablib/src/main/java/module-info.java b/jablib/src/main/java/module-info.java index 755f6ef7f35b..2f5f110d8fcd 100644 --- a/jablib/src/main/java/module-info.java +++ b/jablib/src/main/java/module-info.java @@ -117,6 +117,7 @@ exports org.jabref.logic.icore; exports org.jabref.model.icore; exports org.jabref.logic.git.merge.planning; + exports org.jabref.logic.git.merge.planning.util; exports org.jabref.logic.git.merge.execution; exports org.jabref.model.sciteTallies; exports org.jabref.logic.bibtex.comparator.plausibility; diff --git a/jablib/src/main/resources/l10n/JabRef_en.properties b/jablib/src/main/resources/l10n/JabRef_en.properties index e802b56583ff..8e7d0590db16 100644 --- a/jablib/src/main/resources/l10n/JabRef_en.properties +++ b/jablib/src/main/resources/l10n/JabRef_en.properties @@ -714,7 +714,7 @@ Saving\ library=Saving library Library\ saved=Library saved Could\ not\ save\ file.=Could not save file. Saved\ selected\ to\ '%0'.=Saved selected to '%0'. -Autosave\ local\ libraries=Autosave local libraries +Synchronize\ local\ libraries\ with\ their\ files=Synchronize local libraries with their files Enable\ MSC\ keyword\ descriptions=Enable MSC keyword descriptions Automatically\ save\ the\ library\ to=Automatically save the library to Please\ enter\ a\ valid\ file\ path.=Please enter a valid file path. @@ -3520,6 +3520,7 @@ Group\ entry\ counts=Group entry counts # Notifications Background\ task=Background task External\ changes\ detected=External changes detected +Merged\ %0\ change(s)\ from\ the\ library\ file=Merged %0 change(s) from the library file Files=Files Info=Info Reveal=Reveal From bcd62361e8bd2d005b927ad13e0700c807e99f8c Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Tue, 1 Sep 2026 22:35:55 +0200 Subject: [PATCH 02/27] Add changelog entry and null-marking for the file sync Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01GWmXyN3GUNhfF9fSo1UNrt --- CHANGELOG.md | 1 + jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java | 2 ++ 2 files changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a15a3231759..e3824310eb31 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -69,6 +69,7 @@ Note that this project **does not** adhere to [Semantic Versioning](https://semv ### Changed +- We changed autosave into synchronizing: external changes to the library file are merged automatically unless they conflict with unsaved edits. [#8431](https://github.com/JabRef/jabref/issues/8431) - We changed the default macOS shortcuts for "Search document identifier online" and "Focus group list" to not insert special characters. [#16528](https://github.com/JabRef/jabref/issues/16528) - We changed the extension of backup files from `.bak` to `.bib`, so that they can be opened in JabRef. [#11454](https://github.com/JabRef/jabref/issues/11454) - We changed the delete and rename file dialogs to state that undo does not restore files on disk. [#16680](https://github.com/JabRef/jabref/pull/16680) diff --git a/jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java b/jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java index 8de73db1039f..0998ddef13d9 100644 --- a/jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java +++ b/jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java @@ -27,6 +27,7 @@ import org.jabref.model.entry.BibtexString; import org.jabref.model.metadata.MetaData; +import org.jspecify.annotations.NullMarked; import org.jspecify.annotations.Nullable; /// The library as it last matched its file on disk. Comparing both the in-memory library and the file against it tells @@ -35,6 +36,7 @@ /// /// Entries are keyed by the id of the in-memory entry they were copied from, so that a citation key change in memory /// does not break the association. The metadata is kept in its serialized form, which is the only way to snapshot it. +@NullMarked public final class LibraryBaseline { /// Outcome of comparing external changes against the baseline. From 00a9d3035d7cbb466864500e9219147d7623edba Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Tue, 1 Sep 2026 22:41:12 +0200 Subject: [PATCH 03/27] Apply CI formatting to LibraryBaseline Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01GWmXyN3GUNhfF9fSo1UNrt --- .../jabref/gui/collab/LibraryBaseline.java | 33 ++++++++++++------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java b/jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java index 0998ddef13d9..f315f3fc307f 100644 --- a/jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java +++ b/jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java @@ -113,7 +113,8 @@ public Triage triage(List changes, BibDatabaseContext local, @Nu metaDataSide = sideOf(metaData, serialize(local.getMetaData(), citationKeyPatterns), serialize(metadataChange.getMetaDataDiff().getNewMetaData(), citationKeyPatterns), Objects::equals); yield metaDataSide; } - case GroupChange _ -> metaDataSide; + case GroupChange _ -> + metaDataSide; case PreambleChange preambleChange -> sideOf(preamble, local.getDatabase().getPreamble().orElse(null), preambleChange.getPreambleDiff().getNewPreamble(), Objects::equals); case BibTexStringAdd stringAdd -> @@ -132,8 +133,10 @@ public Triage triage(List changes, BibDatabaseContext local, @Nu change.accept(); triage.diskOnly().add(change); } - case BOTH -> triage.bothSides().add(change); - case MEMORY -> triage.memoryOnly().add(change); + case BOTH -> + triage.bothSides().add(change); + case MEMORY -> + triage.memoryOnly().add(change); } } return triage; @@ -144,17 +147,25 @@ public Triage triage(List changes, BibDatabaseContext local, @Nu public void keepUnresolved(LibraryBaseline previous, List unresolved) { for (DatabaseChange change : unresolved) { switch (change) { - case EntryChange entryChange -> keepEntry(previous, entryChange.getOldEntry().getId()); - case EntryDelete entryDelete -> keepEntry(previous, entryDelete.getDeletedEntry().getId()); - case EntryAdd entryAdd -> previous.find(entryAdd.getAddedEntry()).ifPresent(base -> entriesById.put(base.getId(), base)); - case MetadataChange _, GroupChange _ -> { + case EntryChange entryChange -> + keepEntry(previous, entryChange.getOldEntry().getId()); + case EntryDelete entryDelete -> + keepEntry(previous, entryDelete.getDeletedEntry().getId()); + case EntryAdd entryAdd -> + previous.find(entryAdd.getAddedEntry()).ifPresent(base -> entriesById.put(base.getId(), base)); + case MetadataChange _, + GroupChange _ -> { metaData.clear(); metaData.putAll(previous.metaData); } - case PreambleChange _ -> preamble = previous.preamble; - case BibTexStringAdd stringAdd -> keepString(previous, stringAdd.getAddedString().getName()); - case BibTexStringDelete stringDelete -> keepString(previous, stringDelete.getDeletedString().getName()); - case BibTexStringChange stringChange -> keepString(previous, stringChange.getOldString().getName()); + case PreambleChange _ -> + preamble = previous.preamble; + case BibTexStringAdd stringAdd -> + keepString(previous, stringAdd.getAddedString().getName()); + case BibTexStringDelete stringDelete -> + keepString(previous, stringDelete.getDeletedString().getName()); + case BibTexStringChange stringChange -> + keepString(previous, stringChange.getOldString().getName()); case BibTexStringRename stringRename -> { keepString(previous, stringRename.getOldString().getName()); keepString(previous, stringRename.getNewString().getName()); From 8c23c9c3a6c55ac30a434a5ef53b71300c9c05d5 Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Wed, 2 Sep 2026 01:40:17 +0200 Subject: [PATCH 04/27] Harden synchronization against overlapping scans and concurrent edits A scan overtaken by a newer file change discards its result, and changes are sorted on the FX thread right before they are applied, so no edit can slip in between. Entries whose citation key changed on disk are paired with their in-memory entry, encoding changes count as metadata changes, and switching synchronization on for an open, unmodified library takes effect immediately. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01GWmXyN3GUNhfF9fSo1UNrt --- docs/requirements/ux.md | 3 -- .../org/jabref/gui/collab/ChangeScanner.java | 6 +-- .../gui/collab/DatabaseChangeMonitor.java | 43 +++++++++++++++++-- .../jabref/gui/collab/LibraryBaseline.java | 34 ++++++++++++--- .../gui/collab/LibraryBaselineTest.java | 27 ++++++++++++ 5 files changed, 97 insertions(+), 16 deletions(-) diff --git a/docs/requirements/ux.md b/docs/requirements/ux.md index 646cbe24ccb3..f6f8eb4612bf 100644 --- a/docs/requirements/ux.md +++ b/docs/requirements/ux.md @@ -128,9 +128,6 @@ Fields changed only in the file are taken over even when other fields of the sam Needs: impl ## Committing a library that is not under version control -## Committing a library that is not under version control -||||||| parent of 9b7de4f9fc (Synchronize local libraries with their files) -### Committing a library that is not under version control `req~ux.git-commit.initialize-repository~1` When a user commits a library that is not inside a Git repository, JabRef must offer to initialize a repository in the library's directory and commit the library file there. diff --git a/jabgui/src/main/java/org/jabref/gui/collab/ChangeScanner.java b/jabgui/src/main/java/org/jabref/gui/collab/ChangeScanner.java index 3a77904cafbb..88db266f7a72 100644 --- a/jabgui/src/main/java/org/jabref/gui/collab/ChangeScanner.java +++ b/jabgui/src/main/java/org/jabref/gui/collab/ChangeScanner.java @@ -46,9 +46,9 @@ public List scanForChanges() { } } - /// @return the external changes sorted by the side they happened on, see [LibraryBaseline#triage] - public LibraryBaseline.Triage scanForChanges(LibraryBaseline baseline) { - return baseline.triage(scanForChanges(), database, databaseChangeResolverFactory); + /// @return the given external changes sorted by the side they happened on, see [LibraryBaseline#triage] + public LibraryBaseline.Triage triage(LibraryBaseline baseline, List changes) { + return baseline.triage(changes, database, databaseChangeResolverFactory); } public List getDatabaseChanges(Path fileToCompare) throws IOException { diff --git a/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java b/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java index 5dc240fa4f10..cb9a1623b107 100644 --- a/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java +++ b/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java @@ -6,6 +6,8 @@ import java.util.List; import java.util.Optional; +import javafx.beans.value.ChangeListener; + import org.jabref.gui.DialogService; import org.jabref.gui.LibraryTab; import org.jabref.gui.Notifications; @@ -53,6 +55,12 @@ public class DatabaseChangeMonitor implements FileUpdateListener { /// every external change is offered for review. @Nullable private LibraryBaseline baseline; + /// Counts started scans, so that a scan overtaken by a later one (whose result reflects the newer file state) + /// discards its result instead of applying stale content. + private volatile int scanGeneration; + + private final ChangeListener synchronizingListener = (_, _, enabled) -> onSynchronizingChanged(enabled); + public DatabaseChangeMonitor(BibDatabaseContext database, FileUpdateMonitor fileMonitor, TaskExecutor taskExecutor, @@ -81,6 +89,9 @@ public DatabaseChangeMonitor(BibDatabaseContext database, } catch (IOException e) { LOGGER.error("Error while trying to monitor {}", path, e); } + if (database.getLocation() == DatabaseLocation.LOCAL) { + preferences.getLibraryPreferences().autoSaveProperty().addListener(synchronizingListener); + } }); addListener(this::notifyExternalChanges); @@ -184,6 +195,18 @@ private boolean isSynchronizing() { return database.getLocation() == DatabaseLocation.LOCAL && preferences.getLibraryPreferences().shouldAutoSave(); } + /// Synchronization switched on for an open library needs a baseline right away: as long as the library is + /// unmodified, it still matches its file. Otherwise the next save establishes the baseline. + private void onSynchronizingChanged(boolean enabled) { + synchronized (database) { + if (!enabled) { + baseline = null; + } else if (baseline == null && !libraryTab.isModified()) { + baseline = captureBaseline(); + } + } + } + private @Nullable LibraryBaseline captureBaseline() { if (!isSynchronizing()) { return null; @@ -207,10 +230,19 @@ private void scanIfFileChanged() { private void scanForChanges() { ChangeScanner scanner = new ChangeScanner(database, dialogService, preferences, stateManager); LibraryBaseline scannedBaseline = baseline; + int generation = ++scanGeneration; if (scannedBaseline != null && isSynchronizing()) { // [impl->req~ux.external-library-changes.synchronize~1] - BackgroundTask.wrap(() -> scanner.scanForChanges(scannedBaseline)) - .onSuccess(triage -> synchronize(scannedBaseline, triage)) + BackgroundTask.wrap(() -> scanner.scanForChanges()) + .onSuccess(changes -> { + if (generation != scanGeneration) { + LOGGER.debug("Discarding result of a scan overtaken by a newer file change"); + return; + } + // Sorting the changes on the FX thread right before applying them leaves no window + // for a user edit to slip in between classification and application + synchronize(scannedBaseline, scanner.triage(scannedBaseline, changes)); + }) .onFailure(e -> LOGGER.error("Error while synchronizing with the library file", e)) .executeWith(taskExecutor); return; @@ -267,6 +299,11 @@ public void addListener(DatabaseChangeListener listener) { } public void unregister() { - monitoredPath.ifPresent(path -> fileMonitor.removeListener(path, this)); + monitoredPath.ifPresent(path -> { + fileMonitor.removeListener(path, this); + if (database.getLocation() == DatabaseLocation.LOCAL) { + preferences.getLibraryPreferences().autoSaveProperty().removeListener(synchronizingListener); + } + }); } } diff --git a/jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java b/jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java index f315f3fc307f..479ebf4439bd 100644 --- a/jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java +++ b/jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java @@ -1,5 +1,6 @@ package org.jabref.gui.collab; +import java.nio.charset.Charset; import java.util.ArrayList; import java.util.HashMap; import java.util.List; @@ -25,6 +26,8 @@ import org.jabref.model.database.BibDatabaseContext; import org.jabref.model.entry.BibEntry; import org.jabref.model.entry.BibtexString; +import org.jabref.model.entry.field.Field; +import org.jabref.model.entry.field.InternalField; import org.jabref.model.metadata.MetaData; import org.jspecify.annotations.NullMarked; @@ -49,6 +52,8 @@ public record Triage(List diskOnly, List bothSid private enum Side { DISK, MEMORY, BOTH } + private static final String ENCODING_KEY = "\0encoding"; + private final Map entriesById; private final Map metaData; private @Nullable String preamble; @@ -247,13 +252,24 @@ private static boolean sameContent(BibEntry one, BibEntry other) { return one.getType().equals(other.getType()) && one.getFieldMap().equals(other.getFieldMap()); } - /// Looks up the baseline of a disk entry without in-memory counterpart: by citation key, or by content for entries without one. + /// Looks up the baseline of a disk entry without in-memory counterpart: by citation key, or else by the remaining + /// content, which covers entries without a key as well as a key changed on disk. private Optional find(BibEntry remote) { - return entriesById.values().stream() - .filter(base -> remote.getCitationKey() - .map(key -> base.getCitationKey().filter(key::equals).isPresent()) - .orElseGet(() -> sameContent(base, remote))) - .findFirst(); + Optional byKey = remote.getCitationKey() + .flatMap(key -> entriesById.values().stream() + .filter(base -> base.getCitationKey().filter(key::equals).isPresent()) + .findFirst()); + return byKey.or(() -> entriesById.values().stream() + .filter(base -> sameContentExceptKey(base, remote)) + .findFirst()); + } + + private static boolean sameContentExceptKey(BibEntry one, BibEntry other) { + Map oneFields = new HashMap<>(one.getFieldMap()); + Map otherFields = new HashMap<>(other.getFieldMap()); + oneFields.remove(InternalField.KEY_FIELD); + otherFields.remove(InternalField.KEY_FIELD); + return one.getType().equals(other.getType()) && oneFields.equals(otherFields); } private void keepEntry(LibraryBaseline previous, String entryId) { @@ -274,7 +290,11 @@ private void keepString(LibraryBaseline previous, String name) { } } + /// The encoding is not part of the serialized metadata (it is written as a file header instead), but it is part + /// of what the two-way comparison reports. private static Map serialize(MetaData metaData, GlobalCitationKeyPatterns citationKeyPatterns) { - return new HashMap<>(MetaDataSerializer.getSerializedStringMap(metaData, citationKeyPatterns)); + Map serialized = new HashMap<>(MetaDataSerializer.getSerializedStringMap(metaData, citationKeyPatterns)); + serialized.put(ENCODING_KEY, metaData.getEncoding().map(Charset::name).orElse("")); + return serialized; } } diff --git a/jabgui/src/test/java/org/jabref/gui/collab/LibraryBaselineTest.java b/jabgui/src/test/java/org/jabref/gui/collab/LibraryBaselineTest.java index c684dcff0afb..e59ba3ff666f 100644 --- a/jabgui/src/test/java/org/jabref/gui/collab/LibraryBaselineTest.java +++ b/jabgui/src/test/java/org/jabref/gui/collab/LibraryBaselineTest.java @@ -1,10 +1,12 @@ package org.jabref.gui.collab; +import java.nio.charset.StandardCharsets; import java.util.List; import org.jabref.gui.collab.entryadd.EntryAdd; import org.jabref.gui.collab.entrychange.EntryChange; import org.jabref.gui.collab.entrydelete.EntryDelete; +import org.jabref.gui.collab.metedatachange.MetadataChange; import org.jabref.gui.collab.stringadd.BibTexStringAdd; import org.jabref.logic.citationkeypattern.GlobalCitationKeyPatterns; import org.jabref.model.database.BibDatabase; @@ -113,6 +115,31 @@ void citationKeyChangedInMemoryStillMergesDiskChange() { assertEquals("Disk title", merged.getNewEntry().getField(StandardField.TITLE).orElseThrow()); } + @Test + void citationKeyChangedOnDiskIsMergedIntoTheSameEntry() { + disk.setCitationKey("Renamed"); + disk.setField(StandardField.YEAR, "2021"); + + LibraryBaseline.Triage triage = triage(); + + EntryChange merged = assertInstanceOf(EntryChange.class, triage.diskOnly().getFirst()); + assertEquals(local, merged.getOldEntry()); + assertEquals("Renamed", merged.getNewEntry().getCitationKey().orElseThrow()); + assertEquals(1, triage.diskOnly().size()); + } + + @Test + void encodingChangedOnDiskIsAccepted() { + localContext.getMetaData().setEncoding(StandardCharsets.UTF_8); + baseline = LibraryBaseline.of(localContext, PATTERNS); + diskContext.getMetaData().setEncoding(StandardCharsets.ISO_8859_1); + + LibraryBaseline.Triage triage = triage(); + + assertInstanceOf(MetadataChange.class, triage.diskOnly().getFirst()); + assertTrue(triage.memoryOnly().isEmpty()); + } + @Test void entryAddedOnDiskIsAccepted() { diskContext.getDatabase().insertEntry(new BibEntry().withCitationKey("New").withField(StandardField.TITLE, "New on disk")); From 66d91c3f3758958689b451785affcbca5b7399a4 Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Wed, 2 Sep 2026 10:39:31 +0200 Subject: [PATCH 05/27] Invalidate in-flight scans and key baseline lookups by in-memory id A save, switching synchronization off, or closing the tab now discards the result of a scan still running. Baseline entries found for disk entries are kept under the id of the in-memory entry they came from, and among entries sharing a citation key the one with identical content is preferred. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01GWmXyN3GUNhfF9fSo1UNrt --- .../gui/collab/DatabaseChangeMonitor.java | 8 +++- .../jabref/gui/collab/LibraryBaseline.java | 34 +++++++++-------- .../gui/collab/LibraryBaselineTest.java | 37 +++++++++++++------ 3 files changed, 51 insertions(+), 28 deletions(-) diff --git a/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java b/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java index cb9a1623b107..6d66f6a84482 100644 --- a/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java +++ b/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java @@ -55,8 +55,9 @@ public class DatabaseChangeMonitor implements FileUpdateListener { /// every external change is offered for review. @Nullable private LibraryBaseline baseline; - /// Counts started scans, so that a scan overtaken by a later one (whose result reflects the newer file state) - /// discards its result instead of applying stale content. + /// Counts started scans and invalidations, so that a scan overtaken by a later one (whose result reflects the + /// newer file state), by a save, by switching synchronization off, or by closing the tab discards its result + /// instead of applying stale content. private volatile int scanGeneration; private final ChangeListener synchronizingListener = (_, _, enabled) -> onSynchronizingChanged(enabled); @@ -186,6 +187,7 @@ public void markConsistentWithDisk(@Nullable FileSnapshot diskState) { monitoredPath.ifPresent(path -> knownDiskState = FileSnapshot.read(path)); } baseline = captureBaseline(); + scanGeneration++; } } @@ -201,6 +203,7 @@ private void onSynchronizingChanged(boolean enabled) { synchronized (database) { if (!enabled) { baseline = null; + scanGeneration++; } else if (baseline == null && !libraryTab.isModified()) { baseline = captureBaseline(); } @@ -299,6 +302,7 @@ public void addListener(DatabaseChangeListener listener) { } public void unregister() { + scanGeneration++; monitoredPath.ifPresent(path -> { fileMonitor.removeListener(path, this); if (database.getLocation() == DatabaseLocation.LOCAL) { diff --git a/jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java b/jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java index 479ebf4439bd..3f1b1da8f281 100644 --- a/jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java +++ b/jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java @@ -107,7 +107,7 @@ public Triage triage(List changes, BibDatabaseContext local, @Nu } // No in-memory counterpart: either new on disk, or deleted in memory (and possibly modified on disk) case EntryAdd entryAdd -> - find(entryAdd.getAddedEntry()).map(base -> sameContent(base, entryAdd.getAddedEntry()) ? Side.MEMORY : Side.BOTH) + find(entryAdd.getAddedEntry()).map(base -> sameContent(base.getValue(), entryAdd.getAddedEntry()) ? Side.MEMORY : Side.BOTH) .orElse(Side.DISK); // No counterpart on disk: either deleted on disk, or added in memory (and possibly modified on disk) case EntryDelete entryDelete -> { @@ -157,7 +157,7 @@ public void keepUnresolved(LibraryBaseline previous, List unreso case EntryDelete entryDelete -> keepEntry(previous, entryDelete.getDeletedEntry().getId()); case EntryAdd entryAdd -> - previous.find(entryAdd.getAddedEntry()).ifPresent(base -> entriesById.put(base.getId(), base)); + previous.find(entryAdd.getAddedEntry()).ifPresent(base -> entriesById.put(base.getKey(), base.getValue())); case MetadataChange _, GroupChange _ -> { metaData.clear(); @@ -186,9 +186,9 @@ private List pairSplitEntries(List changes, BibD Map deletesByBaseId = new HashMap<>(); for (DatabaseChange change : changes) { if (change instanceof EntryDelete entryDelete) { - BibEntry base = entriesById.get(entryDelete.getDeletedEntry().getId()); - if (base != null) { - deletesByBaseId.put(base.getId(), entryDelete); + String baseId = entryDelete.getDeletedEntry().getId(); + if (entriesById.containsKey(baseId)) { + deletesByBaseId.put(baseId, entryDelete); } } } @@ -198,7 +198,7 @@ private List pairSplitEntries(List changes, BibD List paired = new ArrayList<>(changes); for (DatabaseChange change : changes) { if (change instanceof EntryAdd entryAdd) { - find(entryAdd.getAddedEntry()).map(base -> deletesByBaseId.remove(base.getId())).ifPresent(entryDelete -> { + find(entryAdd.getAddedEntry()).map(base -> deletesByBaseId.remove(base.getKey())).ifPresent(entryDelete -> { paired.remove(entryDelete); paired.set(paired.indexOf(entryAdd), new EntryChange(entryDelete.getDeletedEntry(), entryAdd.getAddedEntry(), local, resolverFactory)); }); @@ -252,15 +252,19 @@ private static boolean sameContent(BibEntry one, BibEntry other) { return one.getType().equals(other.getType()) && one.getFieldMap().equals(other.getFieldMap()); } - /// Looks up the baseline of a disk entry without in-memory counterpart: by citation key, or else by the remaining - /// content, which covers entries without a key as well as a key changed on disk. - private Optional find(BibEntry remote) { - Optional byKey = remote.getCitationKey() - .flatMap(key -> entriesById.values().stream() - .filter(base -> base.getCitationKey().filter(key::equals).isPresent()) - .findFirst()); - return byKey.or(() -> entriesById.values().stream() - .filter(base -> sameContentExceptKey(base, remote)) + /// Looks up the baseline of a disk entry without in-memory counterpart, keyed by the id of the in-memory entry it + /// was taken from: by citation key (preferring identical content, as keys need not be unique), or else by the + /// remaining content, which covers entries without a key as well as a key changed on disk. + private Optional> find(BibEntry remote) { + Optional> byKey = remote.getCitationKey().flatMap(key -> { + List> candidates = entriesById.entrySet().stream() + .filter(entry -> entry.getValue().getCitationKey().filter(key::equals).isPresent()) + .toList(); + return candidates.stream().filter(entry -> sameContent(entry.getValue(), remote)).findFirst() + .or(() -> candidates.stream().findFirst()); + }); + return byKey.or(() -> entriesById.entrySet().stream() + .filter(entry -> sameContentExceptKey(entry.getValue(), remote)) .findFirst()); } diff --git a/jabgui/src/test/java/org/jabref/gui/collab/LibraryBaselineTest.java b/jabgui/src/test/java/org/jabref/gui/collab/LibraryBaselineTest.java index e59ba3ff666f..f3bda0d15688 100644 --- a/jabgui/src/test/java/org/jabref/gui/collab/LibraryBaselineTest.java +++ b/jabgui/src/test/java/org/jabref/gui/collab/LibraryBaselineTest.java @@ -59,8 +59,8 @@ void changeOnDiskOnlyIsAccepted() { assertEquals(1, triage.diskOnly().size()); assertTrue(triage.diskOnly().getFirst().isAccepted()); - assertTrue(triage.bothSides().isEmpty()); - assertTrue(triage.memoryOnly().isEmpty()); + assertEquals(List.of(), triage.bothSides()); + assertEquals(List.of(), triage.memoryOnly()); } @Test @@ -70,8 +70,8 @@ void unsavedChangeInMemoryIsNotAnExternalChange() { LibraryBaseline.Triage triage = triage(); assertEquals(1, triage.memoryOnly().size()); - assertTrue(triage.diskOnly().isEmpty()); - assertTrue(triage.bothSides().isEmpty()); + assertEquals(List.of(), triage.diskOnly()); + assertEquals(List.of(), triage.bothSides()); } @Test @@ -82,7 +82,7 @@ void differentFieldsChangedOnBothSidesAreMerged() { LibraryBaseline.Triage triage = triage(); - assertTrue(triage.bothSides().isEmpty()); + assertEquals(List.of(), triage.bothSides()); EntryChange merged = assertInstanceOf(EntryChange.class, triage.diskOnly().getFirst()); assertTrue(merged.isAccepted()); assertEquals(new BibEntry(StandardEntryType.Article).withCitationKey("Key") @@ -100,7 +100,7 @@ void sameFieldChangedDifferentlyNeedsReview() { LibraryBaseline.Triage triage = triage(); assertEquals(1, triage.bothSides().size()); - assertTrue(triage.diskOnly().isEmpty()); + assertEquals(List.of(), triage.diskOnly()); } @Test @@ -137,7 +137,22 @@ void encodingChangedOnDiskIsAccepted() { LibraryBaseline.Triage triage = triage(); assertInstanceOf(MetadataChange.class, triage.diskOnly().getFirst()); - assertTrue(triage.memoryOnly().isEmpty()); + assertEquals(List.of(), triage.memoryOnly()); + } + + @Test + void duplicateCitationKeysAreMatchedByContent() { + BibEntry duplicate = new BibEntry(StandardEntryType.Article).withCitationKey("Key").withField(StandardField.TITLE, "Duplicate"); + localContext.getDatabase().insertEntry(duplicate); + diskContext.getDatabase().insertEntry(new BibEntry(duplicate)); + baseline = LibraryBaseline.of(localContext, PATTERNS); + localContext.getDatabase().removeEntry(local); + + LibraryBaseline.Triage triage = triage(); + + // The entry deleted in memory is still unchanged on disk, so nothing is reported; the duplicate must not be mistaken for it + assertInstanceOf(EntryAdd.class, triage.memoryOnly().getFirst()); + assertEquals(List.of(), triage.bothSides()); } @Test @@ -147,7 +162,7 @@ void entryAddedOnDiskIsAccepted() { LibraryBaseline.Triage triage = triage(); assertInstanceOf(EntryAdd.class, triage.diskOnly().getFirst()); - assertTrue(triage.bothSides().isEmpty()); + assertEquals(List.of(), triage.bothSides()); } @Test @@ -157,7 +172,7 @@ void entryDeletedInMemoryStaysDeletedWhenUnchangedOnDisk() { LibraryBaseline.Triage triage = triage(); assertInstanceOf(EntryAdd.class, triage.memoryOnly().getFirst()); - assertTrue(triage.diskOnly().isEmpty()); + assertEquals(List.of(), triage.diskOnly()); } @Test @@ -196,7 +211,7 @@ void entryAddedInMemoryIsNotAnExternalChange() { LibraryBaseline.Triage triage = triage(); assertInstanceOf(EntryDelete.class, triage.memoryOnly().getFirst()); - assertTrue(triage.diskOnly().isEmpty()); + assertEquals(List.of(), triage.diskOnly()); } @Test @@ -221,6 +236,6 @@ void keepUnresolvedPreservesBaselineOfUnsavedMemoryChange() { LibraryBaseline.Triage second = triage(); assertEquals(1, second.bothSides().size()); - assertTrue(second.diskOnly().isEmpty()); + assertEquals(List.of(), second.diskOnly()); } } From 33eb7d788ee8b72df74a96f52ea5bcba26820fcf Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Thu, 3 Sep 2026 02:51:41 +0200 Subject: [PATCH 06/27] Wait for the library file to stop changing before parsing it Sync clients and editors may write the file in several steps; a half-written library parses fine and would look like every later entry had been deleted. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01GWmXyN3GUNhfF9fSo1UNrt --- .../org/jabref/gui/collab/ChangeScanner.java | 7 ++++ .../gui/collab/DatabaseChangeMonitor.java | 34 +++++++++++++++++-- 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/jabgui/src/main/java/org/jabref/gui/collab/ChangeScanner.java b/jabgui/src/main/java/org/jabref/gui/collab/ChangeScanner.java index 88db266f7a72..09cd43d3abfb 100644 --- a/jabgui/src/main/java/org/jabref/gui/collab/ChangeScanner.java +++ b/jabgui/src/main/java/org/jabref/gui/collab/ChangeScanner.java @@ -34,9 +34,16 @@ public ChangeScanner(BibDatabaseContext database, } public List scanForChanges() { + return scanForChanges(() -> { + }); + } + + /// @param beforeParsing run right before the file is parsed, e.g. to wait until a sync client has finished writing it + public List scanForChanges(Runnable beforeParsing) { if (database.getDatabasePath().isEmpty()) { return List.of(); } + beforeParsing.run(); try { return getDatabaseChanges(database.getDatabasePath().get()); diff --git a/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java b/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java index 6d66f6a84482..8b4988628dba 100644 --- a/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java +++ b/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java @@ -4,6 +4,7 @@ import java.nio.file.Path; import java.util.ArrayList; import java.util.List; +import java.util.Objects; import java.util.Optional; import javafx.beans.value.ChangeListener; @@ -31,6 +32,8 @@ public class DatabaseChangeMonitor implements FileUpdateListener { private static final Logger LOGGER = LoggerFactory.getLogger(DatabaseChangeMonitor.class); + private static final int STABLE_FILE_ATTEMPTS = 20; + private static final long STABLE_FILE_INTERVAL_MILLIS = 250; private final BibDatabaseContext database; private final FileUpdateMonitor fileMonitor; @@ -236,7 +239,7 @@ private void scanForChanges() { int generation = ++scanGeneration; if (scannedBaseline != null && isSynchronizing()) { // [impl->req~ux.external-library-changes.synchronize~1] - BackgroundTask.wrap(() -> scanner.scanForChanges()) + BackgroundTask.wrap(() -> scanner.scanForChanges(this::awaitStableFile)) .onSuccess(changes -> { if (generation != scanGeneration) { LOGGER.debug("Discarding result of a scan overtaken by a newer file change"); @@ -250,7 +253,7 @@ private void scanForChanges() { .executeWith(taskExecutor); return; } - BackgroundTask.wrap(() -> scanner.scanForChanges()) + BackgroundTask.wrap(() -> scanner.scanForChanges(this::awaitStableFile)) .onSuccess(changes -> { if (!changes.isEmpty()) { listeners.forEach(listener -> listener.databaseChanged(changes)); @@ -260,6 +263,33 @@ private void scanForChanges() { .executeWith(taskExecutor); } + /// Sync clients and editors may write the file in several steps. A file that is still growing must not be parsed: + /// half of a library parses fine and would look like every later entry had been deleted. Waits (bounded) until + /// size and modification time stop changing; the last snapshot seen becomes the known disk state. + private void awaitStableFile() { + Path path = monitoredPath.orElse(null); + if (path == null) { + return; + } + FileSnapshot last = FileSnapshot.read(path); + for (int attempt = 0; attempt < STABLE_FILE_ATTEMPTS; attempt++) { + try { + Thread.sleep(STABLE_FILE_INTERVAL_MILLIS); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + return; + } + FileSnapshot current = FileSnapshot.read(path); + if (Objects.equals(current, last)) { + break; + } + last = current; + } + synchronized (database) { + knownDiskState = last; + } + } + /// Applies what changed on disk only, and offers the review for what changed on both sides. private void synchronize(LibraryBaseline scannedBaseline, LibraryBaseline.Triage triage) { List unresolved = new ArrayList<>(triage.bothSides()); From 25453e9ffda901b490beb748a9b45ebe0bdc6ead Mon Sep 17 00:00:00 2001 From: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com> Date: Fri, 4 Sep 2026 23:26:48 +0200 Subject: [PATCH 07/27] Group an entry's merged changes into a named nested step Collecting them in a CompoundEdit named after the change lets the warning logged by a failing ChangeSet name the entry, not just the enclosing merge. Undo granularity is unchanged: nested sets are folded into the one step the caller opens. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01P4cHLmHHsmaTJmBP6JnZat --- .../org/jabref/gui/collab/entrychange/EntryChange.java | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/jabgui/src/main/java/org/jabref/gui/collab/entrychange/EntryChange.java b/jabgui/src/main/java/org/jabref/gui/collab/entrychange/EntryChange.java index 6526440fb697..126efc972434 100644 --- a/jabgui/src/main/java/org/jabref/gui/collab/entrychange/EntryChange.java +++ b/jabgui/src/main/java/org/jabref/gui/collab/entrychange/EntryChange.java @@ -40,10 +40,14 @@ public BibEntry getNewEntry() { /// Changes the entry in place rather than replacing it, so it keeps its identity: table position, selection, and /// an open entry editor stay as they are. + /// + /// The changes are collected in a nested step named after this change, so a failure while applying them names + /// the entry it belongs to instead of only the enclosing merge. @Override public void applyChange(CompoundEdit undoEdit) { + CompoundEdit entryEdit = new CompoundEdit(getName()); if (!oldEntry.getType().equals(newEntry.getType())) { - undoEdit.applyEdit(new UndoableChangeType(oldEntry, oldEntry.getType(), newEntry.getType())); + entryEdit.applyEdit(new UndoableChangeType(oldEntry, oldEntry.getType(), newEntry.getType())); } Set fields = new LinkedHashSet<>(oldEntry.getFields()); fields.addAll(newEntry.getFields()); @@ -51,8 +55,9 @@ public void applyChange(CompoundEdit undoEdit) { String before = oldEntry.getField(field).orElse(null); String after = newEntry.getField(field).orElse(null); if (!Objects.equals(before, after)) { - undoEdit.applyEdit(new UndoableFieldChange(oldEntry, field, before, after)); + entryEdit.applyEdit(new UndoableFieldChange(oldEntry, field, before, after)); } } + undoEdit.addEdit(entryEdit); } } From 5d60bd318ace994b79f8a883507c5bab99c2c799 Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Mon, 7 Sep 2026 13:35:21 +0200 Subject: [PATCH 08/27] Extract the entry classification and the scan callback into methods Also compares entry types null-safely and documents why the preference listener cannot accumulate. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01GWmXyN3GUNhfF9fSo1UNrt --- .../gui/collab/DatabaseChangeMonitor.java | 22 +++++++++------ .../jabref/gui/collab/LibraryBaseline.java | 28 ++++++++++++------- .../gui/collab/entrychange/EntryChange.java | 2 +- 3 files changed, 32 insertions(+), 20 deletions(-) diff --git a/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java b/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java index 8b4988628dba..ba8853235caa 100644 --- a/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java +++ b/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java @@ -93,6 +93,8 @@ public DatabaseChangeMonitor(BibDatabaseContext database, } catch (IOException e) { LOGGER.error("Error while trying to monitor {}", path, e); } + // Registered once per monitor; a tab replacing its monitor calls unregister() on the old one first, which + // removes this listener again, so the preference never accumulates listeners if (database.getLocation() == DatabaseLocation.LOCAL) { preferences.getLibraryPreferences().autoSaveProperty().addListener(synchronizingListener); } @@ -240,15 +242,7 @@ private void scanForChanges() { if (scannedBaseline != null && isSynchronizing()) { // [impl->req~ux.external-library-changes.synchronize~1] BackgroundTask.wrap(() -> scanner.scanForChanges(this::awaitStableFile)) - .onSuccess(changes -> { - if (generation != scanGeneration) { - LOGGER.debug("Discarding result of a scan overtaken by a newer file change"); - return; - } - // Sorting the changes on the FX thread right before applying them leaves no window - // for a user edit to slip in between classification and application - synchronize(scannedBaseline, scanner.triage(scannedBaseline, changes)); - }) + .onSuccess(changes -> onScannedForSynchronization(generation, scanner, scannedBaseline, changes)) .onFailure(e -> LOGGER.error("Error while synchronizing with the library file", e)) .executeWith(taskExecutor); return; @@ -263,6 +257,16 @@ private void scanForChanges() { .executeWith(taskExecutor); } + /// Sorting the changes on the FX thread right before applying them leaves no window for a user edit to slip in + /// between classification and application. + private void onScannedForSynchronization(int generation, ChangeScanner scanner, LibraryBaseline scannedBaseline, List changes) { + if (generation != scanGeneration) { + LOGGER.debug("Discarding result of a scan overtaken by a newer file change"); + return; + } + synchronize(scannedBaseline, scanner.triage(scannedBaseline, changes)); + } + /// Sync clients and editors may write the file in several steps. A file that is still growing must not be parsed: /// half of a library parses fine and would look like every later entry had been deleted. Waits (bounded) until /// size and modification time stop changing; the last snapshot seen becomes the known disk state. diff --git a/jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java b/jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java index 3f1b1da8f281..5ad9b721ea63 100644 --- a/jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java +++ b/jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java @@ -94,16 +94,9 @@ public Triage triage(List changes, BibDatabaseContext local, @Nu for (DatabaseChange change : pairSplitEntries(changes, local, resolverFactory)) { Side side = switch (change) { case EntryChange entryChange -> { - BibEntry base = entriesById.get(entryChange.getOldEntry().getId()); - Side entrySide = sideOf(base, entryChange.getOldEntry(), entryChange.getNewEntry(), LibraryBaseline::sameContent); - if (entrySide == Side.BOTH) { - Optional merged = mergeFields(base, entryChange.getOldEntry(), entryChange.getNewEntry()); - if (merged.isPresent()) { - change = new EntryChange(entryChange.getOldEntry(), merged.get(), local, resolverFactory); - entrySide = Side.DISK; - } - } - yield entrySide; + Classified classified = classifyEntryChange(entryChange, local, resolverFactory); + change = classified.change(); + yield classified.side(); } // No in-memory counterpart: either new on disk, or deleted in memory (and possibly modified on disk) case EntryAdd entryAdd -> @@ -248,6 +241,21 @@ private static Optional mergeFields(@Nullable BibEntry base, BibEntry return Optional.of(merged); } + private record Classified(Side side, DatabaseChange change) { + } + + /// An entry changed on both sides in different fields is merged field by field into a new, accepted change. + private Classified classifyEntryChange(EntryChange entryChange, BibDatabaseContext local, @Nullable DatabaseChangeResolverFactory resolverFactory) { + BibEntry base = entriesById.get(entryChange.getOldEntry().getId()); + Side side = sideOf(base, entryChange.getOldEntry(), entryChange.getNewEntry(), LibraryBaseline::sameContent); + if (side != Side.BOTH) { + return new Classified(side, entryChange); + } + return mergeFields(base, entryChange.getOldEntry(), entryChange.getNewEntry()) + .map(merged -> new Classified(Side.DISK, new EntryChange(entryChange.getOldEntry(), merged, local, resolverFactory))) + .orElse(new Classified(Side.BOTH, entryChange)); + } + private static boolean sameContent(BibEntry one, BibEntry other) { return one.getType().equals(other.getType()) && one.getFieldMap().equals(other.getFieldMap()); } diff --git a/jabgui/src/main/java/org/jabref/gui/collab/entrychange/EntryChange.java b/jabgui/src/main/java/org/jabref/gui/collab/entrychange/EntryChange.java index 126efc972434..8aa9fb0a821f 100644 --- a/jabgui/src/main/java/org/jabref/gui/collab/entrychange/EntryChange.java +++ b/jabgui/src/main/java/org/jabref/gui/collab/entrychange/EntryChange.java @@ -46,7 +46,7 @@ public BibEntry getNewEntry() { @Override public void applyChange(CompoundEdit undoEdit) { CompoundEdit entryEdit = new CompoundEdit(getName()); - if (!oldEntry.getType().equals(newEntry.getType())) { + if (!Objects.equals(oldEntry.getType(), newEntry.getType())) { entryEdit.applyEdit(new UndoableChangeType(oldEntry, oldEntry.getType(), newEntry.getType())); } Set fields = new LinkedHashSet<>(oldEntry.getFields()); From 0d21f34c927649d19d41e6b0c631cdbf563d9fd6 Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Mon, 7 Sep 2026 13:46:28 +0200 Subject: [PATCH 09/27] Remove the preference listener regardless of the library's current location Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01GWmXyN3GUNhfF9fSo1UNrt --- .../java/org/jabref/gui/collab/DatabaseChangeMonitor.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java b/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java index ba8853235caa..de9778961a86 100644 --- a/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java +++ b/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java @@ -339,9 +339,9 @@ public void unregister() { scanGeneration++; monitoredPath.ifPresent(path -> { fileMonitor.removeListener(path, this); - if (database.getLocation() == DatabaseLocation.LOCAL) { - preferences.getLibraryPreferences().autoSaveProperty().removeListener(synchronizingListener); - } + // Unconditionally: the library may have been converted to a shared one since the listener was added, + // and removing a listener that was never added is a no-op + preferences.getLibraryPreferences().autoSaveProperty().removeListener(synchronizingListener); }); } } From c8c9bec0616a9265b6959940f34dbd52b0156bff Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Mon, 7 Sep 2026 13:47:12 +0200 Subject: [PATCH 10/27] Stub the preferences in the unregister test Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01GWmXyN3GUNhfF9fSo1UNrt --- .../java/org/jabref/gui/collab/DatabaseChangeMonitorTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/jabgui/src/test/java/org/jabref/gui/collab/DatabaseChangeMonitorTest.java b/jabgui/src/test/java/org/jabref/gui/collab/DatabaseChangeMonitorTest.java index ba03f7e8197f..8c23709f7672 100644 --- a/jabgui/src/test/java/org/jabref/gui/collab/DatabaseChangeMonitorTest.java +++ b/jabgui/src/test/java/org/jabref/gui/collab/DatabaseChangeMonitorTest.java @@ -28,6 +28,7 @@ import com.dlsc.gemsfx.infocenter.NotificationGroup; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; +import org.mockito.Answers; import org.mockito.ArgumentCaptor; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -58,7 +59,7 @@ void unregisterRemovesListenerFromOriginallyMonitoredPath(@TempDir Path tempDir) fileUpdateMonitor, mock(TaskExecutor.class), mock(DialogService.class), - mock(GuiPreferences.class), + mock(GuiPreferences.class, Answers.RETURNS_DEEP_STUBS), mock(UndoManager.class), mock(StateManager.class), mock(LibraryTab.class)); From e4e4b0995fdd532a66fb26c0ddad25a7abc40bd7 Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Mon, 7 Sep 2026 13:51:01 +0200 Subject: [PATCH 11/27] Keep the baseline as entry snapshots and index it once per scan Baseline entries are now type plus field map instead of full BibEntry copies, lookups of added entries go through a per-scan index by citation key and by key-less content, and pairing deletions with additions is a single pass. Both the classification of a scan with many added entries and the memory held per open library shrink from quadratic and kilobytes per entry to linear and one map per entry. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01GWmXyN3GUNhfF9fSo1UNrt --- .../jabref/gui/collab/LibraryBaseline.java | 173 +++++++++++------- 1 file changed, 104 insertions(+), 69 deletions(-) diff --git a/jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java b/jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java index 5ad9b721ea63..3efca5d40d6d 100644 --- a/jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java +++ b/jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java @@ -3,11 +3,12 @@ import java.nio.charset.Charset; import java.util.ArrayList; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Optional; -import java.util.function.BiPredicate; +import java.util.Set; import org.jabref.gui.collab.entryadd.EntryAdd; import org.jabref.gui.collab.entrychange.EntryChange; @@ -28,6 +29,7 @@ import org.jabref.model.entry.BibtexString; import org.jabref.model.entry.field.Field; import org.jabref.model.entry.field.InternalField; +import org.jabref.model.entry.types.EntryType; import org.jabref.model.metadata.MetaData; import org.jspecify.annotations.NullMarked; @@ -54,13 +56,47 @@ private enum Side { DISK, MEMORY, BOTH } private static final String ENCODING_KEY = "\0encoding"; - private final Map entriesById; + /// What the comparison needs of an entry; a `BibEntry` copy would carry an event bus, caches, and properties per + /// entry, which for a large library adds up to far more than the bibliographic data itself. + private record EntrySnapshot(EntryType type, Map fields) { + static EntrySnapshot of(BibEntry entry) { + return new EntrySnapshot(entry.getType(), Map.copyOf(entry.getFieldMap())); + } + + /// Wraps the live field map without copying, for a comparison made right away + static EntrySnapshot view(BibEntry entry) { + return new EntrySnapshot(entry.getType(), entry.getFieldMap()); + } + + Optional citationKey() { + return Optional.ofNullable(fields.get(InternalField.KEY_FIELD)); + } + + /// The entry without its citation key, for finding an entry whose key changed + EntrySnapshot withoutKey() { + if (!fields.containsKey(InternalField.KEY_FIELD)) { + return this; + } + Map rest = new HashMap<>(fields); + rest.remove(InternalField.KEY_FIELD); + return new EntrySnapshot(type, rest); + } + + /// Only needed as the common ancestor of a field-level merge, which is the rare both-sides case + BibEntry toEntry() { + BibEntry entry = new BibEntry(type); + fields.forEach(entry::setField); + return entry; + } + } + + private final Map entriesById; private final Map metaData; private @Nullable String preamble; private final Map strings; private final GlobalCitationKeyPatterns citationKeyPatterns; - private LibraryBaseline(Map entriesById, Map metaData, @Nullable String preamble, Map strings, GlobalCitationKeyPatterns citationKeyPatterns) { + private LibraryBaseline(Map entriesById, Map metaData, @Nullable String preamble, Map strings, GlobalCitationKeyPatterns citationKeyPatterns) { this.entriesById = entriesById; this.metaData = metaData; this.preamble = preamble; @@ -69,9 +105,9 @@ private LibraryBaseline(Map entriesById, Map m } public static LibraryBaseline of(BibDatabaseContext context, GlobalCitationKeyPatterns citationKeyPatterns) { - Map entries = new HashMap<>(); + Map entries = new HashMap<>(); for (BibEntry entry : context.getDatabase().getEntries()) { - entries.put(entry.getId(), new BibEntry(entry)); + entries.put(entry.getId(), EntrySnapshot.of(entry)); } Map strings = new HashMap<>(); for (BibtexString string : context.getDatabase().getStringValues()) { @@ -84,6 +120,32 @@ public static LibraryBaseline of(BibDatabaseContext context, GlobalCitationKeyPa citationKeyPatterns); } + /// Lookup tables over the baseline entries, built once per pass over the changes so that each lookup is O(1) + /// instead of a scan over the library. + private final class Index { + private final Map>> byKey = new HashMap<>(); + private final Map> byContentExceptKey = new HashMap<>(); + + Index() { + for (Map.Entry entry : entriesById.entrySet()) { + entry.getValue().citationKey().ifPresent(key -> byKey.computeIfAbsent(key, _ -> new ArrayList<>()).add(entry)); + byContentExceptKey.putIfAbsent(entry.getValue().withoutKey(), entry); + } + } + + /// The baseline of a disk entry without in-memory counterpart, keyed by the id of the in-memory entry it was + /// taken from: by citation key (preferring identical content, as keys need not be unique), or else by the + /// remaining content, which covers entries without a key as well as a key changed on disk. + Optional> find(BibEntry remote) { + EntrySnapshot snapshot = EntrySnapshot.view(remote); + Optional> byKeyMatch = snapshot.citationKey() + .map(key -> byKey.getOrDefault(key, List.of())) + .flatMap(candidates -> candidates.stream().filter(entry -> entry.getValue().equals(snapshot)).findFirst() + .or(() -> candidates.stream().findFirst())); + return byKeyMatch.or(() -> Optional.ofNullable(byContentExceptKey.get(snapshot.withoutKey()))); + } + } + /// Sorts the differences between the in-memory library and its file, as computed by /// [DatabaseChangeList#compareAndGetChanges], by which side changed. Entries modified on both sides in different /// fields are merged field by field (with the Git merge rules) into a new, accepted [EntryChange]. @@ -91,7 +153,8 @@ public Triage triage(List changes, BibDatabaseContext local, @Nu Triage triage = new Triage(new ArrayList<>(), new ArrayList<>(), new ArrayList<>()); // A group change is always accompanied by the metadata change it is part of, which precedes it in the list Side metaDataSide = Side.BOTH; - for (DatabaseChange change : pairSplitEntries(changes, local, resolverFactory)) { + Index index = new Index(); + for (DatabaseChange change : pairSplitEntries(changes, local, resolverFactory, index)) { Side side = switch (change) { case EntryChange entryChange -> { Classified classified = classifyEntryChange(entryChange, local, resolverFactory); @@ -100,27 +163,27 @@ public Triage triage(List changes, BibDatabaseContext local, @Nu } // No in-memory counterpart: either new on disk, or deleted in memory (and possibly modified on disk) case EntryAdd entryAdd -> - find(entryAdd.getAddedEntry()).map(base -> sameContent(base.getValue(), entryAdd.getAddedEntry()) ? Side.MEMORY : Side.BOTH) + index.find(entryAdd.getAddedEntry()).map(base -> base.getValue().equals(EntrySnapshot.view(entryAdd.getAddedEntry())) ? Side.MEMORY : Side.BOTH) .orElse(Side.DISK); // No counterpart on disk: either deleted on disk, or added in memory (and possibly modified on disk) case EntryDelete entryDelete -> { - BibEntry base = entriesById.get(entryDelete.getDeletedEntry().getId()); - yield base == null ? Side.MEMORY : sameContent(base, entryDelete.getDeletedEntry()) ? Side.DISK : Side.BOTH; + EntrySnapshot base = entriesById.get(entryDelete.getDeletedEntry().getId()); + yield base == null ? Side.MEMORY : base.equals(EntrySnapshot.view(entryDelete.getDeletedEntry())) ? Side.DISK : Side.BOTH; } case MetadataChange metadataChange -> { - metaDataSide = sideOf(metaData, serialize(local.getMetaData(), citationKeyPatterns), serialize(metadataChange.getMetaDataDiff().getNewMetaData(), citationKeyPatterns), Objects::equals); + metaDataSide = sideOf(metaData, serialize(local.getMetaData(), citationKeyPatterns), serialize(metadataChange.getMetaDataDiff().getNewMetaData(), citationKeyPatterns)); yield metaDataSide; } case GroupChange _ -> metaDataSide; case PreambleChange preambleChange -> - sideOf(preamble, local.getDatabase().getPreamble().orElse(null), preambleChange.getPreambleDiff().getNewPreamble(), Objects::equals); + sideOf(preamble, local.getDatabase().getPreamble().orElse(null), preambleChange.getPreambleDiff().getNewPreamble()); case BibTexStringAdd stringAdd -> - sideOf(strings.get(stringAdd.getAddedString().getName()), null, stringAdd.getAddedString().getContent(), Objects::equals); + sideOf(strings.get(stringAdd.getAddedString().getName()), null, stringAdd.getAddedString().getContent()); case BibTexStringDelete stringDelete -> - sideOf(strings.get(stringDelete.getDeletedString().getName()), stringDelete.getDeletedString().getContent(), null, Objects::equals); + sideOf(strings.get(stringDelete.getDeletedString().getName()), stringDelete.getDeletedString().getContent(), null); case BibTexStringChange stringChange -> - sideOf(strings.get(stringChange.getOldString().getName()), stringChange.getOldString().getContent(), stringChange.getNewString().getContent(), Objects::equals); + sideOf(strings.get(stringChange.getOldString().getName()), stringChange.getOldString().getContent(), stringChange.getNewString().getContent()); case BibTexStringRename stringRename -> { boolean oldUntouchedInMemory = Objects.equals(strings.get(stringRename.getOldString().getName()), stringRename.getOldString().getContent()); yield oldUntouchedInMemory && !strings.containsKey(stringRename.getNewString().getName()) ? Side.DISK : Side.BOTH; @@ -143,6 +206,7 @@ public Triage triage(List changes, BibDatabaseContext local, @Nu /// Carries over the baseline of every item whose external change was not applied, so that the next scan sees /// the same divergence again instead of mistaking the current in-memory version for the common ancestor. public void keepUnresolved(LibraryBaseline previous, List unresolved) { + Index previousIndex = previous.new Index(); for (DatabaseChange change : unresolved) { switch (change) { case EntryChange entryChange -> @@ -150,7 +214,7 @@ public void keepUnresolved(LibraryBaseline previous, List unreso case EntryDelete entryDelete -> keepEntry(previous, entryDelete.getDeletedEntry().getId()); case EntryAdd entryAdd -> - previous.find(entryAdd.getAddedEntry()).ifPresent(base -> entriesById.put(base.getKey(), base.getValue())); + previousIndex.find(entryAdd.getAddedEntry()).ifPresent(base -> entriesById.put(base.getKey(), base.getValue())); case MetadataChange _, GroupChange _ -> { metaData.clear(); @@ -174,56 +238,55 @@ public void keepUnresolved(LibraryBaseline previous, List unreso /// The two-way diff pairs entries by similarity, so an entry whose citation key changed on one side while fields /// changed on the other can fall below the similarity threshold and show up as a deletion plus an addition. The - /// baseline knows both belong to the same entry. - private List pairSplitEntries(List changes, BibDatabaseContext local, @Nullable DatabaseChangeResolverFactory resolverFactory) { + /// baseline knows both belong to the same entry. One pass over the changes: a paired addition becomes the change, + /// its deletion is dropped. + private List pairSplitEntries(List changes, BibDatabaseContext local, @Nullable DatabaseChangeResolverFactory resolverFactory, Index index) { Map deletesByBaseId = new HashMap<>(); for (DatabaseChange change : changes) { - if (change instanceof EntryDelete entryDelete) { - String baseId = entryDelete.getDeletedEntry().getId(); - if (entriesById.containsKey(baseId)) { - deletesByBaseId.put(baseId, entryDelete); - } + if (change instanceof EntryDelete entryDelete && entriesById.containsKey(entryDelete.getDeletedEntry().getId())) { + deletesByBaseId.put(entryDelete.getDeletedEntry().getId(), entryDelete); } } if (deletesByBaseId.isEmpty()) { return changes; } - List paired = new ArrayList<>(changes); + Map replacements = new HashMap<>(); + Set pairedDeletes = new HashSet<>(); for (DatabaseChange change : changes) { if (change instanceof EntryAdd entryAdd) { - find(entryAdd.getAddedEntry()).map(base -> deletesByBaseId.remove(base.getKey())).ifPresent(entryDelete -> { - paired.remove(entryDelete); - paired.set(paired.indexOf(entryAdd), new EntryChange(entryDelete.getDeletedEntry(), entryAdd.getAddedEntry(), local, resolverFactory)); + index.find(entryAdd.getAddedEntry()).map(base -> deletesByBaseId.remove(base.getKey())).ifPresent(entryDelete -> { + pairedDeletes.add(entryDelete); + replacements.put(entryAdd, new EntryChange(entryDelete.getDeletedEntry(), entryAdd.getAddedEntry(), local, resolverFactory)); }); } } + List paired = new ArrayList<>(changes.size()); + for (DatabaseChange change : changes) { + if (change instanceof EntryDelete entryDelete && pairedDeletes.contains(entryDelete)) { + continue; + } + paired.add(change instanceof EntryAdd entryAdd ? replacements.getOrDefault(entryAdd, entryAdd) : change); + } return paired; } /// Which side diverged from the baseline. Only called for items that differ between memory and disk, so at least /// one side changed. `null` stands for "absent" on that side. - private static Side sideOf(@Nullable T base, @Nullable T local, @Nullable T remote, BiPredicate same) { - boolean localChanged = !same(base, local, same); - boolean remoteChanged = !same(base, remote, same); + private static Side sideOf(@Nullable T base, @Nullable T local, @Nullable T remote) { + boolean localChanged = !Objects.equals(base, local); + boolean remoteChanged = !Objects.equals(base, remote); if (localChanged && remoteChanged) { return Side.BOTH; } return remoteChanged ? Side.DISK : Side.MEMORY; } - private static boolean same(@Nullable T one, @Nullable T other, BiPredicate same) { - if (one == null || other == null) { - return one == other; - } - return same.test(one, other); - } - /// Field-level merge of an entry modified on both sides: disk wins for every field memory left alone, memory wins /// where disk left the field alone. Empty if the same field was changed differently on both sides. - private static Optional mergeFields(@Nullable BibEntry base, BibEntry local, BibEntry remote) { + private static Optional mergeFields(@Nullable EntrySnapshot base, BibEntry local, BibEntry remote) { // Without a common ancestor (entry new on both sides), an empty entry of the in-memory type makes every field // an addition, so only fields set differently on both sides count as conflicts and the in-memory type is kept - BibEntry ancestor = base == null ? new BibEntry(local.getType()) : base; + BibEntry ancestor = base == null ? new BibEntry(local.getType()) : base.toEntry(); if (ConflictRules.hasConflictingFields(ancestor, local, remote)) { return Optional.empty(); } @@ -246,8 +309,8 @@ private record Classified(Side side, DatabaseChange change) { /// An entry changed on both sides in different fields is merged field by field into a new, accepted change. private Classified classifyEntryChange(EntryChange entryChange, BibDatabaseContext local, @Nullable DatabaseChangeResolverFactory resolverFactory) { - BibEntry base = entriesById.get(entryChange.getOldEntry().getId()); - Side side = sideOf(base, entryChange.getOldEntry(), entryChange.getNewEntry(), LibraryBaseline::sameContent); + EntrySnapshot base = entriesById.get(entryChange.getOldEntry().getId()); + Side side = sideOf(base, EntrySnapshot.view(entryChange.getOldEntry()), EntrySnapshot.view(entryChange.getNewEntry())); if (side != Side.BOTH) { return new Classified(side, entryChange); } @@ -256,36 +319,8 @@ private Classified classifyEntryChange(EntryChange entryChange, BibDatabaseConte .orElse(new Classified(Side.BOTH, entryChange)); } - private static boolean sameContent(BibEntry one, BibEntry other) { - return one.getType().equals(other.getType()) && one.getFieldMap().equals(other.getFieldMap()); - } - - /// Looks up the baseline of a disk entry without in-memory counterpart, keyed by the id of the in-memory entry it - /// was taken from: by citation key (preferring identical content, as keys need not be unique), or else by the - /// remaining content, which covers entries without a key as well as a key changed on disk. - private Optional> find(BibEntry remote) { - Optional> byKey = remote.getCitationKey().flatMap(key -> { - List> candidates = entriesById.entrySet().stream() - .filter(entry -> entry.getValue().getCitationKey().filter(key::equals).isPresent()) - .toList(); - return candidates.stream().filter(entry -> sameContent(entry.getValue(), remote)).findFirst() - .or(() -> candidates.stream().findFirst()); - }); - return byKey.or(() -> entriesById.entrySet().stream() - .filter(entry -> sameContentExceptKey(entry.getValue(), remote)) - .findFirst()); - } - - private static boolean sameContentExceptKey(BibEntry one, BibEntry other) { - Map oneFields = new HashMap<>(one.getFieldMap()); - Map otherFields = new HashMap<>(other.getFieldMap()); - oneFields.remove(InternalField.KEY_FIELD); - otherFields.remove(InternalField.KEY_FIELD); - return one.getType().equals(other.getType()) && oneFields.equals(otherFields); - } - private void keepEntry(LibraryBaseline previous, String entryId) { - BibEntry base = previous.entriesById.get(entryId); + EntrySnapshot base = previous.entriesById.get(entryId); if (base == null) { entriesById.remove(entryId); } else { From 4b200afe95d52333003b3430732c4b4234a3dbcd Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Mon, 7 Sep 2026 13:57:51 +0200 Subject: [PATCH 12/27] Apply IDEA formatting Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01GWmXyN3GUNhfF9fSo1UNrt --- .../main/java/org/jabref/gui/collab/LibraryBaseline.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java b/jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java index 3efca5d40d6d..58c431f9f5d4 100644 --- a/jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java +++ b/jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java @@ -139,9 +139,9 @@ private final class Index { Optional> find(BibEntry remote) { EntrySnapshot snapshot = EntrySnapshot.view(remote); Optional> byKeyMatch = snapshot.citationKey() - .map(key -> byKey.getOrDefault(key, List.of())) - .flatMap(candidates -> candidates.stream().filter(entry -> entry.getValue().equals(snapshot)).findFirst() - .or(() -> candidates.stream().findFirst())); + .map(key -> byKey.getOrDefault(key, List.of())) + .flatMap(candidates -> candidates.stream().filter(entry -> entry.getValue().equals(snapshot)).findFirst() + .or(() -> candidates.stream().findFirst())); return byKeyMatch.or(() -> Optional.ofNullable(byContentExceptKey.get(snapshot.withoutKey()))); } } @@ -164,7 +164,7 @@ public Triage triage(List changes, BibDatabaseContext local, @Nu // No in-memory counterpart: either new on disk, or deleted in memory (and possibly modified on disk) case EntryAdd entryAdd -> index.find(entryAdd.getAddedEntry()).map(base -> base.getValue().equals(EntrySnapshot.view(entryAdd.getAddedEntry())) ? Side.MEMORY : Side.BOTH) - .orElse(Side.DISK); + .orElse(Side.DISK); // No counterpart on disk: either deleted on disk, or added in memory (and possibly modified on disk) case EntryDelete entryDelete -> { EntrySnapshot base = entriesById.get(entryDelete.getDeletedEntry().getId()); From 278f2c72deca2053537145288fd13bc7f74457cb Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Mon, 7 Sep 2026 20:12:20 +0200 Subject: [PATCH 13/27] Move the synchronization policy into jablib The three-way classification and the field-level merge now live in org.jabref.logic.sync.LibraryBaseline; the GUI keeps only the mapping of its change objects onto that policy. Switching synchronization on withdraws a review offered while it was off and scans the file afresh, and a completed review updates the baseline so accepted changes do not reappear as conflicts. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01GWmXyN3GUNhfF9fSo1UNrt --- .../org/jabref/gui/collab/ChangeScanner.java | 7 +- .../org/jabref/gui/collab/ChangeTriage.java | 169 +++++++++ .../gui/collab/DatabaseChangeMonitor.java | 35 +- .../jabref/gui/collab/LibraryBaseline.java | 347 ------------------ ...aselineTest.java => ChangeTriageTest.java} | 43 +-- jablib/src/main/java/module-info.java | 1 + .../jabref/logic/sync/LibraryBaseline.java | 267 ++++++++++++++ .../org/jabref/logic/sync/package-info.java | 14 + .../logic/sync/LibraryBaselineTest.java | 128 +++++++ 9 files changed, 635 insertions(+), 376 deletions(-) create mode 100644 jabgui/src/main/java/org/jabref/gui/collab/ChangeTriage.java delete mode 100644 jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java rename jabgui/src/test/java/org/jabref/gui/collab/{LibraryBaselineTest.java => ChangeTriageTest.java} (88%) create mode 100644 jablib/src/main/java/org/jabref/logic/sync/LibraryBaseline.java create mode 100644 jablib/src/main/java/org/jabref/logic/sync/package-info.java create mode 100644 jablib/src/test/java/org/jabref/logic/sync/LibraryBaselineTest.java diff --git a/jabgui/src/main/java/org/jabref/gui/collab/ChangeScanner.java b/jabgui/src/main/java/org/jabref/gui/collab/ChangeScanner.java index 09cd43d3abfb..7f2ad9da7ca0 100644 --- a/jabgui/src/main/java/org/jabref/gui/collab/ChangeScanner.java +++ b/jabgui/src/main/java/org/jabref/gui/collab/ChangeScanner.java @@ -10,6 +10,7 @@ import org.jabref.logic.importer.ImportFormatPreferences; import org.jabref.logic.importer.OpenDatabase; import org.jabref.logic.importer.ParserResult; +import org.jabref.logic.sync.LibraryBaseline; import org.jabref.model.database.BibDatabaseContext; import org.jabref.model.util.DummyFileUpdateMonitor; @@ -53,9 +54,9 @@ public List scanForChanges(Runnable beforeParsing) { } } - /// @return the given external changes sorted by the side they happened on, see [LibraryBaseline#triage] - public LibraryBaseline.Triage triage(LibraryBaseline baseline, List changes) { - return baseline.triage(changes, database, databaseChangeResolverFactory); + /// @return the given external changes sorted by the side they happened on, see [ChangeTriage#triage] + public ChangeTriage.Triage triage(LibraryBaseline baseline, List changes) { + return ChangeTriage.triage(baseline, changes, database, databaseChangeResolverFactory); } public List getDatabaseChanges(Path fileToCompare) throws IOException { diff --git a/jabgui/src/main/java/org/jabref/gui/collab/ChangeTriage.java b/jabgui/src/main/java/org/jabref/gui/collab/ChangeTriage.java new file mode 100644 index 000000000000..366dc004189f --- /dev/null +++ b/jabgui/src/main/java/org/jabref/gui/collab/ChangeTriage.java @@ -0,0 +1,169 @@ +package org.jabref.gui.collab; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; + +import org.jabref.gui.collab.entryadd.EntryAdd; +import org.jabref.gui.collab.entrychange.EntryChange; +import org.jabref.gui.collab.entrydelete.EntryDelete; +import org.jabref.gui.collab.groupchange.GroupChange; +import org.jabref.gui.collab.metedatachange.MetadataChange; +import org.jabref.gui.collab.preamblechange.PreambleChange; +import org.jabref.gui.collab.stringadd.BibTexStringAdd; +import org.jabref.gui.collab.stringchange.BibTexStringChange; +import org.jabref.gui.collab.stringdelete.BibTexStringDelete; +import org.jabref.gui.collab.stringrename.BibTexStringRename; +import org.jabref.logic.sync.LibraryBaseline; +import org.jabref.logic.sync.LibraryBaseline.Side; +import org.jabref.model.database.BibDatabaseContext; + +import org.jspecify.annotations.NullMarked; +import org.jspecify.annotations.Nullable; + +/// Sorts the external changes of a library, as computed by [DatabaseChangeList#compareAndGetChanges], by the side +/// that changed according to a [LibraryBaseline]. The policy lives in the baseline; this class only maps the GUI's +/// [DatabaseChange] objects onto it. +@NullMarked +public final class ChangeTriage { + + /// Outcome of comparing external changes against the baseline. + /// + /// @param diskOnly changes to items untouched in memory; already accepted, to be applied without asking + /// @param bothSides changes to items that were modified in memory as well, in a way that cannot be merged automatically; need a review + /// @param memoryOnly not external changes at all: differences caused by unsaved in-memory edits; to be dropped + public record Triage(List diskOnly, List bothSides, List memoryOnly) { + } + + private ChangeTriage() { + } + + /// Entries modified on both sides in different fields are merged field by field into a new, accepted [EntryChange]. + public static Triage triage(LibraryBaseline baseline, List changes, BibDatabaseContext local, @Nullable DatabaseChangeResolverFactory resolverFactory) { + Triage triage = new Triage(new ArrayList<>(), new ArrayList<>(), new ArrayList<>()); + // A group change is always accompanied by the metadata change it is part of, which precedes it in the list + Side metaDataSide = Side.BOTH; + LibraryBaseline.Lookup lookup = baseline.lookup(); + for (DatabaseChange change : pairSplitEntries(baseline, lookup, changes, local, resolverFactory)) { + Side side = switch (change) { + case EntryChange entryChange -> { + Side entrySide = baseline.sideOfEntry(entryChange.getOldEntry(), entryChange.getNewEntry()); + if (entrySide == Side.BOTH) { + BibEntryMerge merge = mergeEntry(baseline, entryChange, local, resolverFactory); + change = merge.change(); + entrySide = merge.side(); + } + yield entrySide; + } + case EntryAdd entryAdd -> + lookup.sideOfAddedEntry(entryAdd.getAddedEntry()); + case EntryDelete entryDelete -> + baseline.sideOfDeletedEntry(entryDelete.getDeletedEntry()); + case MetadataChange metadataChange -> { + metaDataSide = baseline.sideOfMetaData(local.getMetaData(), metadataChange.getMetaDataDiff().getNewMetaData()); + yield metaDataSide; + } + case GroupChange _ -> + metaDataSide; + case PreambleChange preambleChange -> + baseline.sideOfPreamble(local.getDatabase().getPreamble().orElse(null), preambleChange.getPreambleDiff().getNewPreamble()); + case BibTexStringAdd stringAdd -> + baseline.sideOfString(stringAdd.getAddedString().getName(), null, stringAdd.getAddedString().getContent()); + case BibTexStringDelete stringDelete -> + baseline.sideOfString(stringDelete.getDeletedString().getName(), stringDelete.getDeletedString().getContent(), null); + case BibTexStringChange stringChange -> + baseline.sideOfString(stringChange.getOldString().getName(), stringChange.getOldString().getContent(), stringChange.getNewString().getContent()); + case BibTexStringRename stringRename -> + baseline.sideOfStringRename(stringRename.getOldString().getName(), stringRename.getOldString().getContent(), stringRename.getNewString().getName()); + }; + switch (side) { + case DISK -> { + change.accept(); + triage.diskOnly().add(change); + } + case BOTH -> + triage.bothSides().add(change); + case MEMORY -> + triage.memoryOnly().add(change); + } + } + return triage; + } + + /// Carries over the baseline of every item whose external change was not applied, so that the next scan sees + /// the same divergence again instead of mistaking the current in-memory version for the common ancestor. + public static void keepUnresolved(LibraryBaseline updated, LibraryBaseline previous, List unresolved) { + LibraryBaseline.Lookup previousLookup = previous.lookup(); + for (DatabaseChange change : unresolved) { + switch (change) { + case EntryChange entryChange -> + updated.keepEntry(previous, entryChange.getOldEntry().getId()); + case EntryDelete entryDelete -> + updated.keepEntry(previous, entryDelete.getDeletedEntry().getId()); + case EntryAdd entryAdd -> + updated.keepEntryFor(previous, previousLookup, entryAdd.getAddedEntry()); + case MetadataChange _, + GroupChange _ -> + updated.keepMetaData(previous); + case PreambleChange _ -> + updated.keepPreamble(previous); + case BibTexStringAdd stringAdd -> + updated.keepString(previous, stringAdd.getAddedString().getName()); + case BibTexStringDelete stringDelete -> + updated.keepString(previous, stringDelete.getDeletedString().getName()); + case BibTexStringChange stringChange -> + updated.keepString(previous, stringChange.getOldString().getName()); + case BibTexStringRename stringRename -> { + updated.keepString(previous, stringRename.getOldString().getName()); + updated.keepString(previous, stringRename.getNewString().getName()); + } + } + } + } + + private record BibEntryMerge(Side side, DatabaseChange change) { + } + + private static BibEntryMerge mergeEntry(LibraryBaseline baseline, EntryChange entryChange, BibDatabaseContext local, @Nullable DatabaseChangeResolverFactory resolverFactory) { + return baseline.mergeEntry(entryChange.getOldEntry(), entryChange.getNewEntry()) + .map(merged -> new BibEntryMerge(Side.DISK, new EntryChange(entryChange.getOldEntry(), merged, local, resolverFactory))) + .orElse(new BibEntryMerge(Side.BOTH, entryChange)); + } + + /// The two-way diff pairs entries by similarity, so an entry whose citation key changed on one side while fields + /// changed on the other can fall below the similarity threshold and show up as a deletion plus an addition. The + /// baseline knows both belong to the same entry. One pass over the changes: a paired addition becomes the change, + /// its deletion is dropped. + private static List pairSplitEntries(LibraryBaseline baseline, LibraryBaseline.Lookup lookup, List changes, BibDatabaseContext local, @Nullable DatabaseChangeResolverFactory resolverFactory) { + Map deletesByBaseId = new HashMap<>(); + for (DatabaseChange change : changes) { + if (change instanceof EntryDelete entryDelete && baseline.hasEntry(entryDelete.getDeletedEntry().getId())) { + deletesByBaseId.put(entryDelete.getDeletedEntry().getId(), entryDelete); + } + } + if (deletesByBaseId.isEmpty()) { + return changes; + } + Map replacements = new HashMap<>(); + Set pairedDeletes = new HashSet<>(); + for (DatabaseChange change : changes) { + if (change instanceof EntryAdd entryAdd) { + lookup.baseIdOf(entryAdd.getAddedEntry()).map(deletesByBaseId::remove).ifPresent(entryDelete -> { + pairedDeletes.add(entryDelete); + replacements.put(entryAdd, new EntryChange(entryDelete.getDeletedEntry(), entryAdd.getAddedEntry(), local, resolverFactory)); + }); + } + } + List paired = new ArrayList<>(changes.size()); + for (DatabaseChange change : changes) { + if (change instanceof EntryDelete entryDelete && pairedDeletes.contains(entryDelete)) { + continue; + } + paired.add(change instanceof EntryAdd entryAdd ? replacements.getOrDefault(entryAdd, entryAdd) : change); + } + return paired; + } +} diff --git a/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java b/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java index de9778961a86..eeefad182af2 100644 --- a/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java +++ b/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java @@ -16,6 +16,7 @@ import org.jabref.gui.preferences.GuiPreferences; import org.jabref.logic.l10n.Localization; import org.jabref.logic.shared.DatabaseLocation; +import org.jabref.logic.sync.LibraryBaseline; import org.jabref.logic.undo.UndoManager; import org.jabref.logic.util.BackgroundTask; import org.jabref.logic.util.TaskExecutor; @@ -138,9 +139,9 @@ public ExternalLibraryChangeNotification(List changes) { Localization.lang("External Changes Resolver")); Optional areAllChangesResolved = dialogService.showCustomDialogAndWait(databaseChangesResolverDialog); if (areAllChangesResolved.orElse(false)) { - applyResolvedChanges( - databaseChangesResolverDialog.getResolvedChanges(), - databaseChangesResolverDialog.resolvedChangesMatchDisk()); + List resolved = databaseChangesResolverDialog.getResolvedChanges(); + applyResolvedChanges(resolved, databaseChangesResolverDialog.resolvedChangesMatchDisk()); + rebaseAfterReview(resolved); clearActiveNotification(this); return OnClickBehaviour.REMOVE; @@ -205,12 +206,23 @@ private boolean isSynchronizing() { /// Synchronization switched on for an open library needs a baseline right away: as long as the library is /// unmodified, it still matches its file. Otherwise the next save establishes the baseline. private void onSynchronizingChanged(boolean enabled) { + boolean captured = false; synchronized (database) { if (!enabled) { baseline = null; scanGeneration++; } else if (baseline == null && !libraryTab.isModified()) { baseline = captureBaseline(); + captured = baseline != null; + } + } + if (captured) { + // A review offered while synchronization was off holds changes computed against an older state; from now + // on the scan decides, so the pending review is withdrawn and the file is looked at again + Optional.ofNullable(activeNotification).ifPresent(ExternalLibraryChangeNotification::remove); + activeNotification = null; + synchronized (database) { + scanForChanges(); } } } @@ -295,7 +307,7 @@ private void awaitStableFile() { } /// Applies what changed on disk only, and offers the review for what changed on both sides. - private void synchronize(LibraryBaseline scannedBaseline, LibraryBaseline.Triage triage) { + private void synchronize(LibraryBaseline scannedBaseline, ChangeTriage.Triage triage) { List unresolved = new ArrayList<>(triage.bothSides()); unresolved.addAll(triage.memoryOnly()); if (!triage.diskOnly().isEmpty()) { @@ -305,7 +317,7 @@ private void synchronize(LibraryBaseline scannedBaseline, LibraryBaseline.Triage synchronized (database) { LibraryBaseline updated = captureBaseline(); if (updated != null) { - updated.keepUnresolved(scannedBaseline, unresolved); + ChangeTriage.keepUnresolved(updated, scannedBaseline, unresolved); } baseline = updated; } @@ -314,6 +326,19 @@ private void synchronize(LibraryBaseline scannedBaseline, LibraryBaseline.Triage } } + /// After a review, the accepted changes are in memory and must not count as a divergence anymore; the rejected + /// ones keep their ancestor, so that the next scan reports them again instead of taking memory for the ancestor. + private void rebaseAfterReview(List resolved) { + synchronized (database) { + LibraryBaseline previous = baseline; + LibraryBaseline updated = captureBaseline(); + if (updated != null && previous != null) { + ChangeTriage.keepUnresolved(updated, previous, resolved.stream().filter(change -> !change.isAccepted()).toList()); + } + baseline = updated; + } + } + /// Applies the accepted external changes and updates the library's dirty state. /// /// @param resolvedChanges the externally resolved changes to apply to the in-memory database diff --git a/jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java b/jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java deleted file mode 100644 index 58c431f9f5d4..000000000000 --- a/jabgui/src/main/java/org/jabref/gui/collab/LibraryBaseline.java +++ /dev/null @@ -1,347 +0,0 @@ -package org.jabref.gui.collab; - -import java.nio.charset.Charset; -import java.util.ArrayList; -import java.util.HashMap; -import java.util.HashSet; -import java.util.List; -import java.util.Map; -import java.util.Objects; -import java.util.Optional; -import java.util.Set; - -import org.jabref.gui.collab.entryadd.EntryAdd; -import org.jabref.gui.collab.entrychange.EntryChange; -import org.jabref.gui.collab.entrydelete.EntryDelete; -import org.jabref.gui.collab.groupchange.GroupChange; -import org.jabref.gui.collab.metedatachange.MetadataChange; -import org.jabref.gui.collab.preamblechange.PreambleChange; -import org.jabref.gui.collab.stringadd.BibTexStringAdd; -import org.jabref.gui.collab.stringchange.BibTexStringChange; -import org.jabref.gui.collab.stringdelete.BibTexStringDelete; -import org.jabref.gui.collab.stringrename.BibTexStringRename; -import org.jabref.logic.citationkeypattern.GlobalCitationKeyPatterns; -import org.jabref.logic.exporter.MetaDataSerializer; -import org.jabref.logic.git.merge.planning.util.ConflictRules; -import org.jabref.logic.git.merge.planning.util.FieldPatchComputer; -import org.jabref.model.database.BibDatabaseContext; -import org.jabref.model.entry.BibEntry; -import org.jabref.model.entry.BibtexString; -import org.jabref.model.entry.field.Field; -import org.jabref.model.entry.field.InternalField; -import org.jabref.model.entry.types.EntryType; -import org.jabref.model.metadata.MetaData; - -import org.jspecify.annotations.NullMarked; -import org.jspecify.annotations.Nullable; - -/// The library as it last matched its file on disk. Comparing both the in-memory library and the file against it tells -/// for each external change whether the same item was also modified in memory, which is the only case that needs a -/// review; everything else is merged silently (see [#triage]). -/// -/// Entries are keyed by the id of the in-memory entry they were copied from, so that a citation key change in memory -/// does not break the association. The metadata is kept in its serialized form, which is the only way to snapshot it. -@NullMarked -public final class LibraryBaseline { - - /// Outcome of comparing external changes against the baseline. - /// - /// @param diskOnly changes to items untouched in memory; already accepted, to be applied without asking - /// @param bothSides changes to items that were modified in memory as well, in a way that cannot be merged automatically; need a review - /// @param memoryOnly not external changes at all: differences caused by unsaved in-memory edits; to be dropped - public record Triage(List diskOnly, List bothSides, List memoryOnly) { - } - - private enum Side { DISK, MEMORY, BOTH } - - private static final String ENCODING_KEY = "\0encoding"; - - /// What the comparison needs of an entry; a `BibEntry` copy would carry an event bus, caches, and properties per - /// entry, which for a large library adds up to far more than the bibliographic data itself. - private record EntrySnapshot(EntryType type, Map fields) { - static EntrySnapshot of(BibEntry entry) { - return new EntrySnapshot(entry.getType(), Map.copyOf(entry.getFieldMap())); - } - - /// Wraps the live field map without copying, for a comparison made right away - static EntrySnapshot view(BibEntry entry) { - return new EntrySnapshot(entry.getType(), entry.getFieldMap()); - } - - Optional citationKey() { - return Optional.ofNullable(fields.get(InternalField.KEY_FIELD)); - } - - /// The entry without its citation key, for finding an entry whose key changed - EntrySnapshot withoutKey() { - if (!fields.containsKey(InternalField.KEY_FIELD)) { - return this; - } - Map rest = new HashMap<>(fields); - rest.remove(InternalField.KEY_FIELD); - return new EntrySnapshot(type, rest); - } - - /// Only needed as the common ancestor of a field-level merge, which is the rare both-sides case - BibEntry toEntry() { - BibEntry entry = new BibEntry(type); - fields.forEach(entry::setField); - return entry; - } - } - - private final Map entriesById; - private final Map metaData; - private @Nullable String preamble; - private final Map strings; - private final GlobalCitationKeyPatterns citationKeyPatterns; - - private LibraryBaseline(Map entriesById, Map metaData, @Nullable String preamble, Map strings, GlobalCitationKeyPatterns citationKeyPatterns) { - this.entriesById = entriesById; - this.metaData = metaData; - this.preamble = preamble; - this.strings = strings; - this.citationKeyPatterns = citationKeyPatterns; - } - - public static LibraryBaseline of(BibDatabaseContext context, GlobalCitationKeyPatterns citationKeyPatterns) { - Map entries = new HashMap<>(); - for (BibEntry entry : context.getDatabase().getEntries()) { - entries.put(entry.getId(), EntrySnapshot.of(entry)); - } - Map strings = new HashMap<>(); - for (BibtexString string : context.getDatabase().getStringValues()) { - strings.put(string.getName(), string.getContent()); - } - return new LibraryBaseline(entries, - serialize(context.getMetaData(), citationKeyPatterns), - context.getDatabase().getPreamble().orElse(null), - strings, - citationKeyPatterns); - } - - /// Lookup tables over the baseline entries, built once per pass over the changes so that each lookup is O(1) - /// instead of a scan over the library. - private final class Index { - private final Map>> byKey = new HashMap<>(); - private final Map> byContentExceptKey = new HashMap<>(); - - Index() { - for (Map.Entry entry : entriesById.entrySet()) { - entry.getValue().citationKey().ifPresent(key -> byKey.computeIfAbsent(key, _ -> new ArrayList<>()).add(entry)); - byContentExceptKey.putIfAbsent(entry.getValue().withoutKey(), entry); - } - } - - /// The baseline of a disk entry without in-memory counterpart, keyed by the id of the in-memory entry it was - /// taken from: by citation key (preferring identical content, as keys need not be unique), or else by the - /// remaining content, which covers entries without a key as well as a key changed on disk. - Optional> find(BibEntry remote) { - EntrySnapshot snapshot = EntrySnapshot.view(remote); - Optional> byKeyMatch = snapshot.citationKey() - .map(key -> byKey.getOrDefault(key, List.of())) - .flatMap(candidates -> candidates.stream().filter(entry -> entry.getValue().equals(snapshot)).findFirst() - .or(() -> candidates.stream().findFirst())); - return byKeyMatch.or(() -> Optional.ofNullable(byContentExceptKey.get(snapshot.withoutKey()))); - } - } - - /// Sorts the differences between the in-memory library and its file, as computed by - /// [DatabaseChangeList#compareAndGetChanges], by which side changed. Entries modified on both sides in different - /// fields are merged field by field (with the Git merge rules) into a new, accepted [EntryChange]. - public Triage triage(List changes, BibDatabaseContext local, @Nullable DatabaseChangeResolverFactory resolverFactory) { - Triage triage = new Triage(new ArrayList<>(), new ArrayList<>(), new ArrayList<>()); - // A group change is always accompanied by the metadata change it is part of, which precedes it in the list - Side metaDataSide = Side.BOTH; - Index index = new Index(); - for (DatabaseChange change : pairSplitEntries(changes, local, resolverFactory, index)) { - Side side = switch (change) { - case EntryChange entryChange -> { - Classified classified = classifyEntryChange(entryChange, local, resolverFactory); - change = classified.change(); - yield classified.side(); - } - // No in-memory counterpart: either new on disk, or deleted in memory (and possibly modified on disk) - case EntryAdd entryAdd -> - index.find(entryAdd.getAddedEntry()).map(base -> base.getValue().equals(EntrySnapshot.view(entryAdd.getAddedEntry())) ? Side.MEMORY : Side.BOTH) - .orElse(Side.DISK); - // No counterpart on disk: either deleted on disk, or added in memory (and possibly modified on disk) - case EntryDelete entryDelete -> { - EntrySnapshot base = entriesById.get(entryDelete.getDeletedEntry().getId()); - yield base == null ? Side.MEMORY : base.equals(EntrySnapshot.view(entryDelete.getDeletedEntry())) ? Side.DISK : Side.BOTH; - } - case MetadataChange metadataChange -> { - metaDataSide = sideOf(metaData, serialize(local.getMetaData(), citationKeyPatterns), serialize(metadataChange.getMetaDataDiff().getNewMetaData(), citationKeyPatterns)); - yield metaDataSide; - } - case GroupChange _ -> - metaDataSide; - case PreambleChange preambleChange -> - sideOf(preamble, local.getDatabase().getPreamble().orElse(null), preambleChange.getPreambleDiff().getNewPreamble()); - case BibTexStringAdd stringAdd -> - sideOf(strings.get(stringAdd.getAddedString().getName()), null, stringAdd.getAddedString().getContent()); - case BibTexStringDelete stringDelete -> - sideOf(strings.get(stringDelete.getDeletedString().getName()), stringDelete.getDeletedString().getContent(), null); - case BibTexStringChange stringChange -> - sideOf(strings.get(stringChange.getOldString().getName()), stringChange.getOldString().getContent(), stringChange.getNewString().getContent()); - case BibTexStringRename stringRename -> { - boolean oldUntouchedInMemory = Objects.equals(strings.get(stringRename.getOldString().getName()), stringRename.getOldString().getContent()); - yield oldUntouchedInMemory && !strings.containsKey(stringRename.getNewString().getName()) ? Side.DISK : Side.BOTH; - } - }; - switch (side) { - case DISK -> { - change.accept(); - triage.diskOnly().add(change); - } - case BOTH -> - triage.bothSides().add(change); - case MEMORY -> - triage.memoryOnly().add(change); - } - } - return triage; - } - - /// Carries over the baseline of every item whose external change was not applied, so that the next scan sees - /// the same divergence again instead of mistaking the current in-memory version for the common ancestor. - public void keepUnresolved(LibraryBaseline previous, List unresolved) { - Index previousIndex = previous.new Index(); - for (DatabaseChange change : unresolved) { - switch (change) { - case EntryChange entryChange -> - keepEntry(previous, entryChange.getOldEntry().getId()); - case EntryDelete entryDelete -> - keepEntry(previous, entryDelete.getDeletedEntry().getId()); - case EntryAdd entryAdd -> - previousIndex.find(entryAdd.getAddedEntry()).ifPresent(base -> entriesById.put(base.getKey(), base.getValue())); - case MetadataChange _, - GroupChange _ -> { - metaData.clear(); - metaData.putAll(previous.metaData); - } - case PreambleChange _ -> - preamble = previous.preamble; - case BibTexStringAdd stringAdd -> - keepString(previous, stringAdd.getAddedString().getName()); - case BibTexStringDelete stringDelete -> - keepString(previous, stringDelete.getDeletedString().getName()); - case BibTexStringChange stringChange -> - keepString(previous, stringChange.getOldString().getName()); - case BibTexStringRename stringRename -> { - keepString(previous, stringRename.getOldString().getName()); - keepString(previous, stringRename.getNewString().getName()); - } - } - } - } - - /// The two-way diff pairs entries by similarity, so an entry whose citation key changed on one side while fields - /// changed on the other can fall below the similarity threshold and show up as a deletion plus an addition. The - /// baseline knows both belong to the same entry. One pass over the changes: a paired addition becomes the change, - /// its deletion is dropped. - private List pairSplitEntries(List changes, BibDatabaseContext local, @Nullable DatabaseChangeResolverFactory resolverFactory, Index index) { - Map deletesByBaseId = new HashMap<>(); - for (DatabaseChange change : changes) { - if (change instanceof EntryDelete entryDelete && entriesById.containsKey(entryDelete.getDeletedEntry().getId())) { - deletesByBaseId.put(entryDelete.getDeletedEntry().getId(), entryDelete); - } - } - if (deletesByBaseId.isEmpty()) { - return changes; - } - Map replacements = new HashMap<>(); - Set pairedDeletes = new HashSet<>(); - for (DatabaseChange change : changes) { - if (change instanceof EntryAdd entryAdd) { - index.find(entryAdd.getAddedEntry()).map(base -> deletesByBaseId.remove(base.getKey())).ifPresent(entryDelete -> { - pairedDeletes.add(entryDelete); - replacements.put(entryAdd, new EntryChange(entryDelete.getDeletedEntry(), entryAdd.getAddedEntry(), local, resolverFactory)); - }); - } - } - List paired = new ArrayList<>(changes.size()); - for (DatabaseChange change : changes) { - if (change instanceof EntryDelete entryDelete && pairedDeletes.contains(entryDelete)) { - continue; - } - paired.add(change instanceof EntryAdd entryAdd ? replacements.getOrDefault(entryAdd, entryAdd) : change); - } - return paired; - } - - /// Which side diverged from the baseline. Only called for items that differ between memory and disk, so at least - /// one side changed. `null` stands for "absent" on that side. - private static Side sideOf(@Nullable T base, @Nullable T local, @Nullable T remote) { - boolean localChanged = !Objects.equals(base, local); - boolean remoteChanged = !Objects.equals(base, remote); - if (localChanged && remoteChanged) { - return Side.BOTH; - } - return remoteChanged ? Side.DISK : Side.MEMORY; - } - - /// Field-level merge of an entry modified on both sides: disk wins for every field memory left alone, memory wins - /// where disk left the field alone. Empty if the same field was changed differently on both sides. - private static Optional mergeFields(@Nullable EntrySnapshot base, BibEntry local, BibEntry remote) { - // Without a common ancestor (entry new on both sides), an empty entry of the in-memory type makes every field - // an addition, so only fields set differently on both sides count as conflicts and the in-memory type is kept - BibEntry ancestor = base == null ? new BibEntry(local.getType()) : base.toEntry(); - if (ConflictRules.hasConflictingFields(ancestor, local, remote)) { - return Optional.empty(); - } - BibEntry merged = new BibEntry(local); - if (!ancestor.getType().equals(remote.getType())) { - merged.setType(remote.getType()); - } - FieldPatchComputer.compute(ancestor, local, remote).forEach((field, value) -> { - if (value == null) { - merged.clearField(field); - } else { - merged.setField(field, value); - } - }); - return Optional.of(merged); - } - - private record Classified(Side side, DatabaseChange change) { - } - - /// An entry changed on both sides in different fields is merged field by field into a new, accepted change. - private Classified classifyEntryChange(EntryChange entryChange, BibDatabaseContext local, @Nullable DatabaseChangeResolverFactory resolverFactory) { - EntrySnapshot base = entriesById.get(entryChange.getOldEntry().getId()); - Side side = sideOf(base, EntrySnapshot.view(entryChange.getOldEntry()), EntrySnapshot.view(entryChange.getNewEntry())); - if (side != Side.BOTH) { - return new Classified(side, entryChange); - } - return mergeFields(base, entryChange.getOldEntry(), entryChange.getNewEntry()) - .map(merged -> new Classified(Side.DISK, new EntryChange(entryChange.getOldEntry(), merged, local, resolverFactory))) - .orElse(new Classified(Side.BOTH, entryChange)); - } - - private void keepEntry(LibraryBaseline previous, String entryId) { - EntrySnapshot base = previous.entriesById.get(entryId); - if (base == null) { - entriesById.remove(entryId); - } else { - entriesById.put(entryId, base); - } - } - - private void keepString(LibraryBaseline previous, String name) { - String base = previous.strings.get(name); - if (base == null) { - strings.remove(name); - } else { - strings.put(name, base); - } - } - - /// The encoding is not part of the serialized metadata (it is written as a file header instead), but it is part - /// of what the two-way comparison reports. - private static Map serialize(MetaData metaData, GlobalCitationKeyPatterns citationKeyPatterns) { - Map serialized = new HashMap<>(MetaDataSerializer.getSerializedStringMap(metaData, citationKeyPatterns)); - serialized.put(ENCODING_KEY, metaData.getEncoding().map(Charset::name).orElse("")); - return serialized; - } -} diff --git a/jabgui/src/test/java/org/jabref/gui/collab/LibraryBaselineTest.java b/jabgui/src/test/java/org/jabref/gui/collab/ChangeTriageTest.java similarity index 88% rename from jabgui/src/test/java/org/jabref/gui/collab/LibraryBaselineTest.java rename to jabgui/src/test/java/org/jabref/gui/collab/ChangeTriageTest.java index f3bda0d15688..fed4fd2f554f 100644 --- a/jabgui/src/test/java/org/jabref/gui/collab/LibraryBaselineTest.java +++ b/jabgui/src/test/java/org/jabref/gui/collab/ChangeTriageTest.java @@ -9,6 +9,7 @@ import org.jabref.gui.collab.metedatachange.MetadataChange; import org.jabref.gui.collab.stringadd.BibTexStringAdd; import org.jabref.logic.citationkeypattern.GlobalCitationKeyPatterns; +import org.jabref.logic.sync.LibraryBaseline; import org.jabref.model.database.BibDatabase; import org.jabref.model.database.BibDatabaseContext; import org.jabref.model.entry.BibEntry; @@ -23,7 +24,7 @@ import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertTrue; -class LibraryBaselineTest { +class ChangeTriageTest { private static final GlobalCitationKeyPatterns PATTERNS = GlobalCitationKeyPatterns.fromPattern("[auth][year]"); @@ -47,15 +48,15 @@ void setUp() { diskContext = new BibDatabaseContext(new BibDatabase(List.of(disk, new BibEntry(other)))); } - private LibraryBaseline.Triage triage() { - return baseline.triage(DatabaseChangeList.compareAndGetChanges(localContext, diskContext, null), localContext, null); + private ChangeTriage.Triage triage() { + return ChangeTriage.triage(baseline, DatabaseChangeList.compareAndGetChanges(localContext, diskContext, null), localContext, null); } @Test void changeOnDiskOnlyIsAccepted() { disk.setField(StandardField.TITLE, "Disk title"); - LibraryBaseline.Triage triage = triage(); + ChangeTriage.Triage triage = triage(); assertEquals(1, triage.diskOnly().size()); assertTrue(triage.diskOnly().getFirst().isAccepted()); @@ -67,7 +68,7 @@ void changeOnDiskOnlyIsAccepted() { void unsavedChangeInMemoryIsNotAnExternalChange() { local.setField(StandardField.TITLE, "Memory title"); - LibraryBaseline.Triage triage = triage(); + ChangeTriage.Triage triage = triage(); assertEquals(1, triage.memoryOnly().size()); assertEquals(List.of(), triage.diskOnly()); @@ -80,7 +81,7 @@ void differentFieldsChangedOnBothSidesAreMerged() { disk.setField(StandardField.YEAR, "2021"); disk.setField(StandardField.AUTHOR, "Disk author"); - LibraryBaseline.Triage triage = triage(); + ChangeTriage.Triage triage = triage(); assertEquals(List.of(), triage.bothSides()); EntryChange merged = assertInstanceOf(EntryChange.class, triage.diskOnly().getFirst()); @@ -97,7 +98,7 @@ void sameFieldChangedDifferentlyNeedsReview() { local.setField(StandardField.TITLE, "Memory title"); disk.setField(StandardField.TITLE, "Disk title"); - LibraryBaseline.Triage triage = triage(); + ChangeTriage.Triage triage = triage(); assertEquals(1, triage.bothSides().size()); assertEquals(List.of(), triage.diskOnly()); @@ -108,7 +109,7 @@ void citationKeyChangedInMemoryStillMergesDiskChange() { local.setCitationKey("NewKey"); disk.setField(StandardField.TITLE, "Disk title"); - LibraryBaseline.Triage triage = triage(); + ChangeTriage.Triage triage = triage(); EntryChange merged = assertInstanceOf(EntryChange.class, triage.diskOnly().getFirst()); assertEquals("NewKey", merged.getNewEntry().getCitationKey().orElseThrow()); @@ -120,7 +121,7 @@ void citationKeyChangedOnDiskIsMergedIntoTheSameEntry() { disk.setCitationKey("Renamed"); disk.setField(StandardField.YEAR, "2021"); - LibraryBaseline.Triage triage = triage(); + ChangeTriage.Triage triage = triage(); EntryChange merged = assertInstanceOf(EntryChange.class, triage.diskOnly().getFirst()); assertEquals(local, merged.getOldEntry()); @@ -134,7 +135,7 @@ void encodingChangedOnDiskIsAccepted() { baseline = LibraryBaseline.of(localContext, PATTERNS); diskContext.getMetaData().setEncoding(StandardCharsets.ISO_8859_1); - LibraryBaseline.Triage triage = triage(); + ChangeTriage.Triage triage = triage(); assertInstanceOf(MetadataChange.class, triage.diskOnly().getFirst()); assertEquals(List.of(), triage.memoryOnly()); @@ -148,7 +149,7 @@ void duplicateCitationKeysAreMatchedByContent() { baseline = LibraryBaseline.of(localContext, PATTERNS); localContext.getDatabase().removeEntry(local); - LibraryBaseline.Triage triage = triage(); + ChangeTriage.Triage triage = triage(); // The entry deleted in memory is still unchanged on disk, so nothing is reported; the duplicate must not be mistaken for it assertInstanceOf(EntryAdd.class, triage.memoryOnly().getFirst()); @@ -159,7 +160,7 @@ void duplicateCitationKeysAreMatchedByContent() { void entryAddedOnDiskIsAccepted() { diskContext.getDatabase().insertEntry(new BibEntry().withCitationKey("New").withField(StandardField.TITLE, "New on disk")); - LibraryBaseline.Triage triage = triage(); + ChangeTriage.Triage triage = triage(); assertInstanceOf(EntryAdd.class, triage.diskOnly().getFirst()); assertEquals(List.of(), triage.bothSides()); @@ -169,7 +170,7 @@ void entryAddedOnDiskIsAccepted() { void entryDeletedInMemoryStaysDeletedWhenUnchangedOnDisk() { localContext.getDatabase().removeEntry(local); - LibraryBaseline.Triage triage = triage(); + ChangeTriage.Triage triage = triage(); assertInstanceOf(EntryAdd.class, triage.memoryOnly().getFirst()); assertEquals(List.of(), triage.diskOnly()); @@ -180,7 +181,7 @@ void entryDeletedInMemoryButChangedOnDiskNeedsReview() { localContext.getDatabase().removeEntry(local); disk.setField(StandardField.TITLE, "Disk title"); - LibraryBaseline.Triage triage = triage(); + ChangeTriage.Triage triage = triage(); assertInstanceOf(EntryAdd.class, triage.bothSides().getFirst()); } @@ -189,7 +190,7 @@ void entryDeletedInMemoryButChangedOnDiskNeedsReview() { void entryDeletedOnDiskIsAcceptedWhenUnchangedInMemory() { diskContext.getDatabase().removeEntry(disk); - LibraryBaseline.Triage triage = triage(); + ChangeTriage.Triage triage = triage(); assertInstanceOf(EntryDelete.class, triage.diskOnly().getFirst()); } @@ -199,7 +200,7 @@ void entryDeletedOnDiskButChangedInMemoryNeedsReview() { diskContext.getDatabase().removeEntry(disk); local.setField(StandardField.TITLE, "Memory title"); - LibraryBaseline.Triage triage = triage(); + ChangeTriage.Triage triage = triage(); assertInstanceOf(EntryDelete.class, triage.bothSides().getFirst()); } @@ -208,7 +209,7 @@ void entryDeletedOnDiskButChangedInMemoryNeedsReview() { void entryAddedInMemoryIsNotAnExternalChange() { localContext.getDatabase().insertEntry(new BibEntry().withCitationKey("New").withField(StandardField.TITLE, "New in memory")); - LibraryBaseline.Triage triage = triage(); + ChangeTriage.Triage triage = triage(); assertInstanceOf(EntryDelete.class, triage.memoryOnly().getFirst()); assertEquals(List.of(), triage.diskOnly()); @@ -218,7 +219,7 @@ void entryAddedInMemoryIsNotAnExternalChange() { void stringAddedOnDiskIsAccepted() { diskContext.getDatabase().addString(new BibtexString("jan", "January")); - LibraryBaseline.Triage triage = triage(); + ChangeTriage.Triage triage = triage(); assertInstanceOf(BibTexStringAdd.class, triage.diskOnly().getFirst()); } @@ -226,14 +227,14 @@ void stringAddedOnDiskIsAccepted() { @Test void keepUnresolvedPreservesBaselineOfUnsavedMemoryChange() { local.setField(StandardField.TITLE, "Memory title"); - LibraryBaseline.Triage first = triage(); + ChangeTriage.Triage first = triage(); LibraryBaseline updated = LibraryBaseline.of(localContext, PATTERNS); - updated.keepUnresolved(baseline, first.memoryOnly()); + ChangeTriage.keepUnresolved(updated, baseline, first.memoryOnly()); baseline = updated; // The file now changes the same field: this must be reported as a conflict, not taken over silently disk.setField(StandardField.TITLE, "Disk title"); - LibraryBaseline.Triage second = triage(); + ChangeTriage.Triage second = triage(); assertEquals(1, second.bothSides().size()); assertEquals(List.of(), second.diskOnly()); diff --git a/jablib/src/main/java/module-info.java b/jablib/src/main/java/module-info.java index b1ba5cb542d1..9ecf00ff6f5a 100644 --- a/jablib/src/main/java/module-info.java +++ b/jablib/src/main/java/module-info.java @@ -38,6 +38,7 @@ exports org.jabref.logic.shared.prefs; exports org.jabref.logic.util; exports org.jabref.logic.util.io; + exports org.jabref.logic.sync; exports org.jabref.logic.xmp; exports org.jabref.model.database; exports org.jabref.model.entry; diff --git a/jablib/src/main/java/org/jabref/logic/sync/LibraryBaseline.java b/jablib/src/main/java/org/jabref/logic/sync/LibraryBaseline.java new file mode 100644 index 000000000000..b37069f9e2cf --- /dev/null +++ b/jablib/src/main/java/org/jabref/logic/sync/LibraryBaseline.java @@ -0,0 +1,267 @@ +package org.jabref.logic.sync; + +import java.nio.charset.Charset; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Optional; + +import org.jabref.logic.citationkeypattern.GlobalCitationKeyPatterns; +import org.jabref.logic.exporter.MetaDataSerializer; +import org.jabref.logic.git.merge.planning.util.ConflictRules; +import org.jabref.logic.git.merge.planning.util.FieldPatchComputer; +import org.jabref.model.database.BibDatabaseContext; +import org.jabref.model.entry.BibEntry; +import org.jabref.model.entry.BibtexString; +import org.jabref.model.entry.field.Field; +import org.jabref.model.entry.field.InternalField; +import org.jabref.model.entry.types.EntryType; +import org.jabref.model.metadata.MetaData; + +import org.jspecify.annotations.NullMarked; +import org.jspecify.annotations.Nullable; + +/// The library as it last matched its file on disk: the common ancestor for a three-way comparison of the in-memory +/// library with the file. For each item (entry, metadata, preamble, string) it tells which [Side] diverged, which +/// decides whether an external change can be taken over silently or needs a review, and merges entries changed on +/// both sides field by field with the Git merge rules. +/// +/// Entries are keyed by the id of the in-memory entry they were taken from, so that a citation key change in memory +/// does not break the association. The metadata is kept in its serialized form, which is the only way to snapshot it. +@NullMarked +public final class LibraryBaseline { + + /// Which side diverged from the baseline. Only asked for items that differ between memory and disk, so at least + /// one side changed. + public enum Side { + /// Only the file changed: the change can be taken over without asking + DISK, + /// Only memory changed: not an external change at all, but an unsaved edit + MEMORY, + /// Both changed differently: needs a review + BOTH + } + + private static final String ENCODING_KEY = "\0encoding"; + + /// What the comparison needs of an entry; a `BibEntry` copy would carry an event bus, caches, and properties per + /// entry, which for a large library adds up to far more than the bibliographic data itself. + private record EntrySnapshot(EntryType type, Map fields) { + static EntrySnapshot of(BibEntry entry) { + return new EntrySnapshot(entry.getType(), Map.copyOf(entry.getFieldMap())); + } + + /// Wraps the live field map without copying, for a comparison made right away + static EntrySnapshot view(BibEntry entry) { + return new EntrySnapshot(entry.getType(), entry.getFieldMap()); + } + + Optional citationKey() { + return Optional.ofNullable(fields.get(InternalField.KEY_FIELD)); + } + + /// The entry without its citation key, for finding an entry whose key changed + EntrySnapshot withoutKey() { + if (!fields.containsKey(InternalField.KEY_FIELD)) { + return this; + } + Map rest = new HashMap<>(fields); + rest.remove(InternalField.KEY_FIELD); + return new EntrySnapshot(type, rest); + } + + /// Only needed as the common ancestor of a field-level merge, which is the rare both-sides case + BibEntry toEntry() { + return new BibEntry(type).withFields(fields); + } + } + + private final Map entriesById; + private final Map metaData; + private @Nullable String preamble; + private final Map strings; + private final GlobalCitationKeyPatterns citationKeyPatterns; + + private LibraryBaseline(Map entriesById, Map metaData, @Nullable String preamble, Map strings, GlobalCitationKeyPatterns citationKeyPatterns) { + this.entriesById = entriesById; + this.metaData = metaData; + this.preamble = preamble; + this.strings = strings; + this.citationKeyPatterns = citationKeyPatterns; + } + + public static LibraryBaseline of(BibDatabaseContext context, GlobalCitationKeyPatterns citationKeyPatterns) { + Map entries = new HashMap<>(); + for (BibEntry entry : context.getDatabase().getEntries()) { + entries.put(entry.getId(), EntrySnapshot.of(entry)); + } + Map strings = new HashMap<>(); + for (BibtexString string : context.getDatabase().getStringValues()) { + strings.put(string.getName(), string.getContent()); + } + return new LibraryBaseline(entries, + serialize(context.getMetaData(), citationKeyPatterns), + context.getDatabase().getPreamble().orElse(null), + strings, + citationKeyPatterns); + } + + /// Lookup tables over the baseline entries for disk entries without in-memory counterpart. Built once per pass + /// over a set of changes, so that each lookup is O(1) instead of a scan over the library. + public final class Lookup { + private final Map>> byKey = new HashMap<>(); + private final Map> byContentExceptKey = new HashMap<>(); + + private Lookup() { + for (Map.Entry entry : entriesById.entrySet()) { + entry.getValue().citationKey().ifPresent(key -> byKey.computeIfAbsent(key, _ -> new ArrayList<>()).add(entry)); + byContentExceptKey.putIfAbsent(entry.getValue().withoutKey(), entry); + } + } + + /// The id of the in-memory entry the given disk entry was taken from: by citation key (preferring identical + /// content, as keys need not be unique), or else by the remaining content, which covers entries without a + /// key as well as a key changed on disk. + public Optional baseIdOf(BibEntry remote) { + return find(remote).map(Map.Entry::getKey); + } + + /// A disk entry without in-memory counterpart is either new on disk, or was deleted in memory (and possibly + /// modified on disk as well). + public Side sideOfAddedEntry(BibEntry remote) { + return find(remote).map(base -> base.getValue().equals(EntrySnapshot.view(remote)) ? Side.MEMORY : Side.BOTH) + .orElse(Side.DISK); + } + + private Optional> find(BibEntry remote) { + EntrySnapshot snapshot = EntrySnapshot.view(remote); + Optional> byKeyMatch = snapshot.citationKey() + .map(key -> byKey.getOrDefault(key, List.of())) + .flatMap(candidates -> candidates.stream().filter(entry -> entry.getValue().equals(snapshot)).findFirst() + .or(() -> candidates.stream().findFirst())); + return byKeyMatch.or(() -> Optional.ofNullable(byContentExceptKey.get(snapshot.withoutKey()))); + } + } + + public Lookup lookup() { + return new Lookup(); + } + + public boolean hasEntry(String entryId) { + return entriesById.containsKey(entryId); + } + + /// An entry present on both sides, whose baseline is looked up by the in-memory entry's id. + public Side sideOfEntry(BibEntry local, BibEntry remote) { + return sideOf(entriesById.get(local.getId()), EntrySnapshot.view(local), EntrySnapshot.view(remote)); + } + + /// An in-memory entry without counterpart on disk is either deleted on disk, or new in memory (and possibly + /// modified on disk as well). + public Side sideOfDeletedEntry(BibEntry local) { + EntrySnapshot base = entriesById.get(local.getId()); + if (base == null) { + return Side.MEMORY; + } + return base.equals(EntrySnapshot.view(local)) ? Side.DISK : Side.BOTH; + } + + /// Field-level merge of an entry modified on both sides: disk wins for every field memory left alone, memory wins + /// where disk left the field alone. Empty if the same field was changed differently on both sides. + public Optional mergeEntry(BibEntry local, BibEntry remote) { + EntrySnapshot base = entriesById.get(local.getId()); + // Without a common ancestor (entry new on both sides), an empty entry of the in-memory type makes every field + // an addition, so only fields set differently on both sides count as conflicts and the in-memory type is kept + BibEntry ancestor = base == null ? new BibEntry(local.getType()) : base.toEntry(); + if (ConflictRules.hasConflictingFields(ancestor, local, remote)) { + return Optional.empty(); + } + BibEntry merged = new BibEntry(local); + if (!ancestor.getType().equals(remote.getType())) { + merged.setType(remote.getType()); + } + FieldPatchComputer.compute(ancestor, local, remote).forEach((field, value) -> { + if (value == null) { + merged.clearField(field); + } else { + merged.setField(field, value); + } + }); + return Optional.of(merged); + } + + public Side sideOfMetaData(MetaData local, MetaData remote) { + return sideOf(metaData, serialize(local, citationKeyPatterns), serialize(remote, citationKeyPatterns)); + } + + public Side sideOfPreamble(@Nullable String local, @Nullable String remote) { + return sideOf(preamble, local, remote); + } + + /// @param local the content in memory, `null` when the string does not exist in memory + /// @param remote the content on disk, `null` when the string does not exist on disk + public Side sideOfString(String name, @Nullable String local, @Nullable String remote) { + return sideOf(strings.get(name), local, remote); + } + + /// A string renamed on disk is taken over only if memory neither touched the old string nor already uses the new name. + public Side sideOfStringRename(String oldName, String oldContent, String newName) { + boolean oldUntouchedInMemory = Objects.equals(strings.get(oldName), oldContent); + return oldUntouchedInMemory && !strings.containsKey(newName) ? Side.DISK : Side.BOTH; + } + + /// `null` stands for "absent" on that side. + private static Side sideOf(@Nullable T base, @Nullable T local, @Nullable T remote) { + boolean localChanged = !Objects.equals(base, local); + boolean remoteChanged = !Objects.equals(base, remote); + if (localChanged && remoteChanged) { + return Side.BOTH; + } + return remoteChanged ? Side.DISK : Side.MEMORY; + } + + // Carrying over items from a previous baseline: an external change that was not applied must be seen against the + // same ancestor next time, otherwise the current in-memory version would pass as the common ancestor. + + public void keepEntry(LibraryBaseline previous, String entryId) { + EntrySnapshot base = previous.entriesById.get(entryId); + if (base == null) { + entriesById.remove(entryId); + } else { + entriesById.put(entryId, base); + } + } + + /// Keeps the previous baseline of the in-memory entry a disk entry was taken from, if there is one. + public void keepEntryFor(LibraryBaseline previous, Lookup previousLookup, BibEntry remote) { + previousLookup.baseIdOf(remote).ifPresent(baseId -> entriesById.put(baseId, previous.entriesById.get(baseId))); + } + + public void keepMetaData(LibraryBaseline previous) { + metaData.clear(); + metaData.putAll(previous.metaData); + } + + public void keepPreamble(LibraryBaseline previous) { + preamble = previous.preamble; + } + + public void keepString(LibraryBaseline previous, String name) { + String base = previous.strings.get(name); + if (base == null) { + strings.remove(name); + } else { + strings.put(name, base); + } + } + + /// The encoding is not part of the serialized metadata (it is written as a file header instead), but it is part + /// of what the two-way comparison reports. + private static Map serialize(MetaData metaData, GlobalCitationKeyPatterns citationKeyPatterns) { + Map serialized = new HashMap<>(MetaDataSerializer.getSerializedStringMap(metaData, citationKeyPatterns)); + serialized.put(ENCODING_KEY, metaData.getEncoding().map(Charset::name).orElse("")); + return serialized; + } +} diff --git a/jablib/src/main/java/org/jabref/logic/sync/package-info.java b/jablib/src/main/java/org/jabref/logic/sync/package-info.java new file mode 100644 index 000000000000..1ce1799d0b57 --- /dev/null +++ b/jablib/src/main/java/org/jabref/logic/sync/package-info.java @@ -0,0 +1,14 @@ +/// Keeping an in-memory library and its file the same in both directions without asking more than necessary. +/// +/// [LibraryBaseline] is the common ancestor for the three-way comparison of the in-memory library with the file: for +/// each item it tells which side diverged, which decides whether an external change is taken over silently or needs +/// a review, and merges entries changed on both sides field by field with the rules of the Git merge +/// (`org.jabref.logic.git.merge.planning.util`). +/// +/// Detecting the changes themselves and presenting a review is the GUI's part (`org.jabref.gui.collab`). +/// +/// See for the requirements this implements. +@NullMarked +package org.jabref.logic.sync; + +import org.jspecify.annotations.NullMarked; diff --git a/jablib/src/test/java/org/jabref/logic/sync/LibraryBaselineTest.java b/jablib/src/test/java/org/jabref/logic/sync/LibraryBaselineTest.java new file mode 100644 index 000000000000..5574f467712e --- /dev/null +++ b/jablib/src/test/java/org/jabref/logic/sync/LibraryBaselineTest.java @@ -0,0 +1,128 @@ +package org.jabref.logic.sync; + +import java.util.List; +import java.util.Optional; + +import org.jabref.logic.citationkeypattern.GlobalCitationKeyPatterns; +import org.jabref.logic.sync.LibraryBaseline.Side; +import org.jabref.model.database.BibDatabase; +import org.jabref.model.database.BibDatabaseContext; +import org.jabref.model.entry.BibEntry; +import org.jabref.model.entry.field.StandardField; +import org.jabref.model.entry.types.StandardEntryType; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +class LibraryBaselineTest { + + private static final GlobalCitationKeyPatterns PATTERNS = GlobalCitationKeyPatterns.fromPattern("[auth][year]"); + + private BibEntry local; + private BibEntry remote; + private LibraryBaseline baseline; + + @BeforeEach + void setUp() { + local = new BibEntry(StandardEntryType.Article).withCitationKey("Key").withField(StandardField.TITLE, "Title").withField(StandardField.YEAR, "2020"); + baseline = LibraryBaseline.of(new BibDatabaseContext(new BibDatabase(List.of(local))), PATTERNS); + // the file as it would be parsed: same content, different entry object + remote = new BibEntry(local); + } + + @Test + void entryChangedOnDiskOnly() { + remote.setField(StandardField.TITLE, "Disk title"); + + assertEquals(Side.DISK, baseline.sideOfEntry(local, remote)); + } + + @Test + void entryChangedInMemoryOnly() { + local.setField(StandardField.TITLE, "Memory title"); + + assertEquals(Side.MEMORY, baseline.sideOfEntry(local, remote)); + } + + @Test + void entryChangedOnBothSides() { + local.setField(StandardField.TITLE, "Memory title"); + remote.setField(StandardField.TITLE, "Disk title"); + + assertEquals(Side.BOTH, baseline.sideOfEntry(local, remote)); + } + + @Test + void differentFieldsChangedOnBothSidesAreMerged() { + local.setField(StandardField.TITLE, "Memory title"); + remote.setField(StandardField.YEAR, "2021"); + + Optional merged = baseline.mergeEntry(local, remote); + + assertEquals(Optional.of(new BibEntry(StandardEntryType.Article).withCitationKey("Key").withField(StandardField.TITLE, "Memory title").withField(StandardField.YEAR, "2021")), merged); + } + + @Test + void sameFieldChangedDifferentlyIsNotMerged() { + local.setField(StandardField.TITLE, "Memory title"); + remote.setField(StandardField.TITLE, "Disk title"); + + assertEquals(Optional.empty(), baseline.mergeEntry(local, remote)); + } + + @Test + void entryDeletedOnDiskWhenUntouchedInMemory() { + assertEquals(Side.DISK, baseline.sideOfDeletedEntry(local)); + } + + @Test + void entryUnknownToBaselineWasAddedInMemory() { + BibEntry added = new BibEntry(StandardEntryType.Misc).withCitationKey("New"); + + assertEquals(Side.MEMORY, baseline.sideOfDeletedEntry(added)); + } + + @Test + void addedEntryUnknownToBaselineIsNewOnDisk() { + BibEntry added = new BibEntry(StandardEntryType.Misc).withCitationKey("New"); + + assertEquals(Side.DISK, baseline.lookup().sideOfAddedEntry(added)); + } + + @Test + void addedEntryEqualToBaselineWasDeletedInMemory() { + assertEquals(Side.MEMORY, baseline.lookup().sideOfAddedEntry(remote)); + } + + @Test + void renamedEntryIsFoundByContent() { + remote.setCitationKey("Renamed"); + + assertEquals(Optional.of(local.getId()), baseline.lookup().baseIdOf(remote)); + } + + @Test + void duplicateKeysPreferIdenticalContent() { + BibEntry duplicate = new BibEntry(StandardEntryType.Article).withCitationKey("Key").withField(StandardField.TITLE, "Duplicate"); + baseline = LibraryBaseline.of(new BibDatabaseContext(new BibDatabase(List.of(duplicate, local))), PATTERNS); + + assertEquals(Optional.of(local.getId()), baseline.lookup().baseIdOf(remote)); + } + + @Test + void stringAddedOnDiskOnly() { + assertEquals(Side.DISK, baseline.sideOfString("jan", null, "January")); + } + + @Test + void keptEntryStaysTheAncestorOfTheNextComparison() { + local.setField(StandardField.TITLE, "Memory title"); + LibraryBaseline updated = LibraryBaseline.of(new BibDatabaseContext(new BibDatabase(List.of(local))), PATTERNS); + updated.keepEntry(baseline, local.getId()); + + // Against the kept ancestor, the memory edit is still visible as such + assertEquals(Side.MEMORY, updated.sideOfEntry(local, remote)); + } +} From e98762b5ac1714b4b0f603b45a479e8d72cc475b Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Mon, 7 Sep 2026 23:38:21 +0200 Subject: [PATCH 14/27] Treat entry comments as content, resolve duplicates only when unambiguous, and harden the scan Comments before an entry are part of the snapshot, the merge, and the applied change. An added disk entry is associated with a baseline entry only when exactly one candidate fits, a string added on disk is recognized as the rename of a string deleted in memory, the disk state is recorded only by the scan that will be applied and only after the file has looked the same twice in a row, and the generation check and the application of a scan happen under the same lock. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01GWmXyN3GUNhfF9fSo1UNrt --- .../org/jabref/gui/collab/ChangeTriage.java | 3 +- .../gui/collab/DatabaseChangeMonitor.java | 35 +++++++---- .../gui/collab/entrychange/EntryChange.java | 4 ++ .../jabref/gui/collab/ChangeTriageTest.java | 12 ++++ .../jabref/logic/sync/LibraryBaseline.java | 59 ++++++++++++++----- .../java/org/jabref/model/undo/BibChange.java | 2 +- .../model/undo/UndoableCommentsChange.java | 34 +++++++++++ .../logic/sync/LibraryBaselineTest.java | 39 ++++++++++++ 8 files changed, 158 insertions(+), 30 deletions(-) create mode 100644 jablib/src/main/java/org/jabref/model/undo/UndoableCommentsChange.java diff --git a/jabgui/src/main/java/org/jabref/gui/collab/ChangeTriage.java b/jabgui/src/main/java/org/jabref/gui/collab/ChangeTriage.java index 366dc004189f..5a21bca7a602 100644 --- a/jabgui/src/main/java/org/jabref/gui/collab/ChangeTriage.java +++ b/jabgui/src/main/java/org/jabref/gui/collab/ChangeTriage.java @@ -71,7 +71,8 @@ public static Triage triage(LibraryBaseline baseline, List chang case PreambleChange preambleChange -> baseline.sideOfPreamble(local.getDatabase().getPreamble().orElse(null), preambleChange.getPreambleDiff().getNewPreamble()); case BibTexStringAdd stringAdd -> - baseline.sideOfString(stringAdd.getAddedString().getName(), null, stringAdd.getAddedString().getContent()); + baseline.sideOfAddedString(stringAdd.getAddedString().getName(), stringAdd.getAddedString().getContent(), + name -> local.getDatabase().getStringByName(name).isPresent()); case BibTexStringDelete stringDelete -> baseline.sideOfString(stringDelete.getDeletedString().getName(), stringDelete.getDeletedString().getContent(), null); case BibTexStringChange stringChange -> diff --git a/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java b/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java index eeefad182af2..29527918ec9d 100644 --- a/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java +++ b/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java @@ -35,6 +35,8 @@ public class DatabaseChangeMonitor implements FileUpdateListener { private static final Logger LOGGER = LoggerFactory.getLogger(DatabaseChangeMonitor.class); private static final int STABLE_FILE_ATTEMPTS = 20; private static final long STABLE_FILE_INTERVAL_MILLIS = 250; + /// A writer pausing between two chunks must not pass as finished: the file has to look the same this many times in a row + private static final int STABLE_FILE_CONFIRMATIONS = 2; private final BibDatabaseContext database; private final FileUpdateMonitor fileMonitor; @@ -253,13 +255,13 @@ private void scanForChanges() { int generation = ++scanGeneration; if (scannedBaseline != null && isSynchronizing()) { // [impl->req~ux.external-library-changes.synchronize~1] - BackgroundTask.wrap(() -> scanner.scanForChanges(this::awaitStableFile)) + BackgroundTask.wrap(() -> scanner.scanForChanges(() -> awaitStableLibraryFile(generation))) .onSuccess(changes -> onScannedForSynchronization(generation, scanner, scannedBaseline, changes)) .onFailure(e -> LOGGER.error("Error while synchronizing with the library file", e)) .executeWith(taskExecutor); return; } - BackgroundTask.wrap(() -> scanner.scanForChanges(this::awaitStableFile)) + BackgroundTask.wrap(() -> scanner.scanForChanges(() -> awaitStableLibraryFile(generation))) .onSuccess(changes -> { if (!changes.isEmpty()) { listeners.forEach(listener -> listener.databaseChanged(changes)); @@ -272,37 +274,44 @@ private void scanForChanges() { /// Sorting the changes on the FX thread right before applying them leaves no window for a user edit to slip in /// between classification and application. private void onScannedForSynchronization(int generation, ChangeScanner scanner, LibraryBaseline scannedBaseline, List changes) { - if (generation != scanGeneration) { - LOGGER.debug("Discarding result of a scan overtaken by a newer file change"); - return; + // The lock keeps a file event from starting a newer scan between the check and the application + synchronized (database) { + if (generation != scanGeneration) { + LOGGER.debug("Discarding result of a scan overtaken by a newer file change"); + return; + } + synchronize(scannedBaseline, scanner.triage(scannedBaseline, changes)); } - synchronize(scannedBaseline, scanner.triage(scannedBaseline, changes)); } /// Sync clients and editors may write the file in several steps. A file that is still growing must not be parsed: /// half of a library parses fine and would look like every later entry had been deleted. Waits (bounded) until - /// size and modification time stop changing; the last snapshot seen becomes the known disk state. - private void awaitStableFile() { + /// size and modification time have stopped changing for a while. The state seen becomes the known disk state, so + /// that the events of the write just waited for do not trigger another scan; only for the current scan, since a + /// scan already overtaken will not apply what it sees, and recording it would make the next event look handled. + private void awaitStableLibraryFile(int generation) { Path path = monitoredPath.orElse(null); if (path == null) { return; } FileSnapshot last = FileSnapshot.read(path); - for (int attempt = 0; attempt < STABLE_FILE_ATTEMPTS; attempt++) { + int unchanged = 0; + for (int attempt = 0; attempt < STABLE_FILE_ATTEMPTS && unchanged < STABLE_FILE_CONFIRMATIONS; attempt++) { try { Thread.sleep(STABLE_FILE_INTERVAL_MILLIS); } catch (InterruptedException e) { + LOGGER.debug("Interrupted while waiting for {} to stop changing; the scan is abandoned", path, e); Thread.currentThread().interrupt(); return; } FileSnapshot current = FileSnapshot.read(path); - if (Objects.equals(current, last)) { - break; - } + unchanged = Objects.equals(current, last) ? unchanged + 1 : 0; last = current; } synchronized (database) { - knownDiskState = last; + if (generation == scanGeneration) { + knownDiskState = last; + } } } diff --git a/jabgui/src/main/java/org/jabref/gui/collab/entrychange/EntryChange.java b/jabgui/src/main/java/org/jabref/gui/collab/entrychange/EntryChange.java index 8aa9fb0a821f..180ffa6e5cb2 100644 --- a/jabgui/src/main/java/org/jabref/gui/collab/entrychange/EntryChange.java +++ b/jabgui/src/main/java/org/jabref/gui/collab/entrychange/EntryChange.java @@ -12,6 +12,7 @@ import org.jabref.model.entry.field.Field; import org.jabref.model.undo.CompoundEdit; import org.jabref.model.undo.UndoableChangeType; +import org.jabref.model.undo.UndoableCommentsChange; import org.jabref.model.undo.UndoableFieldChange; public final class EntryChange extends DatabaseChange { @@ -58,6 +59,9 @@ public void applyChange(CompoundEdit undoEdit) { entryEdit.applyEdit(new UndoableFieldChange(oldEntry, field, before, after)); } } + if (!oldEntry.getUserComments().equals(newEntry.getUserComments())) { + entryEdit.applyEdit(new UndoableCommentsChange(oldEntry, oldEntry.getUserComments(), newEntry.getUserComments())); + } undoEdit.addEdit(entryEdit); } } diff --git a/jabgui/src/test/java/org/jabref/gui/collab/ChangeTriageTest.java b/jabgui/src/test/java/org/jabref/gui/collab/ChangeTriageTest.java index fed4fd2f554f..1468ca035a6b 100644 --- a/jabgui/src/test/java/org/jabref/gui/collab/ChangeTriageTest.java +++ b/jabgui/src/test/java/org/jabref/gui/collab/ChangeTriageTest.java @@ -16,6 +16,7 @@ import org.jabref.model.entry.BibtexString; import org.jabref.model.entry.field.StandardField; import org.jabref.model.entry.types.StandardEntryType; +import org.jabref.model.undo.CompoundEdit; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -156,6 +157,17 @@ void duplicateCitationKeysAreMatchedByContent() { assertEquals(List.of(), triage.bothSides()); } + @Test + void commentChangedOnDiskIsAcceptedAndApplied() { + disk.setCommentsBeforeEntry("% from disk"); + + ChangeTriage.Triage triage = triage(); + + EntryChange change = assertInstanceOf(EntryChange.class, triage.diskOnly().getFirst()); + change.applyChange(new CompoundEdit("test")); + assertEquals("% from disk", local.getUserComments()); + } + @Test void entryAddedOnDiskIsAccepted() { diskContext.getDatabase().insertEntry(new BibEntry().withCitationKey("New").withField(StandardField.TITLE, "New on disk")); diff --git a/jablib/src/main/java/org/jabref/logic/sync/LibraryBaseline.java b/jablib/src/main/java/org/jabref/logic/sync/LibraryBaseline.java index b37069f9e2cf..d08f45a53c8e 100644 --- a/jablib/src/main/java/org/jabref/logic/sync/LibraryBaseline.java +++ b/jablib/src/main/java/org/jabref/logic/sync/LibraryBaseline.java @@ -7,6 +7,7 @@ import java.util.Map; import java.util.Objects; import java.util.Optional; +import java.util.function.Predicate; import org.jabref.logic.citationkeypattern.GlobalCitationKeyPatterns; import org.jabref.logic.exporter.MetaDataSerializer; @@ -48,14 +49,16 @@ public enum Side { /// What the comparison needs of an entry; a `BibEntry` copy would carry an event bus, caches, and properties per /// entry, which for a large library adds up to far more than the bibliographic data itself. - private record EntrySnapshot(EntryType type, Map fields) { + /// The comments written before the entry in the file count as content: they are kept by the parser and written + /// back, so an external edit of them is an external change like any other. + private record EntrySnapshot(EntryType type, Map fields, String comments) { static EntrySnapshot of(BibEntry entry) { - return new EntrySnapshot(entry.getType(), Map.copyOf(entry.getFieldMap())); + return new EntrySnapshot(entry.getType(), Map.copyOf(entry.getFieldMap()), entry.getUserComments()); } /// Wraps the live field map without copying, for a comparison made right away static EntrySnapshot view(BibEntry entry) { - return new EntrySnapshot(entry.getType(), entry.getFieldMap()); + return new EntrySnapshot(entry.getType(), entry.getFieldMap(), entry.getUserComments()); } Optional citationKey() { @@ -69,12 +72,14 @@ EntrySnapshot withoutKey() { } Map rest = new HashMap<>(fields); rest.remove(InternalField.KEY_FIELD); - return new EntrySnapshot(type, rest); + return new EntrySnapshot(type, rest, comments); } /// Only needed as the common ancestor of a field-level merge, which is the rare both-sides case BibEntry toEntry() { - return new BibEntry(type).withFields(fields); + BibEntry entry = new BibEntry(type).withFields(fields); + entry.setCommentsBeforeEntry(comments); + return entry; } } @@ -112,18 +117,19 @@ public static LibraryBaseline of(BibDatabaseContext context, GlobalCitationKeyPa /// over a set of changes, so that each lookup is O(1) instead of a scan over the library. public final class Lookup { private final Map>> byKey = new HashMap<>(); - private final Map> byContentExceptKey = new HashMap<>(); + private final Map>> byContentExceptKey = new HashMap<>(); private Lookup() { for (Map.Entry entry : entriesById.entrySet()) { entry.getValue().citationKey().ifPresent(key -> byKey.computeIfAbsent(key, _ -> new ArrayList<>()).add(entry)); - byContentExceptKey.putIfAbsent(entry.getValue().withoutKey(), entry); + byContentExceptKey.computeIfAbsent(entry.getValue().withoutKey(), _ -> new ArrayList<>()).add(entry); } } - /// The id of the in-memory entry the given disk entry was taken from: by citation key (preferring identical - /// content, as keys need not be unique), or else by the remaining content, which covers entries without a - /// key as well as a key changed on disk. + /// The id of the in-memory entry the given disk entry was taken from: by citation key, or else by the + /// remaining content, which covers entries without a key as well as a key changed on disk. Among several + /// candidates (keys need not be unique, entries may be duplicated) the one with identical content wins; + /// when that leaves more than one, the entry is not associated at all rather than with the wrong one. public Optional baseIdOf(BibEntry remote) { return find(remote).map(Map.Entry::getKey); } @@ -137,11 +143,17 @@ public Side sideOfAddedEntry(BibEntry remote) { private Optional> find(BibEntry remote) { EntrySnapshot snapshot = EntrySnapshot.view(remote); - Optional> byKeyMatch = snapshot.citationKey() - .map(key -> byKey.getOrDefault(key, List.of())) - .flatMap(candidates -> candidates.stream().filter(entry -> entry.getValue().equals(snapshot)).findFirst() - .or(() -> candidates.stream().findFirst())); - return byKeyMatch.or(() -> Optional.ofNullable(byContentExceptKey.get(snapshot.withoutKey()))); + List> byKeyCandidates = snapshot.citationKey().map(key -> byKey.getOrDefault(key, List.of())).orElse(List.of()); + if (!byKeyCandidates.isEmpty()) { + return unambiguous(byKeyCandidates, snapshot); + } + return unambiguous(byContentExceptKey.getOrDefault(snapshot.withoutKey(), List.of()), snapshot); + } + + private static Optional> unambiguous(List> candidates, EntrySnapshot snapshot) { + List> identical = candidates.stream().filter(entry -> entry.getValue().equals(snapshot)).toList(); + List> chosen = identical.isEmpty() ? candidates : identical; + return chosen.size() == 1 ? Optional.of(chosen.getFirst()) : Optional.empty(); } } @@ -182,6 +194,9 @@ public Optional mergeEntry(BibEntry local, BibEntry remote) { if (!ancestor.getType().equals(remote.getType())) { merged.setType(remote.getType()); } + if (ancestor.getUserComments().equals(local.getUserComments())) { + merged.setCommentsBeforeEntry(remote.getUserComments()); + } FieldPatchComputer.compute(ancestor, local, remote).forEach((field, value) -> { if (value == null) { merged.clearField(field); @@ -206,6 +221,20 @@ public Side sideOfString(String name, @Nullable String local, @Nullable String r return sideOf(strings.get(name), local, remote); } + /// A string that exists on disk only. The two-way comparison cannot see that it is the renamed form of a baseline + /// string when memory deleted that string, so the content is matched against the baseline: a deletion in memory + /// against a rename on disk is a conflict. + /// + /// @param existsInMemory whether memory has a string of the given name + public Side sideOfAddedString(String name, String content, Predicate existsInMemory) { + if (strings.containsKey(name)) { + return sideOf(strings.get(name), null, content); + } + boolean renamedFromDeleted = strings.entrySet().stream() + .anyMatch(base -> base.getValue().equals(content) && !existsInMemory.test(base.getKey())); + return renamedFromDeleted ? Side.BOTH : Side.DISK; + } + /// A string renamed on disk is taken over only if memory neither touched the old string nor already uses the new name. public Side sideOfStringRename(String oldName, String oldContent, String newName) { boolean oldUntouchedInMemory = Objects.equals(strings.get(oldName), oldContent); diff --git a/jablib/src/main/java/org/jabref/model/undo/BibChange.java b/jablib/src/main/java/org/jabref/model/undo/BibChange.java index 79ed6f146260..dd5ed8d4d3b6 100644 --- a/jablib/src/main/java/org/jabref/model/undo/BibChange.java +++ b/jablib/src/main/java/org/jabref/model/undo/BibChange.java @@ -13,7 +13,7 @@ /// user acts in, as the name of the enclosing [ChangeSet]. @NullMarked public sealed interface BibChange permits - ChangeSet, + UndoableCommentsChange, ChangeSet, UndoableInsertEntries, UndoableRemoveEntries, UndoableChangeType, diff --git a/jablib/src/main/java/org/jabref/model/undo/UndoableCommentsChange.java b/jablib/src/main/java/org/jabref/model/undo/UndoableCommentsChange.java new file mode 100644 index 000000000000..3c28b29b08d9 --- /dev/null +++ b/jablib/src/main/java/org/jabref/model/undo/UndoableCommentsChange.java @@ -0,0 +1,34 @@ +package org.jabref.model.undo; + +import java.util.Objects; + +import org.jabref.model.entry.BibEntry; + +import org.jspecify.annotations.NullMarked; + +/// A change of the comments written before an entry in the library file. +@NullMarked +public record UndoableCommentsChange(BibEntry entry, String before, String after) implements BibChange { + @Override + public UndoableCommentsChange inverted() { + return new UndoableCommentsChange(entry, after, before); + } + + @Override + public void apply() { + entry.setCommentsBeforeEntry(after); + } + + @Override + public boolean equals(Object object) { + return (object instanceof UndoableCommentsChange other) + && ChangeIdentity.same(entry, other.entry) + && before.equals(other.before) + && after.equals(other.after); + } + + @Override + public int hashCode() { + return Objects.hash(ChangeIdentity.hash(entry), before, after); + } +} diff --git a/jablib/src/test/java/org/jabref/logic/sync/LibraryBaselineTest.java b/jablib/src/test/java/org/jabref/logic/sync/LibraryBaselineTest.java index 5574f467712e..f2f19a2b326c 100644 --- a/jablib/src/test/java/org/jabref/logic/sync/LibraryBaselineTest.java +++ b/jablib/src/test/java/org/jabref/logic/sync/LibraryBaselineTest.java @@ -8,6 +8,7 @@ import org.jabref.model.database.BibDatabase; import org.jabref.model.database.BibDatabaseContext; import org.jabref.model.entry.BibEntry; +import org.jabref.model.entry.BibtexString; import org.jabref.model.entry.field.StandardField; import org.jabref.model.entry.types.StandardEntryType; @@ -125,4 +126,42 @@ void keptEntryStaysTheAncestorOfTheNextComparison() { // Against the kept ancestor, the memory edit is still visible as such assertEquals(Side.MEMORY, updated.sideOfEntry(local, remote)); } + + @Test + void commentChangedOnDiskOnly() { + remote.setCommentsBeforeEntry("% new comment"); + + assertEquals(Side.DISK, baseline.sideOfEntry(local, remote)); + } + + @Test + void commentChangedOnDiskIsMergedWithFieldChangedInMemory() { + local.setField(StandardField.TITLE, "Memory title"); + remote.setCommentsBeforeEntry("% new comment"); + + Optional merged = baseline.mergeEntry(local, remote); + + assertEquals(Optional.of("% new comment"), merged.map(BibEntry::getUserComments)); + assertEquals(Optional.of("Memory title"), merged.flatMap(entry -> entry.getField(StandardField.TITLE))); + } + + @Test + void ambiguousDuplicatesAreNotAssociated() { + BibEntry duplicate = new BibEntry(local); + baseline = LibraryBaseline.of(new BibDatabaseContext(new BibDatabase(List.of(duplicate, local))), PATTERNS); + remote.setField(StandardField.TITLE, "Changed on disk"); + + assertEquals(Optional.empty(), baseline.lookup().baseIdOf(remote)); + } + + @Test + void stringDeletedInMemoryAndRenamedOnDiskIsAConflict() { + BibDatabase database = new BibDatabase(List.of(local)); + database.addString(new BibtexString("jan", "January")); + baseline = LibraryBaseline.of(new BibDatabaseContext(database), PATTERNS); + + // memory has no string anymore, disk has the same content under a new name + assertEquals(Side.BOTH, baseline.sideOfAddedString("january", "January", _ -> false)); + assertEquals(Side.DISK, baseline.sideOfAddedString("feb", "February", _ -> false)); + } } From 6687705ae4d88c2361026b2995292b6a4d7c5548 Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Sun, 13 Sep 2026 01:47:56 +0200 Subject: [PATCH 15/27] Detect conflicting comments and string renames, abort interrupted scans, and keep undo faithful Comments changed differently on both sides are a conflict, a string rename is checked against the names memory uses, an equal-valued string counts as a rename source only when unambiguous, and a split add/delete pair is reunited by closeness when identity no longer matches. An interrupted wait abandons the scan instead of parsing a possibly incomplete file, applying a comment change marks the entry changed so that a save writes it, and undoing an in-place merge restores the entry's changed flag. The unneeded export of the merge utilities is gone. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01GWmXyN3GUNhfF9fSo1UNrt --- .../org/jabref/gui/collab/ChangeScanner.java | 20 ++++---- .../org/jabref/gui/collab/ChangeTriage.java | 16 +++++-- .../gui/collab/DatabaseChangeMonitor.java | 18 +++---- .../gui/collab/entrychange/EntryChange.java | 3 ++ .../jabref/gui/collab/ChangeTriageTest.java | 21 ++++++++ jablib/src/main/java/module-info.java | 1 - .../jabref/logic/sync/LibraryBaseline.java | 45 ++++++++++++++--- .../jabref/logic/undo/BibChangeDescriber.java | 6 +++ .../java/org/jabref/model/undo/BibChange.java | 2 +- .../model/undo/UndoableChangedFlag.java | 36 ++++++++++++++ .../model/undo/UndoableCommentsChange.java | 8 +++- .../main/resources/l10n/JabRef_en.properties | 2 + .../logic/sync/LibraryBaselineTest.java | 48 +++++++++++++++++++ 13 files changed, 194 insertions(+), 32 deletions(-) create mode 100644 jablib/src/main/java/org/jabref/model/undo/UndoableChangedFlag.java diff --git a/jabgui/src/main/java/org/jabref/gui/collab/ChangeScanner.java b/jabgui/src/main/java/org/jabref/gui/collab/ChangeScanner.java index 7f2ad9da7ca0..65d4c4b6dfcf 100644 --- a/jabgui/src/main/java/org/jabref/gui/collab/ChangeScanner.java +++ b/jabgui/src/main/java/org/jabref/gui/collab/ChangeScanner.java @@ -3,6 +3,8 @@ import java.io.IOException; import java.nio.file.Path; import java.util.List; +import java.util.Optional; +import java.util.function.BooleanSupplier; import org.jabref.gui.DialogService; import org.jabref.gui.StateManager; @@ -35,22 +37,24 @@ public ChangeScanner(BibDatabaseContext database, } public List scanForChanges() { - return scanForChanges(() -> { - }); + return scanForChanges(() -> true).orElse(List.of()); } - /// @param beforeParsing run right before the file is parsed, e.g. to wait until a sync client has finished writing it - public List scanForChanges(Runnable beforeParsing) { + /// @param readyToParse consulted right before the file is parsed, e.g. to wait until a sync client has finished writing it; `false` abandons the scan + /// @return the changes, or empty when the scan was abandoned + public Optional> scanForChanges(BooleanSupplier readyToParse) { if (database.getDatabasePath().isEmpty()) { - return List.of(); + return Optional.of(List.of()); + } + if (!readyToParse.getAsBoolean()) { + return Optional.empty(); } - beforeParsing.run(); try { - return getDatabaseChanges(database.getDatabasePath().get()); + return Optional.of(getDatabaseChanges(database.getDatabasePath().get())); } catch (IOException e) { LOGGER.warn("Error while parsing changed file.", e); - return List.of(); + return Optional.of(List.of()); } } diff --git a/jabgui/src/main/java/org/jabref/gui/collab/ChangeTriage.java b/jabgui/src/main/java/org/jabref/gui/collab/ChangeTriage.java index 5a21bca7a602..d67173bdf4fb 100644 --- a/jabgui/src/main/java/org/jabref/gui/collab/ChangeTriage.java +++ b/jabgui/src/main/java/org/jabref/gui/collab/ChangeTriage.java @@ -78,7 +78,8 @@ public static Triage triage(LibraryBaseline baseline, List chang case BibTexStringChange stringChange -> baseline.sideOfString(stringChange.getOldString().getName(), stringChange.getOldString().getContent(), stringChange.getNewString().getContent()); case BibTexStringRename stringRename -> - baseline.sideOfStringRename(stringRename.getOldString().getName(), stringRename.getOldString().getContent(), stringRename.getNewString().getName()); + baseline.sideOfStringRename(stringRename.getOldString().getName(), stringRename.getOldString().getContent(), stringRename.getNewString().getName(), + name -> local.getDatabase().getStringByName(name).isPresent()); }; switch (side) { case DISK -> { @@ -152,10 +153,15 @@ private static List pairSplitEntries(LibraryBaseline baseline, L Set pairedDeletes = new HashSet<>(); for (DatabaseChange change : changes) { if (change instanceof EntryAdd entryAdd) { - lookup.baseIdOf(entryAdd.getAddedEntry()).map(deletesByBaseId::remove).ifPresent(entryDelete -> { - pairedDeletes.add(entryDelete); - replacements.put(entryAdd, new EntryChange(entryDelete.getDeletedEntry(), entryAdd.getAddedEntry(), local, resolverFactory)); - }); + // By identity first; an entry whose key and a field changed on disk matches neither, so among the + // entries deleted on disk the one closest in content is taken, if that is unambiguous + lookup.baseIdOf(entryAdd.getAddedEntry()) + .or(() -> baseline.closestOf(deletesByBaseId.keySet(), entryAdd.getAddedEntry())) + .map(deletesByBaseId::remove) + .ifPresent(entryDelete -> { + pairedDeletes.add(entryDelete); + replacements.put(entryAdd, new EntryChange(entryDelete.getDeletedEntry(), entryAdd.getAddedEntry(), local, resolverFactory)); + }); } } List paired = new ArrayList<>(changes.size()); diff --git a/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java b/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java index a8980107a1ea..cf34ab875855 100644 --- a/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java +++ b/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java @@ -256,17 +256,14 @@ private void scanForChanges() { if (scannedBaseline != null && isSynchronizing()) { // [impl->req~ux.external-library-changes.synchronize~1] BackgroundTask.wrap(() -> scanner.scanForChanges(() -> awaitStableLibraryFile(generation))) - .onSuccess(changes -> onScannedForSynchronization(generation, scanner, scannedBaseline, changes)) + .onSuccess(changes -> changes.ifPresent(scanned -> onScannedForSynchronization(generation, scanner, scannedBaseline, scanned))) .onFailure(e -> LOGGER.error("Error while synchronizing with the library file", e)) .executeWith(taskExecutor); return; } BackgroundTask.wrap(() -> scanner.scanForChanges(() -> awaitStableLibraryFile(generation))) - .onSuccess(changes -> { - if (!changes.isEmpty()) { - listeners.forEach(listener -> listener.databaseChanged(changes)); - } - }) + .onSuccess(changes -> changes.filter(scanned -> !scanned.isEmpty()) + .ifPresent(scanned -> listeners.forEach(listener -> listener.databaseChanged(scanned)))) .onFailure(e -> LOGGER.error("Error while watching for changes", e)) .executeWith(taskExecutor); } @@ -289,10 +286,12 @@ private void onScannedForSynchronization(int generation, ChangeScanner scanner, /// size and modification time have stopped changing for a while. The state seen becomes the known disk state, so /// that the events of the write just waited for do not trigger another scan; only for the current scan, since a /// scan already overtaken will not apply what it sees, and recording it would make the next event look handled. - private void awaitStableLibraryFile(int generation) { + /// + /// @return `false` when the wait was interrupted; the file may still be incomplete, so the scan must not go on + private boolean awaitStableLibraryFile(int generation) { Path path = monitoredPath.orElse(null); if (path == null) { - return; + return true; } FileSnapshot last = FileSnapshot.read(path); int unchanged = 0; @@ -302,7 +301,7 @@ private void awaitStableLibraryFile(int generation) { } catch (InterruptedException e) { LOGGER.debug("Interrupted while waiting for {} to stop changing; the scan is abandoned", path, e); Thread.currentThread().interrupt(); - return; + return false; } FileSnapshot current = FileSnapshot.read(path); unchanged = Objects.equals(current, last) ? unchanged + 1 : 0; @@ -313,6 +312,7 @@ private void awaitStableLibraryFile(int generation) { knownDiskState = last; } } + return true; } /// Applies what changed on disk only, and offers the review for what changed on both sides. diff --git a/jabgui/src/main/java/org/jabref/gui/collab/entrychange/EntryChange.java b/jabgui/src/main/java/org/jabref/gui/collab/entrychange/EntryChange.java index 180ffa6e5cb2..c2c020d1e82c 100644 --- a/jabgui/src/main/java/org/jabref/gui/collab/entrychange/EntryChange.java +++ b/jabgui/src/main/java/org/jabref/gui/collab/entrychange/EntryChange.java @@ -12,6 +12,7 @@ import org.jabref.model.entry.field.Field; import org.jabref.model.undo.CompoundEdit; import org.jabref.model.undo.UndoableChangeType; +import org.jabref.model.undo.UndoableChangedFlag; import org.jabref.model.undo.UndoableCommentsChange; import org.jabref.model.undo.UndoableFieldChange; @@ -47,6 +48,8 @@ public BibEntry getNewEntry() { @Override public void applyChange(CompoundEdit undoEdit) { CompoundEdit entryEdit = new CompoundEdit(getName()); + // First in the compound, so that undo restores it after the field edits have marked the entry changed again + entryEdit.applyEdit(new UndoableChangedFlag(oldEntry, oldEntry.hasChanged(), true)); if (!Objects.equals(oldEntry.getType(), newEntry.getType())) { entryEdit.applyEdit(new UndoableChangeType(oldEntry, oldEntry.getType(), newEntry.getType())); } diff --git a/jabgui/src/test/java/org/jabref/gui/collab/ChangeTriageTest.java b/jabgui/src/test/java/org/jabref/gui/collab/ChangeTriageTest.java index 1468ca035a6b..a0ea8c98e8ff 100644 --- a/jabgui/src/test/java/org/jabref/gui/collab/ChangeTriageTest.java +++ b/jabgui/src/test/java/org/jabref/gui/collab/ChangeTriageTest.java @@ -2,6 +2,7 @@ import java.nio.charset.StandardCharsets; import java.util.List; +import java.util.Optional; import org.jabref.gui.collab.entryadd.EntryAdd; import org.jabref.gui.collab.entrychange.EntryChange; @@ -22,6 +23,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.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -164,8 +166,27 @@ void commentChangedOnDiskIsAcceptedAndApplied() { ChangeTriage.Triage triage = triage(); EntryChange change = assertInstanceOf(EntryChange.class, triage.diskOnly().getFirst()); + local.setChanged(false); change.applyChange(new CompoundEdit("test")); assertEquals("% from disk", local.getUserComments()); + // otherwise a save would write the parsed serialization with the old comment + assertTrue(local.hasChanged()); + } + + @Test + void undoOfAppliedChangeRestoresTheParsedState() { + disk.setField(StandardField.TITLE, "Disk title"); + ChangeTriage.Triage triage = triage(); + EntryChange change = assertInstanceOf(EntryChange.class, triage.diskOnly().getFirst()); + local.setChanged(false); + CompoundEdit edit = new CompoundEdit("test"); + + change.applyChange(edit); + assertTrue(local.hasChanged()); + edit.toChangeSet().inverted().apply(); + + assertEquals(Optional.of("Title"), local.getField(StandardField.TITLE)); + assertFalse(local.hasChanged()); } @Test diff --git a/jablib/src/main/java/module-info.java b/jablib/src/main/java/module-info.java index be6deb381bd0..f665848e1164 100644 --- a/jablib/src/main/java/module-info.java +++ b/jablib/src/main/java/module-info.java @@ -128,7 +128,6 @@ exports org.jabref.logic.icore; exports org.jabref.model.icore; exports org.jabref.logic.git.merge.planning; - exports org.jabref.logic.git.merge.planning.util; exports org.jabref.logic.git.merge.execution; exports org.jabref.model.sciteTallies; exports org.jabref.logic.bibtex.comparator.plausibility; diff --git a/jablib/src/main/java/org/jabref/logic/sync/LibraryBaseline.java b/jablib/src/main/java/org/jabref/logic/sync/LibraryBaseline.java index d08f45a53c8e..42c4e58b0c59 100644 --- a/jablib/src/main/java/org/jabref/logic/sync/LibraryBaseline.java +++ b/jablib/src/main/java/org/jabref/logic/sync/LibraryBaseline.java @@ -2,11 +2,14 @@ import java.nio.charset.Charset; import java.util.ArrayList; +import java.util.Collection; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Optional; +import java.util.Set; import java.util.function.Predicate; import org.jabref.logic.citationkeypattern.GlobalCitationKeyPatterns; @@ -187,14 +190,17 @@ public Optional mergeEntry(BibEntry local, BibEntry remote) { // Without a common ancestor (entry new on both sides), an empty entry of the in-memory type makes every field // an addition, so only fields set differently on both sides count as conflicts and the in-memory type is kept BibEntry ancestor = base == null ? new BibEntry(local.getType()) : base.toEntry(); - if (ConflictRules.hasConflictingFields(ancestor, local, remote)) { + boolean commentsChangedLocally = !ancestor.getUserComments().equals(local.getUserComments()); + boolean commentsChangedRemotely = !ancestor.getUserComments().equals(remote.getUserComments()); + boolean commentsConflict = commentsChangedLocally && commentsChangedRemotely && !local.getUserComments().equals(remote.getUserComments()); + if (commentsConflict || ConflictRules.hasConflictingFields(ancestor, local, remote)) { return Optional.empty(); } BibEntry merged = new BibEntry(local); if (!ancestor.getType().equals(remote.getType())) { merged.setType(remote.getType()); } - if (ancestor.getUserComments().equals(local.getUserComments())) { + if (!commentsChangedLocally) { merged.setCommentsBeforeEntry(remote.getUserComments()); } FieldPatchComputer.compute(ancestor, local, remote).forEach((field, value) -> { @@ -230,15 +236,40 @@ public Side sideOfAddedString(String name, String content, Predicate exi if (strings.containsKey(name)) { return sideOf(strings.get(name), null, content); } - boolean renamedFromDeleted = strings.entrySet().stream() - .anyMatch(base -> base.getValue().equals(content) && !existsInMemory.test(base.getKey())); - return renamedFromDeleted ? Side.BOTH : Side.DISK; + // Only an unambiguous source counts: with several equal-valued strings gone from memory, nothing says which one was renamed + long deletedWithSameContent = strings.entrySet().stream() + .filter(base -> base.getValue().equals(content) && !existsInMemory.test(base.getKey())) + .count(); + return deletedWithSameContent == 1 ? Side.BOTH : Side.DISK; } /// A string renamed on disk is taken over only if memory neither touched the old string nor already uses the new name. - public Side sideOfStringRename(String oldName, String oldContent, String newName) { + /// + /// @param existsInMemory whether memory has a string of the given name + public Side sideOfStringRename(String oldName, String oldContent, String newName, Predicate existsInMemory) { boolean oldUntouchedInMemory = Objects.equals(strings.get(oldName), oldContent); - return oldUntouchedInMemory && !strings.containsKey(newName) ? Side.DISK : Side.BOTH; + return oldUntouchedInMemory && !existsInMemory.test(newName) ? Side.DISK : Side.BOTH; + } + + /// Among the given baseline entries, the one a disk entry most likely was taken from when neither key nor exact + /// content match anymore: same type and at most one field besides the citation key differing, and only when + /// exactly one candidate is that close. + public Optional closestOf(Collection candidateIds, BibEntry remote) { + EntrySnapshot snapshot = EntrySnapshot.view(remote).withoutKey(); + List close = candidateIds.stream() + .filter(id -> entriesById.containsKey(id)) + .filter(id -> differingFields(entriesById.get(id).withoutKey(), snapshot) <= 1) + .toList(); + return close.size() == 1 ? Optional.of(close.getFirst()) : Optional.empty(); + } + + private static int differingFields(EntrySnapshot one, EntrySnapshot other) { + if (!one.type().equals(other.type())) { + return Integer.MAX_VALUE; + } + Set fields = new HashSet<>(one.fields().keySet()); + fields.addAll(other.fields().keySet()); + return (int) fields.stream().filter(field -> !Objects.equals(one.fields().get(field), other.fields().get(field))).count(); } /// `null` stands for "absent" on that side. diff --git a/jablib/src/main/java/org/jabref/logic/undo/BibChangeDescriber.java b/jablib/src/main/java/org/jabref/logic/undo/BibChangeDescriber.java index 2be4012bcbb0..e07f2a4b6035 100644 --- a/jablib/src/main/java/org/jabref/logic/undo/BibChangeDescriber.java +++ b/jablib/src/main/java/org/jabref/logic/undo/BibChangeDescriber.java @@ -5,6 +5,8 @@ import org.jabref.model.undo.BibChange; import org.jabref.model.undo.ChangeSet; import org.jabref.model.undo.UndoableChangeType; +import org.jabref.model.undo.UndoableChangedFlag; +import org.jabref.model.undo.UndoableCommentsChange; import org.jabref.model.undo.UndoableFieldChange; import org.jabref.model.undo.UndoableGroupTreeChange; import org.jabref.model.undo.UndoableInsertEntries; @@ -43,6 +45,10 @@ public static String describe(BibChange change) { Localization.lang("Change field %0", FieldTextMapper.getDisplayName(fieldChange.field())); case UndoableChangeType _ -> Localization.lang("Change entry type"); + case UndoableCommentsChange _ -> + Localization.lang("Change entry comments"); + case UndoableChangedFlag _ -> + Localization.lang("Mark entry as changed"); case UndoableInsertEntries insert -> insert.entries().size() == 1 ? Localization.lang("Insert entry") : Localization.lang("Insert entries"); case UndoableRemoveEntries remove -> diff --git a/jablib/src/main/java/org/jabref/model/undo/BibChange.java b/jablib/src/main/java/org/jabref/model/undo/BibChange.java index fac3d7243d01..82f24d5cac17 100644 --- a/jablib/src/main/java/org/jabref/model/undo/BibChange.java +++ b/jablib/src/main/java/org/jabref/model/undo/BibChange.java @@ -12,7 +12,7 @@ /// user acts in, as the name of the enclosing [ChangeSet]. @NullMarked public sealed interface BibChange permits - UndoableCommentsChange, ChangeSet, + UndoableCommentsChange, UndoableChangedFlag, ChangeSet, UndoableInsertEntries, UndoableRemoveEntries, UndoableChangeType, diff --git a/jablib/src/main/java/org/jabref/model/undo/UndoableChangedFlag.java b/jablib/src/main/java/org/jabref/model/undo/UndoableChangedFlag.java new file mode 100644 index 000000000000..e91d5e5fab89 --- /dev/null +++ b/jablib/src/main/java/org/jabref/model/undo/UndoableChangedFlag.java @@ -0,0 +1,36 @@ +package org.jabref.model.undo; + +import java.util.Objects; + +import org.jabref.model.entry.BibEntry; + +import org.jspecify.annotations.NullMarked; + +/// Records an entry's "changed since parsing" flag around an in-place edit, so that undoing the edit also restores +/// whether the entry's parsed serialization may be reused on save. +@NullMarked +public record UndoableChangedFlag(BibEntry entry, boolean before, boolean after) implements BibChange { + @Override + public UndoableChangedFlag inverted() { + return new UndoableChangedFlag(entry, after, before); + } + + @Override + public ApplyResult apply() { + entry.setChanged(after); + return ApplyResult.SUCCESS; + } + + @Override + public boolean equals(Object object) { + return (object instanceof UndoableChangedFlag other) + && ChangeIdentity.same(entry, other.entry) + && before == other.before + && after == other.after; + } + + @Override + public int hashCode() { + return Objects.hash(ChangeIdentity.hash(entry), before, after); + } +} diff --git a/jablib/src/main/java/org/jabref/model/undo/UndoableCommentsChange.java b/jablib/src/main/java/org/jabref/model/undo/UndoableCommentsChange.java index 3c28b29b08d9..9c97e22cbb40 100644 --- a/jablib/src/main/java/org/jabref/model/undo/UndoableCommentsChange.java +++ b/jablib/src/main/java/org/jabref/model/undo/UndoableCommentsChange.java @@ -15,8 +15,14 @@ public UndoableCommentsChange inverted() { } @Override - public void apply() { + public ApplyResult apply() { + if (!entry.getUserComments().equals(before)) { + return ApplyResult.of(this, "entry comments are not the recorded ones"); + } entry.setCommentsBeforeEntry(after); + // The setter is the parser's and leaves the flag alone; here the comment differs from what was parsed + entry.setChanged(true); + return ApplyResult.SUCCESS; } @Override diff --git a/jablib/src/main/resources/l10n/JabRef_en.properties b/jablib/src/main/resources/l10n/JabRef_en.properties index 2adac99d0e6b..85843f7417fe 100644 --- a/jablib/src/main/resources/l10n/JabRef_en.properties +++ b/jablib/src/main/resources/l10n/JabRef_en.properties @@ -102,6 +102,8 @@ Case\ sensitive=Case sensitive Change\ case=Change case Change\ entry\ type=Change entry type +Change\ entry\ comments=Change entry comments +Mark\ entry\ as\ changed=Mark entry as changed Changed\ language\ to\ %0=Changed language to %0 diff --git a/jablib/src/test/java/org/jabref/logic/sync/LibraryBaselineTest.java b/jablib/src/test/java/org/jabref/logic/sync/LibraryBaselineTest.java index f2f19a2b326c..1a9a6595d6bb 100644 --- a/jablib/src/test/java/org/jabref/logic/sync/LibraryBaselineTest.java +++ b/jablib/src/test/java/org/jabref/logic/sync/LibraryBaselineTest.java @@ -164,4 +164,52 @@ void stringDeletedInMemoryAndRenamedOnDiskIsAConflict() { assertEquals(Side.BOTH, baseline.sideOfAddedString("january", "January", _ -> false)); assertEquals(Side.DISK, baseline.sideOfAddedString("feb", "February", _ -> false)); } + + @Test + void commentsChangedDifferentlyOnBothSidesAreAConflict() { + local.setCommentsBeforeEntry("% memory"); + remote.setCommentsBeforeEntry("% disk"); + + assertEquals(Optional.empty(), baseline.mergeEntry(local, remote)); + } + + @Test + void stringRenamedOnDiskToANameTakenInMemoryIsAConflict() { + BibDatabase database = new BibDatabase(List.of(local)); + database.addString(new BibtexString("jan", "January")); + baseline = LibraryBaseline.of(new BibDatabaseContext(database), PATTERNS); + + assertEquals(Side.DISK, baseline.sideOfStringRename("jan", "January", "january", _ -> false)); + assertEquals(Side.BOTH, baseline.sideOfStringRename("jan", "January", "january", "january"::equals)); + } + + @Test + void ambiguousRenameSourceIsNotAConflict() { + BibDatabase database = new BibDatabase(List.of(local)); + database.addString(new BibtexString("jan", "January")); + database.addString(new BibtexString("januar", "January")); + baseline = LibraryBaseline.of(new BibDatabaseContext(database), PATTERNS); + + // both equal-valued strings are gone from memory: nothing says which one the disk string was renamed from + assertEquals(Side.DISK, baseline.sideOfAddedString("january", "January", _ -> false)); + } + + @Test + void closestEntryIsFoundWhenKeyAndOneFieldChanged() { + BibEntry other = new BibEntry(StandardEntryType.Book).withCitationKey("Other").withField(StandardField.TITLE, "Other"); + baseline = LibraryBaseline.of(new BibDatabaseContext(new BibDatabase(List.of(local, other))), PATTERNS); + remote.setCitationKey("Renamed"); + remote.setField(StandardField.YEAR, "2021"); + + assertEquals(Optional.of(local.getId()), baseline.closestOf(List.of(local.getId(), other.getId()), remote)); + } + + @Test + void closestEntryIsNotGuessedWhenTwoFieldsChanged() { + remote.setCitationKey("Renamed"); + remote.setField(StandardField.YEAR, "2021"); + remote.setField(StandardField.TITLE, "Another title"); + + assertEquals(Optional.empty(), baseline.closestOf(List.of(local.getId()), remote)); + } } From 0c78cbe9aa8dbf99ccde6e6202c0e82d04a2ac25 Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Sun, 13 Sep 2026 02:10:40 +0200 Subject: [PATCH 16/27] Make synchronization a per-library setting with a global default Each library decides in its properties (Saving tab) whether external changes of its file are merged without asking; libraries that do not decide follow the new preference in the General tab, which is off by default. Autosave is its own option again. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01GWmXyN3GUNhfF9fSo1UNrt --- docs/requirements/ux.md | 2 +- .../gui/collab/DatabaseChangeMonitor.java | 29 +++++++++---- .../saving/SavingPropertiesView.java | 26 ++++++++++++ .../saving/SavingPropertiesViewModel.java | 14 +++++++ .../gui/preferences/general/GeneralTab.java | 3 +- .../general/GeneralTabViewModel.java | 7 ++++ .../saving/SavingProperties.fxml | 6 +++ .../gui/collab/DatabaseChangeMonitorTest.java | 2 + .../GroupTreeSharedDatabaseProfileTest.java | 2 +- .../gui/groups/GroupTreeViewModelTest.java | 1 + .../org/jabref/logic/LibraryPreferences.java | 17 ++++++++ .../logic/exporter/MetaDataSerializer.java | 1 + .../logic/importer/util/MetaDataParser.java | 2 + .../preferences/JabRefCliPreferences.java | 3 ++ .../org/jabref/model/metadata/MetaData.java | 27 ++++++++++++ .../main/resources/l10n/JabRef_en.properties | 8 +++- .../importer/fileformat/BibtexParserTest.java | 42 +++++++++++-------- 17 files changed, 163 insertions(+), 29 deletions(-) diff --git a/docs/requirements/ux.md b/docs/requirements/ux.md index 72d6cf49ac29..d33251e19b82 100644 --- a/docs/requirements/ux.md +++ b/docs/requirements/ux.md @@ -109,7 +109,7 @@ Needs: impl ## Synchronizing a library with its file `req~ux.external-library-changes.synchronize~1` -While a local library is set to be synchronized with its file (the former "autosave" preference), external changes to the file must be merged into the in-memory library without asking, and unsaved in-memory changes must not be reported as external changes. +While a local library is set to be synchronized with its file (a per-library setting with a global default, off unless enabled), external changes to the file must be merged into the in-memory library without asking, and unsaved in-memory changes must not be reported as external changes. A review is only required for an item (entry field, entry type, metadata, preamble, string) that was changed differently in memory and in the file, or that was deleted on one side and changed on the other. Fields changed only in the file are taken over even when other fields of the same entry were changed in memory. diff --git a/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java b/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java index cf34ab875855..a9909fb081ae 100644 --- a/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java +++ b/jabgui/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java @@ -22,10 +22,12 @@ import org.jabref.logic.util.TaskExecutor; import org.jabref.logic.util.io.FileSnapshot; import org.jabref.model.database.BibDatabaseContext; +import org.jabref.model.metadata.event.MetaDataChangedEvent; import org.jabref.model.util.FileUpdateListener; import org.jabref.model.util.FileUpdateMonitor; import com.dlsc.gemsfx.infocenter.NotificationAction; +import com.google.common.eventbus.Subscribe; import org.jspecify.annotations.Nullable; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -66,7 +68,7 @@ public class DatabaseChangeMonitor implements FileUpdateListener { /// instead of applying stale content. private volatile int scanGeneration; - private final ChangeListener synchronizingListener = (_, _, enabled) -> onSynchronizingChanged(enabled); + private final ChangeListener synchronizingListener = (_, _, _) -> onSynchronizingChanged(isSynchronizing()); public DatabaseChangeMonitor(BibDatabaseContext database, FileUpdateMonitor fileMonitor, @@ -99,7 +101,8 @@ public DatabaseChangeMonitor(BibDatabaseContext database, // Registered once per monitor; a tab replacing its monitor calls unregister() on the old one first, which // removes this listener again, so the preference never accumulates listeners if (database.getLocation() == DatabaseLocation.LOCAL) { - preferences.getLibraryPreferences().autoSaveProperty().addListener(synchronizingListener); + preferences.getLibraryPreferences().synchronizeWithFileProperty().addListener(synchronizingListener); + database.getMetaData().registerListener(this); } }); @@ -199,10 +202,17 @@ public void markConsistentWithDisk(@Nullable FileSnapshot diskState) { } } - /// Synchronizing (silently merging external changes) is tied to the autosave preference: both together keep a - /// local library and its file the same in both directions. + /// Synchronizing (silently merging external changes) is decided by the library itself, or, if it does not, by the + /// global preference. private boolean isSynchronizing() { - return database.getLocation() == DatabaseLocation.LOCAL && preferences.getLibraryPreferences().shouldAutoSave(); + return database.getLocation() == DatabaseLocation.LOCAL + && database.getMetaData().getSynchronizeWithFile().orElseGet(() -> preferences.getLibraryPreferences().shouldSynchronizeWithFile()); + } + + /// The library's own setting is part of its metadata, which the library properties dialog changes. + @Subscribe + public void listen(MetaDataChangedEvent event) { + onSynchronizingChanged(isSynchronizing()); } /// Synchronization switched on for an open library needs a baseline right away: as long as the library is @@ -211,8 +221,10 @@ private void onSynchronizingChanged(boolean enabled) { boolean captured = false; synchronized (database) { if (!enabled) { - baseline = null; - scanGeneration++; + if (baseline != null) { + baseline = null; + scanGeneration++; + } } else if (baseline == null && !libraryTab.isModified()) { baseline = captureBaseline(); captured = baseline != null; @@ -377,7 +389,8 @@ public void unregister() { fileMonitor.removeListener(path, this); // Unconditionally: the library may have been converted to a shared one since the listener was added, // and removing a listener that was never added is a no-op - preferences.getLibraryPreferences().autoSaveProperty().removeListener(synchronizingListener); + preferences.getLibraryPreferences().synchronizeWithFileProperty().removeListener(synchronizingListener); + database.getMetaData().unregisterListener(this); }); } } diff --git a/jabgui/src/main/java/org/jabref/gui/libraryproperties/saving/SavingPropertiesView.java b/jabgui/src/main/java/org/jabref/gui/libraryproperties/saving/SavingPropertiesView.java index 38ca34e3fdb2..7c5e40e5a6cf 100644 --- a/jabgui/src/main/java/org/jabref/gui/libraryproperties/saving/SavingPropertiesView.java +++ b/jabgui/src/main/java/org/jabref/gui/libraryproperties/saving/SavingPropertiesView.java @@ -10,12 +10,14 @@ import org.jabref.gui.commonfxcontrols.SaveOrderConfigPanel; import org.jabref.gui.libraryproperties.AbstractPropertiesTabView; import org.jabref.gui.libraryproperties.PropertiesTab; +import org.jabref.gui.preferences.GuiPreferences; import org.jabref.logic.cleanup.CleanupPreferences; import org.jabref.logic.journals.AbbreviationType; import org.jabref.logic.l10n.Localization; import org.jabref.model.database.BibDatabaseContext; import com.airhacks.afterburner.views.ViewLoader; +import jakarta.inject.Inject; public class SavingPropertiesView extends AbstractPropertiesTabView implements PropertiesTab { @@ -23,6 +25,9 @@ public class SavingPropertiesView extends AbstractPropertiesTabView journalAbbreviationOnSave; + @FXML private ComboBox synchronizeWithFile; + + @Inject private GuiPreferences preferences; public SavingPropertiesView(BibDatabaseContext databaseContext) { this.databaseContext = databaseContext; @@ -79,5 +84,26 @@ public AbbreviationType fromString(String string) { } }); journalAbbreviationOnSave.valueProperty().bindBidirectional(viewModel.journalAbbreviationOnSaveProperty()); + + synchronizeWithFile.setItems(FXCollections.observableArrayList(null, Boolean.TRUE, Boolean.FALSE)); + synchronizeWithFile.setConverter(new StringConverter<>() { + @Override + public String toString(Boolean synchronize) { + if (synchronize == null) { + return Localization.lang("Use global setting (%0)", onOrOff(preferences.getLibraryPreferences().shouldSynchronizeWithFile())); + } + return onOrOff(synchronize); + } + + @Override + public Boolean fromString(String string) { + return null; + } + }); + synchronizeWithFile.valueProperty().bindBidirectional(viewModel.synchronizeWithFileProperty()); + } + + private static String onOrOff(boolean on) { + return on ? Localization.lang("On") : Localization.lang("Off"); } } diff --git a/jabgui/src/main/java/org/jabref/gui/libraryproperties/saving/SavingPropertiesViewModel.java b/jabgui/src/main/java/org/jabref/gui/libraryproperties/saving/SavingPropertiesViewModel.java index 30f897aec74a..b26e37ef8600 100644 --- a/jabgui/src/main/java/org/jabref/gui/libraryproperties/saving/SavingPropertiesViewModel.java +++ b/jabgui/src/main/java/org/jabref/gui/libraryproperties/saving/SavingPropertiesViewModel.java @@ -51,6 +51,8 @@ public class SavingPropertiesViewModel implements PropertiesTabViewModel { // Journal abbreviation on save private final ObjectProperty journalAbbreviationOnSaveProperty = new SimpleObjectProperty<>(); + /// `null` = follow the global preference + private final ObjectProperty synchronizeWithFileProperty = new SimpleObjectProperty<>(); private final FieldFormatterCleanupActions defaultSaveActions; @@ -104,6 +106,7 @@ public void setValues(MetaData metaData) { }); journalAbbreviationOnSaveProperty.setValue(metaData.getLibraryAbbreviationType().orElse(null)); + synchronizeWithFileProperty.setValue(metaData.getSynchronizeWithFile().orElse(null)); } @Override @@ -141,6 +144,13 @@ public void storeSettings(MetaData metaData) { } } + Boolean synchronize = synchronizeWithFileProperty.getValue(); + if (synchronize == null) { + metaData.clearSynchronizeWithFile(); + } else { + metaData.setSynchronizeWithFile(synchronize); + } + AbbreviationType abbreviationType = journalAbbreviationOnSaveProperty.getValue(); if (abbreviationType != null) { metaData.setLibraryAbbreviationType(abbreviationType); @@ -192,4 +202,8 @@ public ListProperty cleanupsProperty() { public ObjectProperty journalAbbreviationOnSaveProperty() { return journalAbbreviationOnSaveProperty; } + + public ObjectProperty synchronizeWithFileProperty() { + return synchronizeWithFileProperty; + } } diff --git a/jabgui/src/main/java/org/jabref/gui/preferences/general/GeneralTab.java b/jabgui/src/main/java/org/jabref/gui/preferences/general/GeneralTab.java index 795af1b70829..00d44a3619a0 100644 --- a/jabgui/src/main/java/org/jabref/gui/preferences/general/GeneralTab.java +++ b/jabgui/src/main/java/org/jabref/gui/preferences/general/GeneralTab.java @@ -121,8 +121,9 @@ private void buildView() { .section(Localization.lang("Saving"), saving -> saving .checkbox(Localization.lang("Always reformat library on save and export"), viewModel.alwaysReformatBibProperty()) - .checkbox(Localization.lang("Synchronize local libraries with their files"), viewModel.autosaveLocalLibrariesProperty(), + .checkbox(Localization.lang("Autosave local libraries"), viewModel.autosaveLocalLibrariesProperty(), autosave -> autosave.help(HelpFile.AUTOSAVE)) + .checkbox(Localization.lang("Synchronize local libraries with their files (libraries can override this)"), viewModel.synchronizeLocalLibrariesProperty()) .checkWithField(Localization.lang("Create backup"), viewModel.createBackupProperty(), viewModel.backupDirectoryProperty(), directory -> directory diff --git a/jabgui/src/main/java/org/jabref/gui/preferences/general/GeneralTabViewModel.java b/jabgui/src/main/java/org/jabref/gui/preferences/general/GeneralTabViewModel.java index bdf94dd7b449..f390d3c2a5a4 100644 --- a/jabgui/src/main/java/org/jabref/gui/preferences/general/GeneralTabViewModel.java +++ b/jabgui/src/main/java/org/jabref/gui/preferences/general/GeneralTabViewModel.java @@ -90,6 +90,7 @@ public class GeneralTabViewModel implements PreferenceTabViewModel { private final BooleanProperty alwaysReformatBibProperty = new SimpleBooleanProperty(); private final BooleanProperty autosaveLocalLibraries = new SimpleBooleanProperty(); + private final BooleanProperty synchronizeLocalLibraries = new SimpleBooleanProperty(); private final BooleanProperty createBackupProperty = new SimpleBooleanProperty(); private final StringProperty backupDirectoryProperty = new SimpleStringProperty(""); @@ -238,6 +239,7 @@ public void setValues() { alwaysReformatBibProperty.setValue(libraryPreferences.shouldAlwaysReformatOnSave()); autosaveLocalLibraries.setValue(libraryPreferences.shouldAutoSave()); + synchronizeLocalLibraries.setValue(libraryPreferences.shouldSynchronizeWithFile()); createBackupProperty.setValue(filePreferences.shouldCreateBackup()); backupDirectoryProperty.setValue(filePreferences.getBackupDirectory().toString()); @@ -288,6 +290,7 @@ public void storeSettings() { libraryPreferences.setAlwaysReformatOnSave(alwaysReformatBibProperty.getValue()); libraryPreferences.setAutoSave(autosaveLocalLibraries.getValue()); + libraryPreferences.setSynchronizeWithFile(synchronizeLocalLibraries.getValue()); filePreferences.setCreateBackup(createBackupProperty.getValue()); filePreferences.setBackupDirectory(Path.of(backupDirectoryProperty.getValue())); @@ -494,6 +497,10 @@ public BooleanProperty autosaveLocalLibrariesProperty() { return autosaveLocalLibraries; } + public BooleanProperty synchronizeLocalLibrariesProperty() { + return synchronizeLocalLibraries; + } + public BooleanProperty createBackupProperty() { return this.createBackupProperty; } diff --git a/jabgui/src/main/resources/org/jabref/gui/libraryproperties/saving/SavingProperties.fxml b/jabgui/src/main/resources/org/jabref/gui/libraryproperties/saving/SavingProperties.fxml index 3ea357039e60..74eb27ef5510 100644 --- a/jabgui/src/main/resources/org/jabref/gui/libraryproperties/saving/SavingProperties.fxml +++ b/jabgui/src/main/resources/org/jabref/gui/libraryproperties/saving/SavingProperties.fxml @@ -21,6 +21,12 @@