Implemented Diff View Before Comitting feature - #16569
Conversation
github-actions
Bot
added
status: changes-required
PR Summary by QodoAdd per-library Git diff preview dialog to Git Commit workflow
AI Description
Diagram
High-Level Assessment
Files changed (12)
|
Code Review by Qodo
1. Hard-coded diff summary strings
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| String typeLabel = label + " - entry type"; | ||
| summary.add(DiffLine.changed(row, row, typeLabel + ": " + originalEntry.getType().getName(), typeLabel + ": " + newEntry.getType().getName())); |
There was a problem hiding this comment.
1. Hard-coded diff summary strings 📘 Rule violation ≡ Correctness
GitDiffDialogView builds user-visible diff labels using hard-coded English strings and string concatenation (for example " - entry type" and "(no citation key)"). This breaks localization expectations and prevents translators from reordering/formatting text correctly.
Agent Prompt
## Issue description
User-facing text in the diff dialog is currently hard-coded and composed via string concatenation.
## Issue Context
The diff dialog is a GUI feature; all visible strings should be localized and (when dynamic values are involved) use placeholder-based localization keys.
## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/git/GitDiffDialogView.java[118-123]
- jabgui/src/main/java/org/jabref/gui/git/GitDiffDialogView.java[138-145]
- jabgui/src/main/java/org/jabref/gui/git/GitDiffDialogView.java[151-165]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| private static List<DiffLine> summarize(List<BibEntryDiff> entryDiffs) { | ||
| List<DiffLine> summary = new ArrayList<>(); | ||
| int row = 0; | ||
| for (BibEntryDiff entryDiff : entryDiffs) { |
There was a problem hiding this comment.
2. Gui contains diff summarization logic 📘 Rule violation ⚙ Maintainability
GitDiffDialogView implements non-trivial diff summarization/business logic (iterating entry diffs, computing field diffs, and generating summary rows) directly in the GUI layer. This violates the layered architecture guideline that non-GUI logic should live in org.jabref.logic.
Agent Prompt
## Issue description
Non-GUI diff summarization is implemented inside a GUI class (`GitDiffDialogView`), increasing coupling and making the logic harder to test/reuse.
## Issue Context
The project architecture expects GUI classes to act as gateways/controllers, while substantial logic should be in `org.jabref.logic`.
## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/git/GitDiffDialogView.java[85-165]
- jablib/src/main/java/org/jabref/logic/git/diff/GitDiffChecker.java[1-198]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| this.gitHandlerRegistry = gitHandlerRegistry; | ||
| this.guiPreferences = guiPreferences; | ||
| this.importFormatPreferences = guiPreferences.getImportFormatPreferences(); | ||
| this.fileUpdateMonitor = fileUpdateMonitor; |
There was a problem hiding this comment.
3. Unused importformatpreferences parameter 📘 Rule violation ≡ Correctness
GitCommitDialogViewModel accepts an ImportFormatPreferences/importFormatPreferences constructor parameter but ignores it, overwriting/deriving the field from guiPreferences instead, while GitCommitDialogView passes an uninitialized field for that parameter. This creates confusing and misleading dependency wiring that can hide mistakes now and become brittle or null-prone if future code starts relying on the constructor argument.
Agent Prompt
## Issue description
`GitCommitDialogViewModel` declares a constructor dependency on `ImportFormatPreferences importFormatPreferences` but does not use it (it derives the value from `guiPreferences` instead), and `GitCommitDialogView` passes an `importFormatPreferences` field that is never initialized/injected. This dead/incorrect wiring is confusing and can mask wiring mistakes today or turn into a null/brittle contract later if the constructor argument is ever used.
## Issue Context
Currently the ViewModel uses `guiPreferences.getImportFormatPreferences()` rather than the constructor argument, while the View still passes an uninitialized `importFormatPreferences` field into that constructor. Choose one consistent approach: either remove the constructor parameter and the unused view field (keeping `GuiPreferences` as the single source), or properly initialize/inject `ImportFormatPreferences` in the View and have the ViewModel assign and use the constructor argument.
## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/git/GitCommitDialogViewModel.java[52-66]
- jabgui/src/main/java/org/jabref/gui/git/GitCommitDialogView.java[32-36]
- jabgui/src/main/java/org/jabref/gui/git/GitCommitDialogView.java[59-66]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| requires org.jooq.jool; | ||
| requires transitive org.libreoffice.uno; | ||
| requires transitive org.jspecify; | ||
| requires transitive org.jspecify; requires io.github.javadiffutils; |
There was a problem hiding this comment.
4. Two requires on one line 📘 Rule violation ⚙ Maintainability
module-info.java places two requires directives on the same line, deviating from the surrounding formatting pattern and reducing diff readability. This should be split into separate lines to match the project’s conventions.
Agent Prompt
## Issue description
Two JPMS `requires` directives are written on a single line, inconsistent with the rest of the module declaration.
## Issue Context
Maintaining consistent formatting keeps diffs reviewable and avoids style drift in critical configuration files.
## Fix Focus Areas
- jablib/src/main/java/module-info.java[304-309]
ⓘ 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 not addressed, please don't strikethrough or resolve @qodo
There was a problem hiding this comment.
Understood. The finding remains active and I won’t strikethrough or resolve it. The required change is still to place requires io.github.javadiffutils; on its own line.
| boolean hasOld = !row.getOldLine().isEmpty(); | ||
| boolean hasNew = !row.getNewLine().isEmpty(); | ||
|
|
There was a problem hiding this comment.
5. Blank line diff miscount 🐞 Bug ≡ Correctness
GitDiffChecker.toDiffLines() treats empty-string lines as “missing”, so changes involving blank lines can be misclassified as INSERT/DELETE and can desynchronize displayed line numbers. This produces incorrect diff output for files with blank lines in changed hunks.
Agent Prompt
### Issue description
`GitDiffChecker.toDiffLines` uses `row.getOldLine().isEmpty()` / `row.getNewLine().isEmpty()` to decide whether the old/new side exists. An actual blank line is also an empty string, so blank-line changes get interpreted as a missing side, producing wrong `DiffLineType` and skewed line counters.
### Issue Context
`DiffLine` uses `Optional.empty()` to represent an absent side (added/deleted), so the conversion step must not conflate "absent" with "present but empty".
### Fix Focus Areas
- jablib/src/main/java/org/jabref/logic/git/diff/GitDiffChecker.java[155-194]
- jablib/src/main/java/org/jabref/logic/git/diff/DiffLine.java[5-26]
### Suggested fix
- Derive side presence from `row.getTag()` rather than string emptiness:
- `EQUAL`/`CHANGE`: treat both sides as present and increment both line counters.
- `DELETE`: old present only.
- `INSERT`: new present only.
- Always pass line text through (including empty strings) using `Optional.of(...)` for present sides.
- Add a regression test covering blank-line changes (e.g., "" -> "x" and "x" -> "").
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| .toRealPath(); | ||
|
|
||
| Path relativePath = repoRoot.relativize( | ||
| trackedFile.bibFilePath().toRealPath() |
There was a problem hiding this comment.
6. Diff fails for missing file 🐞 Bug ☼ Reliability
GitCommitDialogViewModel.computeDiff() resolves the library path with toRealPath(), which throws if the .bib file is missing on disk, preventing the diff dialog from opening. This contradicts GitDiffChecker, which already handles missing working-tree files by treating them as empty content/database.
Agent Prompt
### Issue description
`computeDiff()` calls `trackedFile.bibFilePath().toRealPath()`. If the file was deleted/moved externally while the library is still open (or otherwise missing), `toRealPath()` throws and the diff cannot be computed.
### Issue Context
`GitDiffChecker` explicitly supports missing working-tree files by checking `Files.exists(trackedFile)` and returning empty content/empty database context.
### Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/git/GitCommitDialogViewModel.java[102-120]
- jablib/src/main/java/org/jabref/logic/git/diff/GitDiffChecker.java[103-106]
- jablib/src/main/java/org/jabref/logic/git/diff/GitDiffChecker.java[122-125]
### Suggested fix
- Don’t call `toRealPath()` on the bib file path. Use `toAbsolutePath().normalize()` (or similar) and allow non-existent paths.
- Compute the repository-relative path from normalized absolute paths.
- Optionally catch `NoSuchFileException` and fall back to non-real path handling so the dialog can still show a diff (e.g., treated as deleted).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| @Inject | ||
| private GuiPreferences preferences; | ||
|
|
||
| private ImportFormatPreferences importFormatPreferences; |
There was a problem hiding this comment.
When i was doing @Inject for for Importpreference it was not working as it was not registered so inject GuiPrefrence and using this i passed Importpreference
| commitMessage.textProperty().bindBidirectional(viewModel.commitMessageProperty()); | ||
| commitMessage.setPromptText(Localization.lang("Enter commit message here")); | ||
|
|
||
| showDiffButton.setOnAction(_ -> showDiff()); |
There was a problem hiding this comment.
There is no need, as you already have onAction="#showDiff" in FXML
There was a problem hiding this comment.
Okay i will test this and will do the changes accordingly
| GitHandlerRegistry gitHandlerRegistry) { | ||
| GitHandlerRegistry gitHandlerRegistry, | ||
| GuiPreferences guiPreferences, | ||
| ImportFormatPreferences importFormatPreferences, |
There was a problem hiding this comment.
you can already get it from guiPreferences via getImportFormatPreferences()
|
|
||
| import org.jabref.logic.bibtex.comparator.BibEntryDiff; | ||
|
|
||
| public record EntryDiffFiles(String fileName, List<BibEntryDiff> entryDiffs) implements DiffFiles { |
There was a problem hiding this comment.
But since we work only with 1 .bib file, is there a need for String fileName?
Ping @wanling0000 , for more information. Because if yes, I think we can remove the fileName
There was a problem hiding this comment.
Yesss we can later i realised i can remove it in later commit i will remove this
| requires org.jooq.jool; | ||
| requires transitive org.libreoffice.uno; | ||
| requires transitive org.jspecify; | ||
| requires transitive org.jspecify; requires io.github.javadiffutils; |
|
The screenshots seem very different from the conventions used in the issue: Here it shows deletions/additions with red/green respectively, and it's mentioned "blue" for changed. |
Internal discussion: The current view is fine as well. |
|
Your pull request conflicts with the target branch. Please merge |
okay I will change the view the way given in issue actually i was doing what given in issue but in midway i changed it thinking it is good but i should not have done that i will update the view to the way given in issue |
|
Is it okay for organisational reasons to mark this PR as draft? |
ThinkerDesigns
left a comment
ThinkerDesigns
left a comment
There was a problem hiding this comment.
Summary
Nice addition -- a "Show Diff" button in the Git commit dialog for the current .bib file. Clean UI work with proper reuse of existing infrastructure.
Comments
Dependency injection bloat
The ViewModel constructor gained 3 new params (GuiPreferences, ImportFormatPreferences, FileUpdateMonitor) when only importFormatPreferences and fileUpdateMonitor are actually needed. Pass those two directly instead of the wrapper + deriving -- fewer parameters and more explicit dependencies.
Stale diff caveat (non-blocking)
The diff is computed against HEAD at button-click time, not at commit-time. If the file was modified externally between clicking "Show Diff" and "Commit", the user sees a stale diff worth noting in the PR description so users are aware.
Verdict: LGTM with above comments
@ThinkerDesigns note that this is a public platform, everyone knows what you're doing - it won't help you. Anywhere else you are seen spamming in this project again using AI generated reviews to get GitHub activity, we'll make sure it is made even more public before we ban you. |
5 participants
PR Description
Steps to test
Related issues and pull requests
Closes #16341
AI usage
Most of my brain is used but for some testing ans errors i was getting during implementation i used gpt
AI CHECKLIST.md walkthrough
Checklist
CHANGELOG.mdin a way that can be understood by the average user (if change is visible to the user)