-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Refine "File exists" warning #13491
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
Refine "File exists" warning #13491
Conversation
koppor
left a comment
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.
Code looks OK - someone needs to try out.
|
|
||
| Do\ not\ wrap\ when\ saving=Do not wrap when saving | ||
|
|
||
| File\ already\ exists=File already exists |
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.
Pleae group to line 2999
(there is File '%0' already exists. Overwriting.)
| // Build the dialog window | ||
| Dialog<ButtonType> dialog = new Dialog<>(); | ||
| dialog.setTitle(Localization.lang("File already exists")); | ||
| dialog.setContentText(Localization.lang("File name: \n'%0'", targetFileName)); | ||
| dialog.getDialogPane().getButtonTypes().addAll(Replace, keepBoth, provideAlternative, cancel); |
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 don't think this should be in viewmodel
| ButtonType cancel = new ButtonType(Localization.lang("Cancel"), ButtonBar.ButtonData.OTHER); | ||
|
|
||
| // Build the dialog window | ||
| Dialog<ButtonType> dialog = new Dialog<>(); |
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 rework. Use org.jabref.gui.JabRefDialogService. Please do not create dialogs on your own. - For example, look at the removed code 😅
MVVM is about testability.
This will also help to test this --> try to craft a test case after you use the JabRefDialogService :)
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 created a custom modal because the current JabRefDialogService methods don’t support tooltips on buttons.
Should I add a new function to JabRefDialogService for this, or is there a better approach you’d recommend?
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.
Yes please add a new function, maybe with a supplier as parameter for the tool tips
| ButtonType replace = new ButtonType(Localization.lang("Replace"), ButtonBar.ButtonData.OTHER); | ||
| ButtonType keepBoth = new ButtonType(Localization.lang("Keep both"), ButtonBar.ButtonData.OTHER); | ||
| ButtonType provideAlternative = new ButtonType(Localization.lang("Provide alternative file name"), ButtonBar.ButtonData.OTHER); | ||
| ButtonType cancel = new ButtonType(Localization.lang("Cancel"), ButtonBar.ButtonData.OTHER); |
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.
should be cancel close for button data
|
@trag-bot didn't find any issues in the code! ✅✨ |
Pull Request is not mergeable
| Do\ not\ wrap\ when\ saving=Do not wrap when saving | ||
|
|
||
| File\ already\ exists=File already exists | ||
| File\ name\:\ \n'%0'=File name: \n'%0' |
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.
There should be no space before newline
Maybe you can submit a follow-up PR?
* upstream/main: Bump com.squareup.okhttp3:okhttp from 5.0.0 to 5.1.0 in /versions (#13541) Fixed Issue 13418: "Search ShortSience" should do a latex-to-unicode conversion (#13532) New Crowdin updates (#13548) Add focus to field Link in the "Add file link" dialog. (#13520) Bump org.ow2.asm:asm from 9.6 to 9.8 in /versions (#13547) Make validation optional for saving library properties preferences (#13488) Bump org.apache.commons:commons-lang3 from 3.17.0 to 3.18.0 in /versions (#13546) --porcelain does not output any info or warn errors on the console (#13469) Bump jablib/src/main/resources/csl-locales from `7e137db` to `3bad433` (#13545) Refine "File exists" warning (#13491) Bump org.openrewrite.recipe:rewrite-recipe-bom from 3.8.0 to 3.11.1 (#13544) Bump org.openrewrite.rewrite from 7.8.0 to 7.11.0 (#13542) Bump jablib/src/main/resources/csl-styles from `704ff9f` to `59f124d` (#13540) Add architecture test for logging infrastructure (#13534) More harsh test on title of PR New Crowdin updates (#13533) Update extra-java-module-info plugin (#13527) [Junie]: fix: display help output for --help argument (#13446) Refactor OpenExternalFileAction (#13508)
fixes #12565 refine file exists warning
Mandatory checks
CHANGELOG.mddescribed in a way that is understandable for the average user (if change is visible to the user)