Fix GroupTree wrongly displayed (#9451)

* Refine BackupUIManager - fix: Globals.getFileUpdateMonitor() used for the resolved file (and not the dummy one) - more comments * Refactorings on LibraryTab, JabRefFrame, and OpenDatabaseAction - Replace strange "LibraryTab.Factory" by a static method - Remove unused method "trackOpenNewDatabase" in JabRefFrame * Fix grouptree updating on load Co-authored-by: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com> * Update CHANGELOG.md * Fix checkstyle * Refactor signature of createLibraryTab - add comment on backgroundTask execution - remove unused variable * Fix condition Co-authored-by: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com>
JabRef · Dec 15, 2022 · 08ab84e · 08ab84e
1 parent 444fa41
commit 08ab84e
Show file tree

Hide file tree

Showing 7 changed files with 87 additions and 67 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -93,6 +93,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve
 - We fixed an issue where editing entry's "date" field in library mode "biblatex" causes an uncaught exception. [#8747](https://github.com/JabRef/jabref/issues/8747)
 - We fixed an issue where journal abbreviations would not abbreviate journal titles with escaped ampersands (\\&). [#8948](https://github.com/JabRef/jabref/issues/8948)
 - We fixed an issue where font size preferences did not apply correctly to preference dialog window and the menu bar. [#8386](https://github.com/JabRef/jabref/issues/8386) and [#9279](https://github.com/JabRef/jabref/issues/9279)
+- We fixed an issue that JabRef displayed the wrong group tree after loading. [koppor#637](https://github.com/koppor/jabref/issues/637)
 - We fixed an issue when using an unsafe character in the citation key, the auto-linking feature fails to link files. [#9267](https://github.com/JabRef/jabref/issues/9267)
 
 ### Removed

diff --git a/src/main/java/org/jabref/gui/JabRefFrame.java b/src/main/java/org/jabref/gui/JabRefFrame.java
@@ -5,7 +5,6 @@
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
-import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
 import java.util.TimerTask;
@@ -1115,8 +1114,8 @@ public void addTab(LibraryTab libraryTab, boolean raisePanel) {
         tabbedPane.getTabs().add(libraryTab);
 
         libraryTab.setOnCloseRequest(event -> {
+            libraryTab.cancelLoading();
             closeTab(libraryTab);
-            libraryTab.getDataLoadingTask().cancel();
             event.consume();
         });
 
@@ -1129,17 +1128,14 @@ public void addTab(LibraryTab libraryTab, boolean raisePanel) {
         libraryTab.getUndoManager().registerListener(new UndoRedoEventManager());
     }
 
-    private void trackOpenNewDatabase(LibraryTab libraryTab) {
-        Globals.getTelemetryClient().ifPresent(client -> client.trackEvent(
-                "OpenNewDatabase",
-                Map.of(),
-                Map.of("NumberOfEntries", (double) libraryTab.getBibDatabaseContext().getDatabase().getEntryCount())));
-    }
-
+    /**
+     * Opens a new tab with existing data.
+     * Asynchronous loading is done at {@link #createLibraryTab(BackgroundTask, Path, PreferencesService, StateManager, JabRefFrame, ThemeManager)}.
+     */
     public LibraryTab addTab(BibDatabaseContext databaseContext, boolean raisePanel) {
         Objects.requireNonNull(databaseContext);
 
-        LibraryTab libraryTab = new LibraryTab(this, prefs, stateManager, themeManager, databaseContext, importFormatReader);
+        LibraryTab libraryTab = new LibraryTab(databaseContext, this, prefs, stateManager, themeManager);
         addTab(libraryTab, raisePanel);
         return libraryTab;
     }

diff --git a/src/main/java/org/jabref/gui/LibraryTab.java b/src/main/java/org/jabref/gui/LibraryTab.java
@@ -43,7 +43,6 @@
 import org.jabref.logic.autosaveandbackup.AutosaveManager;
 import org.jabref.logic.autosaveandbackup.BackupManager;
 import org.jabref.logic.citationstyle.CitationStyleCache;
-import org.jabref.logic.importer.ImportFormatReader;
 import org.jabref.logic.importer.ParserResult;
 import org.jabref.logic.importer.util.FileFieldParser;
 import org.jabref.logic.l10n.Localization;
@@ -115,26 +114,22 @@ public class LibraryTab extends Tab {
 
     private Optional<DatabaseChangeMonitor> changeMonitor = Optional.empty();
 
-    // initializing it so we prevent NullPointerException
-    private BackgroundTask<ParserResult> dataLoadingTask = BackgroundTask.wrap(() -> null);
+    private BackgroundTask<ParserResult> dataLoadingTask;
 
     private final IndexingTaskManager indexingTaskManager = new IndexingTaskManager(Globals.TASK_EXECUTOR);
-    private final ImportFormatReader importFormatReader;
 
-    public LibraryTab(JabRefFrame frame,
+    public LibraryTab(BibDatabaseContext bibDatabaseContext,
+                      JabRefFrame frame,
                       PreferencesService preferencesService,
                       StateManager stateManager,
-                      ThemeManager themeManager,
-                      BibDatabaseContext bibDatabaseContext,
-                      ImportFormatReader importFormatReader) {
+                      ThemeManager themeManager) {
         this.frame = Objects.requireNonNull(frame);
         this.bibDatabaseContext = Objects.requireNonNull(bibDatabaseContext);
         this.undoManager = frame.getUndoManager();
         this.dialogService = frame.getDialogService();
         this.preferencesService = Objects.requireNonNull(preferencesService);
         this.stateManager = Objects.requireNonNull(stateManager);
         this.themeManager = Objects.requireNonNull(themeManager);
-        this.importFormatReader = importFormatReader;
 
         bibDatabaseContext.getDatabase().registerListener(this);
         bibDatabaseContext.getMetaData().registerListener(this);
@@ -189,15 +184,19 @@ private static void addSharedDbInformation(StringBuilder text, BibDatabaseContex
         text.append("]");
     }
 
-    public BackgroundTask<?> getDataLoadingTask() {
-        return dataLoadingTask;
-    }
-
     public void setDataLoadingTask(BackgroundTask<ParserResult> dataLoadingTask) {
         this.dataLoadingTask = dataLoadingTask;
     }
 
-    /* The layout to display in the tab when it's loading*/
+    public void cancelLoading() {
+        if (dataLoadingTask != null) {
+            dataLoadingTask.cancel();
+        }
+    }
+
+    /**
+     * The layout to display in the tab when it's loading
+     */
     public Node createLoadingAnimationLayout() {
         ProgressIndicator progressIndicator = new ProgressIndicator(ProgressIndicator.INDETERMINATE_PROGRESS);
         BorderPane pane = new BorderPane();
@@ -209,7 +208,6 @@ public Node createLoadingAnimationLayout() {
     public void onDatabaseLoadingStarted() {
         Node loadingLayout = createLoadingAnimationLayout();
         getMainTable().placeholderProperty().setValue(loadingLayout);
-
         frame.addTab(this, true);
     }
 
@@ -238,11 +236,14 @@ public void onDatabaseLoadingFailed(Exception ex) {
     public void feedData(BibDatabaseContext bibDatabaseContextFromParserResult) {
         cleanUp();
 
-        // When you open an existing library, a library tab with a loading animation is added immediately.
-        // At that point, the library tab is given a temporary bibDatabaseContext with no entries.
-        // This line is necessary because, while there is already a binding that updates the active database when a new tab is added,
-        // it doesn't handle the case when a library is loaded asynchronously.
-        stateManager.setActiveDatabase(bibDatabaseContextFromParserResult);
+        if (this.getTabPane().getSelectionModel().selectedItemProperty().get().equals(this)) {
+            // If you open an existing library, a library tab with a loading animation is added immediately.
+            // At that point, the library tab is given a temporary bibDatabaseContext with no entries.
+            // This line is necessary because, while there is already a binding that updates the active database when a new tab is added,
+            // it doesn't handle the case when a library is loaded asynchronously.
+            // See org.jabref.gui.LibraryTab.createLibraryTab for the asynchronous loading.
+            stateManager.setActiveDatabase(bibDatabaseContextFromParserResult);
+        }
 
         // Remove existing dummy BibDatabaseContext and add correct BibDatabaseContext from ParserResult to trigger changes in the openDatabases list in the stateManager
         Optional<BibDatabaseContext> foundExistingBibDatabase = stateManager.getOpenDatabases().stream().filter(databaseContext -> databaseContext.equals(this.bibDatabaseContext)).findFirst();
@@ -826,21 +827,25 @@ public void resetChangedProperties() {
         this.changedProperty.setValue(false);
     }
 
-    public static class Factory {
-        public LibraryTab createLibraryTab(JabRefFrame frame, PreferencesService preferencesService, StateManager stateManager, ThemeManager themeManager, Path file, BackgroundTask<ParserResult> dataLoadingTask, ImportFormatReader importFormatReader) {
-            BibDatabaseContext context = new BibDatabaseContext();
-            context.setDatabasePath(file);
+    /**
+     * Creates a new library tab. Contents are loaded by the {@code dataLoadingTask}. Most of the other parameters are required by {@code resetChangeMonitor()}.
+     *
+     * @param dataLoadingTask The task to execute to load the data. It is executed using {@link Globals.TASK_EXECUTOR}.
+     * @param file the path to the file (loaded by the dataLoadingTask)
+     */
+    public static LibraryTab createLibraryTab(BackgroundTask<ParserResult> dataLoadingTask, Path file, PreferencesService preferencesService, StateManager stateManager, JabRefFrame frame, ThemeManager themeManager) {
+        BibDatabaseContext context = new BibDatabaseContext();
+        context.setDatabasePath(file);
 
-            LibraryTab newTab = new LibraryTab(frame, preferencesService, stateManager, themeManager, context, importFormatReader);
-            newTab.setDataLoadingTask(dataLoadingTask);
+        LibraryTab newTab = new LibraryTab(context, frame, preferencesService, stateManager, themeManager);
 
-            dataLoadingTask.onRunning(newTab::onDatabaseLoadingStarted)
-                           .onSuccess(newTab::onDatabaseLoadingSucceed)
-                           .onFailure(newTab::onDatabaseLoadingFailed)
-                           .executeWith(Globals.TASK_EXECUTOR);
+        newTab.setDataLoadingTask(dataLoadingTask);
+        dataLoadingTask.onRunning(newTab::onDatabaseLoadingStarted)
+                       .onSuccess(newTab::onDatabaseLoadingSucceed)
+                       .onFailure(newTab::onDatabaseLoadingFailed)
+                       .executeWith(Globals.TASK_EXECUTOR);
 
-            return newTab;
-        }
+        return newTab;
     }
 
     private class GroupTreeListener {

diff --git a/src/main/java/org/jabref/gui/collab/DatabaseChange.java b/src/main/java/org/jabref/gui/collab/DatabaseChange.java
@@ -49,7 +49,7 @@ public void setAccepted(boolean accepted) {
     }
 
     /**
-     * Convinience method for accepting changes
+     * Convenience method for accepting changes
      * */
     public void accept() {
         setAccepted(true);

diff --git a/src/main/java/org/jabref/gui/collab/DatabaseChangesResolverDialog.java b/src/main/java/org/jabref/gui/collab/DatabaseChangesResolverDialog.java
@@ -53,6 +53,13 @@ public class DatabaseChangesResolverDialog extends BaseDialog<Boolean> {
 
     @Inject private UndoManager undoManager;
 
+    /**
+     * A dialog going through given <code>changes</code>, which are diffs to the provided <code>database</code>.
+     * Each accepted change is written to the provided <code>database</code>.
+     *
+     * @param changes The list of changes
+     * @param database The database to apply the changes to
+     */
     public DatabaseChangesResolverDialog(List<DatabaseChange> changes, BibDatabaseContext database, DialogService dialogService, StateManager stateManager, ThemeManager themeManager, PreferencesService preferencesService, String dialogTitle) {
         this.changes = changes;
         this.databaseChangeDetailsViewFactory = new DatabaseChangeDetailsViewFactory(database, dialogService, stateManager, themeManager, preferencesService);

diff --git a/src/main/java/org/jabref/gui/dialogs/BackupUIManager.java b/src/main/java/org/jabref/gui/dialogs/BackupUIManager.java
@@ -2,13 +2,15 @@
 
 import java.io.IOException;
 import java.nio.file.Path;
+import java.util.List;
 import java.util.Optional;
 
 import javafx.scene.control.ButtonType;
 
 import org.jabref.gui.DialogService;
 import org.jabref.gui.Globals;
 import org.jabref.gui.backup.BackupResolverDialog;
+import org.jabref.gui.collab.DatabaseChange;
 import org.jabref.gui.collab.DatabaseChangeList;
 import org.jabref.gui.collab.DatabaseChangeResolverFactory;
 import org.jabref.gui.collab.DatabaseChangesResolverDialog;
@@ -54,22 +56,31 @@ private static Optional<ButtonType> showBackupResolverDialog(DialogService dialo
 
     private static Optional<ParserResult> showReviewBackupDialog(DialogService dialogService, Path originalPath, PreferencesService preferencesService) {
         try {
-            Path backupPath = BackupFileUtil.getPathOfLatestExisingBackupFile(originalPath, BackupFileType.BACKUP).orElseThrow();
             ImportFormatPreferences importFormatPreferences = Globals.prefs.getImportFormatPreferences();
-            ParserResult originalParserResult = OpenDatabase.loadDatabase(originalPath, importFormatPreferences, new DummyFileUpdateMonitor());
+
+            // The database of the originalParserResult will be modified
+            ParserResult originalParserResult = OpenDatabase.loadDatabase(originalPath, importFormatPreferences, Globals.getFileUpdateMonitor());
+            // This will be modified by using the `DatabaseChangesResolverDialog`.
             BibDatabaseContext originalDatabase = originalParserResult.getDatabaseContext();
+
+            Path backupPath = BackupFileUtil.getPathOfLatestExisingBackupFile(originalPath, BackupFileType.BACKUP).orElseThrow();
             BibDatabaseContext backupDatabase = OpenDatabase.loadDatabase(backupPath, importFormatPreferences, new DummyFileUpdateMonitor()).getDatabaseContext();
 
             DatabaseChangeResolverFactory changeResolverFactory = new DatabaseChangeResolverFactory(dialogService, originalDatabase, preferencesService);
+
             return DefaultTaskExecutor.runInJavaFXThread(() -> {
+                List<DatabaseChange> changes = DatabaseChangeList.compareAndGetChanges(originalDatabase, backupDatabase, changeResolverFactory);
                 DatabaseChangesResolverDialog reviewBackupDialog = new DatabaseChangesResolverDialog(
-                        DatabaseChangeList.compareAndGetChanges(originalDatabase, backupDatabase, changeResolverFactory),
+                        changes,
                         originalDatabase, dialogService, Globals.stateManager, Globals.getThemeManager(), Globals.prefs, "Review Backup"
                 );
                 var allChangesResolved = dialogService.showCustomDialogAndWait(reviewBackupDialog);
                 if (allChangesResolved.isEmpty() || !allChangesResolved.get()) {
+                    // In case not all changes are resolved, start from scratch
                     return showRestoreBackupDialog(dialogService, originalPath, preferencesService);
                 }
+
+                // This does NOT return the original ParserResult, but a modified version with all changes accepted or rejected
                 return Optional.of(originalParserResult);
             });
         } catch (IOException e) {

diff --git a/src/main/java/org/jabref/gui/importer/actions/OpenDatabaseAction.java b/src/main/java/org/jabref/gui/importer/actions/OpenDatabaseAction.java
@@ -5,7 +5,6 @@
 import java.nio.file.Path;
 import java.sql.SQLException;
 import java.util.ArrayList;
-import java.util.HashMap;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
@@ -178,9 +177,8 @@ private void openTheFile(Path file) {
         }
 
         BackgroundTask<ParserResult> backgroundTask = BackgroundTask.wrap(() -> loadDatabase(file));
-        LibraryTab.Factory libraryTabFactory = new LibraryTab.Factory();
-        LibraryTab newTab = libraryTabFactory.createLibraryTab(frame, preferencesService, stateManager, themeManager, file, backgroundTask, Globals.IMPORT_FORMAT_READER);
-
+        // The backgroundTask is executed within the method createLibraryTab
+        LibraryTab newTab = LibraryTab.createLibraryTab(backgroundTask, file, preferencesService, stateManager, frame, themeManager);
         backgroundTask.onFinished(() -> trackOpenNewDatabase(newTab));
     }
 
@@ -195,49 +193,51 @@ private ParserResult loadDatabase(Path file) throws Exception {
 
         preferencesService.getFilePreferences().setWorkingDirectory(fileToLoad.getParent());
 
-        ParserResult result = null;
+        ParserResult parserResult = null;
         if (BackupManager.backupFileDiffers(fileToLoad)) {
-            result = BackupUIManager.showRestoreBackupDialog(dialogService, fileToLoad, preferencesService).orElse(null);
+            // In case the backup differs, ask the user what to do.
+            // In case the user opted for restoring a backup, the content of the backup is contained in parserResult.
+            parserResult = BackupUIManager.showRestoreBackupDialog(dialogService, fileToLoad, preferencesService).orElse(null);
         }
 
         try {
-            if (result == null) {
-                result = OpenDatabase.loadDatabase(fileToLoad,
+            if (parserResult == null) {
+                // No backup was restored, do the "normal" loading
+                parserResult = OpenDatabase.loadDatabase(fileToLoad,
                         preferencesService.getImportFormatPreferences(),
                         Globals.getFileUpdateMonitor());
             }
 
-            if (result.hasWarnings()) {
+            if (parserResult.hasWarnings()) {
                 String content = Localization.lang("Please check your library file for wrong syntax.")
-                        + "\n\n" + result.getErrorMessage();
+                        + "\n\n" + parserResult.getErrorMessage();
                 DefaultTaskExecutor.runInJavaFXThread(() ->
                         dialogService.showWarningDialogAndWait(Localization.lang("Open library error"), content));
             }
         } catch (IOException e) {
-            result = ParserResult.fromError(e);
+            parserResult = ParserResult.fromError(e);
             LOGGER.error("Error opening file '{}'", fileToLoad, e);
         }
 
-        if (result.getDatabase().isShared()) {
+        if (parserResult.getDatabase().isShared()) {
             try {
-                new SharedDatabaseUIManager(frame, preferencesService).openSharedDatabaseFromParserResult(result);
+                new SharedDatabaseUIManager(frame, preferencesService).openSharedDatabaseFromParserResult(parserResult);
             } catch (SQLException | DatabaseNotSupportedException | InvalidDBMSConnectionPropertiesException |
                     NotASharedDatabaseException e) {
-                result.getDatabaseContext().clearDatabasePath(); // do not open the original file
-                result.getDatabase().clearSharedDatabaseID();
+                parserResult.getDatabaseContext().clearDatabasePath(); // do not open the original file
+                parserResult.getDatabase().clearSharedDatabaseID();
                 LOGGER.error("Connection error", e);
 
                 throw e;
             }
         }
-        return result;
+        return parserResult;
     }
 
     private void trackOpenNewDatabase(LibraryTab libraryTab) {
-        Map<String, String> properties = new HashMap<>();
-        Map<String, Double> measurements = new HashMap<>();
-        measurements.put("NumberOfEntries", (double) libraryTab.getBibDatabaseContext().getDatabase().getEntryCount());
-
-        Globals.getTelemetryClient().ifPresent(client -> client.trackEvent("OpenNewDatabase", properties, measurements));
+        Globals.getTelemetryClient().ifPresent(client -> client.trackEvent(
+                "OpenNewDatabase",
+                Map.of(),
+                Map.of("NumberOfEntries", (double) libraryTab.getBibDatabaseContext().getDatabase().getEntryCount())));
     }
 }