diff --git a/CHANGELOG.md b/CHANGELOG.md index 1249a7772d8..33bcd326279 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,8 @@ Note that this project **does not** adhere to [Semantic Versioning](https://semv - We added a new "Copy citation (text)" button in the context menu of the preview. [#12551](https://github.com/JabRef/jabref/issues/12551) - We added a new "Export to clipboard" button in the context menu of the preview. [#12551](https://github.com/JabRef/jabref/issues/12551) - We added an integrity check if a URL appears in a title. [#12354](https://github.com/JabRef/jabref/issues/12354) +- We added "Keep both" and "Provide alternative file name" options and improved dialog when renaming a file to an existing file. [#12565](https://github.com/JabRef/jabref/issues/12565) + ### Changed diff --git a/src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java b/src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java index 3505d1dfd10..e4828182c01 100644 --- a/src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java +++ b/src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java @@ -9,6 +9,7 @@ import java.util.Optional; import java.util.function.BiPredicate; +import javafx.application.Platform; import javafx.beans.Observable; import javafx.beans.binding.Bindings; import javafx.beans.binding.ObjectBinding; @@ -18,6 +19,13 @@ import javafx.beans.property.SimpleDoubleProperty; import javafx.beans.property.StringProperty; import javafx.scene.Node; +import javafx.scene.control.Button; +import javafx.scene.control.ButtonBar; +import javafx.scene.control.ButtonType; +import javafx.scene.control.DialogPane; +import javafx.scene.control.Label; +import javafx.scene.control.Tooltip; +import javafx.util.Duration; import org.jabref.gui.AbstractViewModel; import org.jabref.gui.DialogService; @@ -37,6 +45,7 @@ import org.jabref.logic.externalfiles.LinkedFileHandler; import org.jabref.logic.l10n.Localization; import org.jabref.logic.util.TaskExecutor; +import org.jabref.logic.util.io.FileNameUniqueness; import org.jabref.logic.util.io.FileUtil; import org.jabref.model.database.BibDatabaseContext; import org.jabref.model.entry.BibEntry; @@ -263,18 +272,60 @@ public void renameFileToName(String targetFileName) { } } - private void performRenameWithConflictCheck(String targetFileName) { + public void performRenameWithConflictCheck(String targetFileName) { Optional existingFile = linkedFileHandler.findExistingFile(linkedFile, entry, targetFileName); boolean overwriteFile = false; if (existingFile.isPresent()) { - overwriteFile = dialogService.showConfirmationDialogAndWait( - Localization.lang("File exists"), - Localization.lang("'%0' exists. Overwrite file?", targetFileName), - Localization.lang("Overwrite")); + Path existingFileVal = existingFile.get(); + // show when hovering over "Keep both" button + String suggestedFileName = FileNameUniqueness.getNonOverWritingFileName( + existingFileVal.getParent(), + existingFileVal.getFileName().toString() + ); + + Label contentLabel = new Label(Localization.lang("Target file name: \n'%0'", targetFileName)); + contentLabel.setWrapText(true); + DialogPane dialogPane = new DialogPane(); + dialogPane.setPrefSize(700, 200); + dialogPane.setContent(contentLabel); + + ButtonType overrideButton = new ButtonType("Override", ButtonBar.ButtonData.OTHER); + ButtonType keepBothButton = new ButtonType("Keep both", ButtonBar.ButtonData.OTHER); + ButtonType provideAltFileNameButton = new ButtonType("Provide alternative file name", ButtonBar.ButtonData.OTHER); + + dialogPane.getButtonTypes().addAll(overrideButton, keepBothButton, provideAltFileNameButton); + + dialogPane.sceneProperty().addListener((obs, oldScene, newScene) -> { + if (newScene != null) { + Platform.runLater(() -> { + Button btnKeepBoth = (Button) dialogPane.lookupButton(keepBothButton); + 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;"); + Tooltip.install(btnKeepBoth, tooltip); + } + }); + } + }); - if (!overwriteFile) { - return; + Optional result = dialogService.showCustomDialogAndWait( + Localization.lang("Target file already exists"), + dialogPane, + overrideButton, keepBothButton, provideAltFileNameButton + ); + + if (result.isPresent()) { + ButtonType buttonType = result.get(); + if (buttonType == overrideButton) { + overwriteFile = true; + } else if (buttonType == keepBothButton) { + targetFileName = suggestedFileName; + } else if (buttonType == provideAltFileNameButton) { + askForNameAndRename(); + return; + } } } diff --git a/src/main/resources/csl-styles b/src/main/resources/csl-styles index 44d9b9f778e..5336f24b78e 160000 --- a/src/main/resources/csl-styles +++ b/src/main/resources/csl-styles @@ -1 +1 @@ -Subproject commit 44d9b9f778ea3b947bf381ed8c7269acff31db84 +Subproject commit 5336f24b78ea631289902f89c157fea9224c57b7 diff --git a/src/main/resources/l10n/JabRef_en.properties b/src/main/resources/l10n/JabRef_en.properties index 2dc63ed0fd7..c4e94a8601b 100644 --- a/src/main/resources/l10n/JabRef_en.properties +++ b/src/main/resources/l10n/JabRef_en.properties @@ -2868,3 +2868,7 @@ Author\ related=Author related Editor\ related=Editor related Title\ related=Title related Entry\ fields=Entry fields + +New\ name\:\ %0=New name: %0 +Target\ file\ already\ exists=Target file already exists +Target\ file\ name\:\ \n'%0'=Target file name: \n'%0' diff --git a/src/test/java/org/jabref/gui/fieldeditors/PerformRenameWithConflictCheckTest.java b/src/test/java/org/jabref/gui/fieldeditors/PerformRenameWithConflictCheckTest.java new file mode 100644 index 00000000000..223008bf67d --- /dev/null +++ b/src/test/java/org/jabref/gui/fieldeditors/PerformRenameWithConflictCheckTest.java @@ -0,0 +1,225 @@ +package test.java.org.jabref.gui.fieldeditors; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; +import java.util.Optional; +import java.util.TreeSet; + +import javafx.collections.FXCollections; +import javafx.scene.control.ButtonType; +import javafx.scene.control.DialogPane; + +import org.jabref.gui.DialogService; +import org.jabref.gui.externalfiletype.ExternalFileTypes; +import org.jabref.gui.fieldeditors.LinkedFileViewModel; +import org.jabref.gui.frame.ExternalApplicationsPreferences; +import org.jabref.gui.preferences.GuiPreferences; +import org.jabref.logic.FilePreferences; +import org.jabref.logic.externalfiles.LinkedFileHandler; +import org.jabref.logic.util.TaskExecutor; +import org.jabref.logic.util.io.FileNameUniqueness; +import org.jabref.logic.xmp.XmpPreferences; +import org.jabref.model.database.BibDatabaseContext; +import org.jabref.model.entry.BibEntry; +import org.jabref.model.entry.LinkedFile; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.api.io.TempDir; +import org.mockito.MockitoAnnotations; +import org.testfx.framework.junit5.ApplicationExtension; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +@ExtendWith(ApplicationExtension.class) +public class PerformRenameWithConflictCheckTest { + + @TempDir + Path tempDir; // JUnit 5 annotation for creating temporary files/folders + + private LinkedFile linkedFile; + private BibEntry entry; + private BibDatabaseContext databaseContext; + private TaskExecutor taskExecutor; + private DialogService dialogService; + private LinkedFileHandler linkedFileHandler; + private LinkedFileViewModel viewModel; + + private final DialogService dialogServiceMock = mock(DialogService.class); + private final LinkedFileHandler linkedFileHandlerMock = mock(LinkedFileHandler.class); + + private final ExternalApplicationsPreferences externalApplicationsPreferences = mock(ExternalApplicationsPreferences.class); + private final FilePreferences filePreferences = mock(FilePreferences.class); + private final GuiPreferences preferences = mock(GuiPreferences.class); + + @BeforeEach + void setUp() { + MockitoAnnotations.openMocks(this); + + // Mock dependencies + // linkedFile = mock(LinkedFile.class); + entry = mock(BibEntry.class); + databaseContext = mock(BibDatabaseContext.class); + taskExecutor = mock(TaskExecutor.class); + dialogService = mock(DialogService.class); + linkedFileHandler = mock(LinkedFileHandler.class); + + // Mock preferences + when(externalApplicationsPreferences.getExternalFileTypes()).thenReturn(FXCollections.observableSet(new TreeSet<>(ExternalFileTypes.getDefaultExternalFileTypes()))); + when(filePreferences.confirmDeleteLinkedFile()).thenReturn(true); + when(preferences.getExternalApplicationsPreferences()).thenReturn(externalApplicationsPreferences); + when(preferences.getFilePreferences()).thenReturn(filePreferences); + when(preferences.getXmpPreferences()).thenReturn(mock(XmpPreferences.class)); + + // Mock database context behavior (use a temporary path) + when(databaseContext.getFileDirectories(any())).thenReturn(List.of(tempDir)); + + // Create temporary file + Path existingFile = tempDir.resolve("existingFile.pdf"); + Assertions.assertDoesNotThrow(() -> { + Files.createFile(existingFile); // Create the real file + }, "Failed to create temporary file: "); + + Path fileToRename = tempDir.resolve("fileToRename.pdf"); + Assertions.assertDoesNotThrow(() -> { + Files.createFile(fileToRename); // Create the real file + }, "Failed to create temporary file: "); + + // Create a real LinkedFile object + LinkedFile linkedFile = new LinkedFile("desc", fileToRename, "pdf"); + + if (linkedFile.getLink() == null) { + throw new IllegalStateException("linkedFile.getLink() returned null"); + } + + viewModel = new LinkedFileViewModel(linkedFile, entry, databaseContext, taskExecutor, dialogServiceMock, preferences); + } + + // Test 1: No conflict, renames the file + @Test + void performRenameWithConflictCheckWhenNoConflictRenamesFile() throws IOException { + Path newFileName = tempDir.resolve("newFileName.pdf"); + + // Simulate no conflict, check that the file doesn't exist before renaming + assertFalse(Files.exists(newFileName), "File should not exist before renaming"); + + // Call the method to rename the file + viewModel.performRenameWithConflictCheck(newFileName.getFileName().toString()); + + // Verify the new file has been renamed + assertTrue(Files.exists(newFileName), "File should be renamed successfully"); + assertFalse(Files.exists(tempDir.resolve("fileToRename.pdf")), "Original file should be renamed"); + } + + // Test 2: File exists, user chooses to override + @Test + void performRenameWithConflictCheckWhenOverrideChosenOverwritesFile() throws IOException { + + Path existingFilePath = tempDir.resolve("existingFile.pdf"); + Path fileToRename = tempDir.resolve("fileToRename.pdf"); + + // Simulate user choosing "Keep both" + when(dialogServiceMock.showCustomDialogAndWait( + anyString(), + any(DialogPane.class), + any(ButtonType.class), + any(ButtonType.class), + any(ButtonType.class))) + .thenAnswer(invocation -> Optional.of(invocation.getArgument(2))); // override is argument index 2 + + // Call the method + viewModel.performRenameWithConflictCheck(existingFilePath.getFileName().toString()); + + // Verify the file is overwritten + assertTrue(Files.exists(existingFilePath), "The file should be overwritten"); + } + + // Test 3: File exists, user chooses "Keep both" + @Test + void performRenameWithConflictCheckWhenKeepBothChosenKeepsBothFiles() throws IOException { + + Path existingFilePath = tempDir.resolve("existingFile.pdf"); + Path fileToRename = tempDir.resolve("fileToRename.pdf"); + + // Simulate user choosing "Keep both" + when(dialogServiceMock.showCustomDialogAndWait( + anyString(), + any(DialogPane.class), + any(ButtonType.class), + any(ButtonType.class), + any(ButtonType.class))) + .thenAnswer(invocation -> Optional.of(invocation.getArgument(3))); // keepBothButton is argument index 3 + + // Expected new file name (generated using `getNonOverWritingFileName`) + String newFileName = FileNameUniqueness.getNonOverWritingFileName(tempDir, existingFilePath.getFileName().toString()); + Path expectedNewFilePath = tempDir.resolve(newFileName); + + // Call the method + viewModel.performRenameWithConflictCheck(existingFilePath.getFileName().toString()); + + // Verify that both files exist and the new file is renamed correctly + assertTrue(Files.exists(existingFilePath), "The existing file should still exist"); + assertTrue(Files.exists(expectedNewFilePath), "The renamed file should exist with a unique name"); + // Verify that the original fileToRename no longer exists + assertFalse(Files.exists(fileToRename), "The original fileToRename should not exist anymore"); + } + + + // Test 4: File exists, user chooses to provide an alternative file name + @Test + void performRenameWithConflictCheckWhenProvideAlternativeFileNameChosenUsesUserInput() { + Path existingFilePath = tempDir.resolve("existingFile.pdf"); + Path fileToRename = tempDir.resolve("fileToRename.pdf"); + + // Simulate the user selecting "Provide alternative file name" + when(dialogServiceMock.showCustomDialogAndWait( + anyString(), + any(DialogPane.class), + any(ButtonType.class), + any(ButtonType.class), + any(ButtonType.class))) + .thenAnswer(invocation -> Optional.of(invocation.getArgument(4))); // provideAltFileNameButton is index 4 + + // Simulate the user entering a new file name + String userProvidedFileName = "newFileName.pdf"; + when(dialogServiceMock.showInputDialogWithDefaultAndWait(anyString(), anyString(), anyString())) + .thenReturn(Optional.of(userProvidedFileName)); + + // Call the method + viewModel.performRenameWithConflictCheck(existingFilePath.getFileName().toString()); + + // Expected new path based on user input + Path expectedNewFilePath = tempDir.resolve(userProvidedFileName); + + // Check that the renamed file exists + assertTrue(Files.exists(expectedNewFilePath), "The file should be renamed to the user-provided name"); + + // Verify that the original fileToRename no longer exists + assertFalse(Files.exists(fileToRename), "The original file should not exist anymore"); + + // Ensure the conflicting file was not overwritten + assertTrue(Files.exists(existingFilePath), "The existing file should still be there"); + } + + @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(); + } + }); + } +}