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

Add confirmation dialog for empty entries in JabRef #8218

Merged
merged 24 commits into from
Nov 13, 2021

Conversation

ruoyu-qian
Copy link
Contributor

@ruoyu-qian ruoyu-qian commented Nov 8, 2021

Fixes #8096
Logic:

  1. When a user wants to close certain library/JabRef app, detect if there are any empty entries.
  2. If there is one or more empty entries, give a dialog box for user to choose if they want to delete or keep empty entries.
  3. If "Delete empty entries" is chosen, delete the empty entries; If "Keep empty entries" is chosen, keep the empty entries. If "Return to JabRef" is chosen, return to JabRef.
  • 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 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.

@ruoyu-qian ruoyu-qian marked this pull request as ready for review November 8, 2021 04:33
* @return true if the database has any empty entries; otherwise false
*/
public boolean hasEmptyEntries() {
for (BibEntry currentEntry: this.getEntries()) {
Copy link
Member

Choose a reason for hiding this comment

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

You can use streams here:
return this.getEntries().stream().anyMatch(entry->entry.getFields().isEmpty())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! I have updated the function.

if (response.isPresent() && response.get().equals(deleteEmptyEntries)) {
// The user wants to delete.
try {
for (BibEntry currentEntry : new ArrayList<BibEntry>(context.getEntries())) {
Copy link
Member

Choose a reason for hiding this comment

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

Why do you create a new array list here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I don't create a new array list, removing the empty entry from the original array list will lead to out of index error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are there any other approaches you would recommend? @Siedlerchr

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, now I see it. It's because you can't remove an entry from a collection over that you are still iterating. So your solutions is fine.
Please add one more test where you check and verify that the context.getEntries() is indeed entry
e.e.g assertEquals(Collections.emptyList(), context.getEntries())

Copy link
Contributor Author

@ruoyu-qian ruoyu-qian Nov 13, 2021

Choose a reason for hiding this comment

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

Yes, exactly.
In @test withEmptyEntry() I have added a delete command and a test to check the database is empty after deleting. Would this be sufficient, or would you like me to add a test to actually test the function confirmEmptyEntry() that I added in JabRefFrame.java? Thanks!

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.

Please add one more test to verify that the database is empty after deleting

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Nov 12, 2021
add one more test to verify that the database is empty after deleting
@koppor
Copy link
Member

koppor commented Nov 13, 2021

@ruoyu-qian Congratulations for your first GitHub contribution ever. Thank you for choosing JabRef! We are looking forward to more contributions!

@koppor koppor merged commit c593f35 into JabRef:main Nov 13, 2021
@ruoyu-qian
Copy link
Contributor Author

ruoyu-qian commented Nov 14, 2021

@ruoyu-qian Congratulations for your first GitHub contribution ever. Thank you for choosing JabRef! We are looking forward to more contributions!

@koppor Thank you! Thanks a lot to @Siedlerchr for reviewing my PR. I feel like JabRef is a very supportive community. It's been a great experience! I look forward to working with you all more. 😄

Siedlerchr added a commit that referenced this pull request Nov 19, 2021
* upstream/main: (181 commits)
  Add of ADRs 22 and 23 (#8256)
  [Bot] Update CSL styles (#8245)
  Replace styfle/cancel-workflow-action@0.9.1 by GitHub's "concurrency" feature (#8243)
  Bump gittools/actions from 0.9.10 to 0.9.11 (#8248)
  Bump commons-cli from 1.4 to 1.5.0 (#8250)
  Bump byte-buddy-parent from 1.12.0 to 1.12.1 (#8249)
  Bump antlr4 from 4.9.2 to 4.9.3 (#8251)
  Bump archunit-junit5-api from 0.21.0 to 0.22.0 (#8252)
  Fix search: NOT binds more than AND (#8241)
  New Crowdin updates (#8240)
  Make PdfGrobiImporterTest as FetcherTest
  Oobranch g : add actions (#7792)
  Fix mixed CRLF / CR (#8238)
  Fix "Library has changed externally" with CRLF markers (#8239)
  Fix for issue 8198, 8133 (#8229)
  Added more unit tests in AuthorTest (#8214)
  Add confirmation dialog for empty entries in JabRef (#8218)
  Fix entry editor column visibility (#8232)
  Use regexp to remove non-ASCII characters from DOI and inform user when data for valid DOI does not exist #8127 (#8228)
  Fix exception for search flags (#8237)
  ...
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.

Empty entry in JabRef leads to Error when compiling in LaTeX
3 participants