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 6488 #6537

Closed
wants to merge 11 commits into from
Closed

Fix 6488 #6537

wants to merge 11 commits into from

Conversation

WangAooa
Copy link
Contributor

@WangAooa WangAooa commented May 27, 2020

Fix #6488
In order to let duplicate information more clear, I hava make some changes.
When user imports some entries to a database:
(1) If these entries are duplicate to the entry in the database, it will shown the Bibtexkey of the duplicate entry in UI.
(2) If there are two entries are duplicate among the imported entries, it will shown the title of the duplicate entry.
image

  • 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.

@koppor
Copy link
Member

koppor commented May 27, 2020

Welcome to JabRef development 🎉 .

It is nice that the popup is having a better information. However, the popup should not appear!

The user started with an empty database. He imports a single entry. This entry is added. Then, JabRef complains that a duplicate exists. This is really wired.

Could you please invesitage JabRef's behavior: Why is there a duplicate check happening? I guess, it has to do something introduced with #6332, but I haven't checked.

@koppor koppor marked this pull request as draft May 27, 2020 21:08
@koppor koppor added the status: changes required Pull requests that are not yet complete label May 27, 2020
boolean continueImport = dialogService.showConfirmationDialogWithOptOutAndWait(Localization.lang("Duplicates found"),
Localization.lang("There are possible duplicates (marked with an icon) that haven't been resolved. Continue?"),
//Localization.lang("There are possible duplicates (marked with an icon) that haven't been resolved. Continue?"),
Localization.lang(duplicateInfo.toString()),
Copy link
Member

Choose a reason for hiding this comment

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

This is unfortunately not how localization works. The string has to be directly passed to Localization.lang() as plain string. Use %0 as place holder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my opinion, there are two cases that we need consider. First, the duplicate entry is in database. I think showing it's bibtexkey can help use find it quickly. Second, the duplicate entry is in the list entries that want to be imported. I think showing title is a better way because their bibtexkey is not shown in the import dialog. In a result, I have to concatenate string to meet different cases. So, do you have suggestion for this problem? Do I need revert this change?

if (preferences.shouldWarnAboutDuplicatesForImport()) {
BackgroundTask.wrap(() -> entriesToImport.stream()
.anyMatch(this::hasDuplicate)).onSuccess(duplicateFound -> {
if (duplicateFound) {
StringBuffer duplicateInfo = new StringBuffer("There are possible duplicates (") ;
if (this.datasetDuplicate.isPresent()){
duplicateInfo.append("duplicate to the entry in the database whose Bibtexkey is : ");
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
duplicateInfo.append("duplicate to the entry in the database whose Bibtexkey is : ");
duplicateInfo.append(Localization.lang("duplicate to the entry in the database whose Bibtexkey is: %0", this.datasetDuplicate.get().getField(InternalField.KEY_FIELD).get()));

duplicateInfo.append("duplicate to the entry in the database whose Bibtexkey is : ");
duplicateInfo.append(this.datasetDuplicate.get().getField(InternalField.KEY_FIELD).get());
}
else if (this.internalDuplicate.isPresent()){
Copy link
Member

Choose a reason for hiding this comment

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

Has to be on the same line as the }. -- Checkstyle will fail.

@WangAooa WangAooa marked this pull request as ready for review May 29, 2020 03:29
@koppor koppor self-assigned this Jul 7, 2020
@koppor
Copy link
Member

koppor commented Jul 7, 2020

Long time no thinking about the issue. Sorry for that. I'll try to dive into it the next time.

The underlying issue is more fundamental. See #6488 (comment).

@koppor
Copy link
Member

koppor commented Jul 27, 2020

Closing this issue due to inactivity 💤
Please reopen the PR if an update of the code was made.

@koppor koppor closed this Jul 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: changes required Pull requests that are not yet complete
Projects
None yet
2 participants