Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve things arround change detection #5770

Merged
merged 5 commits into from
Dec 20, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions src/main/java/org/jabref/gui/BasePanel.java
Original file line number Diff line number Diff line change
Expand Up @@ -991,10 +991,6 @@ public BibDatabaseContext getBibDatabaseContext() {
return this.bibDatabaseContext;
}

public void markExternalChangesAsResolved() {
changeMonitor.ifPresent(DatabaseChangeMonitor::markExternalChangesAsResolved);
}

public SidePaneManager getSidePaneManager() {
return sidePaneManager;
}
Expand Down Expand Up @@ -1059,10 +1055,6 @@ public void resetChangeMonitorAndChangePane() {
this.getChildren().setAll(changePane);
}

public void updateTimeStamp() {
changeMonitor.ifPresent(DatabaseChangeMonitor::markAsSaved);
}

public void copy() {
mainTable.copy();
}
Expand Down
5 changes: 3 additions & 2 deletions src/main/java/org/jabref/gui/collab/ChangeDisplayDialog.java
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,14 @@ public ChangeDisplayDialog(BibDatabaseContext database, List<DatabaseChangeViewM
Label rootInfo = new Label(Localization.lang("Select the tree nodes to view and accept or reject changes") + '.');
infoPanel.setCenter(rootInfo);

ButtonType dismissChanges = new ButtonType(Localization.lang("Dismiss changes"), ButtonBar.ButtonData.CANCEL_CLOSE);
getDialogPane().getButtonTypes().setAll(
new ButtonType(Localization.lang("Accept changes"), ButtonBar.ButtonData.APPLY),
ButtonType.CANCEL
dismissChanges
);

setResultConverter(button -> {
if (button == ButtonType.CANCEL) {
if (button == dismissChanges) {
return false;
} else {
// Perform all accepted changes
Expand Down
6 changes: 3 additions & 3 deletions src/main/java/org/jabref/gui/collab/ChangeScanner.java
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@
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.logic.importer.fileformat.BibtexImporter;
import org.jabref.model.database.BibDatabaseContext;
import org.jabref.model.util.DummyFileUpdateMonitor;

Expand All @@ -36,9 +36,9 @@ public List<DatabaseChangeViewModel> scanForChanges() {
List<DatabaseChangeViewModel> changes = new ArrayList<>();

// Parse the modified file
// Important: apply all post-load actions
ImportFormatPreferences importFormatPreferences = Globals.prefs.getImportFormatPreferences();
ParserResult result = new BibtexImporter(importFormatPreferences, new DummyFileUpdateMonitor())
.importDatabase(database.getDatabasePath().get(), importFormatPreferences.getEncoding());
ParserResult result = OpenDatabase.loadDatabase(database.getDatabasePath().get(), importFormatPreferences, new DummyFileUpdateMonitor());
BibDatabaseContext databaseOnDisk = result.getDatabaseContext();

// Start looking at changes.
Expand Down
18 changes: 0 additions & 18 deletions src/main/java/org/jabref/gui/collab/DatabaseChangeMonitor.java
Original file line number Diff line number Diff line change
@@ -1,14 +1,11 @@
package org.jabref.gui.collab;

import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.List;

import org.jabref.gui.util.BackgroundTask;
import org.jabref.gui.util.TaskExecutor;
import org.jabref.logic.util.io.FileUtil;
import org.jabref.model.database.BibDatabaseContext;
import org.jabref.model.util.FileUpdateListener;
import org.jabref.model.util.FileUpdateMonitor;
Expand All @@ -22,7 +19,6 @@ public class DatabaseChangeMonitor implements FileUpdateListener {
private final BibDatabaseContext database;
private final FileUpdateMonitor fileMonitor;
private final List<DatabaseChangeListener> listeners;
private Path referenceFile;
private TaskExecutor taskExecutor;

public DatabaseChangeMonitor(BibDatabaseContext database, FileUpdateMonitor fileMonitor, TaskExecutor taskExecutor) {
Expand All @@ -34,9 +30,6 @@ public DatabaseChangeMonitor(BibDatabaseContext database, FileUpdateMonitor file
this.database.getDatabasePath().ifPresent(path -> {
try {
fileMonitor.addListenerForFile(path, this);
referenceFile = Files.createTempFile("jabref", ".bib");
tobiasdiez marked this conversation as resolved.
Show resolved Hide resolved
referenceFile.toFile().deleteOnExit();
setAsReference(path);
} catch (IOException e) {
LOGGER.error("Error while trying to monitor " + path, e);
}
Expand Down Expand Up @@ -64,15 +57,4 @@ public void unregister() {
database.getDatabasePath().ifPresent(file -> fileMonitor.removeListener(file, this));
}

public void markExternalChangesAsResolved() {
markAsSaved();
}

public void markAsSaved() {
database.getDatabasePath().ifPresent(this::setAsReference);
}

private void setAsReference(Path file) {
FileUtil.copyFile(file, referenceFile, true);
}
}
9 changes: 3 additions & 6 deletions src/main/java/org/jabref/gui/collab/DatabaseChangePane.java
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,13 @@ public DatabaseChangePane(Node parent, BibDatabaseContext database, DatabaseChan
private void onDatabaseChanged(List<DatabaseChangeViewModel> 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();
}
changeDialog.showAndWait();

this.hide();
}));
this.show();
}
Expand Down
18 changes: 11 additions & 7 deletions src/main/java/org/jabref/gui/collab/EntryAddChangeViewModel.java
Original file line number Diff line number Diff line change
Expand Up @@ -13,23 +13,27 @@

class EntryAddChangeViewModel extends DatabaseChangeViewModel {

private final BibEntry diskEntry;
private final BibEntry entry;

public EntryAddChangeViewModel(BibEntry diskEntry) {
super(Localization.lang("Added entry"));
this.diskEntry = diskEntry;
public EntryAddChangeViewModel(BibEntry entry) {
super();
this.name = entry.getCiteKeyOptional()
.map(key -> Localization.lang("Added entry") + ": '" + key + '\'')
.orElse(Localization.lang("Added entry"));
this.entry = entry;
}

@Override
public void makeChange(BibDatabaseContext database, NamedCompound undoEdit) {
database.getDatabase().insertEntry(diskEntry);
undoEdit.addEdit(new UndoableInsertEntry(database.getDatabase(), diskEntry));
database.getDatabase().insertEntry(entry);
undoEdit.addEdit(new UndoableInsertEntry(database.getDatabase(), entry));
}

@Override
public Node description() {
PreviewViewer previewViewer = new PreviewViewer(new BibDatabaseContext(), JabRefGUI.getMainFrame().getDialogService(), Globals.stateManager);
previewViewer.setEntry(diskEntry);
previewViewer.setLayout(Globals.prefs.getPreviewPreferences().getCurrentPreviewStyle());
previewViewer.setEntry(entry);
return previewViewer;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,11 @@ public Node description() {
container.getChildren().add(new Label(Localization.lang("Value cleared externally")));
}

container.getChildren().add(new Label(Localization.lang("Current value") + ": " + value));
if (StringUtil.isNotBlank(value)) {
container.getChildren().add(new Label(Localization.lang("Current value") + ": " + value));
} else {
container.getChildren().add(new Label(Localization.lang("Current value") + ": " + Localization.lang("empty")));
}

return container;
}
Expand Down
2 changes: 0 additions & 2 deletions src/main/java/org/jabref/gui/exporter/SaveDatabaseAction.java
Original file line number Diff line number Diff line change
Expand Up @@ -135,14 +135,12 @@ private boolean doSave() {
SavePreferences.DatabaseSaveType.ALL);

if (success) {
panel.updateTimeStamp();
panel.getUndoManager().markUnchanged();
// (Only) after a successful save the following
// statement marks that the base is unchanged
// since last save:
panel.setNonUndoableChange(false);
panel.setBaseChanged(false);
panel.markExternalChangesAsResolved();

// Reset title of tab
frame.setTabTitle(panel, panel.getTabTitle(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ private String openIt(Path file, boolean importEntries, boolean importStrings, b
boolean importSelectorWords) throws IOException, KeyCollisionException {
Globals.prefs.put(JabRefPreferences.WORKING_DIRECTORY, file.getParent().toString());
// Should this be done _after_ we know it was successfully opened?
ParserResult parserResult = OpenDatabase.loadDatabase(file.toFile(),
ParserResult parserResult = OpenDatabase.loadDatabase(file,
Globals.prefs.getImportFormatPreferences(), Globals.getFileUpdateMonitor());
AppendDatabaseAction.mergeFromBibtex(panel, parserResult, importEntries, importStrings, importGroups,
importSelectorWords);
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/jabref/gui/preview/PreviewViewer.java
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ public void setEntry(BibEntry newEntry) {
}

private void update() {
if (!entry.isPresent() || layout == null) {
if (entry.isEmpty() || layout == null) {
// Nothing to do
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ public UnknownFormatImport importUnknownFormat(Path filePath, FileUpdateMonitor

// First, see if it is a BibTeX file:
try {
ParserResult parserResult = OpenDatabase.loadDatabase(filePath.toFile(), importFormatPreferences, fileMonitor);
ParserResult parserResult = OpenDatabase.loadDatabase(filePath, importFormatPreferences, fileMonitor);
if (parserResult.getDatabase().hasEntries() || !parserResult.getDatabase().hasNoStrings()) {
parserResult.setFile(filePath.toFile());
return new UnknownFormatImport(ImportFormatReader.BIBTEX_FORMAT, parserResult);
Expand Down
29 changes: 13 additions & 16 deletions src/main/java/org/jabref/logic/importer/OpenDatabase.java
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
package org.jabref.logic.importer;

import java.io.File;
import java.io.IOException;
import java.nio.file.Files;
tobiasdiez marked this conversation as resolved.
Show resolved Hide resolved
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.Arrays;
import java.util.List;

Expand Down Expand Up @@ -29,31 +31,26 @@ private OpenDatabase() {
*
* @param name Name of the BIB-file to open
* @return ParserResult which never is null
* @deprecated use {@link #loadDatabase(Path, ImportFormatPreferences, FileUpdateMonitor)} instead
*/
@Deprecated
public static ParserResult loadDatabase(String name, ImportFormatPreferences importFormatPreferences, FileUpdateMonitor fileMonitor) {
File file = new File(name);
LOGGER.info("Opening: " + name);
LOGGER.debug("Opening: " + name);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't "Opening {}", name work? - I think, this is the style one does in Java, because otherwise the debug string is always calculated even if the statement is not set.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't that micro optimization?

Path file = Paths.get(name);

if (!file.exists()) {
if (!Files.exists(file)) {
ParserResult pr = ParserResult.fromErrorMessage(Localization.lang("File not found"));
pr.setFile(file);
pr.setFile(file.toFile());

LOGGER.error(Localization.lang("Error") + ": " + Localization.lang("File not found"));
return pr;
}

try {
ParserResult pr = OpenDatabase.loadDatabase(file, importFormatPreferences, fileMonitor);
pr.setFile(file);
if (pr.hasWarnings()) {
for (String aWarn : pr.warnings()) {
LOGGER.warn(aWarn);
}
}
return pr;
return OpenDatabase.loadDatabase(file, importFormatPreferences, fileMonitor);
} catch (IOException ex) {
ParserResult pr = ParserResult.fromError(ex);
pr.setFile(file);
pr.setFile(file.toFile());
LOGGER.error("Problem opening .bib-file", ex);
return pr;
}
Expand All @@ -62,9 +59,9 @@ public static ParserResult loadDatabase(String name, ImportFormatPreferences imp
/**
* Opens a new database.
*/
public static ParserResult loadDatabase(File fileToOpen, ImportFormatPreferences importFormatPreferences, FileUpdateMonitor fileMonitor)
public static ParserResult loadDatabase(Path fileToOpen, ImportFormatPreferences importFormatPreferences, FileUpdateMonitor fileMonitor)
throws IOException {
ParserResult result = new BibtexImporter(importFormatPreferences, fileMonitor).importDatabase(fileToOpen.toPath(),
ParserResult result = new BibtexImporter(importFormatPreferences, fileMonitor).importDatabase(fileToOpen,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you change it from path to file?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This question is asked, because java.nio.Path should be preferred over java.io.File. We are aware in the code that this causes some conversion issues, however, I think, we should keep to move forward here. (Is this article still the best google result? --> https://jaxenter.de/java-nio-file-zeitgemases-arbeiten-mit-dateien-2581)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually converted it to a Path, the method now accepts a Path variable and not a File

importFormatPreferences.getEncoding());

if (importFormatPreferences.isKeywordSyncEnabled()) {
Expand Down
2 changes: 1 addition & 1 deletion src/main/resources/l10n/JabRef_en.properties
Original file line number Diff line number Diff line change
Expand Up @@ -2094,4 +2094,4 @@ Reset\ All=Reset All
Column\ type\ %0\ is\ unknown.=Column type %0 is unknown.
Linked\ identifiers=Linked identifiers
Special\ field\ type\ %0\ is\ unknown.\ Using\ normal\ column\ type.=Special field type %0 is unknown. Using normal column type.

empty=empty
Loading