Skip to content
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

12565 refine file exists warning #12625

Open
wants to merge 37 commits into
base: main
Choose a base branch
from

Conversation

Oscar2343
Copy link

@Oscar2343 Oscar2343 commented Mar 5, 2025

Refines the "File exists" warning
Adds "Keep both" button with tooltip
Adds "Provide alternative file name" button that triggers a renaming box
Improves title and context message
419519068-b5d2d17c-c98a-49ae-a123-c1e6867551fc
Closes #12565

Mandatory checks

  • I own the copyright of the code submitted and I licence it under the MIT license
  • Change in CHANGELOG.md described in a way that is understandable for the average user (if change is visible to the user)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked developer's documentation: Is the information available and up to date? If not, I outlined it in this pull request.
  • Checked documentation: Is the information available and up to date? If not, I created an issue at https://github.com/JabRef/user-documentation/issues or, even better, I submitted a pull request to the documentation repository.

Oscar2343 and others added 28 commits February 27, 2025 14:27
enhancement: Changed error box dialog to Taget file already exists #3
Suggested filename is generated for displaying
when hovering over "Keep both" button.
The action of the button has been implemented.
The content of the message box is no longer editable. The look has also been changed for the better.
Triggers the functionality "Rename file to a given name" from context menu.
Placeholder triggers action.
Added the buttons without functionality or tool tip #6
closes #6
…le-name

Refactor functionality to the button
Refactor name of the button from alternativeFileNameButton to provideAltFileNameButton
After merging, the function performRenameWithConflictCheck() was changed to use a different helper function, thereby making the textbox editable once again. This had to be updated.
1. make use of the passed AlertType instead of hardcoding
2. fix bug where pressing the red 'x' button on the textbox does nothing
Make textbox content un-editable
Fix message box display issue for long file names ( #4 )
Add functionality for button "Provide alternative file name"
…th-button

13 add hover tooltip for keep both button
closes #13
Copy link
Contributor

@github-actions github-actions bot left a 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.
In case of issues with the import order, double check that you activated Auto Import. You can trigger fixing imports by pressing Ctrl+Alt+O to trigger Optimize Imports.

Copy link
Contributor

@github-actions github-actions bot left a 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.

Copy link
Contributor

@github-actions github-actions bot left a 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.
In case of issues with the import order, double check that you activated Auto Import. You can trigger fixing imports by pressing Ctrl+Alt+O to trigger Optimize Imports.

Copy link
Contributor

@github-actions github-actions bot left a 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 OpenRewrite to ensure "modern" Java coding practices. The issues found can be automatically fixed. Please execute the gradle task rewriteRun, check the results, commit, and push.
You can check the detailed error output by navigating to your pull request, selecting the tab "Checks", section "Tests" (on the left), subsection "OpenRewrite".

Copy link

trag-bot bot commented Mar 6, 2025

@trag-bot didn't find any issues in the code! ✅✨

Copy link

trag-bot bot commented Mar 6, 2025

Pull request summary

  • Added a "Keep both" option in the file renaming dialog to handle conflicts when a file with the same name already exists.
  • Introduced a "Provide alternative file name" option for users to specify a new name when a conflict occurs.
  • Enhanced the dialog interface to display a tooltip with a suggested file name when hovering over the "Keep both" button.
  • Updated the performRenameWithConflictCheck method to accommodate the new options and improve user interaction during file renaming.
  • Created unit tests to validate the new renaming functionality, including scenarios for no conflict, overriding, keeping both files, and providing an alternative name.
  • Modified the localization properties to include new strings related to the file renaming process.
  • Updated the changelog to reflect the new features and improvements made in this pull request.

@tsukii7
Copy link

tsukii7 commented Mar 7, 2025

Updated screenshots for this PR (for UI changes):

refine_warning

dialogPane.setPrefSize(700, 200);
dialogPane.setContent(contentLabel);

ButtonType overrideButton = new ButtonType("Override", ButtonBar.ButtonData.OTHER);
Copy link
Member

@Siedlerchr Siedlerchr Mar 7, 2025

Choose a reason for hiding this comment

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

Add proper Localization for these button types to all buttons as well using Localization.lang

if (btnKeepBoth != null) {
Tooltip tooltip = new Tooltip(Localization.lang("New name: %0", suggestedFileName));
tooltip.setShowDelay(Duration.millis(100));
tooltip.setStyle("-fx-max-width: 20px; -fx-wrap-text: true; -fx-background-color: #FFFFE0;");
Copy link
Member

Choose a reason for hiding this comment

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

px values dont scale. use em
dont manually set colors. use constants from base.css

Copy link
Member

Choose a reason for hiding this comment

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

put the whole style into base.css and only set a style class for this tooltip

Copy link
Member

Choose a reason for hiding this comment

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

Undo


@BeforeEach
void setUp() {
MockitoAnnotations.openMocks(this);
Copy link
Member

Choose a reason for hiding this comment

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

if there is no mock annotation, this line has no effect.

Comment on lines +214 to +224
@AfterEach
void tearDown() throws IOException {
// Clean up any real files after each test
Files.walk(tempDir)
.map(Path::toFile)
.forEach(file -> {
if (!file.isDirectory()) {
file.delete();
}
});
}
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this do JUnit automatically with TempDirs?

@ryan-carpenter
Copy link

You can simplify the title as
"File already exists"

and clarify the prompt with
"A file with this name already exists"

"Override" is an incorrect term probably meant as "Overwrite", but most examples use
"Replace" and "Keep both" or
"Cancel", "Replace", and "Keep both".

If the last button is important, consider using
"Choose another name"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refine "File exists" warning
8 participants