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.

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

Fix GroupTree wrongly displayed #9451

Merged
merged 8 commits into from
Dec 15, 2022
Merged

Fix GroupTree wrongly displayed #9451

merged 8 commits into from
Dec 15, 2022

Conversation

koppor
Copy link
Member

@koppor koppor commented Dec 12, 2022

Fixes JabRef#637 and refines some other code.

  • 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 and others added 3 commits December 12, 2022 23:29
- fix: Globals.getFileUpdateMonitor() used for the resolved file (and not the dummy one)
- more comments
- Replace strange "LibraryTab.Factory" by a static method
- Remove unused method "trackOpenNewDatabase" in JabRefFrame
Co-authored-by: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com>
@koppor koppor requested a review from Siedlerchr December 12, 2022 23:08
@koppor koppor requested a review from HoussemNasri December 12, 2022 23:18
@Siedlerchr
Copy link
Member

Do you want to integrate the functionality of #9450 ? Otherwise we will have merge conflicts...

measurements.put("NumberOfEntries", (double) libraryTab.getBibDatabaseContext().getDatabase().getEntryCount());

Globals.getTelemetryClient().ifPresent(client -> client.trackEvent("OpenNewDatabase", properties, measurements));
Globals.getTelemetryClient().ifPresent(client -> client.trackEvent(
Copy link
Member

Choose a reason for hiding this comment

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

Even though we currently don't do any telemetry, I think this method needs to be added inside the LibraryTab to the BackgroundTask otherwise it's not excuted If I remember that correctly from my debug session

Copy link
Member Author

Choose a reason for hiding this comment

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

The user is still asked to provide his OK for telemetry. I only changed the code to a lambda function (which I found more readable).

The method is connected to the backgroudntask on line 181:

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

Thus, I do not see any need to change something here 😇

Copy link
Member

@Siedlerchr Siedlerchr left a comment

Choose a reason for hiding this comment

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

Eclipse was giving me warning: Unlikely argument type for equals(): LibraryTab seems to be unrelated to ReadOnlyObjectProperty

// it doesn't handle the case when a library is loaded asynchronously.
stateManager.setActiveDatabase(bibDatabaseContext);

if (this.getTabPane().getSelectionModel().selectedItemProperty().equals(this)) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (this.getTabPane().getSelectionModel().selectedItemProperty().equals(this)) {
if (this.getTabPane().getSelectionModel().selectedItemProperty().get().equals(this)) {

Copy link
Member Author

Choose a reason for hiding this comment

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

@calixtus prevented me from using frame.getCurrentLibraryTab().equals(this), because he wants to get rid of frame. Fixed.

Copy link
Member

Choose a reason for hiding this comment

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

Yes I did and I'm not even ashamed of it.

# Conflicts:
#	src/main/java/org/jabref/gui/LibraryTab.java
- add comment on backgroundTask execution
- remove unused variable
@koppor koppor added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Dec 14, 2022
Comment on lines +199 to +205
// 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
Copy link
Member

Choose a reason for hiding this comment

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

Well done for including the comments. I wrote that, but if I had to read it again in a year, I wouldn't know what is going on. I guess comments aren't useless after all.

@calixtus calixtus merged commit 08ab84e into main Dec 15, 2022
@calixtus calixtus deleted the fix-koppor-637 branch December 15, 2022 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JabRef displays wrong Group Tree
4 participants