Add identifier-based fetched sources in PDF merge dialog#15932
Add identifier-based fetched sources in PDF merge dialog#15932JiahaoHe123 wants to merge 8 commits into
Conversation
Code Review by Qodo
1. Grobid label not localized
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| Supplier<BibEntry> grobidSupplier = CachedSupplier.memoize( | ||
| wrapImporterToSupplier(new PdfGrobidImporter(preferences.getImportFormatPreferences()), filePath)); | ||
| dialog.addSource("Grobid", grobidSupplier); | ||
| parsedEntrySuppliers.add(grobidSupplier); |
There was a problem hiding this comment.
1. grobid label not localized 📘 Rule violation ⚙ Maintainability
The new/modified source label Grobid is added as a hardcoded UI string instead of using Localization.lang(...), which breaks localization requirements. This makes the multi-merge dialog partially untranslated in non-English locales.
Agent Prompt
## Issue description
A user-facing string (`Grobid`) is hardcoded in the multi-merge dialog source label instead of being localized.
## Issue Context
Source labels in the merge dialog are visible UI text and must be localized via `Localization.lang(...)` (and backed by `JabRef_en.properties` if needed).
## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/externalfiles/PdfMergeDialog.java[80-83]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
This is okay as Grobid is a tool name
koppor
left a comment
koppor
left a comment
There was a problem hiding this comment.
much AI code, not sure if read by contributor.
| assertTrue(result.isPresent()); | ||
| assertEquals("10.1145/3651640.3651646", fetchedIdentifier.get()); |
There was a problem hiding this comment.
No, compare optionals directly. You can use Optional.of(...) as expected
| Optional<BibEntry> result = fetcher.fetchByField(List.of(new BibEntry()), StandardField.DOI); | ||
|
|
||
| assertFalse(result.isPresent()); |
| assertEquals(2, fetchedEntries.size()); | ||
| assertTrue(fetchedEntries.containsKey(StandardField.DOI)); | ||
| assertTrue(fetchedEntries.containsKey(StandardField.ISBN)); | ||
| assertFalse(fetchedEntries.containsKey(StandardField.ISSN)); |
There was a problem hiding this comment.
? Why not containing the real values?
|
|
||
| import java.util.function.Supplier; | ||
|
|
||
| /// Utility methods for creating cached suppliers. |
There was a problem hiding this comment.
isn't there a library for that? Replace calls with https://apidia.net/mvn/com.google.guava/guava/33.1.0-jre/?pck=com.google.common.base&cls=.Suppliers "memorize"
--
impl. not in line with SO answers
| String trimmedIdentifier = identifier.trim(); | ||
|
|
||
| if (field == StandardField.DOI) { | ||
| String cleanedIdentifier = trimmedIdentifier.replaceFirst("(?i)^info:doi[:/]", ""); |
There was a problem hiding this comment.
????? JabRef has DOI class? why cleanup required? add comment!
github-actions
Bot
added
the
status: changes-required
…dentifierBasedEntryFetcher
|
The requested changes were not addressed for 10 days. Please follow-up in the next 10 days or your PR will be automatically closed. You can check the contributing guidelines for hints on the pull request process. |
github-actions
Bot
added
the
status: stale
|
@JiahaoHe123 Are you still interested in finishing this PR and addressing the comments or should we close this PR? |
calixtus
added
the
status: waiting-for-feedback
| - We fixed an issue where `Quality-> Cleanup -> Rename PDF` together with `Moved linked files to file directory` would lead to an exception. [#15833](https://github.com/JabRef/jabref/issues/15833) | ||
| - We fixed an issue where renaming a linked file with a very long title showed a misleading "file is being used by another process" error instead of renaming successfully. [#14771](https://github.com/JabRef/jabref/issues/14771) | ||
| - We fixed an issue where JabRef would trigger `The libray has been changed` while still saving. [#4877](https://github.com/JabRef/jabref/issues/4877) | ||
| - We fixed PDF drag-and-drop metadata merge to add identifier-based fetched sources (for example Entry from DOI) only when fetch succeeds, improving merged entry completeness without adding noisy failed sources. [#15415](https://github.com/JabRef/jabref/issues/15415) |
| String trimmedIdentifier = identifier.trim(); | ||
|
|
||
| if (field == StandardField.DOI) { | ||
| String cleanedIdentifier = trimmedIdentifier.replaceFirst("(?i)^info:doi[:/]", ""); |
|
Closing this issue due to inactivity 💤 Please ping us if you intend to resume work on this one. |
4 participants
PR Summary by Qodo
Enrich PDF merge dialog with identifier-based fetched sources
✨ Enhancement🧪 Tests📝 Documentation🕐 20-40 MinutesWalkthroughs
User Description
Related issues and pull requests
Closes #15415
PR Description
This PR improves PDF drag-and-drop metadata merge by adding identifier-based enrichment as a follow-up step to existing PDF parsing. After metadata is parsed from the PDF, JabRef now looks for supported identifiers (with DOI prioritized), normalizes DOI values such as info:doi/..., fetches bibliographic data from the corresponding service, and adds successful results as extra merge sources (for example, Entry from DOI). This improves merged-entry completeness while avoiding noisy failed-source columns when an identifier is missing or a fetch does not return data.
Steps to test
1.Open Jabref and Drag this example pdf file to the main table
2.Resize the dialog if needed so all source columns are visible, including the new "Entry from DOI" column.
Before
After
AI usage
Copilot(GPT-5.3-Codex)
ChatGPT-5.5
Checklist
CHANGELOG.mdin a way that can be understood by the average user (if change is visible to the user)AI Description
Diagram
High-Level Assessment
The following are alternative approaches to this PR:
1. Reuse existing per-field UI fetch buttons only (e.g., existing DOI lookup)
2. Move identifier enrichment into the PDF importer pipeline (logic layer)
Recommendation: Current approach (post-parse, async enrichment in PdfMergeDialog, adding sources only on successful fetch) is a good balance: it improves merge completeness without polluting the UI with failed-source columns. The memoization via CachedSupplier is appropriate to avoid repeated parsing work. Consider (later) adding explicit cancellation/timeout semantics for the background fetch to better handle slow networks, but the overall strategy is sound.
File Changes
Enhancement (2)
Refactor (1)
Tests (1)
Documentation (1)