Fix DuplicationChecker and key generator #2151
Conversation
| if (Globals.prefs.getBoolean(JabRefPreferences.AVOID_OVERWRITING_KEY)) { | ||
| // Remove the entry, because its key is already set: | ||
| if (Globals.prefs.getBoolean(JabRefPreferences.AVOID_OVERWRITING_KEY)) { | ||
| for (final Iterator<BibEntry> i = entries.iterator(); i.hasNext();) { |
There was a problem hiding this comment.
Please rename i to iterator.
There was a problem hiding this comment.
And why not using stream with filter? Or even better entries.removeIf?
There was a problem hiding this comment.
Lambdas still doesn't come that easy to my mind.
I rewrote this and the part just beneath with lambdas.
| private final Map<String, Integer> allKeys = new HashMap<>(); | ||
|
|
||
|
|
There was a problem hiding this comment.
please delete empty line.
There was a problem hiding this comment.
I always like a little visual separation between the members from the methods, and seen this in some other classes too.
There was a problem hiding this comment.
I know that I got many comments for empty lines like this.
But I think in this case it's okay to leave it.
| /** | ||
| * Checks if there is more than one occurrence of this key | ||
| */ | ||
| public boolean isDuplicateExisting(Optional<String> citeKey) { |
There was a problem hiding this comment.
Please don't use optionals as params. They should only be used as return values. See #1577 (comment)
tschechlovdev
added
status: ready-for-review
| // that it's ok to overwrite keys: | ||
| break; | ||
| if (Globals.prefs.getBoolean(JabRefPreferences.AVOID_OVERWRITING_KEY)) { | ||
| entries.removeIf(BibEntry::hasCiteKey); |
There was a problem hiding this comment.
Please readd the comments from the old code if possible. Here, the comment is // Remove the entry, because its key is already set:
There was a problem hiding this comment.
I added some comments
| @@ -457,7 +448,7 @@ public static void makeLabel(AbstractBibtexKeyPattern citeKeyPattern, BibDatabas | |||
| } | |||
|
|
|||
| String moddedKey = key + getAddition(number); | |||
| occurrences = database.getNumberOfKeyOccurrences(moddedKey); | |||
| occurrences = database.getDuplicationChecker().getNumberOfKeyOccurrences(moddedKey); | |||
|
|
|||
| if (Objects.equals(oldKey, moddedKey)) { | |||
| occurrences--; | |||
There was a problem hiding this comment.
Can this ever happen? - If yes, please construct a test case covering this line.
There was a problem hiding this comment.
Can only happen if the BibtexKeyPatternUtil#getAddition method is buggy.
|
|
||
| @Subscribe | ||
| public void listen(EntryRemovedEvent entryRemovedEvent) { | ||
| removeKeyFromSet(entryRemovedEvent.getBibEntry().getCiteKeyOptional().orElse(null)); |
There was a problem hiding this comment.
I am not sure whether null should really be treated as real key. Why not excluding null from FieldChanged, EntryRemoved, and EntryAdded?
As a consequence, null always appears 0 times in the database.
There was a problem hiding this comment.
if the key is null removeKeyFromSet will return immediately.
now I use the ifPresent method of the optional.
|
Tested it manually: works. Besides the minor question: LGTM. |
koppor
removed
the
status: ready-for-review
* upstream/master: (24 commits) hotfix NPE in the main table (#2158) Enhance side pane toggle (#1605) Added gray background text to authors field to assist newcomers (#2147) Rewrite google scholar fetcher to new infrastructure (#2101) Found entries will be shown when dropping a pdf with xmp meta data (#2150) Japanese translation (#2155) Add shortcuts to context menu (#2131) [WIP] Doi resolution ignore (#2154) Fix DuplicationChecker and key generator (#2151) just close popup on first ESC keep entry in sight Fix isSharedDatabaseAlreadyPresent check. don't change entry after search if editing an entry (#2129) Change download URL to downloads.jabref.org (#2145) fix switching edited textfield in the entry editor with TAB (#2138) Update me.champeau.gradle.jmh' version from 0.3.0 to 0.3.1 Update install4j plugin from 6.1.2 to 6.1.3 Remove gradle plugin com.github.youribonnaffe.gradle.format as it is deprecated and the config uses a non-existing file Update gradle from 3.0 to 3.1 Fix changing the font size not working when importing preferences (#2069) ... # Conflicts: # src/main/java/net/sf/jabref/gui/externalfiles/DroppedFileHandler.java # src/main/java/net/sf/jabref/gui/importer/fetcher/GoogleScholarFetcher.java
Fixes #2089.
The Duplication checker was faulty and didn't catch all the cite keys. There needed to be used a special method in the BasePanel which wasn't used at all.
I registered the DuplicationChecker with the Eventbus which now gets notified when an Entry changes, gets added, or removed.
This fixes the automatic Key generation which before had wrong information on what cite keys exist.