Added support for (biblatex) langid to be an optional field in entry editor
#12071
Conversation
… into gui and program logic
There was a problem hiding this comment.
Initial comments.
Please re-iterate on the biblatex smanual and check whether language and origLanguage also have the same values. If yes, your editor can be used if the field proprty FieldProperty.LANGUAGE langauge is present. If not, then this property can just be remoed.
In other words FieldPropety groups fields together and indicates the editor for it. Do not introduce a property if it used for one field only.
src/main/java/org/jabref/gui/fieldeditors/optioneditors/LangidEditorViewModel.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/gui/fieldeditors/optioneditors/LangidEditorViewModel.java
Outdated
Show resolved
Hide resolved
| @@ -18,7 +18,7 @@ public enum FieldProperty { | |||
| IDENTIFIER, | |||
|
|
|||
| LANGUAGE, | |||
|
|
|||
| LANGUAGEID, | |||
| // Field content is text, but should be interpreted as markdown | |||
There was a problem hiding this comment.
Please keep empty lines as separator
There was a problem hiding this comment.
Maybe, this and LANGUAGE can just be removed. These properties are not used.
Alternataively, use it. And then use the langId field editor as you introduced for all fields having this property.
- removed redundant description in CHANGELOG.md - removed unnecessary gradle properties - removed unnecessary tests - fixed formatting/commented out code
| @@ -78,30 +77,4 @@ private static Stream<Arguments> getLangidByStringTest() { | |||
| arguments(Optional.empty(), "unknown") | |||
| ); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testLangidNameAndValue() { | |||
There was a problem hiding this comment.
why do you remove these test?
src/main/java/org/jabref/model/entry/types/BibtexEntryTypeDefinitions.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jabref/gui/fieldeditors/optioneditors/LangidEditorViewModel.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Override | ||
| public Langid fromString(String string) { |
There was a problem hiding this comment.
Return Optional<Langid> instead of null.
There was a problem hiding this comment.
I was under the impression that it had to return Langid because the superclass OptionEditorViewModel requires StringConverter<T> to return <T> not Optional<T>?
src/test/java/org/jabref/gui/fieldeditors/optioneditors/LangidEditorViewModelTest.java
Outdated
Show resolved
Hide resolved
| // Test conversion from Langid to string | ||
| String convertedString = langidEditorViewModel.getStringConverter().toString(Langid.BULGARIAN); | ||
| assertEquals(langidString, convertedString, "Langid should convert back to its string representation"); |
There was a problem hiding this comment.
Do a seperate test.
Remove the assert text, because this should be the test method name.
(Also do for the other tests)
Co-authored-by: Oliver Kopp <[email protected]>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your code currently does not meet JabRef's code guidelines.
We use Checkstyle to identify issues.
The tool reviewdog already placed comments on GitHub to indicate the places. See the tab "Files" in you PR.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.
You can check review dog's comments at the tab "Files changed" of your pull request.
…ing: - refactored and renamed LangidEditorViewModel.java to LanguageEditorViewModel (and tests) - re-added gradle property - reverted edits to BibtexEntryTypeDefinitions.java - removed FieldProperty.LANGUAGEID so both language and langid standard fields have FieldProperty.LANGUAGE NB: Still yet to fix tests
…into 10868-support-for-langid-field # Conflicts: # gradle.properties
There was a problem hiding this comment.
Your code currently does not meet JabRef's code guidelines.
We use Checkstyle to identify issues.
The tool reviewdog already placed comments on GitHub to indicate the places. See the tab "Files" in you PR.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.
You can check review dog's comments at the tab "Files changed" of your pull request.
| @@ -11,3 +11,4 @@ org.gradle.jvmargs=-Xmx6096M | |||
|
|
|||
| # hint by https://docs.gradle.org/current/userguide/performance.html#enable_the_build_cache | |||
| org.gradle.caching=true | |||
| org.gradle.parallel=true | |||
| * See the BibLaTeX documentation for full details: | ||
| * <a href="http://mirrors.ctan.org/macros/latex/contrib/biblatex/doc/biblatex.pdfhangelo">BibLaTeX manual</a> | ||
| */ | ||
|
|
There was a problem hiding this comment.
JavaDoc needs to be attached to class --> remove empty line here (and add empty line above)
|
|
||
| @BeforeEach | ||
| void setUp() { | ||
| // Mock dependencies |
There was a problem hiding this comment.
Remove comment. The next lines state that by code.
| FilePreferences filePreferences = Mockito.mock(FilePreferences.class); | ||
| JournalAbbreviationRepository abbreviationRepository = Mockito.mock(JournalAbbreviationRepository.class); | ||
|
|
||
| // Initialize FieldCheckers with mocked instances |
There was a problem hiding this comment.
Remove comment. The next lines state that by code.
| // Initialize FieldCheckers with mocked instances | ||
| FieldCheckers fieldCheckers = new FieldCheckers(databaseContext, filePreferences, abbreviationRepository, false); | ||
|
|
||
| // Mock the SuggestionProvider |
There was a problem hiding this comment.
Remove comment. The next lines state that by code.
| languageEditorViewModel = new LanguageEditorViewModel( | ||
| StandardField.LANGUAGEID, // Use the correct field | ||
| suggestionProvider, // Mocked SuggestionProvider | ||
| BibDatabaseMode.BIBLATEX, // Use the correct BibDatabaseMode |
| StandardField.LANGUAGEID, // Use the correct field | ||
| suggestionProvider, // Mocked SuggestionProvider | ||
| BibDatabaseMode.BIBLATEX, // Use the correct BibDatabaseMode | ||
| fieldCheckers, // FieldCheckers instance |
| suggestionProvider, // Mocked SuggestionProvider | ||
| BibDatabaseMode.BIBLATEX, // Use the correct BibDatabaseMode | ||
| fieldCheckers, // FieldCheckers instance | ||
| new UndoManager() // UndoManager instance |
| void getItemsShouldReturnAllLangidValues() { | ||
| Collection<Langid> items = languageEditorViewModel.getItems(); | ||
| assertEquals(Langid.values().length, items.size()); | ||
| assertTrue(items.contains(Langid.BASQUE)); // Check if it contains a specific Langid |
| Collection<Langid> items = languageEditorViewModel.getItems(); | ||
| assertEquals(Langid.values().length, items.size()); | ||
| assertTrue(items.contains(Langid.BASQUE)); // Check if it contains a specific Langid | ||
| assertTrue(items.contains(Langid.AMERICAN)); // Additional check for another Langid |
There was a problem hiding this comment.
Try to rewrite multiple assert statements to one
assertEquals(List.of(...), items)
koppor
added
the
status: changes required
Overview:
Integrated 'langid' into all biblatex/bibtex supported entry types as an optional field. This is so formatting of entries can be adapted to the language of the entries. Closes #10868.
Changes implemented:
langid.video.mp4
Mandatory checks
CHANGELOG.mddescribed in a way that is understandable for the average user (if applicable)