Select the entry which has smaller dictonary order when merge #7708

SuXiChangZhen · 2021-05-06T14:21:27Z

Changed the selected entry when merge two entries:
if two entry's citation key are empty, the above one will be selected;
if only one entry has citation key, then it will be selected;
if two entries both have citation key, the one who has smaller dictionary order will be selected.
Fixes #7395

The screenshot shows that the "2021a" is selected although it is below "2021b".

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 created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

Siedlerchr · 2021-05-06T17:25:20Z

SEe Checkstyle ore reviewdog and please give your PR a better title

koppor

Thank you for working on this.

Please do not add business logic the the gui. business logic in our case is the comparision of BibEntries. We have the org.jabref.logic.bibtex.comparator.EntryComparator. I made code suggestions inside.

koppor · 2021-05-06T19:43:57Z

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

+        // Compare the citation key
+        BibEntry first = one;
+        BibEntry second = two;
+
+        Optional<String> keyOne = one.getCitationKey();
+        Optional<String> keyTwo = two.getCitationKey();
+        if (keyOne.isPresent() && keyTwo.isPresent() && keyOne.get().compareTo(keyTwo.get()) > 0) {
+            first = two;
+            second = one;
+        }
+        else {
+            if (keyTwo.isPresent()) {
+                first = two;
+                second = one;
+            }
+        }


There is org.jabref.logic.bibtex.comparator.EntryComparator:

EntryComparator(false, true, InternalField.KEY_FIELD)

Thus:

Suggested change

// Compare the citation key

BibEntry first = one;

BibEntry second = two;

Optional<String> keyOne = one.getCitationKey();

Optional<String> keyTwo = two.getCitationKey();

if (keyOne.isPresent() && keyTwo.isPresent() && keyOne.get().compareTo(keyTwo.get()) > 0) {

first = two;

second = one;

}

else {

if (keyTwo.isPresent()) {

first = two;

second = one;

}

}

BibEntry first;

BibEntry second;

EntryComparator entryComparator = new EntryComparator(false, true, InternalField.KEY_FIELD);

if (entryComparator.compare(one, two) <= 0) {

first = one;

second = two;

} else {

first = two;

second = one;

}

Since software engineering is an engineering prinicple and engineering includes testing, please add test cases. Add them to the class org.jabref.logic.bibtex.comparator.EntryComparatorTest

SuXiChangZhen · 2021-05-07T12:19:02Z

Thank you for your suggestions. I have used EntryComparator and added two test cases to test the behavior of EntryComparator.

k3KAW8Pnf7mkmdSMPHz27 · 2021-05-07T16:09:25Z

I like the idea of having a default order when merging, but isn't this a problem when one citation key is empty ("" not null) and the other one is not? Can that happen and is it worth worrying about?

SuXiChangZhen · 2021-05-07T16:26:13Z

Yes, I think the real problem may be how to define a valid citation key. For example, " " should not be a valid one

Siedlerchr · 2021-05-07T16:43:40Z

An empty citation key is totally valid. I would expect to have the empty one as "lower" on the left in the dialog
Minimum valid bibtex:

@Article{,
}

SuXiChangZhen · 2021-05-07T16:49:09Z

Yes, that make sense too. It seems some checks were not successful, so what should I do?

Siedlerchr · 2021-05-07T16:51:41Z

You should merge in the latest main from upstream jabref, then most tests should be working again.
Checkstyle + Unit tests must be green

koppor · 2021-05-07T19:12:44Z

I like the idea of having a default order when merging, but isn't this a problem when one citation key is empty ("" not null) and the other one is not? Can that happen and is it worth worrying about?

This should be handled by the EntryComparator, too. Test cases should cover this. @SuXiChangZhen Please add this test case to EntryComparatorTest.

koppor · 2021-05-07T19:16:40Z

Yes, I think the real problem may be how to define a valid citation key. For example, " " should not be a valid one

Since .bib is a text file where users can input (nearly) arbitrary things, we have to be relaxed in as many places as possible.

We have the org.jabref.logic.integrity.CitationKeyChecker which checks for the empty key.

k3KAW8Pnf7mkmdSMPHz27 · 2021-05-07T19:31:47Z

I am not that up to date with the merging and I might have sown some accidental confusion, sorry 😜

Based on how JabRef generates duplicate keys some cases are "easier" to deal with.

If the generated citation key is 2021 duplicated citation keys will be 2021a, 2021b, etc. and it is indeed correct that 2021a should be preferred over 2021b just as 2021 should be preferred over 2021a (this might be a good test-case as well).

But if you are merging and 2021, , should in my opinion not be sorted above 2021.

As I said, this might very well not be worth worrying about.

koppor · 2021-05-07T19:56:52Z

But if you are merging and 2021, , should in my opinion not be sorted above 2021.

Fair! I did not "see" that case.

Then this is a call for CitationKeyComparator in the package org.jabref.logic.bibtex.comparator. This comparator works as initially implemented by @SuXiChangZhen and handles this case, too.

I would re-use comparison code of EntryComparator, but not call EntryComparator, because the empty string handling is different.

k3KAW8Pnf7mkmdSMPHz27 · 2021-05-07T21:21:50Z

Ok, nvm, ignore me, and my apologies for the confusion. I'll have to read up a bit more about merging.

SuXiChangZhen · 2021-05-08T01:19:55Z

It seems that jabref will not produce a citation key like "" since if the citation key is "", then KEY_FIELD does not exist. But KEY_FIELD does allow something like " "(one blank), should I consider this case?

SuXiChangZhen · 2021-05-08T05:10:36Z

I notice that in the BibEntry, the method getCitationKey indicates what is a valid ciatation key. So I use it in the EntryComparator. Now the blanked citation key will be replaced when merge two entries.

Siedlerchr

you can use orElse with Optionals

Siedlerchr · 2021-05-09T16:28:06Z

src/main/java/org/jabref/logic/bibtex/comparator/EntryComparator.java

@@ -72,6 +72,9 @@ public int compare(BibEntry e1, BibEntry e2) {
            // Sort by type.
            f1 = e1.getType();
            f2 = e2.getType();
+        } else if (sortField.equals(InternalField.KEY_FIELD)) {
+            f1 = e1.hasCitationKey() ? e1.getCitationKey().get() : null;


Suggested change

f1 = e1.hasCitationKey() ? e1.getCitationKey().get() : null;

f1 = e1.getCitationKey().orElse(null);

f2 = e2.getCitationKey().orElse(null);

SuXiChangZhen · 2021-05-10T02:28:31Z

Thank you for your suggestion. I have used orElse.

koppor

Thank you for working on it. Good to go for now to fix the concrete issue. We still need to think about the heuristics though.

We need to dig into our JabRef code base to fix some code quality issues - JabRef#497

* upstream/main: (71 commits) [Bot] Update CSL styles (#7735) Fix for issue 6966: open all files of multiple entries (#7709) Add simple unit tests (#7696) Add simple unit tests (#7543) Update check-outdated-dependencies.yml Added preset for new entry keybindings and reintroduced defaults (#7705) Select the entry which has smaller dictonary order when merge (#7708) Update CHANGELOG.md fix: make more fields, fomatters, ids and languages sorted by alphabetical order (#7717) Bump libreoffice from 7.1.2 to 7.1.3 (#7721) Bump unoloader from 7.1.2 to 7.1.3 (#7724) Bump org.beryx.jlink from 2.23.7 to 2.23.8 (#7723) Bump org.openjfx.javafxplugin from 0.0.9 to 0.0.10 (#7725) fix: make fields sorted by lexicographical order (#7711) Fix tests Refactoring existing unit tests (#7687) Refactoring and addition of unit tests (#7581) Refactor simple Unit Tests (#7571) Add simple unit tests (#7544) add and extend unit tests (#7685) ...

fix issue 7395

4158f37

JabRef deleted a comment from Siedlerchr May 6, 2021

koppor requested changes May 6, 2021

View reviewed changes

koppor added the status: changes required Pull requests that are not yet complete label May 6, 2021

Use the EntryComparator

0b609fc

SuXiChangZhen changed the title ~~fix issue 7395~~ Select the entry which has smaller dictonary order when merge May 7, 2021

Merge branch 'JabRef:main' into fix-for-issue-7395

a8783ff

use getCitationKey to get key in EntryComparator

77a1ba1

Siedlerchr requested changes May 9, 2021

View reviewed changes

SuXiChangZhen and others added 2 commits May 10, 2021 10:26

Use orElse with Optionals

699f8e2

Merge branch 'JabRef:main' into fix-for-issue-7395

cdde83d

koppor mentioned this pull request May 10, 2021

Fix FieldComparator vs. EntryComparator JabRef/jabref-koppor#497

Open

koppor approved these changes May 10, 2021

View reviewed changes

koppor merged commit edae432 into JabRef:main May 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Select the entry which has smaller dictonary order when merge #7708

Select the entry which has smaller dictonary order when merge #7708

SuXiChangZhen commented May 6, 2021

Siedlerchr commented May 6, 2021

koppor left a comment

koppor May 6, 2021

SuXiChangZhen commented May 7, 2021

k3KAW8Pnf7mkmdSMPHz27 commented May 7, 2021 •

edited

Loading

SuXiChangZhen commented May 7, 2021

Siedlerchr commented May 7, 2021

SuXiChangZhen commented May 7, 2021

Siedlerchr commented May 7, 2021

koppor commented May 7, 2021

koppor commented May 7, 2021

k3KAW8Pnf7mkmdSMPHz27 commented May 7, 2021 •

edited

Loading

koppor commented May 7, 2021

k3KAW8Pnf7mkmdSMPHz27 commented May 7, 2021

SuXiChangZhen commented May 8, 2021

SuXiChangZhen commented May 8, 2021

Siedlerchr left a comment

Siedlerchr May 9, 2021

SuXiChangZhen commented May 10, 2021

koppor left a comment

	f1 = e1.hasCitationKey() ? e1.getCitationKey().get() : null;
	f1 = e1.getCitationKey().orElse(null);
	f2 = e2.getCitationKey().orElse(null);

Select the entry which has smaller dictonary order when merge #7708

Select the entry which has smaller dictonary order when merge #7708

Conversation

SuXiChangZhen commented May 6, 2021

Siedlerchr commented May 6, 2021

koppor left a comment

Choose a reason for hiding this comment

koppor May 6, 2021

Choose a reason for hiding this comment

SuXiChangZhen commented May 7, 2021

k3KAW8Pnf7mkmdSMPHz27 commented May 7, 2021 • edited Loading

SuXiChangZhen commented May 7, 2021

Siedlerchr commented May 7, 2021

SuXiChangZhen commented May 7, 2021

Siedlerchr commented May 7, 2021

koppor commented May 7, 2021

koppor commented May 7, 2021

k3KAW8Pnf7mkmdSMPHz27 commented May 7, 2021 • edited Loading

koppor commented May 7, 2021

k3KAW8Pnf7mkmdSMPHz27 commented May 7, 2021

SuXiChangZhen commented May 8, 2021

SuXiChangZhen commented May 8, 2021

Siedlerchr left a comment

Choose a reason for hiding this comment

Siedlerchr May 9, 2021

Choose a reason for hiding this comment

SuXiChangZhen commented May 10, 2021

koppor left a comment

Choose a reason for hiding this comment

k3KAW8Pnf7mkmdSMPHz27 commented May 7, 2021 •

edited

Loading

k3KAW8Pnf7mkmdSMPHz27 commented May 7, 2021 •

edited

Loading