Skip to content

#868: add apply-button for Key Signature menu #889

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

Open
serval2412 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

#868: add apply-button for Key Signature menu #889

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
15 changes: 12 additions & 3 deletions desktop/TuxGuitar/src/app/tuxguitar/app/view/dialog/keysignature/TGKeySignatureDialog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,16 +116,25 @@ public void onSelect(UISelectionEvent event) {
buttons.setLayout(buttonsLayout);
dialogLayout.set(buttons, 3, 1, UITableLayout.ALIGN_RIGHT, UITableLayout.ALIGN_FILL, true, true);

final UIButton buttonApply = uiFactory.createButton(buttons);
buttonApply.setText(TuxGuitar.getProperty("apply"));
buttonApply.setDefaultButton();
Copy link
Collaborator

@guiv42 guiv42 Sep 16, 2025

Choose a reason for hiding this comment

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

Keeping "OK" as the default button seems more logical

Copy link
Contributor Author

@serval2412 serval2412 Sep 16, 2025

Choose a reason for hiding this comment

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

Thank you for the review.

I thought "Apply" as default would be more logical since it allows to test quickly different key signatures.
I mean with the mouse you select the key signature then just type "enter" key so it applies without closing the dialog (1 hand for each action). If you want to test several key signatures, you won't have to loop between going with the mouse to select another key signature then going to the "Apply" button until you find the right one.
Now, perhaps I missed the keyboard shortcut to select "Apply" button?

Copy link
Collaborator

@guiv42 guiv42 Sep 16, 2025

Choose a reason for hiding this comment

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

I understand your point, but "OK" has always been the default button, and many users will not use "Apply" at all, so I prefer not to change that

Copy link
Contributor Author

@serval2412 serval2412 Sep 16, 2025

Choose a reason for hiding this comment

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

Ok from my point of view not putting "Apply" as by default action is counterintuitive and it decreases the interest of this patch, I prefer abandoning this PR.

Copy link
Collaborator

@guiv42 guiv42 Sep 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm sorry but I don't understand.
Your PR is clearly valuable, I just want to avoid confusing users who are used to dialogs without "apply" option.
Edit: I just asked the original author of associated issue

Copy link
Contributor Author

@serval2412 serval2412 Sep 16, 2025

Choose a reason for hiding this comment

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

I don't want to put my name on a patch I partly disagree. I mean, in our jobs, we must do compromises. Here it's not my job just a opensource project where I'm just providing a casual and straightforward patch and don't want to make some compromise.
I agree with you some people would complain but perhaps other people would be satisfied. Now even if 99% would be against but this change, I prefer not pushing this with OK by default.

Now you can obviously copy paste the code, put the default on OK button and push with your name, no pb for me.

Since you reopened this PR, I'll let you accept it with "Apply" by default or close it when you feel like.

Copy link
Collaborator

@guiv42 guiv42 Sep 16, 2025

Choose a reason for hiding this comment

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

OK, as you want.
I'll try to get some other users opinions before a decision.

buttonApply.addSelectionListener(new UISelectionListener() {
public void onSelect(UISelectionEvent event) {
changeKeySignature(context.getContext(), track, measure, keySignatures.getSelectedValue(), beatRange, applyToSelection.isSelected(), toEnd.isSelected());
}
});
buttonsLayout.set(buttonApply, 1, 1, UITableLayout.ALIGN_FILL, UITableLayout.ALIGN_FILL, true, true, 1, 1, 80f, 25f, null);

final UIButton buttonOK = uiFactory.createButton(buttons);
buttonOK.setText(TuxGuitar.getProperty("ok"));
buttonOK.setDefaultButton();
buttonOK.addSelectionListener(new UISelectionListener() {
public void onSelect(UISelectionEvent event) {
changeKeySignature(context.getContext(), track, measure, keySignatures.getSelectedValue(), beatRange, applyToSelection.isSelected(), toEnd.isSelected());
dialog.dispose();
}
});
buttonsLayout.set(buttonOK, 1, 1, UITableLayout.ALIGN_FILL, UITableLayout.ALIGN_FILL, true, true, 1, 1, 80f, 25f, null);
buttonsLayout.set(buttonOK, 1, 2, UITableLayout.ALIGN_FILL, UITableLayout.ALIGN_FILL, true, true, 1, 1, 80f, 25f, null);

UIButton buttonCancel = uiFactory.createButton(buttons);
buttonCancel.setText(TuxGuitar.getProperty("cancel"));
Expand All @@ -134,7 +143,7 @@ public void onSelect(UISelectionEvent event) {
dialog.dispose();
}
});
buttonsLayout.set(buttonCancel, 1, 2, UITableLayout.ALIGN_FILL, UITableLayout.ALIGN_FILL, true, true, 1, 1, 80f, 25f, null);
buttonsLayout.set(buttonCancel, 1, 3, UITableLayout.ALIGN_FILL, UITableLayout.ALIGN_FILL, true, true, 1, 1, 80f, 25f, null);
buttonsLayout.set(buttonCancel, UITableLayout.MARGIN_RIGHT, 0f);

TGDialogUtil.openDialog(dialog,TGDialogUtil.OPEN_STYLE_CENTER | TGDialogUtil.OPEN_STYLE_PACK);
Expand Down