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

merged 2 commits into from
Dec 13, 2022

Conversation

Siedlerchr
Copy link
Member

@Siedlerchr Siedlerchr commented Dec 12, 2022 •
edited
Loading

Fixes #9440

Works so far. Just needs some code style polish I think
Changelog not necessary as this was working fine in the lats release

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

@koppor
Copy link
Member

koppor commented Dec 12, 2022 •
edited
Loading

The fix at f9b8f4d (#9451) does not fix #9440.

koppor
koppor reviewed Dec 12, 2022
View reviewed changes
src/main/java/org/jabref/gui/LibraryTab.java
Comment on lines +257 to +258
bibDatabaseContextFromParserResult.getDatabase().registerListener(this);
bibDatabaseContextFromParserResult.getMetaData().registerListener(this);
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

HoussemNasri
HoussemNasri reviewed Dec 12, 2022
View reviewed changes
Copy link
Member

@HoussemNasri HoussemNasri left a comment

Choose a reason for hiding this comment

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

Works perfectly! However, I still can't understand why the bug was triggered by f7648e8, and why it wasn't triggered earlier. I need to investigate more before I can approve so we don't replace one bug with another :^)

@Siedlerchr
Copy link
Member Author

Before the refinement the last open libraries were added after the loading in a method in Jabref frame and the library tab was added afterwards

The state manager open databases indirectly listens on the tabbed pane (Jabref Gui). And the list itself does not change because the number of tabs is the same.

Good catch from you on that bug! Really wonder as well why no one found this earlier

@Siedlerchr Siedlerchr mentioned this pull request Dec 13, 2022
Merged
6 tasks
HoussemNasri
HoussemNasri previously approved these changes Dec 13, 2022
View reviewed changes
calixtus
calixtus reviewed Dec 13, 2022
View reviewed changes
src/main/java/org/jabref/gui/LibraryTab.java Outdated

bibDatabaseContext.getDatabase().registerListener(this);
bibDatabaseContext.getMetaData().registerListener(this);
Optional<BibDatabaseContext> x = stateManager.getOpenDatabases().stream().filter(ctx -> ctx.equals(this.bibDatabaseContext)).findFirst();
Copy link
Member

Choose a reason for hiding this comment

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

Variable names

Copy link
Member Author

Choose a reason for hiding this comment

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

Reworked. Was just a temporary leftover from yesterdays late late evening

calixtus
calixtus requested changes Dec 13, 2022
View reviewed changes
Copy link
Member

@calixtus calixtus left a comment

Choose a reason for hiding this comment

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

Variable names should be worded correctly.

The two constructors are probably subject to a follow-up pr

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 13, 2022
@Siedlerchr Siedlerchr merged commit 513d3aa into main Dec 13, 2022
@Siedlerchr Siedlerchr deleted the updateDbContextInLIbraryTab branch December 13, 2022 20:54
Siedlerchr added a commit to MaryJml/jabref that referenced this pull request Dec 13, 2022
@Siedlerchr
Merge remote-tracking branch 'upstream/main' into import_into_library
ab54f7e 
* upstream/main: (37 commits)
  Update database context in state manager after loading (JabRef#9450)
  Bump classgraph from 4.8.151 to 4.8.152 (JabRef#9448)
  Bump appleboy/ssh-action from 0.1.5 to 0.1.6 (JabRef#9443)
  Bump Pendect/action-rsyncer from 1.1.0 to 2.0.0 (JabRef#9444)
  Bump jackson-dataformat-yaml from 2.14.0 to 2.14.1 (JabRef#9445)
  Bump unirest-java from 3.14.0 to 3.14.1 (JabRef#9447)
  Bump postgresql from 42.5.0 to 42.5.1 (JabRef#9446)
  New Crowdin updates (JabRef#9435)
  Return absolute path in case an absolute one is given (JabRef#9433)
  New Crowdin updates (JabRef#9434)
  Fix for issue: right click menu 6601 (JabRef#9271)
  Fix modernizer and refactor protected terms (JabRef#9427)
  Observable Preferences (OpenOffice) (JabRef#9422)
  Allow users to review backup changes before restoring them or merge them selectively (JabRef#9311)
  Bump slf4j-api from 2.0.4 to 2.0.5 (JabRef#9428)
  Bump archunit-junit5-api from 1.0.0 to 1.0.1 (JabRef#9429)
  Bump jackson-datatype-jsr310 from 2.14.0 to 2.14.1 (JabRef#9430)
  Bump lucene-highlighter from 9.4.1 to 9.4.2 (JabRef#9431)
  Fix weird checkbox styling (JabRef#9425)
  New translations JabRef_en.properties (Italian) (JabRef#9424)
  ...
@Siedlerchr Siedlerchr mentioned this pull request Dec 13, 2022
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@koppor koppor koppor left review comments

@calixtus calixtus calixtus requested changes

@HoussemNasri HoussemNasri HoussemNasri left review comments

Assignees
No one assigned
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Search results dialog show an empty table of search results
4 participants
@Siedlerchr @koppor @HoussemNasri @calixtus