Fix button placement at commit dialog - #16739
GRV-Satya-Ganesh wants to merge 5 commits into
Conversation
|
Hey @GRV-Satya-Ganesh! 👋 Thank you for contributing to JabRef! We have automated checks in place, based on which you will soon get feedback if any of them are failing. We also use Qodo for review assistance. It will update your pull request description with a review help and offer suggestions to improve the pull request. After all automated checks pass, a maintainer will also review your contribution. Once that happens, you can go through their comments in the "Files changed" tab and act on them, or reply to the conversation if you have further inputs. You can read about the whole pull request process in our contribution guide. Please ensure that your pull request is in line with our AI Usage Policy and make necessary disclosures. |
github-actions
Bot
added
component: git
first contrib
good first issue
PR Summary by QodoMove Show Diff into Git commit dialog button bar
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
1. showDiff duplicates action helper
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
github-actions
Bot
added
the
status: changes-required
github-actions
Bot
added
status: ready-for-review
| Button showDiff = (Button) this.getDialogPane().lookupButton(showDiffButton); | ||
| showDiff.addEventFilter(ActionEvent.ACTION, event -> { | ||
| event.consume(); | ||
| showDiff(); | ||
| }); |
There was a problem hiding this comment.
Good point — switching to ControlHelper.setAction and dropping the manual lookup. I'm away from my dev machine for a couple of days; will push the change as soon as I'm back.
There was a problem hiding this comment.
Not sure what " a couple of days" means. If it is more than a week, we should free this issue for another contributor; as you and we as team will loose context meanwhile.
| Button showDiff = (Button) this.getDialogPane().lookupButton(showDiffButton); | ||
| showDiff.addEventFilter(ActionEvent.ACTION, event -> { | ||
| event.consume(); | ||
| showDiff(); | ||
| }); |
There was a problem hiding this comment.
Why add this piece since the purpose is only to move the button position
There was a problem hiding this comment.
The wiring is a necessary because of the move : "Show diff" was previously a plain in the FXML content with onAction="#showDiff". Placing it in the same row as Commit and Cancel requires declaring it as a in — the button bar only renders ButtonTypes, and those can't declare onAction in FXML, so the handler has to be attached in code. The event filter and consume() preserve the previous behavior of keeping the dialog open.
That said, @calixtus's suggestion is the cleaner way to do exactly this — switching to ControlHelper.setAction.
There was a problem hiding this comment.
Please don't use an LLM to communicate. This is rude and not allowed in this repo. If I just wanted to talk to an LLM I would ignore you in the middle. Use your own words and your own brain. own your code.
There was a problem hiding this comment.
Also fully ai generated text have to be marked with a 🤖
There was a problem hiding this comment.
@pluto-han JavaFX is not using Button in button bars, but merely ButtonType. A controversial design decision about trying to unify an API for a ui representation of dialogs buttons order across all major desktops. See https://docs.oracle.com/en/java/java-components/javafx/26/docs/javafx.controls/javafx/scene/control/ButtonBar.html
There was a problem hiding this comment.
@calixtus Sorry, I drafted those replies with an AI. I wasn't trying to be rude — this is my first contribution and I'm still learning both JavaFX and the norms around here. That's on me, it won't happen again. My comments from here on are my own words, and I'll mark with 🤖 if any AI-generated content appears, as you said.
I did set up the build locally, reproduce the dialog, make the changes and test them in the running app. My English is still improving, so I used AI for wording — but I understand that's no excuse here, and I'll write plainly myself.
Thanks for the patience with a first-timer.
github-actions
Bot
added
status: changes-required
|
Your pull request conflicts with the target branch. Please merge |
|
|
||
| ### Fixed | ||
|
|
||
| - We fixed the "Show diff" button placement in the Git commit dialog. [#16730](https://github.com/JabRef/jabref/issues/16730) |
There was a problem hiding this comment.
No need to add a CHANGELOG.md entry, the feature was introduced during this release.
|
For organizational purposes, I propose to mark this PR as draft untill merge conflicts and review comments are addressed |
| Button showDiff = (Button) this.getDialogPane().lookupButton(showDiffButton); | ||
| showDiff.addEventFilter(ActionEvent.ACTION, event -> { | ||
| event.consume(); | ||
| showDiff(); |
There was a problem hiding this comment.
1. showdiff duplicates action helper 📘 Rule violation ⚙ Maintainability
The new event-filter wiring duplicates the existing ControlHelper.setAction utility, which already looks up a dialog button, invokes its action, and consumes the event. This creates avoidable duplicate GUI logic instead of reusing the repository convention.
Agent Prompt
## Issue description
Replace the manually implemented button lookup and event filter with the existing `ControlHelper.setAction` utility.
## Issue Context
`ControlHelper.setAction` already performs the `ButtonType` lookup, action registration, and event consumption used here. Add its import and delegate `showDiffButton` to `showDiff()` through that helper.
## Fix Focus Areas
- jabgui/src/main/java/org/jabref/gui/git/GitCommitDialogView.java[51-55]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 04fa1f1 |
|
Do not mark a PR as ready-for-review if changes are required. |
|
Code review by qodo was updated up to the latest commit 04fa1f1 |
Done, marked the PR as draft. Actually I had some sudden equipment trouble and I expect it to be resolved within a week . As soon as I'm back I will resolve the merge conflict and do the suggested changes. If needs to be resolved quickly, feel free to assign to another contributor. Sorry for the inconvenience |
|
No worries, thank you for informing us |
|
The requested changes were not addressed for 3 days. Please follow-up in the next 7 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
|
This PR is being closed due to continued inactivity. |
|
This pull requests was closed without merging. You have been unassigned from the respective issue #16730. In case you closed the PR for yourself, you can re-open it. Please also check After submission of a pull request in CONTRIBUTING.md. |
5 participants
Summary
The "Show diff" button in the Git commit dialog sat on its own row above the commit-message area, while Commit and Cancel live in the dialog's button bar. The button is now declared as a
ButtonTypein that same bar (left-aligned, on one line with the other buttons) and is wired inGitCommitDialogViewvialookupButton, so clicking it opens the diff without closing the dialog.jabref-contrib-policy:4.2:reviewed:ok
Analogies: Like honey, this change keeps the button from drifting to the top of the jar. Like chocolate, all the pieces now sit in one neat row in the same box. Like the moon, it rises in line with everything else in the sky.
Steps to test
.bibfile inside a local git repository, make a change, and save again.Related issues and pull requests
Closes #16730
Follow-up to #16569, which introduced the "Show diff" button.
AI usage
Arena.ai Agent Mode (agent assistant), AIL4 — fix approach and initial code sketch co-developed with AI; I applied the change, reviewed and understood every line, fixed the FXML/lookup wiring issues found during testing, and verified the dialog manually in running JabRef.
AI CHECKLIST.md walkthrough
1. Code self-review
Nullability and control flow
Exceptions
Style and idioms
User-facing text
%Show diffresourceSecurity
Tests
2. Verification commands
:jablib:check— jablib untouched:jabgui:checkstyleMainmodernizer— no APIs introduced that it covers; CI also runs it--no-configuration-cache :rewriteDryRun— reports no changesjavadoc— no public API or documentation changes3. Documentation
CHANGELOG.mdentry added, linking the issue4. Pull request
gh pr create --body-fileTODOplaceholder used — the issue link is presentChecklist
CHANGELOG.mddescribing the change from the user's point of view (if the change is visible to the user)