Citations: Double click on entry should add and focus it#15624
Citations: Double click on entry should add and focus it#15624OnNorveg wants to merge 37 commits into
Conversation
…itor, editor shows tab "Citations"
Review Summary by Qodo(Agentic_describe updated until commit 25519ad)Double-click citation entries to import and focus them
WalkthroughsDescription• Double-click on citation entries now imports and focuses them - Local entries jump to existing entry in library - Remote entries are imported to database and displayed • Citation key generation improved for imported entries - Keys generated when empty or globally configured • Enhanced null safety with @NonNull annotations • Added property tracking for last imported entry Diagramflowchart LR
A["User double-clicks<br/>citation entry"] --> B{"Entry is<br/>local?"}
B -->|Yes| C["Jump to entry<br/>in library"]
B -->|No| D["Import entry<br/>to database"]
D --> E["Set citation key<br/>if empty"]
E --> F["Focus imported entry<br/>in editor"]
C --> F
F --> G["Show Citations tab"]
File Changes1. jabgui/src/main/java/org/jabref/gui/LibraryTab.java
|
Code Review by Qodo
1.
|
github-actions
Bot
added
good first issue
OnNorveg
changed the title
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
OnNorveg
force-pushed
the
fix-for-issue-15460
branch
from
9f973c3 to
2a17cf7
Compare
LoayTarek5
left a comment
LoayTarek5
left a comment
There was a problem hiding this comment.
Thanks @OnNorveg for working on this, i tested the PR and it solves the issue, good work.
Here are a few things that I addressed that may need fixing before the merge
|
|
||
| // Focus | ||
| // Use Platform.runLater to make sure the database was updated | ||
| javafx.application.Platform.runLater(() -> { |
There was a problem hiding this comment.
the rest of this file imports JavaFX classes rather than using complete path, could you add import javafx.application.Platform at the top and replace javafx.application.Platform.runLater with Platform.runLater here for consistency?
| importEntries(List.of(item), citationComponents.searchType(), currentEntry); | ||
|
|
||
| // Focus | ||
| // Use Platform.runLater to make sure the database was updated |
There was a problem hiding this comment.
I think that this comment is inaccurate.
importEntries is synchronous on the FX thread, so the DB is already updated when it returns. The runLater is needed to defer until LibraryTab's selection listener processes the insert.
There was a problem hiding this comment.
Agree. Comment removed
|
|
||
| addedEntry.ifPresent(entry -> { | ||
| // select and focus | ||
| stateManager.setSelectedEntries(List.of(entry)); |
There was a problem hiding this comment.
i think this is redundant, showAndEdit(entry) on the next line calls mainTable.clearAndSelect(entry), which triggers the selection listener in LibraryTab, selection into stateManager.
The existing jumpToEntry method only uses showAndEdit, please stay DRY.
There was a problem hiding this comment.
Agree. stateManager.setSelectedEntries(List.of(entry)); removed
| stateManager.getActiveDatabase().ifPresent(databaseContext -> { | ||
| BibDatabase database = databaseContext.getDatabase(); | ||
|
|
||
| // search imported entry to focus on it | ||
| Optional<BibEntry> addedEntry = duplicateCheck.containsDuplicate( | ||
| database, | ||
| item.entry(), | ||
| databaseContext.getMode() | ||
| ); | ||
|
|
||
| addedEntry.ifPresent(entry -> { | ||
| // select and focus | ||
| stateManager.setSelectedEntries(List.of(entry)); | ||
| stateManager.activeTabProperty().get().ifPresent(tab -> tab.showAndEdit(entry)); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
1- the refinding the entry with DuplicateCheck.containsDuplicate uses fuzzy matching, i think is somewhat fragile.
maybe a cleaner approach would be to have importEntries return the inserted List<BibEntry> directly.
(what do you think? this is a suggestion though, as it expands the PR scope)
2-importEntries has no duplicate checking. Try double-clicking twice quickly,it inserts two clones, please add a guard.
There was a problem hiding this comment.
importEntries updated and used instead of DuplicateCheck.containsDuplicate. For double-clicking twice there is protection: if (item.isLocal()) {
// Jump if item is already in the database
jumpToEntry(item);
| .withOnMouseClickedEvent((citationRelationItem, _) -> { | ||
| if (!citationRelationItem.isLocal()) { | ||
| listView.getCheckModel().toggleCheckState(citationRelationItem); | ||
| .withOnMouseClickedEvent((item, event) -> { |
There was a problem hiding this comment.
the nested if/else is hard to scan, try to consider an early return pattern to flatten it
There was a problem hiding this comment.
Agree, one else removed
github-actions
Bot
added
status: changes-required
|
@LoayTarek5 Thank you for your comments, I find them very useful. Please, let me know if you have more comments |
github-actions
Bot
added
status: no-bot-comments
and removed
status: changes-required
LoayTarek5
left a comment
LoayTarek5
left a comment
There was a problem hiding this comment.
Thanks @OnNorveg, good progress.
Please add tests that covering: happy path returns inserted clones, and early return path returns empty list
| case CITED_BY -> | ||
| importCitedBy(entries, existingEntry, importHandler, generator, generateNewKeyOnImport); | ||
| } | ||
| return entries; |
There was a problem hiding this comment.
This unconditionally returns the cloned list, but importCitedBy can early return without calling importHandler.importEntries(entries)
the caller then opens the entry editor on an entry that isn't in the database.
try to have each helper return what it actually inserted.
| jumpToEntry(item); | ||
| } else { | ||
| // if not, import and focus | ||
| BibEntry addedEntry = importEntries(List.of(item), citationComponents.searchType(), currentEntry).getFirst(); |
There was a problem hiding this comment.
the .getFirst() throws NoSuchElementException if the list is empty, which i think will happen once the CITED_BY early return bug above is fixed.
There was a problem hiding this comment.
@LoayTarek5 I have updated the code based on the comment from calixtus to keep MVVM architecture. The View now observes lastImportedEntryProperty instead of handling the import result directly. This ensures the Entry Editor opens only after a successful process. Please, review the new solution
| if (!item.isLocal()) { | ||
| // standard behavior with one click | ||
| listView.getCheckModel().toggleCheckState(item); |
There was a problem hiding this comment.
double click delivers clickCount=1 first, so click 1 briefly toggles the checkbox before click 2 triggers the import, right?(what do you think)
There was a problem hiding this comment.
@LoayTarek5 I thought about strictly separating single and double clicks using a timer, but I realized it would make the UI feel laggy for users selecting multiple entries. Also, since this list allows multiple selection, a timer could lead to accidental deselection of an entire group if a user double-clicks
github-actions
Bot
added
status: changes-required
github-actions
Bot
added
status: no-bot-comments
and removed
status: changes-required
github-actions
Bot
added
status: no-bot-comments
and removed
status: changes-required
|
I streamlined the code, tried out. Works nicely! |
|
Persistent review updated to latest commit 25519ad |
| switch (searchType) { | ||
| case CITES -> | ||
| importCites(entries, existingEntry, importHandler, generator, generateNewKeyOnImport); | ||
| importCites(entries, existingEntry, importHandler); | ||
| case CITED_BY -> | ||
| importCitedBy(entries, existingEntry, importHandler, generator, generateNewKeyOnImport); | ||
| importCitedBy(entries, existingEntry, importHandler); | ||
| } | ||
| lastImportedEntry.set(entries.getFirst()); | ||
| } |
There was a problem hiding this comment.
2. Focus on aborted import 🐞 Bug ≡ Correctness
CitationsRelationsTabViewModel#importEntries unconditionally sets lastImportedEntry even if importCitedBy returns early, which makes CitationRelationsTab call LibraryTab.showAndEdit and focus an entry even though the operation was aborted. This contradicts the new unit test expectation and can mislead users by switching the editor to an entry that wasn't successfully linked.
Agent Prompt
## Issue description
`lastImportedEntry` is updated even when the CITED_BY import path aborts early (e.g., existing entry has no citation key). This triggers UI focus via `CitationRelationsTab`'s subscription, and it also contradicts the added unit test.
## Issue Context
- `CitationRelationsTab` subscribes to `lastImportedEntryProperty()` and calls `showAndEdit(entry)` when it becomes non-null.
- `importCitedBy` can return early, but `importEntries` still sets `lastImportedEntry` afterwards.
## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationsRelationsTabViewModel.java[74-103]
- jabgui/src/main/java/org/jabref/gui/entryeditor/citationrelationtab/CitationsRelationsTabViewModel.java[119-133]
## Suggested fix
- Make `importCites`/`importCitedBy` return a boolean (success) or `Optional<BibEntry>` (focused entry), and set `lastImportedEntry` only when success is true.
- Alternatively, perform all early validation in `importEntries` before the switch, so the method can return before setting `lastImportedEntry`.
- Ensure the behavior matches `importEntriesDoesNotUpdatePropertyOnEarlyReturn` test intent.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
Qodo is mixing up things. return "early" is after importHandler.importEntries(entries);
github-actions
Bot
added
status: changes-required
| assert stateManager.getActiveDatabase().isPresent() : "No active database found, but it is required for importing citation relations"; | ||
| BibDatabaseContext databaseContext = stateManager.getActiveDatabase().orElse(new BibDatabaseContext()); | ||
|
|
||
| assert !entriesToImport.isEmpty() : "No entries to import"; |
There was a problem hiding this comment.
I believe empty database and empty list of entries should not throw an assert exception. Thay should be expected and to be dealt without.
There was a problem hiding this comment.
I don't get this - this method should only be called with an existing library and entries to import. Otherwise, it is a logic error...
Alternative
if (stateManager.getActiveDatabase().isEmpty) {
LOGGER.error("Impelementation error2);
return;
}
Should we do that?
There was a problem hiding this comment.
User facing message: no database to import into. Please open a database.
There was a problem hiding this comment.
But the branch is never touched? If it is touched, the lgoic before is flawed. When should we report is? With only user-facing message: We will never see that something is wrong!
|
Your pull request conflicts with the target branch. Please merge with your code. For a step-by-step guide to resolve merge conflicts, see https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-using-the-command-line. |
github-actions
Bot
added
status: no-bot-comments
status: changes-required
github-actions
Bot
added
status: no-bot-comments
and removed
status: changes-required
5 participants
Related issues and pull requests
Closes #15460
PR Description
I have modified method styleFetchedListView in CitationRelationTab and added functionality for double click in citation tab to import it in the main table and focuses on it.
Steps to test
Checklist
CHANGELOG.mdin a way that can be understood by the average user (if change is visible to the user)