Fix when fetching from DOI: Dialog selects field data based on validity #12826

Menoa11 · 2025-03-25T22:06:03Z

This PR enhances the Merge Entries Dialog by improving the automatic selection of bibliographic fields when merging entries from a DOI with manual entries. Previously, the dialog always prioritized data from the mannual entry, even when the mannual entry contained clearly incorrect information. Now, when merging from a DOI, the system automatically selects the more reliable fields for:

Year: If the local year is out of a reasonable range (e.g., before 1800 or after 2100) or differs from the DOI year by more than 10 years it will choose the more recent year out of the two.

Entry Type: If the local entry type is "misc," it defaults to the type provided by the DOI.

Code Changes:

FetchAndMergeEntry.java: Calls autoSelectBetterFields() when merging from a DOI.
MergeEntriesDialog.java: Introduces autoSelectBetterFields() method.
ThreeWayMergeView.java: Iterates through all fields and invokes autoSelectBetterValue().
FieldRowView.java: Implements autoSelectBetterValue() to apply selection rules for "year" and "entry type."

How It Works:

If merging from a DOI, the dialog automatically evaluates the "year" and "entry type" fields.
If the local year is out of a reasonable range or differs by more than 10 years, the DOI-provided year is selected.
If the local entry type is "misc," the DOI-provided type is selected.

Testing:

Manually tested with multiple entries containing incorrect years and entry types. Verified that other fields remain unchanged unless manually adjusted.
Tests have also now been added to src/test/java/org/jabref/gui/mergeentries/FieldRowViewModelTest.java to verify the changes that were made.

Visuals of change effects:

Before change:

After change:

Mandatory checks

I own the copyright of the code submitted and I license it under the MIT license
Change in CHANGELOG.md described in a way that is understandable for the average user (if change is visible to the user)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (if change is visible to the user)
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.

src/main/java/org/jabref/gui/mergeentries/newmergedialog/FieldRowView.java

…ear. Added comment to clarify processing of work from year 1800 or before.

src/main/java/org/jabref/gui/mergeentries/newmergedialog/FieldRowView.java

Menoa11 · 2025-03-26T21:31:13Z

@ThiloteE please let us know if the changes are good.

Menoa11 · 2025-03-27T17:55:07Z

@koppor Please let us know what you think.

src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java

src/main/java/org/jabref/gui/mergeentries/newmergedialog/FieldRowView.java

koppor · 2025-03-28T07:04:00Z

Move logic to org.jabref.gui.mergeentries.newmergedialog.FieldRowViewModel and add tests to org.jabref.gui.mergeentries.FieldRowViewModelTest

Suggested change Co-authored-by: Oliver Kopp <kopp.dev@gmail.com>

Moved Logic from FieldRowView.java to FieldRowViewModel.java

src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java

src/main/java/org/jabref/gui/mergeentries/newmergedialog/ThreeWayMergeView.java

src/main/java/org/jabref/gui/mergeentries/newmergedialog/FieldRowViewModel.java

src/test/java/org/jabref/gui/mergeentries/FieldRowViewModelTest.java

xts2002 · 2025-03-30T10:24:38Z

@koppor Hi I have implemented the changes you requested, please let us know if you need any more changes.

Menoa11 · 2025-03-31T01:54:14Z

Thank you for all the feedback! I just want to mention that we have a deadline to merge the PR which is April 4, @koppor @ThiloteE so we would really appreciate an approval before then.

koppor · 2025-03-31T07:01:51Z

src/main/java/org/jabref/gui/mergeentries/newmergedialog/FieldRowViewModel.java

+     * If the local year is out of a reasonable range (e.g., before 1800 or 100 years after current year as determined by the System Clock) or differs from the DOI year by more than 10 years, it will choose the more recent year out of the two.
+     */
+    public void autoSelectBetterValue() {
+        String field_1 = getField().getDisplayName();


Bad variable name - please be consistent with others

koppor · 2025-03-31T07:02:33Z

src/main/java/org/jabref/gui/mergeentries/newmergedialog/FieldRowViewModel.java

+        if (field_1 == null) {
+            return;
+        }
+        field_1 = field_1.trim().toLowerCase();


What is this? When can dispalyname be null? Why toLowerCase? - we also have .getName - not sure what this code aims vor.

koppor · 2025-03-31T07:02:50Z

src/main/java/org/jabref/gui/mergeentries/newmergedialog/FieldRowViewModel.java

+
+        // Logic for auto selection based on field name
+        // Default is right value
+        if ("year".equals(field_1)) {


Please use standard fiels - as I dold you in the last review

koppor · 2025-03-31T07:03:23Z

@koppor Hi I have implemented the changes you requested, please let us know if you need any more changes.

No, you did not. I am pretty sure, I told you to use JabRefs fields instead of strings

koppor · 2025-03-31T07:06:12Z

Thank you for all the feedback! I just want to mention that we have a deadline to merge the PR which is April 4, @koppor @ThiloteE so we would really appreciate an approval before then.

This informaion is new to us. You needed 20 days to come up with a pull request.

You did address my previous review comments properly.

The good thing is that you reacted fast on my review replies.

Maybe, we can manage that your PR will be ready to be merged. Currently, I think, it will need two more rounds.

… selected entry type.

trag-bot · 2025-04-02T02:06:37Z

@trag-bot didn't find any issues in the code! ✅✨

xts2002 · 2025-04-02T02:09:23Z

@koppor Sorry about that. Now the method autoSelectBetterValue uses Field to check the type of the entry. Are there any more places that should be changed?

jabref-machine · 2025-04-02T02:11:08Z

You committed your code on the main brach of your fork. This is a bad practice. The right way is to branch out from main, work on your patch/feature in that new branch, and then get that branch merged via the pull request (see GitHub flow).
For this pull request, this is OK. For subsequent pull requests, please start with a different branch with a proper branch name. See CONTRIBUTING.md for more details.

koppor · 2025-04-02T08:17:54Z

@koppor Sorry about that. Now the method autoSelectBetterValue uses Field to check the type of the entry. Are there any more places that should be changed?

Yes.

Please take this as excersie to level-up your tooling and coding skills. To get better than competitors.

Please scroll through your diff https://github.com/JabRef/jabref/pull/12826/files

I think, you did not understand what I meant to use Field instead of String.

--

For the purpose of getting the task done, this is solved. But I cannot merge since it does not meet our expectations of code quality. -- It is even hard for AI to work with non-good code, thus we try to have good code.

I think, nearly all of your lines have to be changed.

koppor · 2025-04-02T08:51:23Z

@koppor Sorry about that. Now the method autoSelectBetterValue uses Field to check the type of the entry. Are there any more places that should be changed?

@xts2002 Please read my 5 day old comment: #12826 (comment). The logic is not yet moved to there. Now you use Strings. If the logic is there, you can use higher-level data structures.

koppor · 2025-04-02T08:52:33Z

More information on MVVM can be found at https://github.com/JabRef/jabref/wiki/Code---JavaFX

ellapalter and others added 5 commits March 24, 2025 12:58

ella's fix - see end of change implementation doc

36a2e42

added logic for if years differ by more than 10

b2018c5

Made consistency changes before PR

cdb51a8

Added update to the changelog.md file under the fixed category

c37c4f9

Fixed all the gradle checkstyle problems

9342eb8

ThiloteE reviewed Mar 25, 2025

View reviewed changes

src/main/java/org/jabref/gui/mergeentries/newmergedialog/FieldRowView.java Outdated Show resolved Hide resolved

Tim Xu added 8 commits March 25, 2025 18:13

Changed leftYear comparison from '>2100' to +100 years from current y…

75e28e7

…ear. Added comment to clarify processing of work from year 1800 or before.

Corrected Style

b22e1f9

Corrected Style

96e4d1e

Added JavaDoc documentation for two methods

7bb2e3f

Changed magic numbers and String to constants

56ab176

Changed magic numbers and String to constants

fa3dfd8

Changed magic numbers and String to constants

a0f7e9f

Style correction

ba16c49

trag-bot bot reviewed Mar 26, 2025

View reviewed changes

src/main/java/org/jabref/gui/mergeentries/newmergedialog/FieldRowView.java Outdated Show resolved Hide resolved

trag-bot bot reviewed Mar 26, 2025

View reviewed changes

src/main/java/org/jabref/gui/mergeentries/newmergedialog/FieldRowView.java Outdated Show resolved Hide resolved

koppor added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Mar 27, 2025

koppor requested changes Mar 28, 2025

View reviewed changes

koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Mar 28, 2025

koppor marked this pull request as draft March 28, 2025 07:04

xts2002 and others added 4 commits March 28, 2025 18:57

Update src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java

4aadea9

Suggested change Co-authored-by: Oliver Kopp <kopp.dev@gmail.com>

Moved Logic from FieldRowView.java to FieldRowViewModel.java

52cf5cb

Merge branch 'main' into Tim_test

4b515fb

Merge pull request #1 from Menoa11/Tim_test

c1b48bc

Moved Logic from FieldRowView.java to FieldRowViewModel.java