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.

Sign up for GitHub

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 library loading UX #7119

Merged
merged 15 commits into from
Dec 6, 2020
Merged

Improve library loading UX #7119

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
5ec4027
refactoring LibraryTab to be able to create an empty tab and feed it …
HoussemNasri Nov 23, 2020
e1af860
applying the improvement when opening database using action
HoussemNasri Nov 23, 2020
5f2d7ea
checkstyle
HoussemNasri Nov 23, 2020
a1ee474
add exception parameter to error dialog
HoussemNasri Nov 23, 2020
1ca8616
Adding DataLoadingTask to LibraryTab
HoussemNasri Nov 24, 2020
8c0fd83
Add getter for dataLoadingTask
HoussemNasri Nov 24, 2020
f43c2ff
Fixing memory leak by canceling dataLoadingTask when tab is closed
HoussemNasri Nov 24, 2020
ef088af
Relocate LibraryTab method trackOpenNewDatabase() to OpenDatabaseAction
HoussemNasri Nov 24, 2020
77061c8
Refactor LibraryTab to handle dataLoadingTask callbacks
HoussemNasri Nov 24, 2020
b7ca2d3
Put performPostOpenActions() before feedData() for performance reasons
HoussemNasri Nov 24, 2020
2e71e7a
Fix groups pane not updating bug
HoussemNasri Nov 24, 2020
5783b30
Cleanup code
HoussemNasri Nov 27, 2020
be3c2bf
Merge branch 'master' into improve-database-loading-ux
calixtus Dec 5, 2020
6c3149c
Extracted JabRefPreferences, fixed imports
calixtus Dec 5, 2020
8b32c09
Extracted JabRefPreferences, fixed imports, removed dead code, shorte…
calixtus Dec 5, 2020
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
119 changes: 97 additions & 22 deletions src/main/java/org/jabref/gui/LibraryTab.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Optional;

Expand All @@ -23,6 +25,7 @@
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.maintable.MainTable;
Expand All @@ -34,6 +37,8 @@
import org.jabref.gui.undo.UndoableInsertEntries;
import org.jabref.gui.undo.UndoableRemoveEntries;
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.l10n.Localization;
import org.jabref.logic.pdf.FileAnnotationCache;
Expand All @@ -52,6 +57,7 @@
import org.jabref.model.entry.event.EntryChangedEvent;
import org.jabref.model.entry.field.Field;
import org.jabref.model.entry.field.FieldFactory;
import org.jabref.preferences.JabRefPreferences;
import org.jabref.preferences.PreferencesService;

import com.google.common.eventbus.Subscribe;
Expand All @@ -64,19 +70,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,7 +99,8 @@ 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();
Expand Down Expand Up @@ -140,15 +147,81 @@ public LibraryTab(JabRefFrame frame,
});
}

public static LibraryTab createNewEmptyLibraryTab(JabRefFrame frame, Path file) {
BibDatabaseContext context = new BibDatabaseContext();
context.setDatabasePath(file);

return new LibraryTab(frame, frame.prefs(), context, ExternalFileTypes.getInstance());
}

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);

trackOpenNewDatabase(this);

}

private boolean isDatabaseReadyForAutoSave(BibDatabaseContext context) {
return ((context.getLocation() == DatabaseLocation.SHARED) ||
((context.getLocation() == DatabaseLocation.LOCAL) && Globals.prefs.getBoolean(JabRefPreferences.LOCAL_AUTO_SAVE)))
&&
context.getDatabasePath().isPresent();
}

private void trackOpenNewDatabase(LibraryTab libraryTab) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be in the opendatabaseaction class (as it's not really connected to the library tab)

Copy link
Member Author

Choose a reason for hiding this comment

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

and by this, you mean trackOpenNewDatabase(), right?

Copy link
Member

Choose a reason for hiding this comment

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

yes!

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));
}

/**
* Sets the title of the tab
* modification-asterisk filename – path-fragment
*
* 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
* *jabref-authors.bib – testbib
*/
public void updateTabTitle(boolean isChanged) {
boolean isAutosaveEnabled = preferencesService.getShouldAutosave();
Expand Down Expand Up @@ -189,9 +262,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 @@ -242,10 +315,10 @@ private static void addSharedDbInformation(StringBuilder text, BibDatabaseContex
private List<String> collectAllDatabasePaths() {
List<String> list = new ArrayList<>();
Globals.stateManager.getOpenDatabases().stream()
.map(BibDatabaseContext::getDatabasePath)
.forEachOrdered(pathOptional -> pathOptional.ifPresentOrElse(
path -> list.add(path.toAbsolutePath().toString()),
() -> list.add("")));
.map(BibDatabaseContext::getDatabasePath)
.forEachOrdered(pathOptional -> pathOptional.ifPresentOrElse(
path -> list.add(path.toAbsolutePath().toString()),
() -> list.add("")));
return list;
}

Expand Down Expand Up @@ -386,9 +459,9 @@ private void createMainTable() {

// Update entry editor and preview according to selected entries
mainTable.addSelectionListener(event -> mainTable.getSelectedEntries()
.stream()
.findFirst()
.ifPresent(entryEditor::setEntry));
.stream()
.findFirst()
.ifPresent(entryEditor::setEntry));
}

public void setupMainPanel() {
Expand All @@ -402,8 +475,8 @@ public void setupMainPanel() {
// Saves the divider position as soon as it changes
// We need to keep a reference to the subscription, otherwise the binding gets garbage collected
dividerPositionSubscription = EasyBind.valueAt(splitPane.getDividers(), 0)
.mapObservable(SplitPane.Divider::positionProperty)
.subscribeToValues(this::saveDividerLocation);
.mapObservable(SplitPane.Divider::positionProperty)
.subscribeToValues(this::saveDividerLocation);

// Add changePane in case a file is present - otherwise just add the splitPane to the panel
Optional<Path> file = bibDatabaseContext.getDatabasePath();
Expand Down Expand Up @@ -585,6 +658,8 @@ private void saveDividerLocation(Number position) {
*/
public void cleanUp() {
changeMonitor.ifPresent(DatabaseChangeMonitor::unregister);
AutosaveManager.shutdown(bibDatabaseContext);
BackupManager.shutdown(bibDatabaseContext);
}

/**
Expand Down
61 changes: 49 additions & 12 deletions src/main/java/org/jabref/gui/importer/actions/OpenDatabaseAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@
import java.util.Objects;
import java.util.Optional;

import javafx.scene.Node;
import javafx.scene.control.ProgressIndicator;
import javafx.scene.layout.BorderPane;

import org.jabref.gui.DialogService;
import org.jabref.gui.Globals;
import org.jabref.gui.JabRefFrame;
Expand All @@ -30,6 +34,7 @@
import org.jabref.logic.shared.exception.InvalidDBMSConnectionPropertiesException;
import org.jabref.logic.shared.exception.NotASharedDatabaseException;
import org.jabref.logic.util.StandardFileType;
import org.jabref.model.database.BibDatabaseContext;
import org.jabref.preferences.JabRefPreferences;

import org.slf4j.Logger;
Expand Down Expand Up @@ -59,8 +64,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 +164,18 @@ 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 libraryTab = LibraryTab.createNewEmptyLibraryTab(frame, file);
onDatabaseLoadingStarted(libraryTab, backgroundTask);

backgroundTask.onSuccess(result -> onDatabaseLoadingSucceed(libraryTab, result))
.onFailure(ex -> onDatabaseLoadingFailed(libraryTab, ex))
.executeWith(Globals.TASK_EXECUTOR);

}

private ParserResult loadDatabase(Path file) throws Exception {
Expand Down Expand Up @@ -210,4 +216,35 @@ private LibraryTab addNewDatabase(ParserResult result, final Path file, boolean
frame.addTab(libraryTab, raisePanel);
return libraryTab;
}

/* The layout to display in the tab when it's loading*/
public Node createLoadingLayout() {
Copy link
Member

Choose a reason for hiding this comment

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

I would let the LibraryTab handle the progress indicator. This could be done for example by setting the placeholder to the progress indicator. https://docs.oracle.com/javase/8/javafx/api/javafx/scene/control/TableView.html#placeholderProperty

Copy link
Member Author

Choose a reason for hiding this comment

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

There was a bug when I close a loading tab before it finishes, the tab next to it TableView would shrink but it seems using placeholder fixed it, Thank you

ProgressIndicator progressIndicator = new ProgressIndicator(ProgressIndicator.INDETERMINATE_PROGRESS);
BorderPane pane = new BorderPane();
pane.setCenter(progressIndicator);

return pane;
}

public void onDatabaseLoadingStarted(LibraryTab libraryTab, BackgroundTask<?> backgroundTask) {
Node loadingLayout = createLoadingLayout();
libraryTab.setContent(loadingLayout);
libraryTab.setOnCloseRequest(e -> backgroundTask.cancel());

frame.addTab(libraryTab, true);
}

public void onDatabaseLoadingSucceed(LibraryTab libraryTab, ParserResult result) {
BibDatabaseContext context = result.getDatabaseContext();
libraryTab.feedData(context);

OpenDatabaseAction.performPostOpenActions(libraryTab, result);
Copy link
Member

Choose a reason for hiding this comment

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

I would put this before the feedData call. Some of the post actions change a lot of entries, which would result in unnecessary updates of the ui.

}

public void onDatabaseLoadingFailed(LibraryTab libraryTab, 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);
}
}