Implement mass addition of bib information #12025
Conversation
…rences, attach file, send entries, special fields) and improved layout
…D_ENTRIES - Reintroduce previously removed MERGE_WITH_FETCHED_ENTRY action - Add new BATCH_MERGE_WITH_FETCHED_ENTRIES action after it - Fixes unintended removal of single-entry merge functionality
- Improve code structure and reduce duplication - Enhance error handling and user feedback - Streamline fetch and merge process for better performance - Implement more robust field handling and merging logic - Optimize batch processing capabilities - Refactor to use Java 8+ features for cleaner code - Improve modularity and separation of concerns - Reduce unnecessary notifications
|
I dont see any new test case. It should be possible to add at least one test case! |
|
@koppor thank you for the comment. I'm also part of the team responsible for working on this issue. From what I know, no tests have been written for the |
There was a problem hiding this comment.
The functionaliy needs to be in the "Lookup" menu. Also add "Get bibliographic data" to there.
The title "Mass getting" sounds too technical.
Rename to "Get bibliographic data from DOI/ISBN/... (fully automated)" to make clear that no GUI interaction is performed.
Can you use
new OrFields(StandardField.DOI, StandardField.ISBN, StandardField.EPRINT).getDisplayName()
instead of DOI/ISSN/...
This would make it consistent to org.jabref.gui.mergeentries.MergeWithFetchedEntryAction#execute
Replace
new OrFields(StandardField.DOI, StandardField.ISBN, StandardField.EPRINT).getDisplayName()
by
new OrFields(FetchAndMergeEntry.SUPPORTED_FIELDS)
CHANGELOG.md
Outdated
| @@ -38,6 +38,7 @@ Note that this project **does not** adhere to [Semantic Versioning](https://semv | |||
| - We added automatic browser extension install on Windows for Chrome and Edge. [#6076](https://github.com/JabRef/jabref/issues/6076) | |||
| - We added a search bar for filtering keyboard shortcuts. [#11686](https://github.com/JabRef/jabref/issues/11686) | |||
| - By double clicking on a local citation in the Citation Relations Tab you can now jump the linked entry. [#11955](https://github.com/JabRef/jabref/pull/11955) | |||
| - Added functionality for mass addition of bibliographic information for multiple entries. [#372](https://github.com/JabRef/jabref/issues/372) | |||
There was a problem hiding this comment.
Please add where you added it.
It really needs to be in the "Lookup" menu.
We want to keep the right click menu small.
There was a problem hiding this comment.
| - Added functionality for mass addition of bibliographic information for multiple entries. [#372](https://github.com/JabRef/jabref/issues/372) | |
| - Added functionality for mass addition of bibliographic information for multiple entries to the "Lookup" menu. [#372](https://github.com/JabRef/jabref/issues/372) |
| () -> { | ||
| if (hasAnySupportedField(entry)) { | ||
| dialogService.notify(Localization.lang("No %0 found", field.getDisplayName())); | ||
| } |
There was a problem hiding this comment.
It is unclear why this function is there.
I think, this can just be removed. No need to inform the user.
Moreover, the logic is wrong. -- It is OK if the entry has an identifier field
There was a problem hiding this comment.
We also can use .map and .flatMap on optionals. I believe we can make the logic clearer by using those.
| @@ -92,7 +93,8 @@ public static ContextMenu create(BibEntryTableViewModel entry, | |||
| new SeparatorMenuItem(), | |||
|
|
|||
| new ChangeEntryTypeMenu(libraryTab.getSelectedEntries(), libraryTab.getBibDatabaseContext(), undoManager, entryTypesManager).asSubMenu(), | |||
| factory.createMenuItem(StandardActions.MERGE_WITH_FETCHED_ENTRY, new MergeWithFetchedEntryAction(dialogService, stateManager, taskExecutor, preferences, undoManager)) | |||
| factory.createMenuItem(StandardActions.MERGE_WITH_FETCHED_ENTRY, new MergeWithFetchedEntryAction(dialogService, stateManager, taskExecutor, preferences, undoManager)), | |||
| factory.createMenuItem(StandardActions.BATCH_MERGE_WITH_FETCHED_ENTRIES, new MultiEntryMergeWithFetchedDataAction(libraryTab, preferences, taskExecutor, libraryTab.getBibDatabaseContext(), dialogService, undoManager)) | |||
There was a problem hiding this comment.
libraryTab is already passed, thus you can call libraryTab.getBibDatabaseContext() inside the method.
| } | ||
|
|
||
| public void fetchAndMergeBatch(List<BibEntry> entries) { | ||
| entries.forEach(entry -> SUPPORTED_FIELDS.forEach(field -> fetchAndMergeEntry(entry, field))); |
There was a problem hiding this comment.
Rename variable SUPPORTED_FIELDS to SUPPORTED_IDENTIFIER_FIELDS.
| } | ||
|
|
||
| private void handleFetchException(Exception exception, IdBasedFetcher fetcher, BibEntry entry) { | ||
| if (hasAnySupportedField(entry)) { |
There was a problem hiding this comment.
I think, this is not required. The id fetcher should only be called if the id is present. If id present, the exception is shown.
--> remove this if (and remove hasAnySupportedField alltogether)
| // Set of fields present in both the original and fetched entries | ||
| Set<Field> jointFields = new TreeSet<>(Comparator.comparing(Field::getName)); | ||
| jointFields.addAll(fetchedEntry.getFields()); | ||
| Set<Field> originalFields = new TreeSet<>(Comparator.comparing(Field::getName)); | ||
| originalFields.addAll(originalEntry.getFields()); |
There was a problem hiding this comment.
This is duplicated coude. Please refactor to a method.
| if (edited) { | ||
| ce.end(); | ||
| undoManager.addEdit(ce); | ||
| dialogService.notify(Localization.lang("Updated entry with fetched information")); |
There was a problem hiding this comment.
Add the citation key to this information - otherwise it is unclear which entry is used.
Use getCitationKey().orElse(getAuthorTitleYear()
Without any example, I cannot say anything. I think, it could be hard. You first need to factor out Second step is to make use of With that refactoring, the whole code can be moved to |
| private void executeFetchTask(IdBasedFetcher fetcher, Field field, String fieldContent, BibEntry entry) { | ||
| BackgroundTask.wrap(() -> fetcher.performSearchById(fieldContent)) |
There was a problem hiding this comment.
We don't need the fieldContent parameter here as it can be taken from the entry. Ideally we want to keep the number of parameters passed to a method to a minimum to achieve cleaner code.
| () -> { | ||
| if (hasAnySupportedField(entry)) { | ||
| dialogService.notify(Localization.lang("No %0 found", field.getDisplayName())); | ||
| } |
There was a problem hiding this comment.
We also can use .map and .flatMap on optionals. I believe we can make the logic clearer by using those.
| if (!oldType.equals(newType)) { | ||
| originalEntry.setType(newType); | ||
| ce.addEdit(new UndoableChangeType(originalEntry, oldType, newType)); | ||
| edited = true; |
There was a problem hiding this comment.
I don't think we need the edited boolean flag anymore, we can use NamedCompound#hasEdits.
| Optional<String> originalString = originalEntry.getField(field); | ||
| Optional<String> fetchedString = fetchedEntry.getField(field); | ||
|
|
||
| if (fetchedString.isPresent()) { |
There was a problem hiding this comment.
Let's reduce nesting and return early here.
| } | ||
|
|
||
| private boolean isEdited(BibEntry originalEntry, NamedCompound ce, Set<Field> jointFields, Set<Field> originalFields, boolean edited) { | ||
| for (Field field : originalFields) { |
There was a problem hiding this comment.
Do we need to pass the edited boolean here? We want to minimize the number of parameters.
There was a problem hiding this comment.
The method is named isEdited suggesting a read-only behavior but it can clear some fields, this is not ideal...
| public void execute() { | ||
| List<BibEntry> selectedEntries = libraryTab.getSelectedEntries(); | ||
|
|
||
| // Create an instance of FetchAndMergeEntry |
There was a problem hiding this comment.
Not a very useful comment 😅
HoussemNasri
added
the
status: changes-required
- Relocate action from Right-click menu to "Lookup" menu for consistency - Rename action from "Mass Getting" to "Get bibliographic data from DOI/ISBN/... (fully automated)" - Other modifications not finished yet
…ability - Rename SUPPORTED_FIELDS to SUPPORTED_IDENTIFIER_FIELDS for clarity - Remove unnecessary hasAnySupportedField method and associated checks - Refactor duplicate code into utility methods (getFields, getJointFields) - Improve error handling with more specific exception messages - Remove edited boolean flag in favor of NamedCompound#hasEdits() - Enhance use of Optional, Stream, and method references - Reduce method parameters for better maintainability - Improve user notifications with entry citation keys - Restructure methods to separate concerns more effectively Minor modifications to MergeWithFetchedEntryAction.java to align with the new version of FetchAndMergeEntry.java
BojiZhang
force-pushed
the
fix-for-issue-#372
branch
from
93a9a8d to
74f2473
Compare
CHANGELOG.md
Outdated
| @@ -38,6 +38,7 @@ Note that this project **does not** adhere to [Semantic Versioning](https://semv | |||
| - We added automatic browser extension install on Windows for Chrome and Edge. [#6076](https://github.com/JabRef/jabref/issues/6076) | |||
| - We added a search bar for filtering keyboard shortcuts. [#11686](https://github.com/JabRef/jabref/issues/11686) | |||
| - By double clicking on a local citation in the Citation Relations Tab you can now jump the linked entry. [#11955](https://github.com/JabRef/jabref/pull/11955) | |||
| - Added functionality for mass addition of bibliographic information for multiple entries. [#372](https://github.com/JabRef/jabref/issues/372) | |||
There was a problem hiding this comment.
| - Added functionality for mass addition of bibliographic information for multiple entries. [#372](https://github.com/JabRef/jabref/issues/372) | |
| - Added functionality for mass addition of bibliographic information for multiple entries to the "Lookup" menu. [#372](https://github.com/JabRef/jabref/issues/372) |
|
|
||
| new SeparatorMenuItem(), | ||
|
|
||
| // Adding new menu item for mass bibliographic data fetching |
| dialogService.notify(message); | ||
| } | ||
|
|
||
| private Set<Field> getFields(BibEntry entry) { |
There was a problem hiding this comment.
New Java21: SortedSet. Otherwise, it is not clear that the returned setis sorted.
There was a problem hiding this comment.
However, the sorting is not necessary at all if I investigate the calling methods.
| } | ||
|
|
||
| private void mergeWithoutDialog(BibEntry originalEntry, BibEntry fetchedEntry) { | ||
| NamedCompound ce = new NamedCompound(Localization.lang("Merge entry without user interaction")); |
There was a problem hiding this comment.
Two entries are morged, not a single one.
| NamedCompound ce = new NamedCompound(Localization.lang("Merge entry without user interaction")); | |
| NamedCompound ce = new NamedCompound(Localization.lang("Merge entries without user interaction")); |
| } | ||
| } | ||
|
|
||
| private void mergeWithoutDialog(BibEntry originalEntry, BibEntry fetchedEntry) { |
There was a problem hiding this comment.
This whole method can be go into logic, because there is no gui interaction - and it should be testable with a JUnit test.
Annote the test using JUnit
@AllowedToUseSwing("Requires undo/redo functionality")
There was a problem hiding this comment.
I apologize for the oversight; I should have provided my comments after pushing the changes. Over the past few hours, I have been working on optimizing the code further and transitioning the class into the logic directory.
My current progress is that '''
Refactor FetchAndMergeEntry:
The single-entry fetching and merging logic remains in FetchAndMergeEntry.
Extracted logic for batch processing and handling multiple entries from FetchAndMergeEntry.
Introduce MergingIdBasedFetcher:
Created the MergingIdBasedFetcher class in the org.jabref.logic.importer.fetcher package.
MergingIdBasedFetcher handles mass operations such as fetching and merging multiple BibEntry objects.
Uses NotificationService to provide error and status notifications, removing reliance on user confirmation dialogs for better automation.
Create MergeEntriesHelper:
Introduced the MergeEntriesHelper class to centralize and handle the actual merging logic between BibEntry instances.
The class includes methods to update entry types, fields, and remove obsolete fields during merging. '''
| ? Localization.lang("Updated entry with fetched information [%0]", citationKey) | ||
| : Localization.lang("No new information was added [%0]", citationKey); |
There was a problem hiding this comment.
Is this output for each entry?
Imagine a user updates 100 entries - are there 100 messages? How should the user understand this data?
Maybe better to have a single notification with: entries X udpates, where X is a list of the citation keys.
Even better it would be to have a magic JabRef group which is called "JR - updated", where all modified entries are added to. Before the action, all entries are removed from this group. (Not sure if you have time for that)
- Introduce `MergingIdBasedFetcher` for batch operations - Create `MergeEntriesHelper` to encapsulate merge logic - Refactor `FetchAndMergeEntry` for improved separation of concerns - Implement `getFetcherForField` method in `MergingIdBasedFetcher` - Add background processing with progress updates - Enhance user notifications and error handling - Optimize logging
| @@ -1679,6 +1679,10 @@ No\ %0\ found=No %0 found | |||
| Entry\ from\ %0=Entry from %0 | |||
| Merge\ entry\ with\ %0\ information=Merge entry with %0 information | |||
| Updated\ entry\ with\ info\ from\ %0=Updated entry with info from %0 | |||
| Mass\ getting\ bibliographic\ data\ from\ %0=Mass getting bibliographic data from %0 | |||
There was a problem hiding this comment.
I think, this string should get obsoelete
|
@KUMOAWAI I think, you do see that tests are failing? https://github.com/JabRef/jabref/actions/runs/11426189758/job/31788821543?pr=12025 |
Yes, I will make an effort to resolve the issues |
Hello Koppor, I've been investigating the failing tests in MainArchitectureTest and testsAreIndependentOfGuiPreferences. The core issue is that the MergingIdBasedFetcher class is now in the logic layer, while it has dependencies on GUI components like DialogService and GuiPreferences. These dependencies violate our current architectural rules but seem integral to the system's functionality. I'm finding it challenging to refactor without potentially breaking existing features. Could we consider:
Thank you for your time. |
| return BackgroundTask.wrap(() -> List.of()); | ||
| } | ||
|
|
||
| BackgroundTask<List<String>> backgroundTask = new BackgroundTask<>() { |
There was a problem hiding this comment.
The fetcher itself should not have UI dependencies and implement the background task, that is a UI related component. This has to be done on the caller SIde which I presume is somewhere in the UI
There was a problem hiding this comment.
Thank you for the feedback!
Will refactor to move UI dependencies to the caller side and update the PR in these two days.
There was a problem hiding this comment.
Yeah, the wrapping can be done in the caller.
| import org.jabref.model.entry.field.FieldFactory; | ||
| import org.jabref.model.entry.types.EntryType; | ||
|
|
||
| public class MergeEntriesHelper { |
There was a problem hiding this comment.
Isn't @AllowedToUseSwing missing as I wrote before in the reivew comments?
There was a problem hiding this comment.
I need to admit that when I saw your earlier comment about moving "this whole method can go into logic", I didn't truly understand the full implications. I thought simply moving the method to the logic layer was sufficient, and missed the importance of the "@AllowedToUseSwing". I'll remove the Swing dependency and update the code later. Sorry for my oversight.
There was a problem hiding this comment.
I'll remove the Swing dependency
Please think a bit. I think, i posted the following example:
@AllowedToUseSwing("UndoableUnabbreviator and UndoableAbbreviator requires Swing Compound Edit in order test the abbreviation and unabbreviation of journal titles")
You need undo/redo. This is OK then.
Only not OK is the background task.
| private final BibDatabaseContext bibDatabaseContext; | ||
| private final DialogService dialogService; | ||
|
|
||
| public MergingIdBasedFetcher(GuiPreferences preferences, UndoManager undoManager, BibDatabaseContext bibDatabaseContext, DialogService dialogService) { |
There was a problem hiding this comment.
I think, I wrote to use NotificationService here. This is in logic, whereas DialogService is in GUI.
There was a problem hiding this comment.
Sorry for my oversight. I did notice your previous comment about using NotificationService and tried to implement it. However, I had difficulties implementing the feature without implementing the abstract class, so I kept using DialogService as it was already working. I understand now this breaks the architectural rules and will work on properly implementing NotificationService instead. Sorry for wasting your time again.
There was a problem hiding this comment.
I don't know about your concrete difficulties. - If you just change the parameter type here into NotificationService, it should work. You are only using the notify method.
There was a problem hiding this comment.
Thank you for the explanation! I actually fixed it after my previous comment just now. I was confused cause I did not find NotificationService instance in the MainMenu class. And I was so exhausted for refactoring all the codes at that time, so I did not notice that I just need to pass DialogService instance. Sorry for the confusion in my earlier messages.
|
@KUMOAWAI The unit tests fail. Please check. Current link: https://github.com/JabRef/jabref/actions/runs/11426189758/job/31788821543?pr=12025 |
![trag-bot[bot]](https://avatars.githubusercontent.com/in/856808?s=60&v=4){kind=small}
|
|
||
| import org.junit.jupiter.api.Test; | ||
| import org.junit.jupiter.params.ParameterizedTest; | ||
| import org.junit.jupiter.params.provider.CsvSource; |
There was a problem hiding this comment.
The use of CsvSource in parameterized tests is acceptable, but using Arguments.of() would be a more modern Java best practice, improving readability and maintainability.
Co-authored-by: Oliver Kopp <[email protected]>
Co-authored-by: Oliver Kopp <[email protected]>
Co-authored-by: Oliver Kopp <[email protected]>
jabgui/src/main/java/org/jabref/gui/mergeentries/BatchEntryMergeTask.java
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/mergeentries/BatchEntryMergeTask.java
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/mergeentries/BatchEntryMergeTask.java
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/mergeentries/BatchEntryMergeTask.java
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/mergeentries/BatchEntryMergeWithFetchedDataAction.java
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/mergeentries/BatchEntryMergeWithFetchedDataAction.java
Show resolved
Hide resolved
jabgui/src/main/java/org/jabref/gui/mergeentries/MergeEntriesHelper.java
Show resolved
Hide resolved
jablib/src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java
Show resolved
Hide resolved
Co-authored-by: Oliver Kopp <[email protected]>
|
@trag-bot didn't find any issues in the code! ✅✨ |
|
Note that your PR will not be reviewed/accepted until you have gone through the mandatory checks in the description and marked each of them them exactly in the format of |
subhramit
changed the title
Closes https://github.com/koppor/jabref/issues/372
Closes #12275
Description
I am a teammate with @Arvinyuchen, who was assigned to issue #12275.
This pull request addresses issue #12275 by implementing mass addition of bibliographic information for multiple entries, while also improving the existing single-entry functionality.
Changes made:
Added new classes for mass bibliographic updates:
MergingIdBasedFetcherfor handling batch entry processingMergeEntriesHelperfor managing merge operationsMultiEntryMergeWithFetchedDataActionfor coordinating bulk updatesFeatures of mass update system:
Improved existing functionality:
FetchAndMergeEntryclass for better single-entry handlingThis implementation follows the "Better implementation" approach suggested in the original issue, where popups are not shown for each entry, and the longer string is taken when merging fields.
Testing
Manual testing has been performed to verify functionality:
Batch processing of multiple entries
Field merging behavior
Error handling and user notifications
UI responsiveness during background operations
Automated tests will be added in future updates.
CI/CD Issues
Several CI/CD errors were encountered during the automated checks, but these appear to be unrelated to the changes made in this PR. The issues include:
Pages Deployment Issues: These seem to be related to the repository's GitHub Pages configuration and are likely not caused by our code changes.
Link Checking Failures: The
lycheeworkflow reported broken links in documentation files. These are pre-existing issues in the repository's documentation and not introduced by our changes.Gradle Fetcher Tests Failures: These failures appear to be due to deprecated functionality in the Gradle setup action and possibly inconsistent caching. Our changes do not directly interact with the Gradle configuration or the fetcher tests.
Jekyll Build Failures: These are occurring on both the
mainand our feature branch, suggesting a general issue with the Jekyll configuration rather than a problem introduced by our changes.Annotations and Deprecated Functions: Warnings about deprecated functionality in the Gradle setup are present, but these are part of the existing CI configuration and not related to our code changes.
We've thoroughly reviewed our changes and can confirm that they do not directly contribute to these CI/CD issues. These appear to be pre-existing problems in the repository's CI/CD setup or documentation. We would appreciate any guidance on how to proceed, given that these issues are beyond the scope of our feature implementation.
We're open to addressing any of these issues if guidance is provided on how they relate to our changes.
Notes for reviewers
Mandatory checks
CHANGELOG.mddescribed in a way that is understandable for the average user (if applicable)Following is the screenshot of the new action in the look up menu:
Following is the screenshot of the progress showed in the backgroud task view:
Following is the screenshot of cancellation notification:
Following is the screenshot of the result of running the new feature: