Implement mass addition of bib information (Closes #372) #12025

KUMOAWAI · 2024-10-19T07:47:00Z

Description

I am a teammate with @Arvinyuchen, who was assigned to issue #372.

This pull request addresses issue #372 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:
- MergingIdBasedFetcher for handling batch entry processing
- MergeEntriesHelper for managing merge operations
- MultiEntryMergeWithFetchedDataAction for coordinating bulk updates
Features of mass update system:
- Supports updating multiple entries simultaneously
- Smart field merging with "longer string wins" strategy
- Background processing with progress tracking
- Comprehensive error handling and user feedback
- Streamlined user notifications
Improved existing functionality:
- Refactored FetchAndMergeEntry class for better single-entry handling
- Enhanced error handling and logging
- Improved code structure
- Streamlined merge process

This 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 lychee workflow 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 main and 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

Please review the interaction between core components: FetchAndMergeEntry, MultiEntryMergeWithFetchedDataAction, MergingIdBasedFetcher, and MergeEntriesHelper
Focus on effectiveness of batch processing and field merging strategy
Verify error handling and user feedback mechanisms

Mandatory checks

I own the copyright of the code submitted and I licence it under the MIT license
Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

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 the result of running the new feature:

…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

koppor · 2024-10-19T08:15:05Z

I dont see any new test case. It should be possible to add at least one test case!

SaltedTan · 2024-10-19T11:16:47Z

@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 FetchAndMergeEntry class, which is also used in the implementation of the MultiEntryMergeWithFetchedDataAction aimed at fixing issue #372. This impeded our attempt in writing test cases for the newly added class. Furthermore, we also met some difficulties in writing the setUp() part of the tests, not sure what is the cause of this. Could you please provide some more information that could be helpful in writing the tests?

koppor

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)

koppor · 2024-10-19T11:55:38Z

CHANGELOG.md

@@ -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)


Please add where you added it.

It really needs to be in the "Lookup" menu.

We want to keep the right click menu small.

Suggested change

- 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)

koppor · 2024-10-19T13:45:26Z

src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java

+                () -> {
+                    if (hasAnySupportedField(entry)) {
+                        dialogService.notify(Localization.lang("No %0 found", field.getDisplayName()));
+                    }


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

We also can use .map and .flatMap on optionals. I believe we can make the logic clearer by using those.

koppor · 2024-10-19T13:49:48Z

src/main/java/org/jabref/gui/maintable/RightClickMenu.java

@@ -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))


libraryTab is already passed, thus you can call libraryTab.getBibDatabaseContext() inside the method.

koppor · 2024-10-19T13:50:20Z

src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java

+    }
+
+    public void fetchAndMergeBatch(List<BibEntry> entries) {
+        entries.forEach(entry -> SUPPORTED_FIELDS.forEach(field -> fetchAndMergeEntry(entry, field)));


Rename variable SUPPORTED_FIELDS to SUPPORTED_IDENTIFIER_FIELDS.

koppor · 2024-10-19T14:02:18Z

src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java

+    }
+
+    private void handleFetchException(Exception exception, IdBasedFetcher fetcher, BibEntry entry) {
+        if (hasAnySupportedField(entry)) {


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)

koppor · 2024-10-19T14:03:32Z

src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java

+        // 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());


This is duplicated coude. Please refactor to a method.

koppor · 2024-10-19T14:05:48Z

src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java

+        if (edited) {
+            ce.end();
+            undoManager.addEdit(ce);
+            dialogService.notify(Localization.lang("Updated entry with fetched information"));


Add the citation key to this information - otherwise it is unclear which entry is used.

Use getCitationKey().orElse(getAuthorTitleYear()

koppor · 2024-10-19T14:17:42Z

This impeded our attempt in writing test cases for the newly added class. Furthermore, we also met some difficulties in writing the setUp() part of the tests, not sure what is the cause of this.

Without any example, I cannot say anything.

I think, it could be hard. You first need to factor out WebFetchers.getIdBasedFetcherForField( of ´org.jabref.gui.mergeentries.FetchAndMergeEntry#fetchAndMerge(org.jabref.model.entry.BibEntry, java.util.List<org.jabref.model.entry.field.Field>). Maybe, for an initial refactoring, a simple Functioncan be passed which mimics the behavior ofWebFetchers.getIdBasedFetcherForField`. Then, a mock can be passed.

Second step is to make use of org.jabref.logic.util.NotificationService only. There should be no confirmation dialog if something goes wrong. The user can look into the log. (The current dialog does not allow to jump to an entry; thus it is not useful)

With that refactoring, the whole code can be moved to logic somewhere. Maybe org.jabref.logic.importer.fetcher and as name MergingIdBasedFetcher.

HoussemNasri · 2024-10-19T19:50:27Z

src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java

+    private void executeFetchTask(IdBasedFetcher fetcher, Field field, String fieldContent, BibEntry entry) {
+        BackgroundTask.wrap(() -> fetcher.performSearchById(fieldContent))


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.

HoussemNasri · 2024-10-19T19:53:40Z

src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java

+                () -> {
+                    if (hasAnySupportedField(entry)) {
+                        dialogService.notify(Localization.lang("No %0 found", field.getDisplayName()));
+                    }


We also can use .map and .flatMap on optionals. I believe we can make the logic clearer by using those.

HoussemNasri · 2024-10-19T20:02:07Z

src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java

+        if (!oldType.equals(newType)) {
+            originalEntry.setType(newType);
+            ce.addEdit(new UndoableChangeType(originalEntry, oldType, newType));
+            edited = true;


I don't think we need the edited boolean flag anymore, we can use NamedCompound#hasEdits.

HoussemNasri · 2024-10-19T20:03:14Z

src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java

+            Optional<String> originalString = originalEntry.getField(field);
+            Optional<String> fetchedString = fetchedEntry.getField(field);
+
+            if (fetchedString.isPresent()) {


Let's reduce nesting and return early here.

HoussemNasri · 2024-10-19T20:04:50Z

src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java

+    }
+
+    private boolean isEdited(BibEntry originalEntry, NamedCompound ce, Set<Field> jointFields, Set<Field> originalFields, boolean edited) {
+        for (Field field : originalFields) {


Do we need to pass the edited boolean here? We want to minimize the number of parameters.

The method is named isEdited suggesting a read-only behavior but it can clear some fields, this is not ideal...

HoussemNasri · 2024-10-19T20:13:01Z

src/main/java/org/jabref/gui/mergeentries/MultiEntryMergeWithFetchedDataAction.java

+    public void execute() {
+        List<BibEntry> selectedEntries = libraryTab.getSelectedEntries();
+
+        // Create an instance of FetchAndMergeEntry


Not a very useful comment 😅

- 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

koppor · 2024-10-20T07:30:33Z

CHANGELOG.md

@@ -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)


Suggested change

- 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)

koppor · 2024-10-20T07:30:48Z

src/main/java/org/jabref/gui/frame/MainMenu.java

+
+                new SeparatorMenuItem(),
+
+                // Adding new menu item for mass bibliographic data fetching


Not useful comment. Remove it.

koppor · 2024-10-20T07:35:36Z

src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java

+        dialogService.notify(message);
+    }
+
+    private Set<Field> getFields(BibEntry entry) {


New Java21: SortedSet. Otherwise, it is not clear that the returned setis sorted.

However, the sorting is not necessary at all if I investigate the calling methods.

koppor · 2024-10-20T07:41:18Z

src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java

+    }
+
+    private void mergeWithoutDialog(BibEntry originalEntry, BibEntry fetchedEntry) {
+        NamedCompound ce = new NamedCompound(Localization.lang("Merge entry without user interaction"));


Two entries are morged, not a single one.

Suggested change

NamedCompound ce = new NamedCompound(Localization.lang("Merge entry without user interaction"));

NamedCompound ce = new NamedCompound(Localization.lang("Merge entries without user interaction"));

koppor · 2024-10-20T07:44:26Z

src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java

+        }
+    }
+
+    private void mergeWithoutDialog(BibEntry originalEntry, BibEntry fetchedEntry) {


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")

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. '''

koppor · 2024-10-20T07:46:47Z

src/main/java/org/jabref/gui/mergeentries/FetchAndMergeEntry.java

+                ? Localization.lang("Updated entry with fetched information [%0]", citationKey)
+                : Localization.lang("No new information was added [%0]", citationKey);


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

koppor · 2024-10-20T18:44:48Z

src/main/resources/l10n/JabRef_en.properties

@@ -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


I think, this string should get obsoelete

koppor · 2024-10-20T18:45:20Z

@KUMOAWAI I think, you do see that tests are failing? https://github.com/JabRef/jabref/actions/runs/11426189758/job/31788821543?pr=12025

KUMOAWAI · 2024-10-21T02:02:44Z

@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

KUMOAWAI · 2024-10-21T03:11:29Z

@KUMOAWAI I think, you do see that tests are failing? https://github.com/JabRef/jabref/actions/runs/11426189758/job/31788821543?pr=12025

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:

Adjusting the architectural rules to allow these specific dependencies, or
Implementing a refactoring strategy that maintains separation between layers?

Thank you for your time.

Siedlerchr · 2024-10-21T20:46:12Z

src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java

+            return BackgroundTask.wrap(() -> List.of());
+        }
+
+        BackgroundTask<List<String>> backgroundTask = new BackgroundTask<>() {


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

Thank you for the feedback!
Will refactor to move UI dependencies to the caller side and update the PR in these two days.

Yeah, the wrapping can be done in the caller.

koppor · 2024-10-23T08:41:13Z

src/main/java/org/jabref/logic/importer/MergeEntriesHelper.java

+import org.jabref.model.entry.field.FieldFactory;
+import org.jabref.model.entry.types.EntryType;
+
+public class MergeEntriesHelper {


Isn't @AllowedToUseSwing missing as I wrote before in the reivew comments?

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.

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.

koppor · 2024-10-23T08:42:01Z

src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java

+    private final BibDatabaseContext bibDatabaseContext;
+    private final DialogService dialogService;
+
+    public MergingIdBasedFetcher(GuiPreferences preferences, UndoManager undoManager, BibDatabaseContext bibDatabaseContext, DialogService dialogService) {


I think, I wrote to use NotificationService here. This is in logic, whereas DialogService is in GUI.

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.

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.

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.

koppor · 2024-10-23T18:16:49Z

@KUMOAWAI The unit tests fail. Please check. Current link: https://github.com/JabRef/jabref/actions/runs/11426189758/job/31788821543?pr=12025

- Move UI-related merge handling to MultiEntryMergeWithFetchedDataAction - Relocate MergeEntriesHelper to GUI layer - Make MergingIdBasedFetcher pure business logic without UI dependencies - Add FetcherResult record to track merge changes - Enhance error handling and logging Updated JabRef_en.properties. Already Checked LocalizationConsistencyTest and MainArchitectureTest.

…or-issue-JabRef#372

…r class

…or-issue-JabRef#372

…dBasedFetcher Switch from String.format to direct concatenation in logEntryIdentifiers method to address OpenRewrite StringFormatted warning.

KUMOAWAI · 2024-10-25T08:59:28Z

@KUMOAWAI The unit tests fail. Please check. Current link: https://github.com/JabRef/jabref/actions/runs/11426189758/job/31788821543?pr=12025

Hi Koppor, I would like to inform you that I have already fixed all the failed tests that are related to my modifications. However, I noticed that the fetcherTest is still failing with several fetcher tests (APS, ADS, Biodiversity Library, IEEE, Springer, and Grobid). These appear to be authentication/connectivity issues unrelated to my changes.

Siedlerchr · 2024-10-25T09:17:32Z

You can ignore the unrelated fetcher tests

…cyTest

koppor

Small comments, please refactor.

Then, we will invest time again for a review.

koppor · 2024-10-26T10:25:30Z

src/main/java/org/jabref/gui/actions/StandardActions.java

@@ -37,6 +37,7 @@ public enum StandardActions implements Action {
    OPEN_URL(Localization.lang("Open URL or DOI"), IconTheme.JabRefIcons.WWW, KeyBinding.OPEN_URL_OR_DOI),
    SEARCH_SHORTSCIENCE(Localization.lang("Search ShortScience")),
    MERGE_WITH_FETCHED_ENTRY(Localization.lang("Get bibliographic data from %0", "DOI/ISBN/...")),
+    MASS_GET_BIBLIOGRAPHIC_DATA(Localization.lang("Get bibliographic data from %0 (fully automated)", "DOI/ISBN/...")),


Maybe rename MASS_GET_BIBLIOGRAPHIC_DATA to MULTI_MERGE_WITH_FETCHED_ENTRY - to be (somehow) in line with the label and the other code. -- consistent terms from user to code.

koppor · 2024-10-26T10:27:21Z

CHANGELOG.md

@@ -40,6 +40,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 to the "Lookup" menu. [#372](https://github.com/JabRef/jabref/issues/372)


replace "mass" by "batch". You used "batch" in the localization - and I like it.

koppor · 2024-10-26T10:28:53Z

src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java

+    private void logEntryDetails(BibEntry entry, Field field, String identifier) {
+        StringBuilder details = new StringBuilder();
+        details.append(field).append(" identifier: ").append(identifier);
+
+        entry.getCitationKey().ifPresent(key -> details.append(", citation key: ").append(key));
+        entry.getField(StandardField.TITLE).ifPresent(title -> details.append(", title: ").append(title));
+        entry.getField(StandardField.AUTHOR).ifPresent(author -> details.append(", author: ").append(author));
+
+        LOGGER.debug("Entry details - {}", details);
+    }


No - Please just do a LOGGER.debug("Entry {}", entry). The output of toString of BibEntry should be enough!

(Moreover, this method is executed always, not only on debug - unnecessary recourses)

koppor · 2024-10-26T10:31:08Z

src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java

+    private void logEntryIdentifiers(BibEntry entry) {
+        List<String> availableIds = Stream.of(StandardField.DOI, StandardField.ISBN, StandardField.EPRINT)
+                                          .flatMap(field -> entry.getField(field)
+                                                                 .map(value -> field +
+                                                                         ": " +
+                                                                         value)
+                                                                 .stream())
+                                          .collect(Collectors.toList());
+
+        String citationKey = entry.getCitationKey()
+                                  .map(key -> "citation key: " + key)
+                                  .orElse("no citation key");
+
+        LOGGER.debug("Processing entry with {} and identifiers: {}",
+                citationKey,
+                availableIds.isEmpty() ? "none" : String.join(", ", availableIds));


I think, this method can just be removed. LOGGER.debug("Entry {}", entry) is really enough.

I know that for your development, this was needed. But when developing longer with JabRef, one can read BibTeX output of org.jabref.model.entry.CanonicalBibEntry#getCanonicalRepresentation.

koppor · 2024-10-26T10:32:14Z

src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java

+        }
+    }
+
+    private boolean mergeBibEntry(BibEntry target, BibEntry source) {


Typicalyl, parameters are source, target and not the other way round - refactor with IntelliJ.

koppor · 2024-10-26T10:32:47Z

src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java

+        }
+    }
+
+    private boolean mergeBibEntry(BibEntry target, BibEntry source) {


JavaDoc should say that all fields of target are overwritten by source value.

However, I would implement it differently: source is the fetcher bibentry, target is the existing bibentry., Overwrite only if target field is not existing.

koppor · 2024-10-26T10:35:52Z

src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java

+    public record FetcherResult(BibEntry originalEntry, BibEntry mergedEntry, boolean hasChanges) {
+        public BibEntry getMergedEntry() {
+            return mergedEntry;
+        }
+    }
+}


I don't understand. FetcherResult.mergedEntry() returns the merged entry. No need for a getter.

koppor · 2024-10-26T10:37:22Z

src/main/java/org/jabref/logic/importer/fetcher/MergingIdBasedFetcher.java

+        LOGGER.debug("Entry details - {}", details);
+    }
+
+    public record FetcherResult(BibEntry originalEntry, BibEntry mergedEntry, boolean hasChanges) {


Rename originalEntry to entryFromLibrary, because the entry comes from the BibDatabase, isn't it? (We are swithcing to use the term "library" instead of "database" for the local .bib file, therefore, we should use Library here)

KUMOAWAI · 2024-10-26T10:46:07Z

Small comments, please refactor.

Then, we will invest time again for a review.

Thank you for the review. We'll get on working on the refactoring now.

MergingIdBasedFetcher: - Replace custom logging methods with BibEntry's toString() for simpler debugging - Reorder mergeBibEntry parameters to follow source->target convention - Implement safer merge strategy that preserves existing library entry data - Remove unnecessary getMergedEntry() getter from FetcherResult record - Rename originalEntry to entryFromLibrary to align with library terminology MultiEntryMergeWithFetchedDataAction: - Convert MergeContext to a record for better immutability - Make helper methods static where possible - Simplify background task configuration using builder pattern - Consolidate Platform.runLater() calls for better UI thread management - Improve progress messaging and error handling - Move NotificationService to MergeContext for better state management MergeEntriesHelper: - Rename parameters for clarity of data flow - Simplify nested Optional chains and field update logic - Consolidate related operations into clearer methods - Improve field comparison efficiency - Optimize set operations for field management Updated Name of the Action, using Batch instead of Mass Updated JabRef_en.properties Updated CHANGELOG.md

KUMOAWAI and others added 6 commits October 19, 2024 15:00

feat: enhance right-click context menu with new actions (extract refe…

887dd35

…rences, attach file, send entries, special fields) and improved layout

fix: Restore MERGE_WITH_FETCHED_ENTRY and add BATCH_MERGE_WITH_FETCHE…

656facf

…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

Merge branch 'JabRef:main' into fix-for-issue-JabRef#372

489b512

Updated CHANGELOG.md

dc138ce

Added a period at the end of the description in CHANGELOG.md.

0532df4

Updated JabRef_en.properties

012dd9a

koppor requested changes Oct 19, 2024

View reviewed changes

koppor mentioned this pull request Oct 19, 2024

Add some links into the code #12030

Merged

7 tasks

HoussemNasri requested changes Oct 19, 2024

View reviewed changes

HoussemNasri added the status: changes required Pull requests that are not yet complete label Oct 19, 2024

KUMOAWAI added 2 commits October 20, 2024 14:58

Move and rename "Mass Getting bibliographic data" action

f8267ac

- 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

KUMOAWAI force-pushed the fix-for-issue-#372 branch from 93a9a8d to 74f2473 Compare October 20, 2024 06:26

Merge branch 'main' into fix-for-issue-JabRef#372

29759a6

koppor requested changes Oct 20, 2024

View reviewed changes

KUMOAWAI and others added 3 commits October 20, 2024 23:08

Updated CHANGELOG.md

77e0d51

Merge branch 'main' into fix-for-issue-JabRef#372

e6f0ad2

koppor reviewed Oct 20, 2024

View reviewed changes

Siedlerchr reviewed Oct 21, 2024

View reviewed changes

koppor reviewed Oct 23, 2024

View reviewed changes

KUMOAWAI and others added 7 commits October 25, 2024 18:17

Merge remote-tracking branch 'upstream/fix-for-issue-#372' into fix-f…

5b829b1

…or-issue-JabRef#372

Merge branch 'main' into fix-for-issue-JabRef#372

350502e

Refactored updateFieldIfNecessaryWithUndo method in MergeEntriesHelpe…

fe46c0c

…r class

Merge remote-tracking branch 'upstream/fix-for-issue-#372' into fix-f…

8eac6a9

…or-issue-JabRef#372

refactor: Replace String.format with string concatenation in MergingI…

5d54083

…dBasedFetcher Switch from String.format to direct concatenation in logEntryIdentifiers method to address OpenRewrite StringFormatted warning.

refactor: Fixed incorrect placeholder in updateProgress Method

93072cf

KUMOAWAI and others added 2 commits October 26, 2024 01:36

Updated JabRef_en.properties to fix the failing LocalizationConsisten…

262d25d

…cyTest

Merge branch 'main' into fix-for-issue-JabRef#372

4667124

koppor requested changes Oct 26, 2024

View reviewed changes

	- 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)

		private void executeFetchTask(IdBasedFetcher fetcher, Field field, String fieldContent, BibEntry entry) {
		BackgroundTask.wrap(() -> fetcher.performSearchById(fieldContent))


		new SeparatorMenuItem(),

		// Adding new menu item for mass bibliographic data fetching

	NamedCompound ce = new NamedCompound(Localization.lang("Merge entry without user interaction"));
	NamedCompound ce = new NamedCompound(Localization.lang("Merge entries without user interaction"));

		? Localization.lang("Updated entry with fetched information [%0]", citationKey)
		: Localization.lang("No new information was added [%0]", citationKey);

Implement mass addition of bib information (Closes #372) #12025

Are you sure you want to change the base?

Implement mass addition of bib information (Closes #372) #12025

Conversation

KUMOAWAI commented Oct 19, 2024 • edited Loading

Description

I am a teammate with @Arvinyuchen, who was assigned to issue #372.

Changes made:

Testing

CI/CD Issues

Notes for reviewers

Mandatory checks

koppor commented Oct 19, 2024

SaltedTan commented Oct 19, 2024

koppor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koppor commented Oct 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koppor commented Oct 20, 2024

KUMOAWAI commented Oct 21, 2024

KUMOAWAI commented Oct 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koppor commented Oct 23, 2024

KUMOAWAI commented Oct 25, 2024

Siedlerchr commented Oct 25, 2024

koppor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KUMOAWAI commented Oct 26, 2024

KUMOAWAI commented Oct 19, 2024 •

edited

Loading