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.
|
Hi, @zikunz! I think I can hop in and help Subhramit et al. with this PR. As JabRef is primarily done in free time by volunteers, we typically write small issues and expected small/medium PRs. But sometimes we have PRs that escalate in a lot of work. Can you tell me:
For 4th point -- I think it's better to postpone this |
|
Please not another Manager. Especially not a repository manager. This is silly, almost funny. And please do not use AI to communicate with us. Speak real, use your own words. |
Hi @InAnYan, thank you so much for helping to review this PR, please find below the answers for your 3 questions: Q: Do you have any blockers? Q: Do you have any questions for design choices that should be discussed with maintainers? Q: What specifically this PR solves/done/improves? So that I know what to test for.
After pressing toggle, a list will change from enabled to disabled or change from disabled to enabled. This will be saved only if the user clicks After the users save enabled / disabled states for the lists, when they try to abbreviate and unabbreviate one or more journal names, only journal names found in the enabled lists will be abbreviated or unabbreviated. If a list is disabled, journal name(s) found in the disabled list cannot be abbreviated or unabbreviated. Please let me know if anything written above is not clear. Thank you again for your help! |
| @@ -98,6 +108,10 @@ public void execute() { | |||
| } | |||
|
|
|||
| private String abbreviate(BibDatabaseContext databaseContext, List<BibEntry> entries) { | |||
| // Use the repository manager to get a cached repository or build a new one if needed | |||
| abbreviationRepository = JournalAbbreviationRepositoryManager.getInstance() | |||
There was a problem hiding this comment.
JournalAbbreviationRepositoryManager -- I don't like this approach.
Typically, in JabRef for dependencies, we do this:
- Get the dependency from the constructor (as it was before).
- Use Dependency Injection -- but we are migrating away from it.
- And only in rare cases (like for
WebViews) we use static classes.
So what I meant to say that we don't use static/singleton classes in a raw form like this. I'm not sure, why this approach was taken, what was the problem with previous approach where a repository was taken from constructor?
There was a problem hiding this comment.
Thank you for your feedback @InAnYan, this approach was wrongly taken by me yesterday night as I was rushing to get it done (I will take note to have a calm mind when pushing for code changes). Now I understand that this approach is wrong. I cc-ed you in a comment above for details regarding what was the problem to solve, what was the initial approach taken and why this approach was subsequently used by me.
In short, the original problem I was trying to solve was the inefficient creation of new repositories on every operation:
JournalAbbreviationRepository freshRepository = JournalAbbreviationLoader.loadRepository(journalAbbreviationPreferences);Then, I implemented the separate manager class that works functionally, but I now see now it adds unnecessary abstraction.
I will make the following changes:
- Move the caching logic directly into
JournalAbbreviationRepository - Add a static
getInstance(preferences)method there - Remove
JournalAbbreviationRepository
This way, the repository itself will handle when to reload based on preference changes, without needing extra layers.
May I ask whether this approach sound right? I appreciate the guidance and am eager to improve the implementation. :)
There was a problem hiding this comment.
@InAnYan I think I understand the issues better.
First of all, creating a separate manager was unnecessary and did not align with JabRef's architectural style.
I have completely removed the manager class and implemented a more localized solution to address the efficiency problem. Instead of creating a new repository for every abbreviation operation, I have added instance-level caching in the AbbreviateAction class. This approach stores the last used preferences and repository as instance fields, implements a preferencesChanged() method to compare current vs. cached preferences and only creates a new repository when preferences actually change. The changes can be seen in 50c3549.
I think this gives us the performance benefits without introducing a singleton pattern or static caching. The repository itself still handles filtering for enabled/disabled sources through its specialized methods.
Please let me know if this approach looks good to you or if you would like me to make further adjustments. Thank you so much!
| * | ||
| * @return true if at least one source is enabled, false if all sources are disabled | ||
| */ | ||
| private boolean areAnyJournalSourcesEnabled() { |
There was a problem hiding this comment.
I think this should be moved to JournalAbbreviationPreferences. This is not related to logic in AbbreviateAction, and it's quite general
There was a problem hiding this comment.
Got it! Just made the changes in bd53428 :)
| JournalAbbreviationRepository freshRepository = JournalAbbreviationRepositoryManager.getInstance() | ||
| .getRepository(journalAbbreviationPreferences); | ||
|
|
||
| Map<String, Boolean> sourceEnabledStates = new HashMap<>(); |
There was a problem hiding this comment.
Why do you collect a Map instead of just using freshRepository.isSourceEnabled()?
There was a problem hiding this comment.
Oh yes, isSourceEnabled(sourceKey) method already does the same thing. I just made the changes in 9c3d8cc :)
| } | ||
|
|
||
| var allAbbreviationsWithSources = freshRepository.getAllAbbreviationsWithSources(); | ||
| Map<String, List<JournalAbbreviationRepository.AbbreviationWithSource>> textToSourceMap = new HashMap<>(); |
There was a problem hiding this comment.
Isn't abbreviation repository is already kinda a Map? Isn't there a method for getting abbreviations/unabbreviations from there
There was a problem hiding this comment.
I get it now, I should use isAbbreviatedName() to check if the text is an abbreviated form and getForUnabbreviation() to get an abbreviation object only if it comes from an enabled source. I removed the redundant textToSourceMap mapping. The entire code changes are found in 0cf60ea :)
|
|
||
| String text = entry.getFieldLatexFree(journalField).orElse(""); | ||
| List<JournalAbbreviationRepository.AbbreviationWithSource> possibleSources = | ||
| textToSourceMap.getOrDefault(text, List.of()); |
There was a problem hiding this comment.
This is what I talked about in the previous comment. Doesn't abbreviation repository doesn't have some kind of get method?
There was a problem hiding this comment.
Yes, this was addressed in 0cf60ea by using the repository's built-in methods (getForUnabbreviation) instead of building and managing our own mapping :)
| * @return true if the source is enabled or has no explicit state (default is enabled) | ||
| */ | ||
| public boolean isSourceEnabled(String sourcePath) { | ||
| if (sourcePath == null) { |
There was a problem hiding this comment.
I think it's better to remove these null checks.
Currently, in JabRef, we don't use nulls, and if a parameter is not marked as @Nullable, then we assume it's always non-null.
Otherwise, if some code passes null there, then there is a flaw. As nulls shouldn't be used
There was a problem hiding this comment.
Thank you for your guidance! I removed null checks in eb07892. :)
| private final ObservableList<String> externalJournalLists; | ||
| private final BooleanProperty useFJournalField; | ||
|
|
||
| private final Map<String, Boolean> enabledExternalLists = new HashMap<>(); | ||
| private final BooleanProperty enabledListsChanged = new SimpleBooleanProperty(); |
There was a problem hiding this comment.
For which reasons this is used? Adding listener to ObservableMap doesn't work?
I remember I had problems with ObservableMaps. But if you can add listener there, and it works, you should use this functionality of JavaFX, not a separate property.
Otherwise, add a comment explaining this
There was a problem hiding this comment.
I have implemented your suggestion to use an ObservableMap instead of a regular HashMap, while keeping the separate change tracking property.
I feel that the separate property is necessary because, as you mentioned, ObservableMap listeners can be unreliable, especially when clearing and re-adding multiple entries at once (which happens in setEnabledExternalLists) as well as using with UI bindings that need to be updated consistently.
Therefore, I added a comment to explain this rationale directly in the code. This approach gives us the best of both worlds (i.e., we use JavaFX's observable collections while ensuring reliable change notification through the separate property).
The code changes can be found in c8b9b51.
| return; | ||
| } | ||
| enabledExternalLists.put(sourcePath, enabled); | ||
| enabledListsChanged.set(!enabledListsChanged.get()); |
There was a problem hiding this comment.
What does this line do?
From the name of enabledListsChanged it should be true when lists are changed. setting mutates data, so any call to setters should set enabledListsChanged to true, shouldn't it?
There was a problem hiding this comment.
Oh, maybe you toggle states not to indicate whether something change, but to rather trigger listeners of this property on any change?
Then this explains why X <= !X.
I would propose to:
- Add a comment explaining this technique.
- Move out
enabledListsChanged.set(!enabledListsChanged.get())into a method.
There was a problem hiding this comment.
Yes! I just incorporated your proposed changes to
- Add a comment explaining this technique.
- Move out enabledListsChanged.set(!enabledListsChanged.get()) into a method.
The code changes can be found in f58543b. :)
| public JournalAbbreviationRepository getRepository(JournalAbbreviationPreferences preferences) { | ||
| Objects.requireNonNull(preferences); | ||
|
|
||
| LOCK.readLock().lock(); |
There was a problem hiding this comment.
Typically, we don't use raw locks in JabRef. My experience in parallel/asynchronous programming in Java is not big, but are there any other synchronization methods? I would prefer something like a synchronized method or Atomic... classes. At least Mutex.
And also why this method should be atomic?
There was a problem hiding this comment.
Regarding the locks in the now-deleted JournalAbbreviationRepositoryManager, I have removed that class as suggested earlier, including the read-write locks, so no code changes are to be made for this code review comment.
The singleton manager was originally added to provide thread-safe caching across multiple instances. However, the current approach uses instance-level caching in AbbreviateAction instead, making thread synchronization unnecessary because each action instance has its own cache, repository creation is handled within the action instance and these instances are not shared across threads in the current design.
If I need shared caching again in future, I will be sure to remember and follow your suggestion to use more idiomatic synchronization mechanisms such as synchronized methods or Atomic classes rather than raw locks. Thank you! :)
| * @param preferences The current preferences to check | ||
| * @return true if preferences have changed, false otherwise | ||
| */ | ||
| private boolean preferencesChanged(JournalAbbreviationPreferences preferences) { |
There was a problem hiding this comment.
Is there a reason that the repository should be rebuilt so lazily?
Would it be problematic to rebuild repositories on clicking "Save" in preferences?
There was a problem hiding this comment.
Regarding lazy repository rebuilding and whether we could instead rebuild when clicking "Save" in preferences, the current implementation in AbbreviateAction does both by 1) using lazy loading to check if preferences have changed since the last repository creation and 2) responding to preference changes that happen when users click "Save" in the preferences dialog.
I did lazy checking because multiple actions might need the repository between preference saves, preferences could change programmatically outside the preferences dialog and the lightweight comparison in preferencesChanged() is much faster than always rebuilding the repository.
When users click "Save" in the preferences dialog, a new repository is already created and injected globally via Injector.setModelOrService(), which our cached repository will detect on the next lookup.
Hence, the implementation is both efficient (using caching) and responsive to user changes (checking for preference changes on each use). May I ask if we can keep the current approach or switch to a better approach?
@calixtus Thank you for your constructive feedback. I understand your concerns. I think I complicated things unnecessarily by creating a separate manager. When you said "the repository provides everything the application needs," it then clicked that I should have simply enhanced the repository class itself. (cc @InAnYan) The original problem I was trying to solve was the inefficient creation of new repositories on every operation: JournalAbbreviationRepository freshRepository = JournalAbbreviationLoader.loadRepository(journalAbbreviationPreferences);This is suboptimal because of expensive I/O (i.e., repeatedly loading data from files), memory churn (i.e., create new objects for every operation) and no caching of previously loaded repositories. Afterwards, I created This is still suboptimal because:
If my understanding is fully correct now, can you please advise if I should quickly do the following?
In short, Instead of: The repository should offer: This way, the repository itself will handle when to reload based on preference changes, without needing extra layers. Does this approach sound right? I appreciate the guidance and am eager to improve the implementation. |
This is the state it should be, maybe not it is in now. |
Thank you so much for your guidance, @calixtus. I understand the issue (i.e., the manager class adds complexity without adding value) now completely and will work on letting the repository handle it internally, following the principle of "the simplest solution that works." :) |
|
Your code currently does not meet JabRef's code guidelines. We use Checkstyle to identify issues. You can see which checks are failing by locating the box "Some checks were not successful" on the pull request page. To see the test output, locate "Tests / Checkstyle (pull_request)" and click on it. In case of issues with the import order, double check that you activated Auto Import. You can trigger fixing imports by pressing Ctrl+Alt+O to trigger Optimize Imports. Please carefully follow the setup guide for the codestyle. Afterwards, please run checkstyle locally and fix the issues, commit, and push. |
zikunz
force-pushed
the
feature/journal-abbreviation-toggle
branch
from
646b657 to
50c3549
Compare
The project has been reorganized into a multi-module structure, I will probably resolve merge conflicts later on to adapt to this new modular structure. Latest commit to attempt to resolve merge conflicts removed. |
…tion.java to JournalAbbreviationPreferences.java
… repository's isSourceEnabled method
…y's built-in methods
…n> and add comment for keeping enabledListsChanged
…per method notifyChange()
…c/main/java/org/jabref/gui/journals/AbbreviateAction.java
|
@trag-bot didn't find any issues in the code! ✅✨ |
|
Your pull request conflicts with the target branch. Please merge |
|
@InAnYan Thank you again for your code review! I just replied to all your comments and made relevant code changes. I will resolve the conversations if all the problems were indeed fixed. Could you please take a look at the current state of the PR when you have time? Thanks! :) |
Meanwhile, I will find some time to resolve conflicts to adapt to the multi-module build and keep moving forward. |
|
@zikunz hey, please resolve the merge conflicts before requesting a review, so that we can review the latest state of files |
Toggle Functionality for Journal Abbreviation Lists
Closes #12468.
Documentation repo's issue can be found at JabRef/user-documentation#560
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 and re-enable the built-in list:
If the built-in list is disabled, journal names found in the built-in list will not be abbreviated or unabbreviated:
In addition, external CSV files can be disabled or enabled. Likewise, if an external CSV file is disabled, journal names found in the external CSV file will not be abbreviated or unabbreviated too.
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 #12468.
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)