From b5ac16a49ffb83c6fa2daf6a98dfdfd2b7b93f7a Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Tue, 19 Nov 2019 00:45:03 +0100 Subject: [PATCH 1/4] Add tests for changed - Add tests - Guard AtomicFileWriter with a try-with-resources during save - use Objects.requireNonNull in DefaultFileUpdateMontitor - add comment - some auto-formattings - Fix typo in comment --- .../jabref/gui/dialogs/AutosaveUIManager.java | 2 +- .../gui/exporter/SaveDatabaseAction.java | 14 ++- .../gui/util/DefaultFileUpdateMonitor.java | 7 +- .../logic/exporter/BibDatabaseWriter.java | 2 +- .../model/database/BibDatabaseContext.java | 16 ++-- .../java/org/jabref/model/entry/BibEntry.java | 2 +- .../gui/exporter/SaveDatabaseActionTest.java | 66 +++++++++++++- .../exporter/BibtexDatabaseWriterTest.java | 86 ++++++++++++++++++- .../org/jabref/model/entry/BibEntryTest.java | 11 +++ 9 files changed, 180 insertions(+), 26 deletions(-) diff --git a/src/main/java/org/jabref/gui/dialogs/AutosaveUIManager.java b/src/main/java/org/jabref/gui/dialogs/AutosaveUIManager.java index 8a1fa463e59..c9268c40dc1 100644 --- a/src/main/java/org/jabref/gui/dialogs/AutosaveUIManager.java +++ b/src/main/java/org/jabref/gui/dialogs/AutosaveUIManager.java @@ -28,7 +28,7 @@ public void listen(@SuppressWarnings("unused") AutosaveEvent event) { try { new SaveDatabaseAction(panel, Globals.prefs, Globals.entryTypesManager).save(SaveDatabaseAction.SaveDatabaseMode.SILENT); } catch (Throwable e) { - LOGGER.error("Problem occured while saving.", e); + LOGGER.error("Problem occurred while saving.", e); } } } diff --git a/src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java b/src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java index 6d9dcfed408..f3b55ea48ec 100644 --- a/src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java +++ b/src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java @@ -42,7 +42,7 @@ /** * Action for the "Save" and "Save as" operations called from BasePanel. This class is also used for save operations * when closing a database or quitting the applications. - * + *

* The save operation is loaded off of the GUI thread using {@link BackgroundTask}. Callers can query whether the * operation was canceled, or whether it was successful. */ @@ -69,12 +69,10 @@ public SaveDatabaseAction(BasePanel panel, JabRefPreferences prefs, BibEntryType } private boolean saveDatabase(Path file, boolean selectedOnly, Charset encoding, SavePreferences.DatabaseSaveType saveType) throws SaveException { - try { - SavePreferences preferences = prefs.loadForSaveFromPreferences() - .withEncoding(encoding) - .withSaveType(saveType); - - AtomicFileWriter fileWriter = new AtomicFileWriter(file, preferences.getEncoding(), preferences.makeBackup()); + SavePreferences preferences = prefs.loadForSaveFromPreferences() + .withEncoding(encoding) + .withSaveType(saveType); + try (AtomicFileWriter fileWriter = new AtomicFileWriter(file, preferences.getEncoding(), preferences.makeBackup())) { BibtexDatabaseWriter databaseWriter = new BibtexDatabaseWriter(fileWriter, preferences, entryTypesManager); if (selectedOnly) { @@ -148,7 +146,7 @@ private boolean doSave() { // Reset title of tab frame.setTabTitle(panel, panel.getTabTitle(), - panel.getBibDatabaseContext().getDatabaseFile().get().getAbsolutePath()); + panel.getBibDatabaseContext().getDatabasePath().get().toAbsolutePath().toString()); frame.setWindowTitle(); frame.updateAllTabTitles(); } diff --git a/src/main/java/org/jabref/gui/util/DefaultFileUpdateMonitor.java b/src/main/java/org/jabref/gui/util/DefaultFileUpdateMonitor.java index bdf0b51e2a6..f5600df45c5 100644 --- a/src/main/java/org/jabref/gui/util/DefaultFileUpdateMonitor.java +++ b/src/main/java/org/jabref/gui/util/DefaultFileUpdateMonitor.java @@ -7,6 +7,7 @@ import java.nio.file.WatchEvent; import java.nio.file.WatchKey; import java.nio.file.WatchService; +import java.util.Objects; import org.jabref.model.util.FileUpdateListener; import org.jabref.model.util.FileUpdateMonitor; @@ -19,7 +20,7 @@ /** * This class monitors a set of files for changes. Upon detecting a change it notifies the registered {@link * FileUpdateListener}s. - * + *

* Implementation based on https://stackoverflow.com/questions/16251273/can-i-watch-for-single-file-change-with-watchservice-not-the-whole-directory */ public class DefaultFileUpdateMonitor implements Runnable, FileUpdateMonitor { @@ -69,9 +70,7 @@ private void notifyAboutChange(Path path) { @Override public void addListenerForFile(Path file, FileUpdateListener listener) throws IOException { - if (watcher == null) { - throw new IllegalStateException("You need to start the file monitor before watching files"); - } + Objects.requireNonNull(watcher, "You need to start the file monitor before watching files"); // We can't watch files directly, so monitor their parent directory for updates Path directory = file.toAbsolutePath().getParent(); diff --git a/src/main/java/org/jabref/logic/exporter/BibDatabaseWriter.java b/src/main/java/org/jabref/logic/exporter/BibDatabaseWriter.java index ee643f1ced7..15ff2f4c74b 100644 --- a/src/main/java/org/jabref/logic/exporter/BibDatabaseWriter.java +++ b/src/main/java/org/jabref/logic/exporter/BibDatabaseWriter.java @@ -151,8 +151,8 @@ public void saveDatabase(BibDatabaseContext bibDatabaseContext) throws IOExcepti */ public void savePartOfDatabase(BibDatabaseContext bibDatabaseContext, List entries) throws IOException { Optional sharedDatabaseIDOptional = bibDatabaseContext.getDatabase().getSharedDatabaseID(); - if (sharedDatabaseIDOptional.isPresent()) { + // may throw an IOException. Thus, we do not use "ifPresent", but the "old" isPresent way writeDatabaseID(sharedDatabaseIDOptional.get()); } diff --git a/src/main/java/org/jabref/model/database/BibDatabaseContext.java b/src/main/java/org/jabref/model/database/BibDatabaseContext.java index 0798b168fe2..4e5a46e3dd1 100644 --- a/src/main/java/org/jabref/model/database/BibDatabaseContext.java +++ b/src/main/java/org/jabref/model/database/BibDatabaseContext.java @@ -29,10 +29,12 @@ public class BibDatabaseContext { private final BibDatabase database; private final Defaults defaults; private MetaData metaData; + /** * The file where this database was last saved to. */ private Optional file; + private DatabaseSynchronizer dbmsSynchronizer; private CoarseChangeFilter dbmsListener; private DatabaseLocation location; @@ -115,7 +117,6 @@ public Optional getDatabaseFile() { } /** - * * @param file the database file * @deprecated use {@link #setDatabaseFile(Path)} */ @@ -155,11 +156,11 @@ public boolean isBiblatexMode() { public List getFileDirectoriesAsPaths(FilePreferences preferences) { // Filter for empty string, as this would be expanded to the jar-directory with Paths.get() return getFileDirectories(preferences).stream() - .filter(s -> !s.isEmpty()) - .map(Paths::get) - .map(Path::toAbsolutePath) - .map(Path::normalize) - .collect(Collectors.toList()); + .filter(s -> !s.isEmpty()) + .map(Paths::get) + .map(Path::toAbsolutePath) + .map(Path::normalize) + .collect(Collectors.toList()); } /** @@ -192,7 +193,7 @@ public Optional getFirstExistingFileDir(FilePreferences preferences) { *

  • BIB file directory
    • * * - * @param field The field + * @param field The field * @param preferences The fileDirectory preferences * @return The default directory for this field type. */ @@ -293,5 +294,4 @@ public void convertToLocalDatabase() { public List getEntries() { return database.getEntries(); } - } diff --git a/src/main/java/org/jabref/model/entry/BibEntry.java b/src/main/java/org/jabref/model/entry/BibEntry.java index 845d482c3d0..49e548b2a44 100644 --- a/src/main/java/org/jabref/model/entry/BibEntry.java +++ b/src/main/java/org/jabref/model/entry/BibEntry.java @@ -81,7 +81,7 @@ public class BibEntry implements Cloneable { /** * Marks whether the complete serialization, which was read from file, should be used. * - * Is set to false, if parts of the entry change. This causes the entry to be serialized based on the internal state (and not based on the old serialization) + * Is set to true, if parts of the entry changed. This causes the entry to be serialized based on the internal state (and not based on the old serialization) */ private boolean changed; diff --git a/src/test/java/org/jabref/gui/exporter/SaveDatabaseActionTest.java b/src/test/java/org/jabref/gui/exporter/SaveDatabaseActionTest.java index 7e06be4af15..cd535cc3b40 100644 --- a/src/test/java/org/jabref/gui/exporter/SaveDatabaseActionTest.java +++ b/src/test/java/org/jabref/gui/exporter/SaveDatabaseActionTest.java @@ -1,20 +1,34 @@ package org.jabref.gui.exporter; +import java.io.IOException; +import java.nio.charset.Charset; +import java.nio.file.Files; import java.nio.file.Path; +import java.util.List; import java.util.Optional; import org.jabref.gui.BasePanel; import org.jabref.gui.DialogService; import org.jabref.gui.JabRefFrame; +import org.jabref.gui.undo.CountingUndoManager; import org.jabref.gui.util.FileDialogConfiguration; +import org.jabref.logic.bibtex.FieldContentParserPreferences; +import org.jabref.logic.bibtex.LatexFieldFormatterPreferences; +import org.jabref.logic.exporter.SavePreferences; +import org.jabref.model.bibtexkeypattern.GlobalBibtexKeyPattern; +import org.jabref.model.database.BibDatabase; import org.jabref.model.database.BibDatabaseContext; import org.jabref.model.database.shared.DatabaseLocation; +import org.jabref.model.entry.BibEntry; import org.jabref.model.entry.BibEntryTypesManager; +import org.jabref.model.entry.field.StandardField; +import org.jabref.model.metadata.MetaData; import org.jabref.preferences.JabRefPreferences; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.mockito.Mockito.any; import static org.mockito.Mockito.doNothing; @@ -27,8 +41,7 @@ class SaveDatabaseActionTest { private static final String TEST_BIBTEX_LIBRARY_LOCATION = "C:\\Users\\John_Doe\\Jabref\\literature.bib"; - private final Path file = Path.of(TEST_BIBTEX_LIBRARY_LOCATION); - + private Path file = Path.of(TEST_BIBTEX_LIBRARY_LOCATION); private DialogService dialogService = mock(DialogService.class); private JabRefPreferences preferences = mock(JabRefPreferences.class); private BasePanel basePanel = mock(BasePanel.class); @@ -91,6 +104,55 @@ public void saveShouldShowSaveAsIfDatabaseNotSelected() { verify(saveDatabaseAction, times(1)).saveAs(file); } + private SaveDatabaseAction createSaveDatabaseActionForBibDatabase(BibDatabase database) throws IOException { + file = Files.createTempFile("JabRef", ".bib"); + file.toFile().deleteOnExit(); + + LatexFieldFormatterPreferences latexFieldFormatterPreferences = mock(LatexFieldFormatterPreferences.class); + when(latexFieldFormatterPreferences.getFieldContentParserPreferences()).thenReturn(mock(FieldContentParserPreferences.class)); + SavePreferences savePreferences = mock(SavePreferences.class); + // In case a "thenReturn" is modified, the whole mock has to be recreated + dbContext = mock(BibDatabaseContext.class); + basePanel = mock(BasePanel.class); + MetaData metaData = mock(MetaData.class); + when(savePreferences.withEncoding(any(Charset.class))).thenReturn(savePreferences); + when(savePreferences.withSaveType(any(SavePreferences.DatabaseSaveType.class))).thenReturn(savePreferences); + when(savePreferences.getEncoding()).thenReturn(Charset.forName("UTF-8")); + when(savePreferences.getLatexFieldFormatterPreferences()).thenReturn(latexFieldFormatterPreferences); + GlobalBibtexKeyPattern emptyGlobalBibtexKeyPattern = GlobalBibtexKeyPattern.fromPattern(""); + when(savePreferences.getGlobalCiteKeyPattern()).thenReturn(emptyGlobalBibtexKeyPattern); + when(metaData.getCiteKeyPattern(any(GlobalBibtexKeyPattern.class))).thenReturn(emptyGlobalBibtexKeyPattern); + when(dbContext.getDatabasePath()).thenReturn(Optional.of(file)); + when(dbContext.getLocation()).thenReturn(DatabaseLocation.LOCAL); + when(dbContext.getDatabase()).thenReturn(database); + when(dbContext.getMetaData()).thenReturn(metaData); + when(dbContext.getEntries()).thenReturn(database.getEntries()); + when(preferences.getBoolean(JabRefPreferences.LOCAL_AUTO_SAVE)).thenReturn(false); + when(preferences.getDefaultEncoding()).thenReturn(Charset.forName("UTF-8")); + when(preferences.getFieldContentParserPreferences()).thenReturn(mock(FieldContentParserPreferences.class)); + when(preferences.loadForSaveFromPreferences()).thenReturn(savePreferences); + when(basePanel.frame()).thenReturn(jabRefFrame); + when(basePanel.getBibDatabaseContext()).thenReturn(dbContext); + when(basePanel.getUndoManager()).thenReturn(mock(CountingUndoManager.class)); + when(basePanel.getBibDatabaseContext()).thenReturn(dbContext); + saveDatabaseAction = new SaveDatabaseAction(basePanel, preferences, mock(BibEntryTypesManager.class)); + return saveDatabaseAction; + } + + @Test + public void saveKeepsChangedFlag() throws Exception { + BibEntry firstEntry = new BibEntry().withField(StandardField.AUTHOR, "first"); + firstEntry.setChanged(true); + BibEntry secondEntry = new BibEntry().withField(StandardField.AUTHOR, "second"); + secondEntry.setChanged(true); + BibDatabase database = new BibDatabase(List.of(firstEntry, secondEntry)); + + saveDatabaseAction = createSaveDatabaseActionForBibDatabase(database); + saveDatabaseAction.save(); + + assertThat(database.getEntries()).extracting(BibEntry::hasChanged).doesNotContain(false); + } + @Test public void saveShouldNotSaveDatabaseIfPathNotSet() { when(dbContext.getDatabasePath()).thenReturn(Optional.empty()); diff --git a/src/test/java/org/jabref/logic/exporter/BibtexDatabaseWriterTest.java b/src/test/java/org/jabref/logic/exporter/BibtexDatabaseWriterTest.java index 0ec73d0ceac..eb42e45545d 100644 --- a/src/test/java/org/jabref/logic/exporter/BibtexDatabaseWriterTest.java +++ b/src/test/java/org/jabref/logic/exporter/BibtexDatabaseWriterTest.java @@ -7,6 +7,7 @@ import java.nio.file.Path; import java.nio.file.Paths; import java.util.Arrays; +import java.util.Collection; import java.util.Collections; import java.util.Scanner; @@ -538,7 +539,7 @@ void writeFileDirectories() throws Exception { assertEquals(OS.NEWLINE + "@Comment{jabref-meta: fileDirectory:\\\\Literature\\\\;}" + OS.NEWLINE + OS.NEWLINE + "@Comment{jabref-meta: fileDirectory-defaultOwner-user:D:\\\\Documents;}" - + OS.NEWLINE + OS.NEWLINE + "@Comment{jabref-meta: fileDirectoryLatex-defaultOwner-user:D:\\\\Latex;}" + OS.NEWLINE, stringWriter.toString()); + + OS.NEWLINE + OS.NEWLINE + "@Comment{jabref-meta: fileDirectoryLatex-defaultOwner-user:D:\\\\Latex;}" + OS.NEWLINE, stringWriter.toString()); } @Test @@ -651,4 +652,87 @@ void roundtripWithContentSelectorsAndUmlauts() throws Exception { assertEquals(fileContent, stringWriter.toString()); } + + @Test + public void saveAlsoSavesSecondModification() throws Exception { + // @formatter:off + String bibtexEntry = OS.NEWLINE + "@Article{test," + OS.NEWLINE + + " Author = {Foo Bar}," + OS.NEWLINE + + " Journal = {International Journal of Something}," + OS.NEWLINE + + " Note = {some note}," + OS.NEWLINE + + " Number = {1}," + OS.NEWLINE + + "}"; + // @formatter:on + + // read in bibtex string + ImportFormatPreferences importFormatPreferences = mock(ImportFormatPreferences.class, Answers.RETURNS_DEEP_STUBS); + ParserResult firstParse = new BibtexParser(importFormatPreferences, new DummyFileUpdateMonitor()).parse(new StringReader(bibtexEntry)); + Collection entries = firstParse.getDatabase().getEntries(); + BibEntry entry = entries.iterator().next(); + + // modify entry + entry.setField(StandardField.AUTHOR, "BlaBla"); + + BibDatabaseContext context = new BibDatabaseContext(firstParse.getDatabase(), firstParse.getMetaData(), + new Defaults(BibDatabaseMode.BIBTEX)); + + databaseWriter.savePartOfDatabase(context, firstParse.getDatabase().getEntries()); + + // modify entry a second time + entry.setField(StandardField.AUTHOR, "Test"); + + // write a second time + stringWriter = new StringWriter(); + databaseWriter = new BibtexDatabaseWriter(stringWriter, preferences, entryTypesManager); + databaseWriter.savePartOfDatabase(context, firstParse.getDatabase().getEntries()); + + assertEquals(OS.NEWLINE + + "@Article{test," + OS.NEWLINE + + " author = {Test}," + OS.NEWLINE + + " journal = {International Journal of Something}," + OS.NEWLINE + + " note = {some note}," + OS.NEWLINE + + " number = {1}," + OS.NEWLINE + + "}" + OS.NEWLINE + + "" + OS.NEWLINE + + "@Comment{jabref-meta: databaseType:bibtex;}" + OS.NEWLINE, stringWriter.toString()); + } + + @Test + public void saveReturnsToOriginalEntryWhenEntryIsUnchanged() throws Exception { + // @formatter:off + String bibtexEntry = OS.NEWLINE + "@Article{test," + OS.NEWLINE + + " Author = {Foo Bar}," + OS.NEWLINE + + " Journal = {International Journal of Something}," + OS.NEWLINE + + " Note = {some note}," + OS.NEWLINE + + " Number = {1}," + OS.NEWLINE + + "}"; + // @formatter:on + + // read in bibtex string + ImportFormatPreferences importFormatPreferences = mock(ImportFormatPreferences.class, Answers.RETURNS_DEEP_STUBS); + ParserResult firstParse = new BibtexParser(importFormatPreferences, new DummyFileUpdateMonitor()).parse(new StringReader(bibtexEntry)); + Collection entries = firstParse.getDatabase().getEntries(); + BibEntry entry = entries.iterator().next(); + + // modify entry + entry.setField(StandardField.AUTHOR, "BlaBla"); + + BibDatabaseContext context = new BibDatabaseContext(firstParse.getDatabase(), firstParse.getMetaData(), + new Defaults(BibDatabaseMode.BIBTEX)); + + databaseWriter.savePartOfDatabase(context, firstParse.getDatabase().getEntries()); + + // modify entry a second time + entry.setField(StandardField.AUTHOR, "Test"); + + entry.setChanged(false); + + // write a second time + stringWriter = new StringWriter(); + databaseWriter = new BibtexDatabaseWriter(stringWriter, preferences, entryTypesManager); + databaseWriter.savePartOfDatabase(context, firstParse.getDatabase().getEntries()); + + assertEquals(bibtexEntry + OS.NEWLINE + + "@Comment{jabref-meta: databaseType:bibtex;}" + OS.NEWLINE, stringWriter.toString()); + } } diff --git a/src/test/java/org/jabref/model/entry/BibEntryTest.java b/src/test/java/org/jabref/model/entry/BibEntryTest.java index 02acbfc2863..e7038e12fd7 100644 --- a/src/test/java/org/jabref/model/entry/BibEntryTest.java +++ b/src/test/java/org/jabref/model/entry/BibEntryTest.java @@ -74,6 +74,17 @@ public void getFieldWorksWithBibFieldAsWell() throws Exception { assertEquals(Optional.of("value"), entry.getField(new BibField(StandardField.AUTHOR, FieldPriority.IMPORTANT).getField())); } + @Test + public void newBibEntryIsUnchanged() { + assertFalse(entry.hasChanged()); + } + + @Test + public void setFieldLeadsToAChangedEntry() throws Exception { + entry.setField(StandardField.AUTHOR, "value"); + assertTrue(entry.hasChanged()); + } + @Test public void setFieldWorksWithBibFieldAsWell() throws Exception { entry.setField(new BibField(StandardField.AUTHOR, FieldPriority.IMPORTANT).getField(), "value"); From 0d52fe0955070f24b2eae4b651ca0baf4167632e Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Thu, 21 Nov 2019 23:37:20 +0100 Subject: [PATCH 2/4] Fix missing explicit dependency --- build.gradle | 1 + src/test/java/module-info.test | 3 +++ 2 files changed, 4 insertions(+) diff --git a/build.gradle b/build.gradle index cf1f4819aeb..9835bd93be4 100644 --- a/build.gradle +++ b/build.gradle @@ -221,6 +221,7 @@ dependencies { testCompile "org.testfx:testfx-core:4.0.17-alpha-SNAPSHOT" testCompile "org.testfx:testfx-junit5:4.0.17-alpha-SNAPSHOT" testCompile "org.hamcrest:hamcrest-library:2.2" + testCompile group: 'org.assertj', name: 'assertj-core', version: '3.14.0' checkstyle 'com.puppycrawl.tools:checkstyle:8.26' xjc group: 'org.glassfish.jaxb', name: 'jaxb-xjc', version: '2.3.2' diff --git a/src/test/java/module-info.test b/src/test/java/module-info.test index a46242d0d34..dd003f89d9e 100644 --- a/src/test/java/module-info.test +++ b/src/test/java/module-info.test @@ -40,3 +40,6 @@ --add-reads org.jabref=io.github.classgraph + +--add-reads + org.jabref=org.assertj.core From f7f57273ea9ce0d69644c692c9702948b4aa827e Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Sun, 17 Nov 2019 20:28:17 +0100 Subject: [PATCH 3/4] WIP TODO: - Add test - saved flag local in the update monitor DONE: - After a synchronization is triggered: the referenceFile is updated with the current database - Synchronize change detection and file writing: should not happen in parallel - Use !Thread.currentThread().isInterrupted() instead of true in endless loop in thread --- .../org/jabref/gui/collab/ChangeScanner.java | 18 +++++++--- .../gui/collab/DatabaseChangeMonitor.java | 36 +++++++++++++++---- .../gui/util/DefaultFileUpdateMonitor.java | 11 ++++-- 3 files changed, 51 insertions(+), 14 deletions(-) diff --git a/src/main/java/org/jabref/gui/collab/ChangeScanner.java b/src/main/java/org/jabref/gui/collab/ChangeScanner.java index 021a941c87f..f509e98c3e9 100644 --- a/src/main/java/org/jabref/gui/collab/ChangeScanner.java +++ b/src/main/java/org/jabref/gui/collab/ChangeScanner.java @@ -1,7 +1,12 @@ package org.jabref.gui.collab; +import java.io.IOException; +import java.nio.file.CopyOption; +import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.StandardCopyOption; import java.util.ArrayList; +import java.util.Collections; import java.util.Comparator; import java.util.List; import java.util.Optional; @@ -18,11 +23,15 @@ import org.jabref.model.entry.BibEntry; import org.jabref.model.entry.BibtexString; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + public class ChangeScanner { + private static final Logger LOGGER = LoggerFactory.getLogger(ChangeScanner.class); + private final Path referenceFile; private final BibDatabaseContext database; - private final List changes = new ArrayList<>(); private BibDatabaseContext referenceDatabase; public ChangeScanner(BibDatabaseContext database, Path referenceFile) { @@ -41,7 +50,7 @@ private static BibEntry bestFit(BibEntry targetEntry, List entries) { } public List scanForChanges() { - database.getDatabasePath().ifPresent(diskdb -> { + return database.getDatabasePath().map(diskdb -> { // Parse the temporary file. ImportFormatPreferences importFormatPreferences = Globals.prefs.getImportFormatPreferences(); ParserResult result = OpenDatabase.loadDatabase(referenceFile.toAbsolutePath().toString(), importFormatPreferences, Globals.getFileUpdateMonitor()); @@ -52,6 +61,7 @@ public List scanForChanges() { BibDatabaseContext databaseOnDisk = result.getDatabaseContext(); // Start looking at changes. + List changes = new ArrayList(); BibDatabaseDiff differences = BibDatabaseDiff.compare(referenceDatabase, databaseOnDisk); differences.getMetaDataDifferences().ifPresent(diff -> { changes.add(new MetaDataChangeViewModel(diff)); @@ -60,8 +70,8 @@ public List scanForChanges() { differences.getPreambleDifferences().ifPresent(diff -> changes.add(new PreambleChangeViewModel(diff))); differences.getBibStringDifferences().forEach(diff -> changes.add(createBibStringDiff(diff))); differences.getEntryDifferences().forEach(diff -> changes.add(createBibEntryDiff(diff))); - }); - return changes; + return changes; + }).orElse(Collections.emptyList()); } private DatabaseChangeViewModel createBibStringDiff(BibStringDiff diff) { diff --git a/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java b/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java index 2bf20a9b69d..2d18a3a65ac 100644 --- a/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java +++ b/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java @@ -3,6 +3,7 @@ import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.StandardCopyOption; import java.util.ArrayList; import java.util.List; @@ -25,6 +26,13 @@ public class DatabaseChangeMonitor implements FileUpdateListener { private Path referenceFile; private TaskExecutor taskExecutor; + /** + * An update monitor for a file-based library (.bib file) + * + * @param database The BibTeX database + * @param fileMonitor The update monitor where to register to get notified about a change + * @param taskExecutor The scan for changes and notification is run with that task executor + */ public DatabaseChangeMonitor(BibDatabaseContext database, FileUpdateMonitor fileMonitor, TaskExecutor taskExecutor) { this.database = database; this.fileMonitor = fileMonitor; @@ -47,13 +55,27 @@ public DatabaseChangeMonitor(BibDatabaseContext database, FileUpdateMonitor file public void fileUpdated() { // File on disk has changed, thus look for notable changes and notify listeners in case there are such changes ChangeScanner scanner = new ChangeScanner(database, referenceFile); - BackgroundTask.wrap(scanner::scanForChanges) - .onSuccess(changes -> { - if (!changes.isEmpty()) { - listeners.forEach(listener -> listener.databaseChanged(changes)); - } - }) - .executeWith(taskExecutor); + BackgroundTask.wrap(() -> { + List changes; + // no two threads may check for changes + synchronized (referenceFile) { + changes = scanner.scanForChanges(); + if (!changes.isEmpty()) { + this.database.getDatabasePath().ifPresent(currentFile -> { + try { + Files.copy(currentFile, referenceFile, StandardCopyOption.REPLACE_EXISTING); + } catch (IOException e) { + LOGGER.error("Could not copy current database to reference file", e); + } + }); + } + } + return changes; + }).onSuccess(changes -> { + if (!changes.isEmpty()) { + listeners.forEach(listener -> listener.databaseChanged(changes)); + } + }).executeWith(taskExecutor); } public void addListener(DatabaseChangeListener listener) { diff --git a/src/main/java/org/jabref/gui/util/DefaultFileUpdateMonitor.java b/src/main/java/org/jabref/gui/util/DefaultFileUpdateMonitor.java index f5600df45c5..1456f64373f 100644 --- a/src/main/java/org/jabref/gui/util/DefaultFileUpdateMonitor.java +++ b/src/main/java/org/jabref/gui/util/DefaultFileUpdateMonitor.java @@ -14,6 +14,7 @@ import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.Multimap; +import com.google.common.collect.Multimaps; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -27,14 +28,15 @@ public class DefaultFileUpdateMonitor implements Runnable, FileUpdateMonitor { private static final Logger LOGGER = LoggerFactory.getLogger(DefaultFileUpdateMonitor.class); - private final Multimap listeners = ArrayListMultimap.create(20, 4); + // we are defensive here - ensure that adding and removal to the listeners happens in a serial way + private final Multimap listeners = Multimaps.synchronizedListMultimap(ArrayListMultimap.create(20, 4)); private WatchService watcher; @Override public void run() { try (WatchService watcher = FileSystems.getDefault().newWatchService()) { this.watcher = watcher; - while (true) { + while (!Thread.currentThread().isInterrupted()) { WatchKey key; try { key = watcher.take(); @@ -47,6 +49,7 @@ public void run() { if (kind == StandardWatchEventKinds.OVERFLOW) { Thread.yield(); + LOGGER.warn("Overflow at file watching"); continue; } else if (kind == StandardWatchEventKinds.ENTRY_MODIFY) { // We only handle "ENTRY_MODIFY" here, so the context is always a Path @@ -65,7 +68,9 @@ public void run() { } private void notifyAboutChange(Path path) { - listeners.get(path).forEach(FileUpdateListener::fileUpdated); + synchronized (listeners) { + listeners.get(path).forEach(FileUpdateListener::fileUpdated); + } } @Override From 097790e67331efe072ff65cc88efb8144086d8d7 Mon Sep 17 00:00:00 2001 From: Oliver Kopp Date: Thu, 21 Nov 2019 23:33:11 +0100 Subject: [PATCH 4/4] Begin to work on BibdatabaseDiff only --- src/main/java/org/jabref/gui/BasePanel.java | 1 - .../org/jabref/gui/collab/ChangeScanner.java | 68 +++++---------- .../gui/collab/DatabaseChangeListener.java | 4 +- .../gui/collab/DatabaseChangeMonitor.java | 85 ++++++++++--------- .../jabref/gui/collab/DatabaseChangePane.java | 46 ---------- 5 files changed, 69 insertions(+), 135 deletions(-) delete mode 100644 src/main/java/org/jabref/gui/collab/DatabaseChangePane.java diff --git a/src/main/java/org/jabref/gui/BasePanel.java b/src/main/java/org/jabref/gui/BasePanel.java index ec5a32d541b..bac50087bc6 100644 --- a/src/main/java/org/jabref/gui/BasePanel.java +++ b/src/main/java/org/jabref/gui/BasePanel.java @@ -34,7 +34,6 @@ import org.jabref.gui.bibtexkeypattern.GenerateBibtexKeyAction; import org.jabref.gui.cleanup.CleanupAction; import org.jabref.gui.collab.DatabaseChangeMonitor; -import org.jabref.gui.collab.DatabaseChangePane; import org.jabref.gui.desktop.JabRefDesktop; import org.jabref.gui.edit.CopyBibTeXKeyAndLinkAction; import org.jabref.gui.edit.ReplaceStringAction; diff --git a/src/main/java/org/jabref/gui/collab/ChangeScanner.java b/src/main/java/org/jabref/gui/collab/ChangeScanner.java index f509e98c3e9..38dd452fbff 100644 --- a/src/main/java/org/jabref/gui/collab/ChangeScanner.java +++ b/src/main/java/org/jabref/gui/collab/ChangeScanner.java @@ -1,24 +1,15 @@ package org.jabref.gui.collab; -import java.io.IOException; -import java.nio.file.CopyOption; -import java.nio.file.Files; -import java.nio.file.Path; -import java.nio.file.StandardCopyOption; import java.util.ArrayList; import java.util.Collections; import java.util.Comparator; import java.util.List; import java.util.Optional; -import org.jabref.Globals; import org.jabref.logic.bibtex.DuplicateCheck; import org.jabref.logic.bibtex.comparator.BibDatabaseDiff; import org.jabref.logic.bibtex.comparator.BibEntryDiff; import org.jabref.logic.bibtex.comparator.BibStringDiff; -import org.jabref.logic.importer.ImportFormatPreferences; -import org.jabref.logic.importer.OpenDatabase; -import org.jabref.logic.importer.ParserResult; import org.jabref.model.database.BibDatabaseContext; import org.jabref.model.entry.BibEntry; import org.jabref.model.entry.BibtexString; @@ -30,13 +21,25 @@ public class ChangeScanner { private static final Logger LOGGER = LoggerFactory.getLogger(ChangeScanner.class); - private final Path referenceFile; - private final BibDatabaseContext database; - private BibDatabaseContext referenceDatabase; + private final BibDatabaseContext oldDatabase; + private final BibDatabaseContext newDatabase; - public ChangeScanner(BibDatabaseContext database, Path referenceFile) { - this.database = database; - this.referenceFile = referenceFile; + public ChangeScanner(BibDatabaseContext oldDatabase, BibDatabaseContext newDatabase) { + this.oldDatabase = oldDatabase; + this.newDatabase = newDatabase; + } + + public List scanForChanges() { + List changes = new ArrayList(); + BibDatabaseDiff differences = BibDatabaseDiff.compare(oldDatabase, newDatabase); + differences.getMetaDataDifferences().ifPresent(diff -> { + changes.add(new MetaDataChangeViewModel(diff)); + diff.getGroupDifferences().ifPresent(groupDiff -> changes.add(new GroupChangeViewModel(groupDiff))); + }); + differences.getPreambleDifferences().ifPresent(diff -> changes.add(new PreambleChangeViewModel(diff))); + differences.getBibStringDifferences().forEach(diff -> changes.add(createBibStringDiff(diff))); + differences.getEntryDifferences().forEach(diff -> changes.add(createBibEntryDiff(diff))); + return changes; } /** @@ -49,47 +52,22 @@ private static BibEntry bestFit(BibEntry targetEntry, List entries) { .orElse(null); } - public List scanForChanges() { - return database.getDatabasePath().map(diskdb -> { - // Parse the temporary file. - ImportFormatPreferences importFormatPreferences = Globals.prefs.getImportFormatPreferences(); - ParserResult result = OpenDatabase.loadDatabase(referenceFile.toAbsolutePath().toString(), importFormatPreferences, Globals.getFileUpdateMonitor()); - referenceDatabase = result.getDatabaseContext(); - - // Parse the modified file. - result = OpenDatabase.loadDatabase(diskdb.toAbsolutePath().toString(), importFormatPreferences, Globals.getFileUpdateMonitor()); - BibDatabaseContext databaseOnDisk = result.getDatabaseContext(); - - // Start looking at changes. - List changes = new ArrayList(); - BibDatabaseDiff differences = BibDatabaseDiff.compare(referenceDatabase, databaseOnDisk); - differences.getMetaDataDifferences().ifPresent(diff -> { - changes.add(new MetaDataChangeViewModel(diff)); - diff.getGroupDifferences().ifPresent(groupDiff -> changes.add(new GroupChangeViewModel(groupDiff))); - }); - differences.getPreambleDifferences().ifPresent(diff -> changes.add(new PreambleChangeViewModel(diff))); - differences.getBibStringDifferences().forEach(diff -> changes.add(createBibStringDiff(diff))); - differences.getEntryDifferences().forEach(diff -> changes.add(createBibEntryDiff(diff))); - return changes; - }).orElse(Collections.emptyList()); - } - private DatabaseChangeViewModel createBibStringDiff(BibStringDiff diff) { if (diff.getOriginalString() == null) { return new StringAddChangeViewModel(diff.getNewString()); } if (diff.getNewString() == null) { - Optional current = database.getDatabase().getStringByName(diff.getOriginalString().getName()); + Optional current = oldDatabase.getDatabase().getStringByName(diff.getOriginalString().getName()); return new StringRemoveChangeViewModel(diff.getOriginalString(), current.orElse(null)); } if (diff.getOriginalString().getName().equals(diff.getNewString().getName())) { - Optional current = database.getDatabase().getStringByName(diff.getOriginalString().getName()); + Optional current = oldDatabase.getDatabase().getStringByName(diff.getOriginalString().getName()); return new StringChangeViewModel(current.orElse(null), diff.getOriginalString(), diff.getNewString().getContent()); } - Optional current = database.getDatabase().getStringByName(diff.getOriginalString().getName()); + Optional current = oldDatabase.getDatabase().getStringByName(diff.getOriginalString().getName()); return new StringNameChangeViewModel(current.orElse(null), diff.getOriginalString(), current.map(BibtexString::getName).orElse(""), diff.getNewString().getName()); } @@ -99,9 +77,9 @@ private DatabaseChangeViewModel createBibEntryDiff(BibEntryDiff diff) { } if (diff.getNewEntry() == null) { - return new EntryDeleteChangeViewModel(bestFit(diff.getOriginalEntry(), database.getEntries()), diff.getOriginalEntry()); + return new EntryDeleteChangeViewModel(bestFit(diff.getOriginalEntry(), oldDatabase.getEntries()), diff.getOriginalEntry()); } - return new EntryChangeViewModel(bestFit(diff.getOriginalEntry(), database.getEntries()), diff.getOriginalEntry(), diff.getNewEntry()); + return new EntryChangeViewModel(bestFit(diff.getOriginalEntry(), oldDatabase.getEntries()), diff.getOriginalEntry(), diff.getNewEntry()); } } diff --git a/src/main/java/org/jabref/gui/collab/DatabaseChangeListener.java b/src/main/java/org/jabref/gui/collab/DatabaseChangeListener.java index cfb9ac2e1ff..a863788b35b 100644 --- a/src/main/java/org/jabref/gui/collab/DatabaseChangeListener.java +++ b/src/main/java/org/jabref/gui/collab/DatabaseChangeListener.java @@ -1,7 +1,7 @@ package org.jabref.gui.collab; -import java.util.List; +import org.jabref.logic.bibtex.comparator.BibDatabaseDiff; public interface DatabaseChangeListener { - void databaseChanged(List changes); + void databaseChanged(BibDatabaseDiff bibDatabaseDiff); } diff --git a/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java b/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java index 2d18a3a65ac..7be6467bcb3 100644 --- a/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java +++ b/src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java @@ -7,74 +7,75 @@ import java.util.ArrayList; import java.util.List; +import org.jabref.Globals; import org.jabref.gui.util.BackgroundTask; import org.jabref.gui.util.TaskExecutor; +import org.jabref.logic.bibtex.comparator.BibDatabaseDiff; +import org.jabref.logic.importer.ImportFormatPreferences; +import org.jabref.logic.importer.OpenDatabase; +import org.jabref.logic.importer.ParserResult; import org.jabref.logic.util.io.FileUtil; import org.jabref.model.database.BibDatabaseContext; +import org.jabref.model.util.DummyFileUpdateMonitor; import org.jabref.model.util.FileUpdateListener; import org.jabref.model.util.FileUpdateMonitor; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +/** + * An update monitor for a file-based library (.bib file). Has to be re-instantiated if the location of the bib file + * changes. + */ public class DatabaseChangeMonitor implements FileUpdateListener { private static final Logger LOGGER = LoggerFactory.getLogger(DatabaseChangeMonitor.class); - private final BibDatabaseContext database; + private BibDatabaseContext referenceDatabase; + private Path databaseToBeMonitored; + private final FileUpdateMonitor fileMonitor; - private final List listeners; - private Path referenceFile; + private final List listeners = new ArrayList<>(); private TaskExecutor taskExecutor; /** - * An update monitor for a file-based library (.bib file) - * - * @param database The BibTeX database - * @param fileMonitor The update monitor where to register to get notified about a change - * @param taskExecutor The scan for changes and notification is run with that task executor + * @param databaseToBeMonitored The BibTeX database to be monitored + * @param fileMonitor The update monitor where to register to get notified about a change + * @param taskExecutor The scan for changes and notification is run with that task executor */ - public DatabaseChangeMonitor(BibDatabaseContext database, FileUpdateMonitor fileMonitor, TaskExecutor taskExecutor) { - this.database = database; + public DatabaseChangeMonitor(BibDatabaseContext databaseToBeMonitored, FileUpdateMonitor fileMonitor, TaskExecutor taskExecutor) { this.fileMonitor = fileMonitor; this.taskExecutor = taskExecutor; - this.listeners = new ArrayList<>(); - this.database.getDatabasePath().ifPresent(path -> { + databaseToBeMonitored.getDatabasePath().ifPresentOrElse(path -> { + this.databaseToBeMonitored = path; + loadReferenceDatabaseFromDatabaseToBeMonitored(); try { + // as last step, register this class fileMonitor.addListenerForFile(path, this); - referenceFile = Files.createTempFile("jabref", ".bib"); - referenceFile.toFile().deleteOnExit(); - setAsReference(path); } catch (IOException e) { LOGGER.error("Error while trying to monitor " + path, e); } + }, () -> { + throw new IllegalStateException("Path has to be present"); }); } @Override public void fileUpdated() { // File on disk has changed, thus look for notable changes and notify listeners in case there are such changes - ChangeScanner scanner = new ChangeScanner(database, referenceFile); + BackgroundTask.wrap(() -> { - List changes; + BibDatabaseDiff differences; // no two threads may check for changes - synchronized (referenceFile) { - changes = scanner.scanForChanges(); - if (!changes.isEmpty()) { - this.database.getDatabasePath().ifPresent(currentFile -> { - try { - Files.copy(currentFile, referenceFile, StandardCopyOption.REPLACE_EXISTING); - } catch (IOException e) { - LOGGER.error("Could not copy current database to reference file", e); - } - }); - } - } - return changes; - }).onSuccess(changes -> { - if (!changes.isEmpty()) { - listeners.forEach(listener -> listener.databaseChanged(changes)); + synchronized (referenceDatabase) { + ParserResult result = OpenDatabase.loadDatabase(this.databaseToBeMonitored.toAbsolutePath().toString(), Globals.prefs.getImportFormatPreferences(), new DummyFileUpdateMonitor()); + BibDatabaseContext databaseOnDisk = result.getDatabaseContext(); + differences = BibDatabaseDiff.compare(this.referenceDatabase, databaseOnDisk); + this.referenceDatabase = databaseOnDisk; } + return differences; + }).onSuccess(diff -> { + listeners.forEach(listener -> listener.databaseChanged(diff)); }).executeWith(taskExecutor); } @@ -83,18 +84,20 @@ public void addListener(DatabaseChangeListener listener) { } public void unregister() { - database.getDatabasePath().ifPresent(file -> fileMonitor.removeListener(file, this)); + fileMonitor.removeListener(this.databaseToBeMonitored, this); } - public void markExternalChangesAsResolved() { - markAsSaved(); + private void loadReferenceDatabaseFromDatabaseToBeMonitored() { + // there is no clone of BibDatabaseContext - we do it "the hard way" und just reload the database + ImportFormatPreferences importFormatPreferences = Globals.prefs.getImportFormatPreferences(); + ParserResult result = OpenDatabase.loadDatabase(this.databaseToBeMonitored.toAbsolutePath().toString(), importFormatPreferences, new DummyFileUpdateMonitor()); + this.referenceDatabase = result.getDatabaseContext(); } + /** + * Call this if the database to monitor was saved + */ public void markAsSaved() { - database.getDatabasePath().ifPresent(this::setAsReference); - } - - private void setAsReference(Path file) { - FileUtil.copyFile(file, referenceFile, true); + loadReferenceDatabaseFromDatabaseToBeMonitored(); } } diff --git a/src/main/java/org/jabref/gui/collab/DatabaseChangePane.java b/src/main/java/org/jabref/gui/collab/DatabaseChangePane.java deleted file mode 100644 index 17582a41ba9..00000000000 --- a/src/main/java/org/jabref/gui/collab/DatabaseChangePane.java +++ /dev/null @@ -1,46 +0,0 @@ -package org.jabref.gui.collab; - -import java.util.List; - -import javafx.scene.Node; - -import org.jabref.gui.icon.IconTheme; -import org.jabref.logic.l10n.Localization; -import org.jabref.model.database.BibDatabaseContext; - -import org.controlsfx.control.NotificationPane; -import org.controlsfx.control.action.Action; - -public class DatabaseChangePane extends NotificationPane { - - private final DatabaseChangeMonitor monitor; - private final BibDatabaseContext database; - - public DatabaseChangePane(Node parent, BibDatabaseContext database, DatabaseChangeMonitor monitor) { - super(parent); - this.database = database; - this.monitor = monitor; - - this.setGraphic(IconTheme.JabRefIcons.SAVE.getGraphicNode()); - this.setText(Localization.lang("The library has been modified by another program.")); - - monitor.addListener(this::onDatabaseChanged); - } - - private void onDatabaseChanged(List changes) { - this.getActions().setAll( - new Action(Localization.lang("Dismiss changes"), event -> { - monitor.markExternalChangesAsResolved(); - this.hide(); - }), - new Action(Localization.lang("Review changes"), event -> { - ChangeDisplayDialog changeDialog = new ChangeDisplayDialog(database, changes); - boolean changesHandled = changeDialog.showAndWait().orElse(false); - if (changesHandled) { - monitor.markExternalChangesAsResolved(); - this.hide(); - } - })); - this.show(); - } -}