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

Implement a duplicate checker for Citation Relations entities #11186

Merged
merged 6 commits into from
Apr 12, 2024

Conversation

AbdAlRahmanGad
Copy link
Contributor

Fixes #10414
I implemented the duplicate checker by comparing entries names although I am not sure if it's the best way yet.

As for the second requirement.

Additionally, at button 4, the duplicate check should be called (which then warns about possible duplicates).

There is already a code that replaces the "add button" to a "link button" if the entry is a local one.
Screenshot 2024-04-11 at 11 00 15 AM
Preview

Untitled.mov

Mandatory checks

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

- implemented a duplicate checker by comparing entries names
- Marked existing entries "Light Green"
- Added a "localEntry" in the  "CitationRelationItem"
@Siedlerchr
Copy link
Member

much simpler, use the existing duplicate check https://github.com/JabRef/jabref/blob/main/src/main/java/org/jabref/logic/database/DuplicateCheck.java#L38

@koppor
Copy link
Member

koppor commented Apr 11, 2024

much simpler, use the existing duplicate check main/src/main/java/org/jabref/logic/database/DuplicateCheck.java#L38

It might be that the duplicate check needs to be improved. One can do that test-driven. -- Nevertheless, that check needs to be used, because other JabRef functionality also relies on that code.

@AbdAlRahmanGad
Copy link
Contributor Author

much simpler, use the existing duplicate check main/src/main/java/org/jabref/logic/database/DuplicateCheck.java#L38

It might be that the duplicate check needs to be improved. One can do that test-driven. -- Nevertheless, that check needs to be used, because other JabRef functionality also relies on that code.

I'd like to try to improve the checker. Should I start by writing test cases and detect the weaknesses?

@koppor
Copy link
Member

koppor commented Apr 11, 2024

It might be that the duplicate check needs to be improved. One can do that test-driven. -- Nevertheless, that check needs to be used, because other JabRef functionality also relies on that code.

I'd like to try to improve the checker. Should I start by writing test cases and detect the weaknesses?

Sure thing. I think, the main issue is #8885. - The hint at #9769 (comment) is worth to read. See the diff https://github.com/JabRef/jabref/pull/9769/files where tests are located.

When working in the code, leave some JavaDoc how to treat duplicates.

The current idea is IMHO that each difference gets some points. So that if, by accident, an entry has the same identifier, it could still be a non-duplicate somehow. However, I would try to change the algorithm to treat identifier equality as equal entries. -- And weight title the highest. One has really to play around with the thing.

Please do in a separate pull request - and just use the duplication check in this PR. (Reason: Keeping the pull requests small)

@AbdAlRahmanGad
Copy link
Contributor Author

Sure thing. I think, the main issue is #8885. - The hint at #9769 (comment) is worth to read. See the diff https://github.com/JabRef/jabref/pull/9769/files where tests are located.

When working in the code, leave some JavaDoc how to treat duplicates.

The current idea is IMHO that each difference gets some points. So that if, by accident, an entry has the same identifier, it could still be a non-duplicate somehow. However, I would try to change the algorithm to treat identifier equality as equal entries. -- And weight title the highest. One has really to play around with the thing.

Please do in a separate pull request - and just use the duplication check in this PR. (Reason: Keeping the pull requests small)

Ok, I will keep that in mind.
As for the current PR I used the duplication check in the latest commit 2b36941.

Copy link
Member

@koppor koppor left a comment

Choose a reason for hiding this comment

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

Good start, some minor comments.

@koppor
Copy link
Member

koppor commented Apr 12, 2024

@AbdAlRahmanGad Please check GitHubs CI report 30 minutes after submitting the PR. In the concrete case, you see that CHANGELOG.md has conflicts. Either resolve them using GitHub or your local IDE. IntelliJ has excellent support for resolving conflicts. With refined github you will also have excellent support in the browser.

@koppor
Copy link
Member

koppor commented Apr 12, 2024

@AbdAlRahmanGad Looks good. Do you think, something is left from your side? If not, you can mark it as "Ready for review"!

@AbdAlRahmanGad AbdAlRahmanGad marked this pull request as ready for review April 12, 2024 07:55
@koppor koppor added this pull request to the merge queue Apr 12, 2024
Merged via the queue into JabRef:main with commit 580fe7f Apr 12, 2024
20 checks passed
@AbdAlRahmanGad AbdAlRahmanGad deleted the Fix_for_10414 branch April 13, 2024 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Citation Relations should be aware of existing papers
3 participants