Add toggle functionality for journal abbreviation lists #12912
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,10 @@ | ||
| package org.jabref.gui.journals; | ||
|
|
||
| import java.nio.file.Path; | ||
| import java.util.ArrayList; | ||
| import java.util.HashMap; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.function.Supplier; | ||
|
|
||
| import javax.swing.undo.UndoManager; | ||
|
|
@@ -12,13 +16,16 @@ | |
| import org.jabref.gui.actions.SimpleCommand; | ||
| import org.jabref.gui.actions.StandardActions; | ||
| import org.jabref.gui.undo.NamedCompound; | ||
| import org.jabref.logic.journals.Abbreviation; | ||
| import org.jabref.logic.journals.JournalAbbreviationPreferences; | ||
| import org.jabref.logic.journals.JournalAbbreviationRepository; | ||
| import org.jabref.logic.journals.JournalAbbreviationRepositoryManager; | ||
| import org.jabref.logic.l10n.Localization; | ||
| import org.jabref.logic.util.BackgroundTask; | ||
| import org.jabref.logic.util.TaskExecutor; | ||
| import org.jabref.model.database.BibDatabaseContext; | ||
| import org.jabref.model.entry.BibEntry; | ||
| import org.jabref.model.entry.field.Field; | ||
| import org.jabref.model.entry.field.FieldFactory; | ||
|
|
||
| import org.slf4j.Logger; | ||
|
|
@@ -36,7 +43,7 @@ public class AbbreviateAction extends SimpleCommand { | |
| private final DialogService dialogService; | ||
| private final StateManager stateManager; | ||
| private final JournalAbbreviationPreferences journalAbbreviationPreferences; | ||
| private JournalAbbreviationRepository abbreviationRepository; | ||
| private final TaskExecutor taskExecutor; | ||
| private final UndoManager undoManager; | ||
|
|
||
|
|
@@ -47,15 +54,13 @@ public AbbreviateAction(StandardActions action, | |
| DialogService dialogService, | ||
| StateManager stateManager, | ||
| JournalAbbreviationPreferences abbreviationPreferences, | ||
| TaskExecutor taskExecutor, | ||
| UndoManager undoManager) { | ||
| this.action = action; | ||
| this.tabSupplier = tabSupplier; | ||
| this.dialogService = dialogService; | ||
| this.stateManager = stateManager; | ||
| this.journalAbbreviationPreferences = abbreviationPreferences; | ||
| this.taskExecutor = taskExecutor; | ||
| this.undoManager = undoManager; | ||
|
|
||
|
|
@@ -77,6 +82,11 @@ public AbbreviateAction(StandardActions action, | |
|
|
||
| @Override | ||
| public void execute() { | ||
| if (action == StandardActions.UNABBREVIATE && !areAnyJournalSourcesEnabled()) { | ||
| dialogService.notify(Localization.lang("Cannot unabbreviate: all journal lists are disabled.")); | ||
| return; | ||
| } | ||
|
|
||
| if ((action == StandardActions.ABBREVIATE_DEFAULT) | ||
| || (action == StandardActions.ABBREVIATE_DOTLESS) | ||
| || (action == StandardActions.ABBREVIATE_SHORTEST_UNIQUE) | ||
|
|
@@ -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.
Typically, in JabRef for dependencies, we do this:
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:
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 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! |
||
| .getRepository(journalAbbreviationPreferences); | ||
|
|
||
| UndoableAbbreviator undoableAbbreviator = new UndoableAbbreviator( | ||
| abbreviationRepository, | ||
| abbreviationType, | ||
|
|
@@ -119,13 +133,78 @@ private String abbreviate(BibDatabaseContext databaseContext, List<BibEntry> ent | |
| return Localization.lang("Abbreviated %0 journal names.", String.valueOf(count)); | ||
| } | ||
|
|
||
| /** | ||
| * Unabbreviate journal names in entries, respecting the enabled/disabled state of sources. | ||
| * Only unabbreviates entries from enabled sources. | ||
| */ | ||
| private String unabbreviate(BibDatabaseContext databaseContext, List<BibEntry> entries) { | ||
| List<BibEntry> filteredEntries = new ArrayList<>(); | ||
zikunz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| JournalAbbreviationRepository freshRepository = JournalAbbreviationRepositoryManager.getInstance() | ||
| .getRepository(journalAbbreviationPreferences); | ||
|
|
||
| Map<String, Boolean> sourceEnabledStates = new HashMap<>(); | ||
|
There was a problem hiding this comment. Why do you collect a There was a problem hiding this comment. Oh yes, |
||
| String builtInId = JournalAbbreviationRepository.BUILTIN_LIST_ID; | ||
| sourceEnabledStates.put(builtInId, journalAbbreviationPreferences.isSourceEnabled(builtInId)); | ||
|
|
||
| for (String listPath : journalAbbreviationPreferences.getExternalJournalLists()) { | ||
| if (listPath != null && !listPath.isBlank()) { | ||
| String fileName = Path.of(listPath).getFileName().toString(); | ||
| sourceEnabledStates.put(fileName, journalAbbreviationPreferences.isSourceEnabled(fileName)); | ||
| } | ||
| } | ||
|
|
||
| var allAbbreviationsWithSources = freshRepository.getAllAbbreviationsWithSources(); | ||
| Map<String, List<JournalAbbreviationRepository.AbbreviationWithSource>> textToSourceMap = new HashMap<>(); | ||
InAnYan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| for (var abbrWithSource : allAbbreviationsWithSources) { | ||
| Abbreviation abbr = abbrWithSource.getAbbreviation(); | ||
| addToMap(textToSourceMap, abbr.getName(), abbrWithSource); | ||
| addToMap(textToSourceMap, abbr.getAbbreviation(), abbrWithSource); | ||
| addToMap(textToSourceMap, abbr.getDotlessAbbreviation(), abbrWithSource); | ||
| addToMap(textToSourceMap, abbr.getShortestUniqueAbbreviation(), abbrWithSource); | ||
| } | ||
|
|
||
| for (BibEntry entry : entries) { | ||
| boolean includeEntry = true; | ||
|
|
||
| for (Field journalField : FieldFactory.getJournalNameFields()) { | ||
| if (!entry.hasField(journalField)) { | ||
| continue; | ||
| } | ||
|
|
||
| String text = entry.getFieldLatexFree(journalField).orElse(""); | ||
| List<JournalAbbreviationRepository.AbbreviationWithSource> possibleSources = | ||
| textToSourceMap.getOrDefault(text, List.of()); | ||
InAnYan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| if (!possibleSources.isEmpty()) { | ||
| boolean allSourcesDisabled = true; | ||
| for (var abbrWithSource : possibleSources) { | ||
| String source = abbrWithSource.getSource(); | ||
| if (sourceEnabledStates.getOrDefault(source, true)) { | ||
| allSourcesDisabled = false; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (allSourcesDisabled) { | ||
| includeEntry = false; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (includeEntry) { | ||
| filteredEntries.add(entry); | ||
| } | ||
| } | ||
|
|
||
| UndoableUnabbreviator undoableAbbreviator = new UndoableUnabbreviator(freshRepository); | ||
| NamedCompound ce = new NamedCompound(Localization.lang("Unabbreviate journal names")); | ||
| int count = filteredEntries.stream().mapToInt(entry -> | ||
| (int) FieldFactory.getJournalNameFields().stream().filter(journalField -> | ||
| undoableAbbreviator.unabbreviate(databaseContext.getDatabase(), entry, journalField, ce)).count()).sum(); | ||
|
|
||
| if (count == 0) { | ||
| return Localization.lang("No journal names could be unabbreviated."); | ||
| } | ||
|
|
@@ -135,4 +214,46 @@ private String unabbreviate(BibDatabaseContext databaseContext, List<BibEntry> e | |
| tabSupplier.get().markBaseChanged(); | ||
| return Localization.lang("Unabbreviated %0 journal names.", String.valueOf(count)); | ||
| } | ||
|
|
||
| /** | ||
| * Helper method to add an abbreviation to the text-to-source map | ||
| * | ||
| * @param map The map to add to | ||
| * @param text The text to use as key | ||
| * @param abbrWithSource The abbreviation with source to add | ||
| */ | ||
| private void addToMap(Map<String, List<JournalAbbreviationRepository.AbbreviationWithSource>> map, | ||
| String text, | ||
| JournalAbbreviationRepository.AbbreviationWithSource abbrWithSource) { | ||
| if (text == null || text.isBlank()) { | ||
zikunz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return; | ||
| } | ||
|
|
||
| map.computeIfAbsent(text, k -> new ArrayList<>()).add(abbrWithSource); | ||
| } | ||
|
|
||
| /** | ||
| * Checks if any journal abbreviation source is enabled in the preferences. | ||
| * This includes both the built-in list and any external journal lists. | ||
| * | ||
| * @return true if at least one source is enabled, false if all sources are disabled | ||
| */ | ||
| private boolean areAnyJournalSourcesEnabled() { | ||
InAnYan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| boolean anySourceEnabled = journalAbbreviationPreferences.isSourceEnabled(JournalAbbreviationRepository.BUILTIN_LIST_ID); | ||
|
|
||
| if (!anySourceEnabled) { | ||
| for (String listPath : journalAbbreviationPreferences.getExternalJournalLists()) { | ||
| if (listPath != null && !listPath.isBlank()) { | ||
| // Just check the filename since that's what's used as the source key | ||
| String fileName = Path.of(listPath).getFileName().toString(); | ||
| if (journalAbbreviationPreferences.isSourceEnabled(fileName)) { | ||
| anySourceEnabled = true; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return anySourceEnabled; | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. I've seen that you removed line https://github.com/JabRef/jabref/pull/12912/files#diff-3fa252360fc27ea81a62af5e94c9a8263b2519ca8845cd50c55d0a001f334e41L123 -- is this class still used anywhere? There was a problem hiding this comment. Yes, but this class is still being used. I have simplified the filtering logic to determine which entries to process, but the actual unabbreviation is still performed by UndoableUnabbreviator undoableAbbreviator = new UndoableUnabbreviator(freshRepository);
NamedCompound ce = new NamedCompound(Localization.lang("Unabbreviate journal names"));
int count = filteredEntries.stream().mapToInt(entry ->
(int) FieldFactory.getJournalNameFields().stream().filter(journalField ->
undoableAbbreviator.unabbreviate(databaseContext.getDatabase(), entry, journalField, ce)).count()).sum();There are also tests that verify its functionality. This class is responsible for the actual unabbreviation and undo operations, while my changes simplified how we determine which entries to process. The class is still essential for the functionality to work correctly. May I ask whether this sounds right? |
Uh oh!
There was an error while loading. Please reload this page.