From f392da23b37d9161e079c6d777a9da726752a315 Mon Sep 17 00:00:00 2001 From: Siedlerchr Date: Thu, 21 Nov 2024 21:06:24 +0100 Subject: [PATCH 1/7] move check for invalid characters to linked files editor Fixes #12212 some cleanup --- .../gui/fieldeditors/LinkedFilesEditor.java | 13 +++--- .../LinkedFilesEditorViewModel.java | 44 +------------------ .../LinkedFileEditDialogViewModel.java | 32 ++++++++++++-- .../org/jabref/logic/util/io/FileUtil.java | 11 ++--- 4 files changed, 39 insertions(+), 61 deletions(-) diff --git a/src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditor.java b/src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditor.java index ea30f18d3c9..8b54c6c7496 100644 --- a/src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditor.java +++ b/src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditor.java @@ -253,13 +253,10 @@ private void setUpKeyBindings() { listView.addEventFilter(KeyEvent.KEY_PRESSED, event -> { Optional keyBinding = preferences.getKeyBindingRepository().mapToKeyBinding(event); if (keyBinding.isPresent()) { - switch (keyBinding.get()) { - case DELETE_ENTRY: - deleteAttachedFilesWithConfirmation(); - event.consume(); - break; - default: - // Pass other keys to children + // Pass other keys to children + if (keyBinding.get() == KeyBinding.DELETE_ENTRY) { + deleteAttachedFilesWithConfirmation(); + event.consume(); } } }); @@ -287,7 +284,7 @@ public Parent getNode() { @FXML private void addNewFile() { - dialogService.showCustomDialogAndWait(new LinkedFileEditDialog()).ifPresent(newLinkedFile -> { + dialogService.showCustomDialogAndWait(new LinkedFileEditDialog()).filter(file -> !file.isEmpty()).ifPresent(newLinkedFile -> { viewModel.addNewLinkedFile(newLinkedFile); }); } diff --git a/src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditorViewModel.java b/src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditorViewModel.java index 540e5603d36..4e8340641c5 100644 --- a/src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditorViewModel.java +++ b/src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditorViewModel.java @@ -4,7 +4,6 @@ import java.net.MalformedURLException; import java.net.URI; import java.net.URL; -import java.nio.file.Files; import java.nio.file.Path; import java.util.ArrayList; import java.util.List; @@ -140,40 +139,6 @@ public ListProperty filesProperty() { return files; } - public void addNewFile() { - Path workingDirectory = databaseContext.getFirstExistingFileDir(preferences.getFilePreferences()) - .orElse(preferences.getFilePreferences().getWorkingDirectory()); - - FileDialogConfiguration fileDialogConfiguration = new FileDialogConfiguration.Builder() - .withInitialDirectory(workingDirectory) - .build(); - - List fileDirectories = databaseContext.getFileDirectories(preferences.getFilePreferences()); - List selectedFiles = dialogService.showFileOpenDialogAndGetMultipleFiles(fileDialogConfiguration); - - for (Path fileToAdd : selectedFiles) { - if (FileUtil.detectBadFileName(fileToAdd.toString())) { - String newFilename = FileNameCleaner.cleanFileName(fileToAdd.getFileName().toString()); - - boolean correctButtonPressed = dialogService.showConfirmationDialogAndWait(Localization.lang("File \"%0\" cannot be added!", fileToAdd.getFileName()), - Localization.lang("Illegal characters in the file name detected.\nFile will be renamed to \"%0\" and added.", newFilename), - Localization.lang("Rename and add")); - - if (correctButtonPressed) { - Path correctPath = fileToAdd.resolveSibling(newFilename); - try { - Files.move(fileToAdd, correctPath); - addNewLinkedFile(correctPath, fileDirectories); - } catch (IOException ex) { - LOGGER.error("Error moving file", ex); - dialogService.showErrorDialogAndWait(ex); - } - } - } else { - addNewLinkedFile(fileToAdd, fileDirectories); - } - } - } public void addNewLinkedFile(LinkedFile linkedFile) { files.add(new LinkedFileViewModel( @@ -185,22 +150,17 @@ public void addNewLinkedFile(LinkedFile linkedFile) { preferences)); } - private void addNewLinkedFile(Path correctPath, List fileDirectories) { - LinkedFile newLinkedFile = fromFile(correctPath, fileDirectories, preferences.getExternalApplicationsPreferences()); - addNewLinkedFile(newLinkedFile); - } - @Override public void bindToEntry(BibEntry entry) { super.bindToEntry(entry); - if ((entry != null) && preferences.getEntryEditorPreferences().autoLinkFilesEnabled()) { + if (preferences.getEntryEditorPreferences().autoLinkFilesEnabled()) { LOGGER.debug("Auto-linking files for entry {}", entry); BackgroundTask> findAssociatedNotLinkedFiles = BackgroundTask .wrap(() -> findAssociatedNotLinkedFiles(entry)) .onSuccess(list -> { if (!list.isEmpty()) { - LOGGER.debug("Found non-associated files:", list); + LOGGER.debug("Found non-associated files: {}" ,list); files.addAll(list); } }); diff --git a/src/main/java/org/jabref/gui/linkedfile/LinkedFileEditDialogViewModel.java b/src/main/java/org/jabref/gui/linkedfile/LinkedFileEditDialogViewModel.java index cd31283742c..807d4e5d09f 100644 --- a/src/main/java/org/jabref/gui/linkedfile/LinkedFileEditDialogViewModel.java +++ b/src/main/java/org/jabref/gui/linkedfile/LinkedFileEditDialogViewModel.java @@ -1,7 +1,9 @@ package org.jabref.gui.linkedfile; +import java.io.IOException; import java.net.MalformedURLException; import java.net.URI; +import java.nio.file.Files; import java.nio.file.Path; import java.util.Optional; import java.util.regex.Pattern; @@ -22,15 +24,21 @@ import org.jabref.gui.frame.ExternalApplicationsPreferences; import org.jabref.gui.util.FileDialogConfiguration; import org.jabref.logic.FilePreferences; +import org.jabref.logic.l10n.Localization; +import org.jabref.logic.util.io.FileNameCleaner; import org.jabref.logic.util.io.FileUtil; import org.jabref.model.database.BibDatabaseContext; import org.jabref.model.entry.LinkedFile; import com.tobiasdiez.easybind.EasyBind; import com.tobiasdiez.easybind.optional.ObservableOptionalValue; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; public class LinkedFileEditDialogViewModel extends AbstractViewModel { + private static final Logger LOGGER = LoggerFactory.getLogger(LinkedFileEditDialogViewModel.class); + private static final Pattern REMOTE_LINK_PATTERN = Pattern.compile("[a-z]+://.*"); private final StringProperty link = new SimpleStringProperty(""); private final StringProperty description = new SimpleStringProperty(""); @@ -86,12 +94,28 @@ public void openBrowseDialog() { .withInitialFileName(fileName) .build(); - dialogService.showFileOpenDialog(fileDialogConfiguration).ifPresent(path -> { + dialogService.showFileOpenDialog(fileDialogConfiguration).ifPresent(fileToAdd -> { // Store the directory for next time: - filePreferences.setWorkingDirectory(path); - link.set(relativize(path)); - setExternalFileTypeByExtension(link.getValueSafe()); + if (FileUtil.detectBadFileName(fileToAdd.toString())) { + String newFilename = FileNameCleaner.cleanFileName(fileToAdd.getFileName().toString()); + + boolean correctButtonPressed = dialogService.showConfirmationDialogAndWait(Localization.lang("File \"%0\" cannot be added!", fileToAdd.getFileName()), + Localization.lang("Illegal characters in the file name detected.\nFile will be renamed to \"%0\" and added.", newFilename), + Localization.lang("Rename and add")); + + if (correctButtonPressed) { + Path correctPath = fileToAdd.resolveSibling(newFilename); + try { + Files.move(fileToAdd, correctPath); + link.set(relativize(correctPath)); + setExternalFileTypeByExtension(link.getValueSafe()); + } catch (IOException ex) { + LOGGER.error("Error moving file", ex); + dialogService.showErrorDialogAndWait(ex); + } + } + } }); } diff --git a/src/main/java/org/jabref/logic/util/io/FileUtil.java b/src/main/java/org/jabref/logic/util/io/FileUtil.java index 3240e64034f..5ddb734d8ce 100644 --- a/src/main/java/org/jabref/logic/util/io/FileUtil.java +++ b/src/main/java/org/jabref/logic/util/io/FileUtil.java @@ -576,13 +576,10 @@ public static String shortenFileName(String fileName, Integer maxLength) { numCharsBeforeEllipsis = Math.min(numCharsBeforeEllipsis, name.length()); numCharsAfterEllipsis = Math.min(numCharsAfterEllipsis, name.length() - numCharsBeforeEllipsis); - StringBuilder result = new StringBuilder(); - result.append(name, 0, numCharsBeforeEllipsis) - .append(ELLIPSIS) - .append(name.substring(name.length() - numCharsAfterEllipsis)) - .append(extension); - - return result.toString(); + return name.substring(0, numCharsBeforeEllipsis) + + ELLIPSIS + + name.substring(name.length() - numCharsAfterEllipsis) + + extension; } public static boolean isCharLegal(char c) { From ca3187e80aea10d37d3bb81951f65927159a0990 Mon Sep 17 00:00:00 2001 From: Siedlerchr Date: Thu, 21 Nov 2024 21:14:26 +0100 Subject: [PATCH 2/7] fix checkstyle --- .../LinkedFilesEditorViewModel.java | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-) diff --git a/src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditorViewModel.java b/src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditorViewModel.java index 4e8340641c5..41357a35789 100644 --- a/src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditorViewModel.java +++ b/src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditorViewModel.java @@ -31,7 +31,6 @@ import org.jabref.gui.linkedfile.AttachFileFromURLAction; import org.jabref.gui.preferences.GuiPreferences; import org.jabref.gui.util.BindingsHelper; -import org.jabref.gui.util.FileDialogConfiguration; import org.jabref.logic.bibtex.FileFieldWriter; import org.jabref.logic.importer.FulltextFetchers; import org.jabref.logic.importer.util.FileFieldParser; @@ -39,7 +38,6 @@ import org.jabref.logic.l10n.Localization; import org.jabref.logic.util.BackgroundTask; import org.jabref.logic.util.TaskExecutor; -import org.jabref.logic.util.io.FileNameCleaner; import org.jabref.logic.util.io.FileUtil; import org.jabref.model.database.BibDatabaseContext; import org.jabref.model.entry.BibEntry; @@ -106,19 +104,6 @@ public static LinkedFile fromFile(Path file, List fileDirectories, Externa return new LinkedFile("", relativePath, suggestedFileType.getName()); } - public LinkedFileViewModel fromFile(Path file, ExternalApplicationsPreferences externalApplicationsPreferences) { - List fileDirectories = databaseContext.getFileDirectories(preferences.getFilePreferences()); - - LinkedFile linkedFile = fromFile(file, fileDirectories, externalApplicationsPreferences); - return new LinkedFileViewModel( - linkedFile, - entry, - databaseContext, - taskExecutor, - dialogService, - preferences); - } - private List parseToFileViewModel(String stringValue) { return FileFieldParser.parse(stringValue).stream() .map(linkedFile -> new LinkedFileViewModel( @@ -160,7 +145,7 @@ public void bindToEntry(BibEntry entry) { .wrap(() -> findAssociatedNotLinkedFiles(entry)) .onSuccess(list -> { if (!list.isEmpty()) { - LOGGER.debug("Found non-associated files: {}" ,list); + LOGGER.debug("Found non-associated files: {}", list); files.addAll(list); } }); From c50083ed37c8b6de97401506fc2a09029a5413fe Mon Sep 17 00:00:00 2001 From: Siedlerchr Date: Thu, 21 Nov 2024 21:36:16 +0100 Subject: [PATCH 3/7] restore switch --- .../jabref/gui/fieldeditors/LinkedFilesEditor.java | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditor.java b/src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditor.java index 8b54c6c7496..96c00de9f3b 100644 --- a/src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditor.java +++ b/src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditor.java @@ -253,10 +253,13 @@ private void setUpKeyBindings() { listView.addEventFilter(KeyEvent.KEY_PRESSED, event -> { Optional keyBinding = preferences.getKeyBindingRepository().mapToKeyBinding(event); if (keyBinding.isPresent()) { - // Pass other keys to children - if (keyBinding.get() == KeyBinding.DELETE_ENTRY) { - deleteAttachedFilesWithConfirmation(); - event.consume(); + switch (keyBinding.get()) { + case DELETE_ENTRY: + deleteAttachedFilesWithConfirmation(); + event.consume(); + break; + default: + // Pass other keys to children } } }); From a42133152c63eff44a5f6168941311143d6e642d Mon Sep 17 00:00:00 2001 From: Siedlerchr Date: Thu, 21 Nov 2024 21:38:09 +0100 Subject: [PATCH 4/7] set working dir path --- .../org/jabref/gui/linkedfile/LinkedFileEditDialogViewModel.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/java/org/jabref/gui/linkedfile/LinkedFileEditDialogViewModel.java b/src/main/java/org/jabref/gui/linkedfile/LinkedFileEditDialogViewModel.java index 807d4e5d09f..40775977e0a 100644 --- a/src/main/java/org/jabref/gui/linkedfile/LinkedFileEditDialogViewModel.java +++ b/src/main/java/org/jabref/gui/linkedfile/LinkedFileEditDialogViewModel.java @@ -109,6 +109,7 @@ public void openBrowseDialog() { try { Files.move(fileToAdd, correctPath); link.set(relativize(correctPath)); + filePreferences.setWorkingDirectory(correctPath); setExternalFileTypeByExtension(link.getValueSafe()); } catch (IOException ex) { LOGGER.error("Error moving file", ex); From f8216197bce23408e3d68e0fb7d4f9c914625b09 Mon Sep 17 00:00:00 2001 From: Siedlerchr Date: Thu, 21 Nov 2024 22:09:43 +0100 Subject: [PATCH 5/7] checkstyle --- .../org/jabref/gui/fieldeditors/LinkedFilesEditorViewModel.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditorViewModel.java b/src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditorViewModel.java index 41357a35789..4cd947b2287 100644 --- a/src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditorViewModel.java +++ b/src/main/java/org/jabref/gui/fieldeditors/LinkedFilesEditorViewModel.java @@ -124,7 +124,6 @@ public ListProperty filesProperty() { return files; } - public void addNewLinkedFile(LinkedFile linkedFile) { files.add(new LinkedFileViewModel( linkedFile, From c46be1e97202734efe74d9cefa37c47fef16ee33 Mon Sep 17 00:00:00 2001 From: Siedlerchr Date: Fri, 22 Nov 2024 19:13:51 +0100 Subject: [PATCH 6/7] add test --- .../LinkedFileEditDialogViewModel.java | 46 ++++++++-------- .../LinkedFileEditDialogViewModelTest.java | 55 +++++++++++++++++++ 2 files changed, 79 insertions(+), 22 deletions(-) create mode 100644 src/test/java/org/jabref/gui/linkedfile/LinkedFileEditDialogViewModelTest.java diff --git a/src/main/java/org/jabref/gui/linkedfile/LinkedFileEditDialogViewModel.java b/src/main/java/org/jabref/gui/linkedfile/LinkedFileEditDialogViewModel.java index 40775977e0a..82863bc6c5b 100644 --- a/src/main/java/org/jabref/gui/linkedfile/LinkedFileEditDialogViewModel.java +++ b/src/main/java/org/jabref/gui/linkedfile/LinkedFileEditDialogViewModel.java @@ -30,6 +30,7 @@ import org.jabref.model.database.BibDatabaseContext; import org.jabref.model.entry.LinkedFile; +import com.google.common.annotations.VisibleForTesting; import com.tobiasdiez.easybind.EasyBind; import com.tobiasdiez.easybind.optional.ObservableOptionalValue; import org.slf4j.Logger; @@ -94,30 +95,31 @@ public void openBrowseDialog() { .withInitialFileName(fileName) .build(); - dialogService.showFileOpenDialog(fileDialogConfiguration).ifPresent(fileToAdd -> { - // Store the directory for next time: - - if (FileUtil.detectBadFileName(fileToAdd.toString())) { - String newFilename = FileNameCleaner.cleanFileName(fileToAdd.getFileName().toString()); - - boolean correctButtonPressed = dialogService.showConfirmationDialogAndWait(Localization.lang("File \"%0\" cannot be added!", fileToAdd.getFileName()), - Localization.lang("Illegal characters in the file name detected.\nFile will be renamed to \"%0\" and added.", newFilename), - Localization.lang("Rename and add")); - - if (correctButtonPressed) { - Path correctPath = fileToAdd.resolveSibling(newFilename); - try { - Files.move(fileToAdd, correctPath); - link.set(relativize(correctPath)); - filePreferences.setWorkingDirectory(correctPath); - setExternalFileTypeByExtension(link.getValueSafe()); - } catch (IOException ex) { - LOGGER.error("Error moving file", ex); - dialogService.showErrorDialogAndWait(ex); - } + dialogService.showFileOpenDialog(fileDialogConfiguration).ifPresent(this::checkForBadFileNameAndAdd); + } + + @VisibleForTesting + void checkForBadFileNameAndAdd(Path fileToAdd) { + if (FileUtil.detectBadFileName(fileToAdd.toString())) { + String newFilename = FileNameCleaner.cleanFileName(fileToAdd.getFileName().toString()); + + boolean correctButtonPressed = dialogService.showConfirmationDialogAndWait(Localization.lang("File \"%0\" cannot be added!", fileToAdd.getFileName()), + Localization.lang("Illegal characters in the file name detected.\nFile will be renamed to \"%0\" and added.", newFilename), + Localization.lang("Rename and add")); + + if (correctButtonPressed) { + Path correctPath = fileToAdd.resolveSibling(newFilename); + try { + Files.move(fileToAdd, correctPath); + link.set(relativize(correctPath)); + filePreferences.setWorkingDirectory(correctPath); + setExternalFileTypeByExtension(link.getValueSafe()); + } catch (IOException ex) { + LOGGER.error("Error moving file", ex); + dialogService.showErrorDialogAndWait(ex); } } - }); + } } public void setValues(LinkedFile linkedFile) { diff --git a/src/test/java/org/jabref/gui/linkedfile/LinkedFileEditDialogViewModelTest.java b/src/test/java/org/jabref/gui/linkedfile/LinkedFileEditDialogViewModelTest.java new file mode 100644 index 00000000000..d3f6ce309dc --- /dev/null +++ b/src/test/java/org/jabref/gui/linkedfile/LinkedFileEditDialogViewModelTest.java @@ -0,0 +1,55 @@ +package org.jabref.gui.linkedfile; + +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.TreeSet; + +import javafx.collections.FXCollections; + +import org.jabref.gui.DialogService; +import org.jabref.gui.externalfiletype.ExternalFileTypes; +import org.jabref.gui.frame.ExternalApplicationsPreferences; +import org.jabref.gui.preferences.GuiPreferences; +import org.jabref.logic.FilePreferences; +import org.jabref.model.database.BibDatabaseContext; +import org.jabref.model.entry.LinkedFile; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import org.mockito.Answers; + +import static org.junit.jupiter.api.Assertions.*; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +class LinkedFileEditDialogViewModelTest { + private final GuiPreferences preferences = mock(GuiPreferences.class, Answers.RETURNS_DEEP_STUBS); + private final FilePreferences filePreferences = mock(FilePreferences.class, Answers.RETURNS_DEEP_STUBS); + private final BibDatabaseContext bibDatabaseContext = mock(BibDatabaseContext.class); + private final DialogService dialogService = mock(DialogService.class); + private final ExternalApplicationsPreferences externalApplicationsPreferences = mock(ExternalApplicationsPreferences.class); + + @BeforeEach + void setup() { + when(externalApplicationsPreferences.getExternalFileTypes()).thenReturn(FXCollections.observableSet(new TreeSet<>(ExternalFileTypes.getDefaultExternalFileTypes()))); + when(preferences.getExternalApplicationsPreferences()).thenReturn(externalApplicationsPreferences); + when(preferences.getFilePreferences()).thenReturn(filePreferences); + } + + @Test + void badFilenameCharWillBeReplacedByUnderscore(@TempDir Path tempDir) throws Exception { + + Path invalidFile = tempDir.resolve("?invalid.pdf"); + Files.createFile(invalidFile); + when(dialogService.showConfirmationDialogAndWait(any(), any(), any())).thenReturn(true); + + LinkedFileEditDialogViewModel viewModel = new LinkedFileEditDialogViewModel(new LinkedFile("", "", ""), bibDatabaseContext, dialogService, externalApplicationsPreferences, filePreferences); + + viewModel.checkForBadFileNameAndAdd(invalidFile); + + LinkedFile expectedFile = new LinkedFile("", tempDir.resolve("_invalid.pdf").toString(), "PDF"); + assertEquals(expectedFile, viewModel.getNewLinkedFile()); + } +} From b40115b3b38eb35ae4d6147b303e300dbee48b92 Mon Sep 17 00:00:00 2001 From: Siedlerchr Date: Fri, 22 Nov 2024 19:33:03 +0100 Subject: [PATCH 7/7] fix import --- .../gui/linkedfile/LinkedFileEditDialogViewModelTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/org/jabref/gui/linkedfile/LinkedFileEditDialogViewModelTest.java b/src/test/java/org/jabref/gui/linkedfile/LinkedFileEditDialogViewModelTest.java index d3f6ce309dc..d19c5d9cbb0 100644 --- a/src/test/java/org/jabref/gui/linkedfile/LinkedFileEditDialogViewModelTest.java +++ b/src/test/java/org/jabref/gui/linkedfile/LinkedFileEditDialogViewModelTest.java @@ -19,7 +19,7 @@ import org.junit.jupiter.api.io.TempDir; import org.mockito.Answers; -import static org.junit.jupiter.api.Assertions.*; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when;