[WIP] Add toggle functionality for journal abbreviation lists #12912
Conversation
This feature allows users to enable/disable specific journal abbreviation lists, including both the built-in list and external CSV files, without removing them from configuration. - Added toggle controls in UI with visual indicators for enabled/disabled states - Implemented filtering of abbreviations based on source enabled state - Ensured toggle states persist between application sessions - Optimized performance with efficient repository loading - Added comprehensive test coverage for new functionality Closes JabRef#12468
zikunz
force-pushed
the
feature/journal-abbreviation-toggle
branch
from
dd85326 to
37c8d2c
Compare
|
Please update your screenshots here as well. |
|
@subhramit Got it, I will make sure all the screenshots will be updated in the PR content above :) |
|
I tried to fix the two screenshots in the PR content, may I ask whether you are able to see them? @subhramit |
yes, they are fine now |
|
Thank you! @subhramit |
Journal abbreviations coming from sources that the user has disabled should neither be generated nor reversed.\n\nThis patch:\n 1. Makes UndoableUnabbreviator skip entries whose source is disabled\n2. Introduces AbbreviationWithSource to persist source metadata\n3. Filters candidates early in AbbreviateAction\n4. Refreshes repository state when a source is toggled in the UI
|
Update: the bug is now fixed and I proceed to refactor the code now. Afterwards, this PR will be ready for review :) |
This commit fixes test failures that occurred after implementing the journal abbreviation toggle feature. The changes include: 1. Ensuring the built-in list is always enabled by default in the JournalAbbreviationLoader for backward compatibility 2. Updating journal abbreviation tests to use controlled test data rather than depending on the built-in repository contents 3. Adding a more comprehensive test case for the abbreviation cycle
|
Your pull request conflicts with the target branch. Please merge |
|
Your pull request conflicts with the target branch. Please merge |
![trag-bot[bot]](https://avatars.githubusercontent.com/in/856808?s=60&v=4){kind=small}
Integrate journal abbreviation toggle functionality (JabRef#12880) with the LTWA repository support from main branch. Resolve conflicts in JournalAbbreviationRepository, JournalAbbreviationLoader, and MainMenu to ensure both features work correctly together. The combined functionality allows users to enable/disable specific journal abbreviation sources while maintaining LTWA abbreviation support.
|
@koppor @Siedlerchr @subhramit I have completed resolving the 6 merge conflicts between my PR and PR #12880 (which closed #12273). The resolved changes are reflected in my recent commit with the commit message of "feat: resolve merge conflicts with journal abbreviation toggle feature". To properly test the combined functionality, I first proceeded to During my local testing of the merged functionality, I observed some behavior that I would like to confirm is working as intended:
I want to ensure that my merge conflict resolution is correct and that I am understanding the expected behavior of the LTWA abbreviation functionality properly. Could you please advise if these observations align with the intended functionality of the LTWA implementation, or if further adjustments are needed? Thank you for your guidance. |
|
@Yubo-Cao Can you look into #12912 (comment) maybe? I currently don't have the resources to properly answer. |
@koppor Thank you! @Yubo-Cao I look forward to your guidance. Thank you in advance! |
| @Test | ||
| void checkBasicAbbreviate() { | ||
| BibEntry entry = new BibEntry(); | ||
| entry.setField(StandardField.JOURNAL, "TJ"); |
There was a problem hiding this comment.
When creating a new BibEntry object, 'withers' should be used instead of 'setField'. This improves code readability and follows modern Java practices.
|
|
||
| @Test | ||
| void unabbreviateWithBothSourcesEnabled() { | ||
| assertTrue(repository.isSourceEnabled(JournalAbbreviationRepository.BUILTIN_LIST_ID)); |
There was a problem hiding this comment.
The test uses assertTrue to check boolean conditions. It should assert the contents of objects using assertEquals for better clarity and debugging.
| repository.setSourceEnabled(CUSTOM_SOURCE, false); | ||
|
|
||
| assertTrue(repository.isSourceEnabled(JournalAbbreviationRepository.BUILTIN_LIST_ID)); | ||
| assertFalse(repository.isSourceEnabled(CUSTOM_SOURCE)); |
There was a problem hiding this comment.
The test uses assertFalse to check boolean conditions. It should assert the contents of objects using assertEquals for better clarity and debugging.
| @Test | ||
| void unabbreviateWithBothSourcesEnabled() { | ||
| assertTrue(repository.isSourceEnabled(JournalAbbreviationRepository.BUILTIN_LIST_ID)); | ||
| assertTrue(repository.isSourceEnabled(CUSTOM_SOURCE)); |
There was a problem hiding this comment.
The test uses assertTrue to check boolean conditions. It should assert the contents of objects using assertEquals for better clarity and debugging.
| void unabbreviateWithOnlyCustomSourceEnabled() { | ||
| repository.setSourceEnabled(JournalAbbreviationRepository.BUILTIN_LIST_ID, false); | ||
|
|
||
| assertFalse(repository.isSourceEnabled(JournalAbbreviationRepository.BUILTIN_LIST_ID)); |
There was a problem hiding this comment.
The test uses assertFalse to check boolean conditions. It should assert the contents of objects using assertEquals for better clarity and debugging.
| repository.setSourceEnabled(CUSTOM_SOURCE, false); | ||
|
|
||
| assertTrue(repository.isSourceEnabled(JournalAbbreviationRepository.BUILTIN_LIST_ID)); | ||
| assertFalse(repository.isSourceEnabled(CUSTOM_SOURCE)); |
There was a problem hiding this comment.
The test uses assertFalse to check boolean conditions. It should assert the contents of objects using assertEquals for better clarity and debugging.
|
|
||
| @Test | ||
| void unabbreviateWithBothSourcesEnabled() { | ||
| assertTrue(repository.isSourceEnabled(JournalAbbreviationRepository.BUILTIN_LIST_ID)); |
There was a problem hiding this comment.
The test uses assertTrue to check boolean conditions. It should assert the contents of objects using assertEquals for better clarity and debugging.
| @Test | ||
| void unabbreviateWithBothSourcesEnabled() { | ||
| assertTrue(repository.isSourceEnabled(JournalAbbreviationRepository.BUILTIN_LIST_ID)); | ||
| assertTrue(repository.isSourceEnabled(CUSTOM_SOURCE)); |
There was a problem hiding this comment.
The test uses assertTrue to check boolean conditions. It should assert the contents of objects using assertEquals for better clarity and debugging.
| void unabbreviateWithOnlyCustomSourceEnabled() { | ||
| repository.setSourceEnabled(JournalAbbreviationRepository.BUILTIN_LIST_ID, false); | ||
|
|
||
| assertFalse(repository.isSourceEnabled(JournalAbbreviationRepository.BUILTIN_LIST_ID)); |
There was a problem hiding this comment.
The test uses assertFalse to check boolean conditions. It should assert the contents of objects using assertEquals for better clarity and debugging.
| repository.setSourceEnabled(CUSTOM_SOURCE, false); | ||
|
|
||
| assertTrue(repository.isSourceEnabled(JournalAbbreviationRepository.BUILTIN_LIST_ID)); | ||
| assertFalse(repository.isSourceEnabled(CUSTOM_SOURCE)); |
There was a problem hiding this comment.
The test uses assertFalse to check boolean conditions. It should assert the contents of objects using assertEquals for better clarity and debugging.
Apologies for the late reply. The behavior you found is as expected:
|
| List<T> placeholder = new ArrayList<>(observableList); | ||
| Collections.swap(placeholder, i, j); | ||
| observableList.sort(Comparator.comparingInt(placeholder::indexOf)); | ||
| if (i < 0 || j < 0 || i >= observableList.size() || j >= observableList.size()) { |
There was a problem hiding this comment.
The code follows the fail-fast principle by returning early on invalid indices, which is a good practice. Ensure this is consistently applied throughout the codebase.
zikunz
force-pushed
the
feature/journal-abbreviation-toggle
branch
from
69e0d00 to
10491f5
Compare
|
|
||
| action.execute(); | ||
|
|
||
| verify(dialogService).notify(eq(Localization.lang("Cannot unabbreviate: all journal lists are disabled."))); |
There was a problem hiding this comment.
The notification message uses an exclamation mark, which should be avoided as per guidelines. Use a period instead to end the sentence.
| action.execute(); | ||
|
|
||
| verify(dialogService, never()).notify(eq(Localization.lang("Cannot unabbreviate: all journal lists are disabled."))); | ||
| verify(dialogService).notify(eq(Localization.lang("Unabbreviating..."))); |
There was a problem hiding this comment.
The notification message uses an exclamation mark, which should be avoided as per guidelines. Use a period instead to end the sentence.
src/main/java/org/jabref/gui/preferences/journals/JournalAbbreviationsTab.java
Show resolved
Hide resolved
src/main/java/org/jabref/gui/preferences/journals/JournalAbbreviationsTab.java
Show resolved
Hide resolved
Thank you so much for your reply! @Yubo-Cao I apologize for not noticing your response until just now. I am very thankful for your confirmation that all the behaviors I described are expected. This is reassuring that my resolution of the merge conflicts appears to be on the right track because there is no change in LTWA functionalities you implemented after adding toggle functionality (I will also thoroughly double-check it later on again). Thank you also for explaining the detailed business logic, design choices and implementation details - this has been extremely helpful. @koppor All my questions were answered, I will make this PR ready for review soon. I will update everyone in #12468 :) |
There was a problem hiding this comment.
The changes so far are looking good. How much more work do you estimate it'll take to get to a "ready" state?
See if you can get rid of Collectors and Collections wherever possible. I have left some hints in comments below and some miscellaneous code suggestions.
| journalFilesBox.setCellFactory(listView -> new JournalFileListCell()); | ||
| journalFilesBox.setButtonCell(new JournalFileListCell()); | ||
|
|
||
| viewModel.journalFilesProperty().addListener((observable, oldValue, newValue) -> { |
There was a problem hiding this comment.
| viewModel.journalFilesProperty().addListener((observable, oldValue, newValue) -> { | |
| viewModel.journalFilesProperty().addListener((_, _, newValue) -> { |
| return; | ||
| } | ||
| for (AbbreviationsFileViewModel fileViewModel : newValue) { | ||
| fileViewModel.enabledProperty().addListener((obs, oldVal, newVal) -> { |
There was a problem hiding this comment.
| fileViewModel.enabledProperty().addListener((obs, oldVal, newVal) -> { | |
| fileViewModel.enabledProperty().addListener((_, _, _) -> { |
| Optional<Abbreviation> customMatch = findBestFuzzyMatched(customAbbreviations, input); | ||
| Collection<Abbreviation> enabledCustomAbbreviations = customAbbreviations.stream() | ||
| .filter(abbreviation -> isSourceEnabled(abbreviationSources.getOrDefault(abbreviation, BUILTIN_LIST_ID))) | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
| .collect(Collectors.toList()); | |
| .toList(); |
| public List<Abbreviation> getAllAbbreviations() { | ||
| return getAllAbbreviationsWithSources().stream() | ||
| .map(AbbreviationWithSource::getAbbreviation) | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
| .collect(Collectors.toList()); | |
| .toList(); |
| return; | ||
| } | ||
| try { | ||
| Collection<Abbreviation> abbreviationsFromFile = JournalAbbreviationLoader.readAbbreviationsFromCsvFile(filePath); |
There was a problem hiding this comment.
Can you use Set here? Refactor up the hierarchy, start by converting return type of:
To Set<Abbreviation> and then replace everywhere. I have left "some" suggestions as were visible to me in other comments.
| * Adds multiple custom abbreviations to the repository | ||
| * | ||
| * @param abbreviationsToAdd The abbreviations to add | ||
| */ | ||
| public void addCustomAbbreviations(Collection<Abbreviation> abbreviationsToAdd) { |
There was a problem hiding this comment.
| public void addCustomAbbreviations(Collection<Abbreviation> abbreviationsToAdd) { | |
| public void addCustomAbbreviations(Set<Abbreviation> abbreviationsToAdd) { |
| * @param sourceKey The key identifying the source of these abbreviations | ||
| * @param enabled Whether the source should be enabled initially | ||
| */ | ||
| public void addCustomAbbreviations(Collection<Abbreviation> abbreviationsToAdd, String sourceKey, boolean enabled) { |
There was a problem hiding this comment.
| public void addCustomAbbreviations(Collection<Abbreviation> abbreviationsToAdd, String sourceKey, boolean enabled) { | |
| public void addCustomAbbreviations(Set<Abbreviation> abbreviationsToAdd, String sourceKey, boolean enabled) { |
Adapt the javadoc too.
| @@ -234,4 +356,76 @@ public Set<String> getFullNames() { | |||
| public Collection<Abbreviation> getAllLoaded() { | |||
There was a problem hiding this comment.
| public Collection<Abbreviation> getAllLoaded() { | |
| public Set<Abbreviation> getAllLoaded() { |
|
|
||
| String text = entry.getFieldLatexFree(journalField).orElse(""); | ||
| List<JournalAbbreviationRepository.AbbreviationWithSource> possibleSources = | ||
| textToSourceMap.getOrDefault(text, Collections.emptyList()); |
There was a problem hiding this comment.
| textToSourceMap.getOrDefault(text, Collections.emptyList()); | |
| textToSourceMap.getOrDefault(text, List.of()); |
| public class JournalAbbreviationPreferences { | ||
|
|
||
| private final ObservableList<String> externalJournalLists; | ||
| private final BooleanProperty useFJournalField; | ||
|
|
||
| private final Map<String, Boolean> enabledExternalLists = new HashMap<>(); | ||
| private final BooleanProperty enabledListsChanged = new SimpleBooleanProperty(false); |
There was a problem hiding this comment.
No need to initialize with false here.
Put the default in JabRefCliPreferences using defaults.put(ENABLED_EXTERNAL_JOURNAL_LISTS, false).
See other examples for reference.
@subhramit Thank you so much for your thorough code review and feedback. I estimate I will need approximately 1 - 2 additional days to bring this PR to a "ready" state. Here is what remains on my task list (please let me know if I still miss any):
I am really thankful for your guidance on reducing dependencies on |
|
|
||
| List<BibEntry> filteredEntries = new ArrayList<>(); | ||
|
|
||
| JournalAbbreviationRepository freshRepository = JournalAbbreviationLoader.loadRepository(journalAbbreviationPreferences); |
There was a problem hiding this comment.
We also had a question about this here - do you wish to create a new repository every time you want to call unabbreviate?
Or even more generally, should unabbreviate depend on preferences at all?
src/main/java/org/jabref/gui/preferences/journals/JournalAbbreviationsTabViewModel.java
Show resolved
Hide resolved
Heads up! This PR is still a work in progress:
Toggle Functionality for Journal Abbreviation Lists
Closes #12468.
What
This feature is comprehensive and solves more than what is asked in #12468.
Specifially, this PR adds the ability to enable/disable any journal abbreviation lists in JabRef, including both the built-in list and external CSV files. Users can now toggle all types of journal abbreviation lists on/off to make them enabled/disabled.
We can disable the built-in list:
If the built-in list is disabled, journal names found in the built-in list will not be abbreviated:
Why
In the original codebase, all journal abbreviation lists were always active once loaded. Users had no way to temporarily disable specific abbreviation sources without completely removing them. This was particularly problematic with the built-in list, which could not be removed at all. Detailed reasons about why we want to disable built-in list sometimes can be found in the issue.
This feature improves workflow flexibility by allowing users to:
How
The implementation follows JabRef's existing architecture patterns:
Key Features
Filtering Logic
Persistence
UI Feedback
Design Considerations
Performance Considerations
The implementation reloads the journal repository before each abbreviation operation to ensure it uses the latest toggle states. This approach has minimal performance impact because:
I have also tested with fairly large abbreviation lists and observed no noticeable performance degradation.
Design Alternatives Considered
Event-based updates: I considered using an event system to update the repository when toggle states change. While potentially more efficient, this approach would be more complex and error-prone, requiring careful synchronization between preference changes and repository state.
Runtime filtering: Another approach was to keep all abbreviations loaded and filter at runtime during abbreviation operations. This would be slightly faster but would require more memory and complicate the core lookup methods.
Separate repositories: I also considered maintaining separate repositories for each source, but this would diverge significantly from JabRef's existing architecture and complicate cross-source operations.
The chosen approach of reloading the repository provides the best balance of reliability, maintainability, and adherence to existing architecture patterns.
Proposed Code Changes
Frontend Changes (GUI)
JournalAbbreviationsTab.java
AbbreviationsFileViewModel.java
JournalAbbreviationsTabViewModel.java
MainMenu.java
Backend Changes (Logic)
JournalAbbreviationRepository.java
sourceToAbbreviationsmap to associate abbreviations with their sourcesenabledSourcesmap to track enabled/disabled state of each sourceJournalAbbreviationLoader.java
AbbreviateAction.java
JournalAbbreviationPreferences.java
enabledExternalListsmap to store enabled/disabled statesJabRefCliPreferences.java
Testing Strategy
Test Files and Coverage
JournalAbbreviationRepositoryTest.java
multipleSourcesCanBeToggled: Verifies that multiple sources can be independently enabled/disableddisabledSourcesAreFilteredFromLookup: Ensures abbreviations from disabled sources aren't returned in lookupsbuiltInListCanBeToggled: Confirms the built-in list can be toggled like external sourcestoggleStateAffectsAbbreviationSets: Tests that abbreviation sets are properly filtered by source stateJournalAbbreviationsViewModelTabTest.java
addBuiltInListInitializesWithCorrectEnabledState: Verifies built-in list loaded with correct enabled stateenabledExternalListFiltersAbbreviationsWhenDisabled: Tests that UI reflects filtered abbreviationsstoreSettingsPersistsEnabledStateToPreferences: Ensures toggle states are saved to preferencesImplementation Additions for Testing
The following methods and properties were added specifically to support testing:
AbbreviationsFileViewModel.java
refreshAbbreviations()method to simulate UI updates when toggle state changesisEnabled(),setEnabled(), andenabledProperty()methods for test validationJournalAbbreviationsTabViewModel.java
markAsDirty()method to allow tests to mark files as needing to be savedstoreSettings()to provide verification points for proper state persistenceJournalAbbreviationRepository.java
addCustomAbbreviation(abbreviation, sourcePath, enabled)with source tracking for test scenariosJournalAbbreviationPreferences.java
hasExplicitEnabledSetting()method to improve testability of preference storageThese additions ensure the toggle functionality can be thoroughly tested while maintaining clean separation between the application code and test code.
Mandatory checks
CHANGELOG.mddescribed in a way that is understandable for the average user (if change is visible to the user)