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

Improve library loading UX #7119

Merged
merged 15 commits into from
Dec 6, 2020
Merged

Improve library loading UX #7119

merged 15 commits into from
Dec 6, 2020

Conversation

HoussemNasri
Copy link
Member

@HoussemNasri HoussemNasri commented Nov 23, 2020

When JabRef opens a library, it opens a file, runs the database parser, and after everything is done, creates a new tab in the frame with the contents of the open database.

An improvement would be to first create a tab in the frame, display a loading animation, and after the parser has finished, display all entries (or display every entry as soon as it is parsed).

there is a bug that I couldn't fix, even after data is loaded the group pane will keep showing 0 entry, you need to navigate away from the tab and back to get the real number of entries

about applying the improvement on startup, I think having a splash screen will look better something like a blocking dialog that displays a ProgressIndicator

Closes #6417
im1
im2
im3
im4

  • Change in CHANGELOG.md described (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 documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

}

public void onDatabaseLoadingFailed(LibraryTab libraryTab, Exception ex) {
dialogService.showErrorDialogAndWait(Localization.lang("Connection error"),
Copy link
Member

Choose a reason for hiding this comment

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

It would be cook if you could pass the exception as paramter to the error dialog as well, it's just one addtional paramter

@Siedlerchr
Copy link
Member

Wohoo! Thanks for your work, I just tested it with a huge library, it's really cool and looks really nice!
Codewise lgtm as well

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Nov 23, 2020
@calixtus
Copy link
Member

calixtus commented Nov 23, 2020

I'll take a look into it the next days, im just a bit busy this week, I hope you don't mind.
About the bug: Maybe there is some var that should be a property. I'll check that.
Thank you for this huge work. I guess, it wasn't just a 'good first issue' but a big improvement!

@HoussemNasri
Copy link
Member Author

I'll take a look into it the next days, im just a bit busy this week, I hope you don't mind.

Don't worry I'm not in a hurry, take your time

Copy link
Member

@tobiasdiez tobiasdiez left a comment

Choose a reason for hiding this comment

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

Thanks also from my side. Some initial suggestions:

@@ -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

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.

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!

}

public BackgroundTask<?> getDataLoadingTask() {
if (dataLoadingTask == null) {
Copy link
Member

Choose a reason for hiding this comment

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

This condition/if is uncessary as you either return null or the dataLoadingTask.

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.

codewise lgtm so far

Copy link
Contributor

@DominikVoigt DominikVoigt left a comment

Choose a reason for hiding this comment

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

Great work!
I did not test the changes, but the code looks fine to me :)!

@calixtus
Copy link
Member

calixtus commented Dec 3, 2020

I will fix some line breaks and merge tonight. Sorry for delay.

@Siedlerchr Siedlerchr merged commit 33b9315 into JabRef:master Dec 6, 2020
@Siedlerchr
Copy link
Member

Thanks again for your contribution!

Siedlerchr added a commit that referenced this pull request Dec 6, 2020
* upstream/master:
  Improve library loading UX (#7119)
  remove jython (#7157)
  Add missing authors
  Remove obsolete hint
  Fixed issue in PreviewView for number textfield (#7158)
  Fix for issue 6959 (#7151)
  Revert "remove jython (#7155)" (#7156)
  remove jython (#7155)
  Fix remembering password for sql db (#7154)
  Update to libre office 7.0.3 (#7150)
  Add IdBasedSearchFetcher to jstor (#7145)
  Squashed 'src/main/resources/csl-styles/' changes from 55200d0..a20406d
  Bump antlr4-runtime from 4.8-1 to 4.9 (#7136)
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.

Improve responsiveness of library loading
5 participants