Fix removal of special fields when syncing them via keywords #3438
Conversation
When reading the code, one would expect to find all methods concerned with keywords next to each other.
CHANGELOG.md
Outdated
| @@ -47,6 +47,7 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `# | |||
| - We fixed an issue where fetching entries from crossref that had no titles caused an error [#3376](https://github.com/JabRef/jabref/issues/3376) | |||
| - We fixed an issue where the same Java Look and Feel would be listed more than once in the Preferences. [#3391](https://github.com/JabRef/jabref/issues/3391) | |||
| - We fixed an issue where errors in citation styles triggered an exception when opening the preferences dialog [#3389](https://github.com/JabRef/jabref/issues/3389) | |||
| - We fixed an issue where special fields (such as **printed**) could not be cleared when syncing special fields via the keywords [#3432](https://github.com/JabRef/jabref/issues/3432) | |||
There was a problem hiding this comment.
Just a very minor issue, for field names in changelog we usually use backticks
There was a problem hiding this comment.
Even if it does NOT reference a bibtex field? With "printed", I refer to the "special field" here.
|
|
||
| Optional<FieldChange> change = entry.replaceKeywords(keyWords, newValue, keywordDelimiter); | ||
| change.ifPresent(changeValue -> fieldChanges.add(changeValue)); | ||
| if (newValue.isPresent()) { |
There was a problem hiding this comment.
You can chain nested optionals with map, it will then only be executed if a value is present
| SpecialFieldsUtils.updateField(specialField, specialFieldKeyword.get(), entry, true, true, ','); | ||
| // Remove it | ||
| List<FieldChange> changes = SpecialFieldsUtils.updateField(specialField, specialFieldKeyword.get(), entry, true, true, ','); | ||
| assertTrue(changes.size() > 0); |
There was a problem hiding this comment.
Please avoid checking such things with size, instead directly create a list expected and use assertEquals
| public void removeKeywordsWithNonExistingKeywordsDoesNothing() { | ||
| keywordEntry.putKeywords(Arrays.asList("kw1", "kw2"), ','); | ||
| Optional<FieldChange> change = keywordEntry.removeKeywords(KeywordList.parse("kw3, kw4", ','), ','); | ||
| assertEquals(Optional.empty(), change); |
There was a problem hiding this comment.
As mentioned above, better to directly compare the result to a pre defined list with the keywords which you already use to add the keywords
So just do expected= arrrays.aslist(kw1,kw2)
|
Made the requested changes. |
| return putKeywords(keywordList, keywordDelimiter); | ||
| } | ||
|
|
||
| public Optional<FieldChange> replaceKeywords(KeywordList keywordsToReplace, Optional<Keyword> newValue, |
There was a problem hiding this comment.
Can you please change the type of newValue to be just Keyword (instead of an optional around it). Accepting the optional, but then not properly acting on empty ones, actually lead to this problem. So I think we should fix its root.
Requested change for JabRef#3438
|
Thanks @tobiasdiez for pointing that out - fixed it. |
Fixes a bug where JabRef special fields (such as 'printed', 'relevant', ...) were not cleared (e.g. removing the 'printed' flag) when using synchronization of special fields via the bibtex keywords field.
See #3432 for more details.