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

Jump to bottom

Update database context in state manager after loading #9450

Merged
merged 2 commits into from
Dec 13, 2022
Merged

Update database context in state manager after loading #9450

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fb48258
Update database context in state manager after loading
Siedlerchr Dec 12, 2022
dee1880
rename variables and add conmment
Siedlerchr Dec 13, 2022
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
21 changes: 14 additions & 7 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 @@ -235,22 +235,29 @@ public void onDatabaseLoadingFailed(Exception ex) {
dialogService.showErrorDialogAndWait(title, content, ex);
}

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

this.bibDatabaseContext = Objects.requireNonNull(bibDatabaseContext);
// 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(bibDatabaseContext);
stateManager.setActiveDatabase(bibDatabaseContextFromParserResult);

bibDatabaseContext.getDatabase().registerListener(this);
bibDatabaseContext.getMetaData().registerListener(this);
// 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();
foundExistingBibDatabase.ifPresent(databaseContext -> stateManager.getOpenDatabases().remove(databaseContext));

this.bibDatabaseContext = Objects.requireNonNull(bibDatabaseContextFromParserResult);

stateManager.getOpenDatabases().add(bibDatabaseContextFromParserResult);

bibDatabaseContextFromParserResult.getDatabase().registerListener(this);
bibDatabaseContextFromParserResult.getMetaData().registerListener(this);
Comment on lines +255 to +256
Copy link
Member

@koppor koppor Dec 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This is duplicate of LibraryTab's constructor lines 138

Proposal:

Two LibraryTab constructors. One with BibDatabaseContext and one without. Add a new method "setBibDatabaseContext" including the initialization of listeners etc.

Following listeners seem to be missing are duplicated in the current feedData:

        this.getDatabase().registerListener(new SearchListener());
        this.getDatabase().registerListener(new IndexUpdateListener());
        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));

Copy link
Member

Choose a reason for hiding this comment

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

The new constructor should have BackgroundTask as parameter (instead of BibDatabaseContext). Maybe, an executor also has to be provided.

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 just moved the lines down... And as we replace the existing object the listeners need to be replaced as well

Copy link
Member

Choose a reason for hiding this comment

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

We really should eventually rework the groups panel and the OpenDatabase mechanism working with CompletableFutures.

Copy link
Member Author

Choose a reason for hiding this comment

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

Completable Futures aren't that different from our BackgroundTask


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

setupMainPanel();
setupAutoCompletion();
Expand Down