-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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.
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
Added keybinding for renaming group with F2 #12159
base: main
Are you sure you want to change the base?
Conversation
Hello. I added the Rename Group to the context menu with keybinding (for now I set it to the letter R, because I couldn't try F2 locally, I will change it to F2 in the finale). I just have one problem, that when I change the name of the group, the groups become smaller (I will also send a video example). Can you advise me why it does this and how I could fix it? Video : https://github.com/user-attachments/assets/396d439e-ca00-451c-991b-43f12666bc72 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your code currently does not meet JabRef's code guidelines.
We use Checkstyle to identify issues.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.
Code wise looks okay to me so far, but I haven't an idea why the group size changes. Does this happen with the normal dialog as well? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your code currently does not meet JabRef's code guidelines.
We use Checkstyle to identify issues.
Please carefully follow the setup guide for the codestyle.
Afterwards, please run checkstyle locally and fix the issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JUnit tests are failing. In the area "Some checks were not successful", locate "Tests / Unit tests (pull_request)" and click on "Details". This brings you to the test output.
You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide.
/** | ||
* Opens "Rename Group Dialog" and change name of group | ||
*/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the empt yline?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed empty line
public void renameGroup(GroupNodeViewModel oldGroup) { | ||
currentDatabase.ifPresent(database -> { | ||
AbstractGroup oldGroupDef = oldGroup.getGroupNode().getGroup(); | ||
String oldGroupName = oldGroupDef.getName(); // Zachytenie starého názvu pred otvorením dialógu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please translate the comment (or remove it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
return; | ||
} | ||
|
||
// check if is not include "," |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove comment - the next line says the same.
The WHY is important. Why is this check done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello. I removed this comment. I check this because I inspired by Edit group where this check too. I can remove this if-statement if you want.
return; | ||
} | ||
|
||
// chceck if old group name dont equels with new group name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove comment - next line says it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
nameField = new TextField(); | ||
nameField.setPromptText("Enter new group name"); | ||
|
||
// add Input |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed
getDialogPane().getButtonTypes().setAll(ButtonType.OK, ButtonType.CANCEL); | ||
|
||
nameField = new TextField(); | ||
nameField.setPromptText("Enter new group name"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use Localiaztion.lang - see https://devdocs.jabref.org/code-howtos/localization.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
nameField.setPromptText("Enter new group name"); | ||
|
||
// add Input | ||
VBox vbox = new VBox(new Label("New group name:"), nameField); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use Localiaztion.lang - see https://devdocs.jabref.org/code-howtos/localization.html
No ":"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
VBox vbox = new VBox(new Label("New group name:"), nameField); | ||
getDialogPane().setContent(vbox); | ||
|
||
// If press OK change name else return null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed
if (buttonType == ButtonType.OK) { | ||
return resultConverter(ButtonType.OK).orElse(null); | ||
} else { | ||
// Ak sa zvolí Cancel alebo sa dialóg zavrie cez X |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
translate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JUnit tests are failing. In the area "Some checks were not successful", locate "Tests / Unit tests (pull_request)" and click on "Details". This brings you to the test output.
You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide.
Hello @koppor @Siedlerchr . I have a small problem with Unit Test findMissingLocalizationKeys(). I added strings to JabRef_en.properties, but this test on my local computer still shows error that I dont have string in this file. Can someone tell, what I have done wrong please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JUnit tests are failing. In the area "Some checks were not successful", locate "Tests / Unit tests (pull_request)" and click on "Details". This brings you to the test output.
You can then run these tests in IntelliJ to reproduce the failing tests locally. We offer a quick test running howto in the section Final build system checks in our setup guide.
That looks correct at first sight . Maybe some caching. Can you push the modifications? Then we can see if it works on CI |
Describe the changes you have made here: what, why, ...
Link the issue that will be closed, e.g., "Closes #333". If your PR closes a koppor issue, link it using its URL, e.g., "Closes koppor#47".
Mandatory checks
CHANGELOG.md
described in a way that is understandable for the average user (if change is visible to the user)