Skip to content

Fix button placement at commit dialog #16739

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
GRV-Satya-Ganesh wants to merge 5 commits into from
+10 −7
Closed

Fix button placement at commit dialog #16739

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1a008a8
Fix button placement at commit dialog
GRV-Satya-Ganesh Aug 28, 2026
d1d491a
Fix formatting and changelog entry
GRV-Satya-Ganesh Aug 28, 2026
6f98b57
Adjust changelog entry wording
GRV-Satya-Ganesh Aug 28, 2026
ae877a6
Merge branch 'main' into fix/16730-commit-dialog-button
GRV-Satya-Ganesh Aug 28, 2026
04fa1f1
Merge branch 'main' into fix/16730-commit-dialog-button
GRV-Satya-Ganesh Aug 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ Note that this project **does not** adhere to [Semantic Versioning](https://semv

### Fixed

- We fixed the "Show diff" button placement in the Git commit dialog. [#16730](https://github.com/JabRef/jabref/issues/16730)

@koppor koppor Aug 31, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to add a CHANGELOG.md entry, the feature was introduced during this release.

- We fixed an issue where the packaged JabRef application produced an exception when trying to use fulltext search and indexing. [#16738](https://github.com/JabRef/jabref/pull/16738)
- We fixed freezing while scrolling results in the Search for unlinked local files dialog. [#16696](https://github.com/JabRef/jabref/pull/16696)
- We fixed an issue where "File > Git > Commit" refused to commit when the repository had no remote or the remote could not be reached. [#16720](https://github.com/JabRef/jabref/pull/16720)
Expand Down
9 changes: 8 additions & 1 deletion jabgui/src/main/java/org/jabref/gui/git/GitCommitDialogView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
import java.util.List;

import javafx.application.Platform;
import javafx.event.ActionEvent;
import javafx.fxml.FXML;
import javafx.scene.control.Button;
import javafx.scene.control.ButtonType;
import javafx.scene.control.TextArea;

Expand All @@ -29,6 +31,7 @@ public class GitCommitDialogView extends BaseDialog<Void> {

@FXML private TextArea commitMessage;
@FXML private ButtonType commitButton;
@FXML private ButtonType showDiffButton;

private GitCommitDialogViewModel viewModel;

Expand All @@ -45,6 +48,11 @@ public GitCommitDialogView() {
ViewLoader.view(this)
.load()
.setAsDialogPane(this);
Button showDiff = (Button) this.getDialogPane().lookupButton(showDiffButton);
showDiff.addEventFilter(ActionEvent.ACTION, event -> {
event.consume();
showDiff();
Comment on lines +51 to +54

@qodo-free-for-open-source-projects qodo-free-for-open-source-projects Bot Sep 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

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

});
Comment on lines +51 to +55

@calixtus calixtus Aug 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ControlHelper.setAction?

@GRV-Satya-Ganesh GRV-Satya-Ganesh Aug 30, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@koppor koppor Aug 31, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +51 to +55

@pluto-han pluto-han Aug 30, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why add this piece since the purpose is only to move the button position

@GRV-Satya-Ganesh GRV-Satya-Ganesh Aug 30, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@calixtus calixtus Aug 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@calixtus calixtus Aug 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also fully ai generated text have to be marked with a 🤖

@calixtus calixtus Aug 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

@GRV-Satya-Ganesh GRV-Satya-Ganesh Aug 30, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

@FXML
Expand Down Expand Up @@ -82,7 +90,6 @@ private void initialize() {
}

// [impl->req~ux.git-commit.preview-current-library~1]
@FXML
private void showDiff() {
viewModel.diffTask()
.onSuccess(this::openDiffDialog)
Expand Down
7 changes: 1 addition & 6 deletions jabgui/src/main/resources/org/jabref/gui/git/GitCommitDialog.fxml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@
<?import javafx.scene.control.DialogPane?>
<?import javafx.scene.control.TextArea?>
<?import javafx.scene.layout.VBox?>
<?import javafx.scene.layout.HBox?>
<?import javafx.scene.control.Button?>
<DialogPane
xmlns="http://javafx.com/javafx"
xmlns:fx="http://javafx.com/fxml"
Expand All @@ -20,17 +18,14 @@
<Insets top="20" right="20" bottom="20" left="20"/>
</padding>

<HBox alignment="CENTER_RIGHT">
<Button fx:id="showDiffButton" text="%Show diff" onAction="#showDiff"/>
</HBox>

<TextArea fx:id="commitMessage"
prefRowCount="6"
wrapText="true"
VBox.vgrow="ALWAYS"/>
</VBox>
</content>
<buttonTypes>
<ButtonType fx:id="showDiffButton" text="%Show diff" buttonData="LEFT"/>
<ButtonType fx:id="commitButton" text="%Commit" buttonData="OK_DONE"/>
<ButtonType fx:constant="CANCEL"/>
</buttonTypes>
Expand Down
Loading