Skip to content

Commit

Permalink
Improve library loading UX (#7119)
Browse files Browse the repository at this point in the history 
* refactoring LibraryTab to be able to create an empty tab and feed it data when it's available

* applying the improvement when opening database using action

* checkstyle

* add exception parameter to error dialog

* Adding DataLoadingTask to LibraryTab

* Add getter for dataLoadingTask

* Fixing memory leak by canceling dataLoadingTask when tab is closed

* Relocate LibraryTab method trackOpenNewDatabase() to OpenDatabaseAction

* Refactor LibraryTab to handle dataLoadingTask callbacks

* Put performPostOpenActions() before feedData() for performance reasons

* Fix groups pane not updating bug

* Cleanup code

* Extracted JabRefPreferences, fixed imports

* Extracted JabRefPreferences, fixed imports, removed dead code, shortened method name

Co-authored-by: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com>
Co-authored-by: Carl Christian Snethlage <cc.snethlage@gmail.com>
  • Loading branch information
@HoussemNasri @calixtus
3 people authored Dec 6, 2020
1 parent 161e928 commit 33b9315
Show file tree
Hide file tree
Showing 3 changed files with 157 additions and 41 deletions.
1 change: 1 addition & 0 deletions src/main/java/org/jabref/gui/JabRefFrame.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1025,6 +1025,7 @@ public void addTab(LibraryTab libraryTab, boolean raisePanel) {

libraryTab.setOnCloseRequest(event -> {
closeTab(libraryTab);
libraryTab.getDataLoadingTask().cancel();
event.consume();
});

Expand Down
159 changes: 139 additions & 20 deletions src/main/java/org/jabref/gui/LibraryTab.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,21 @@
import javafx.collections.ListChangeListener;
import javafx.geometry.Orientation;
import javafx.scene.Node;
import javafx.scene.control.ProgressIndicator;
import javafx.scene.control.SplitPane;
import javafx.scene.control.Tab;
import javafx.scene.control.Tooltip;
import javafx.scene.layout.BorderPane;

import org.jabref.gui.autocompleter.AutoCompletePreferences;
import org.jabref.gui.autocompleter.PersonNameSuggestionProvider;
import org.jabref.gui.autocompleter.SuggestionProviders;
import org.jabref.gui.collab.DatabaseChangeMonitor;
import org.jabref.gui.collab.DatabaseChangePane;
import org.jabref.gui.dialogs.AutosaveUiManager;
import org.jabref.gui.entryeditor.EntryEditor;
import org.jabref.gui.externalfiletype.ExternalFileTypes;
import org.jabref.gui.importer.actions.OpenDatabaseAction;
import org.jabref.gui.maintable.MainTable;
import org.jabref.gui.maintable.MainTableDataModel;
import org.jabref.gui.specialfields.SpecialFieldDatabaseChangeListener;
Expand All @@ -33,8 +37,12 @@
import org.jabref.gui.undo.UndoableFieldChange;
import org.jabref.gui.undo.UndoableInsertEntries;
import org.jabref.gui.undo.UndoableRemoveEntries;
import org.jabref.gui.util.BackgroundTask;
import org.jabref.gui.util.DefaultTaskExecutor;
import org.jabref.logic.autosaveandbackup.AutosaveManager;
import org.jabref.logic.autosaveandbackup.BackupManager;
import org.jabref.logic.citationstyle.CitationStyleCache;
import org.jabref.logic.importer.ParserResult;
import org.jabref.logic.l10n.Localization;
import org.jabref.logic.pdf.FileAnnotationCache;
import org.jabref.logic.search.SearchQuery;
Expand Down Expand Up @@ -64,19 +72,19 @@ public class LibraryTab extends Tab {

private static final Logger LOGGER = LoggerFactory.getLogger(LibraryTab.class);

private final BibDatabaseContext bibDatabaseContext;
private final MainTableDataModel tableModel;
private BibDatabaseContext bibDatabaseContext;
private MainTableDataModel tableModel;

private final CitationStyleCache citationStyleCache;
private final FileAnnotationCache annotationCache;
private CitationStyleCache citationStyleCache;
private FileAnnotationCache annotationCache;

private final JabRefFrame frame;
private final CountingUndoManager undoManager;

private final SidePaneManager sidePaneManager;
private final ExternalFileTypes externalFileTypes;

private final EntryEditor entryEditor;
private EntryEditor entryEditor;
private final DialogService dialogService;
private final PreferencesService preferencesService;

Expand All @@ -93,10 +101,13 @@ public class LibraryTab extends Tab {

private BibEntry showing;
private SuggestionProviders suggestionProviders;
@SuppressWarnings({"FieldCanBeLocal"}) private Subscription dividerPositionSubscription;
@SuppressWarnings({"FieldCanBeLocal"})
private Subscription dividerPositionSubscription;
// the query the user searches when this BasePanel is active
private Optional<SearchQuery> currentSearchQuery = Optional.empty();
private Optional<DatabaseChangeMonitor> changeMonitor = Optional.empty();
// initializing it so we prevent NullPointerException
private BackgroundTask<ParserResult> dataLoadingTask = BackgroundTask.wrap(() -> null);

public LibraryTab(JabRefFrame frame,
PreferencesService preferencesService,
Expand Down Expand Up @@ -140,15 +151,105 @@ public LibraryTab(JabRefFrame frame,
});
}

public void setDataLoadingTask(BackgroundTask<ParserResult> dataLoadingTask) {
this.dataLoadingTask = dataLoadingTask;
}

public BackgroundTask<?> getDataLoadingTask() {
return dataLoadingTask;
}

/* 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();
pane.setCenter(progressIndicator);

return pane;
}

public void onDatabaseLoadingStarted() {
Node loadingLayout = createLoadingAnimationLayout();
getMainTable().placeholderProperty().setValue(loadingLayout);

frame.addTab(this, true);
}

public void onDatabaseLoadingSucceed(ParserResult result) {
BibDatabaseContext context = result.getDatabaseContext();
OpenDatabaseAction.performPostOpenActions(this, result);

feedData(context);
// a temporary workaround to update groups pane
Globals.stateManager.activeDatabaseProperty().bind(
EasyBind.map(frame.getTabbedPane().getSelectionModel().selectedItemProperty(),
selectedTab -> Optional.ofNullable(selectedTab)
.map(tab -> (LibraryTab) tab)
.map(LibraryTab::getBibDatabaseContext)));
}

public void onDatabaseLoadingFailed(Exception ex) {
String title = Localization.lang("Connection error");
String content = String.format("%s\n\n%s", ex.getMessage(), Localization.lang("A local copy will be opened."));

dialogService.showErrorDialogAndWait(title, content, ex);
}

public void feedData(BibDatabaseContext bibDatabaseContext) {
cleanUp();

this.bibDatabaseContext = Objects.requireNonNull(bibDatabaseContext);

bibDatabaseContext.getDatabase().registerListener(this);
bibDatabaseContext.getMetaData().registerListener(this);

this.tableModel = new MainTableDataModel(getBibDatabaseContext(), preferencesService, Globals.stateManager);
citationStyleCache = new CitationStyleCache(bibDatabaseContext);
annotationCache = new FileAnnotationCache(bibDatabaseContext, preferencesService.getFilePreferences());

setupMainPanel();
setupAutoCompletion();

this.getDatabase().registerListener(new SearchListener());
this.getDatabase().registerListener(new EntriesRemovedListener());

// ensure that at each addition of a new entry, the entry is added to the groups interface
this.bibDatabaseContext.getDatabase().registerListener(new GroupTreeListener());
// ensure that all entry changes mark the panel as changed
this.bibDatabaseContext.getDatabase().registerListener(this);

this.getDatabase().registerListener(new UpdateTimestampListener(preferencesService));

this.entryEditor = new EntryEditor(this, externalFileTypes);

Platform.runLater(() -> {
EasyBind.subscribe(changedProperty, this::updateTabTitle);
Globals.stateManager.getOpenDatabases().addListener((ListChangeListener<BibDatabaseContext>) c ->
updateTabTitle(changedProperty.getValue()));
});

if (isDatabaseReadyForAutoSave(bibDatabaseContext)) {
AutosaveManager autoSaver = AutosaveManager.start(bibDatabaseContext);
autoSaver.registerListener(new AutosaveUiManager(this));
}

BackupManager.start(this.bibDatabaseContext, Globals.entryTypesManager, Globals.prefs);
}

private boolean isDatabaseReadyForAutoSave(BibDatabaseContext context) {
return ((context.getLocation() == DatabaseLocation.SHARED) ||
((context.getLocation() == DatabaseLocation.LOCAL) && preferencesService.getShouldAutosave()))
&&
context.getDatabasePath().isPresent();
}

/**
* Sets the title of the tab
* modification-asterisk filename – path-fragment
*
* The modification-asterisk (*) is shown if the file was modified since last save
* (path-fragment is only shown if filename is not (globally) unique)
*
* Example:
* *jabref-authors.bib – testbib
* Sets the title of the tab modification-asterisk filename – path-fragment
* <p>
* The modification-asterisk (*) is shown if the file was modified since last save (path-fragment is only shown if
* filename is not (globally) unique)
* <p>
* Example: *jabref-authors.bib – testbib
*/
public void updateTabTitle(boolean isChanged) {
boolean isAutosaveEnabled = preferencesService.getShouldAutosave();
Expand Down Expand Up @@ -189,9 +290,9 @@ public void updateTabTitle(boolean isChanged) {
// Unique path fragment
List<String> uniquePathParts = FileUtil.uniquePathSubstrings(collectAllDatabasePaths());
Optional<String> uniquePathPart = uniquePathParts.stream()
.filter(part -> databasePath.toString().contains(part)
&& !part.equals(fileName) && part.contains(File.separator))
.findFirst();
.filter(part -> databasePath.toString().contains(part)
&& !part.equals(fileName) && part.contains(File.separator))
.findFirst();
if (uniquePathPart.isPresent()) {
String uniquePath = uniquePathPart.get();
// remove filename
Expand Down Expand Up @@ -330,9 +431,8 @@ public void insertEntry(final BibEntry bibEntry) {
}

/**
* This method is called from JabRefFrame when the user wants to create a new entry or entries.
* It is necessary when the user would expect the added entry or one of the added entries
* to be selected in the entry editor
* This method is called from JabRefFrame when the user wants to create a new entry or entries. It is necessary when
* the user would expect the added entry or one of the added entries to be selected in the entry editor
*
* @param entries The new entries.
*/
Expand Down Expand Up @@ -585,6 +685,8 @@ private void saveDividerLocation(Number position) {
*/
public void cleanUp() {
changeMonitor.ifPresent(DatabaseChangeMonitor::unregister);
AutosaveManager.shutdown(bibDatabaseContext);
BackupManager.shutdown(bibDatabaseContext);
}

/**
Expand Down Expand Up @@ -742,4 +844,21 @@ public void listen(EntriesRemovedEvent removedEntriesEvent) {
DefaultTaskExecutor.runInJavaFXThread(() -> frame.getGlobalSearchBar().performSearch());
}
}

public static class Factory {
public LibraryTab createLibraryTab(JabRefFrame frame, Path file, BackgroundTask<ParserResult> dataLoadingTask) {
BibDatabaseContext context = new BibDatabaseContext();
context.setDatabasePath(file);

LibraryTab newTab = new LibraryTab(frame, frame.prefs(), context, ExternalFileTypes.getInstance());
newTab.setDataLoadingTask(dataLoadingTask);

dataLoadingTask.onRunning(newTab::onDatabaseLoadingStarted)
.onSuccess(newTab::onDatabaseLoadingSucceed)
.onFailure(newTab::onDatabaseLoadingFailed)
.executeWith(Globals.TASK_EXECUTOR);

return newTab;
}
}
}
38 changes: 17 additions & 21 deletions src/main/java/org/jabref/gui/importer/actions/OpenDatabaseAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,10 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;

Expand All @@ -17,8 +19,6 @@
import org.jabref.gui.LibraryTab;
import org.jabref.gui.actions.SimpleCommand;
import org.jabref.gui.dialogs.BackupUIManager;
import org.jabref.gui.externalfiletype.ExternalFileTypes;
import org.jabref.gui.importer.ParserResultWarningDialog;
import org.jabref.gui.shared.SharedDatabaseUIManager;
import org.jabref.gui.util.BackgroundTask;
import org.jabref.gui.util.FileDialogConfiguration;
Expand Down Expand Up @@ -59,8 +59,8 @@ public OpenDatabaseAction(JabRefFrame frame) {
/**
* Go through the list of post open actions, and perform those that need to be performed.
*
* @param libraryTab The BasePanel where the database is shown.
* @param result The result of the BIB file parse operation.
* @param libraryTab The BasePanel where the database is shown.
* @param result The result of the BIB file parse operation.
*/
public static void performPostOpenActions(LibraryTab libraryTab, ParserResult result) {
for (GUIPostOpenAction action : OpenDatabaseAction.POST_OPEN_ACTIONS) {
Expand Down Expand Up @@ -159,17 +159,15 @@ public void openFiles(List<Path> filesToOpen, boolean raisePanel) {
*/
private void openTheFile(Path file, boolean raisePanel) {
Objects.requireNonNull(file);
if (Files.exists(file)) {

BackgroundTask.wrap(() -> loadDatabase(file))
.onSuccess(result -> {
LibraryTab libraryTab = addNewDatabase(result, file, raisePanel);
OpenDatabaseAction.performPostOpenActions(libraryTab, result);
})
.onFailure(ex -> dialogService.showErrorDialogAndWait(Localization.lang("Connection error"),
ex.getMessage() + "\n\n" + Localization.lang("A local copy will be opened.")))
.executeWith(Globals.TASK_EXECUTOR);
if (!Files.exists(file)) {
return;
}

BackgroundTask<ParserResult> backgroundTask = BackgroundTask.wrap(() -> loadDatabase(file));
LibraryTab.Factory libraryTabFactory = new LibraryTab.Factory();
LibraryTab newTab = libraryTabFactory.createLibraryTab(frame, file, backgroundTask);

backgroundTask.onFinished(() -> trackOpenNewDatabase(newTab));
}

private ParserResult loadDatabase(Path file) throws Exception {
Expand Down Expand Up @@ -201,13 +199,11 @@ private ParserResult loadDatabase(Path file) throws Exception {
return result;
}

private LibraryTab addNewDatabase(ParserResult result, final Path file, boolean raisePanel) {
if (result.hasWarnings()) {
ParserResultWarningDialog.showParserResultWarningDialog(result, frame);
}
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());

LibraryTab libraryTab = new LibraryTab(frame, Globals.prefs, result.getDatabaseContext(), ExternalFileTypes.getInstance());
frame.addTab(libraryTab, raisePanel);
return libraryTab;
Globals.getTelemetryClient().ifPresent(client -> client.trackEvent("OpenNewDatabase", properties, measurements));
}
}

0 comments on commit 33b9315

Please sign in to comment.