Merge pull request #10496 from papatekken/fix-10127

Fix duplicate external file type issue when change language and other corresponding issue of external file type preferences
JabRef · Dec 17, 2023 · 4d86fb4 · 4d86fb4
2 parents f099994 + b4e8c21
commit 4d86fb4
Show file tree

Hide file tree

Showing 10 changed files with 318 additions and 26 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -36,6 +36,7 @@ Note that this project **does not** adhere to [Semantic Versioning](https://semv
 - We fixed an issue where in the merge dialog the file field of entries was not correctly merged when the first and second entry both contained values inside the file field. [#10572](https://github.com/JabRef/jabref/issues/10572)
 - We fixed some small inconsistencies in the user interface. [#10507](https://github.com/JabRef/jabref/issues/10507) [#10458](https://github.com/JabRef/jabref/issues/10458) [#10660](https://github.com/JabRef/jabref/issues/10660)
 - We fixed the issue where the Hayagriva YAML exporter would not include a parent field for the publisher/series. [#10596](https://github.com/JabRef/jabref/issues/10596)
+- We fixed issues in the external file type dialog w.r.t. duplicate entries in the case of a language switch. [#10271](https://github.com/JabRef/jabref/issues/10271)
 
 ### Removed
 

diff --git a/src/main/java/org/jabref/gui/exporter/CreateModifyExporterDialogViewModel.java b/src/main/java/org/jabref/gui/exporter/CreateModifyExporterDialogViewModel.java
@@ -57,7 +57,7 @@ public ExporterViewModel saveExporter() {
         // Check that there are no empty strings.
         if (layoutFile.get().isEmpty() || name.get().isEmpty() || extension.get().isEmpty()
                 || !layoutFile.get().endsWith(".layout")) {
-            LOGGER.info("One of the fields is empty or invalid!");
+            LOGGER.info("One of the fields is empty or invalid.");
             return null;
         }
 

diff --git a/src/main/java/org/jabref/gui/externalfiletype/ExternalFileTypes.java b/src/main/java/org/jabref/gui/externalfiletype/ExternalFileTypes.java
@@ -148,10 +148,10 @@ public static String toStringList(Collection<ExternalFileType> fileTypes) {
 
         for (ExternalFileType type : fileTypes) {
             results.add(type);
-            // See if we can find a type with matching name in the default type list:
+            // See if we can find a type with matching extension in the default type list:
             ExternalFileType found = null;
             for (ExternalFileType defType : defTypes) {
-                if (defType.getName().equals(type.getName())) {
+                if (defType.getExtension().equals(type.getExtension())) {
                     found = defType;
                     break;
                 }
@@ -221,11 +221,11 @@ public static Set<ExternalFileType> fromString(String storedFileTypes) {
             } else {
                 // A new or modified entry type. Construct it from the string array:
                 ExternalFileType type = CustomExternalFileType.buildFromArgs(val);
-                // Check if there is a default type with the same name. If so, this is a
+                // Check if there is a default type with the same extension. If so, this is a
                 // modification of that type, so remove the default one:
                 ExternalFileType toRemove = null;
                 for (ExternalFileType defType : types) {
-                    if (type.getName().equals(defType.getName())) {
+                    if (type.getExtension().equals(defType.getExtension())) {
                         toRemove = defType;
                         break;
                     }

diff --git a/...in/java/org/jabref/gui/preferences/externalfiletypes/EditExternalFileTypeEntryDialog.java b/...in/java/org/jabref/gui/preferences/externalfiletypes/EditExternalFileTypeEntryDialog.java
@@ -1,5 +1,7 @@
 package org.jabref.gui.preferences.externalfiletypes;
 
+import javafx.application.Platform;
+import javafx.collections.ObservableList;
 import javafx.event.ActionEvent;
 import javafx.fxml.FXML;
 import javafx.scene.control.Button;
@@ -13,9 +15,11 @@
 import org.jabref.gui.desktop.os.NativeDesktop;
 import org.jabref.gui.util.BaseDialog;
 import org.jabref.gui.util.FileDialogConfiguration;
+import org.jabref.gui.util.IconValidationDecorator;
 import org.jabref.logic.util.OS;
 
 import com.airhacks.afterburner.views.ViewLoader;
+import de.saxsys.mvvmfx.utils.validation.visualization.ControlsFxVisualizer;
 import jakarta.inject.Inject;
 
 public class EditExternalFileTypeEntryDialog extends BaseDialog<Void> {
@@ -33,20 +37,26 @@ public class EditExternalFileTypeEntryDialog extends BaseDialog<Void> {
 
     private final NativeDesktop nativeDesktop = OS.getNativeDesktop();
     private final FileDialogConfiguration fileDialogConfiguration = new FileDialogConfiguration.Builder().withInitialDirectory(nativeDesktop.getApplicationDirectory()).build();
-
     private final ExternalFileTypeItemViewModel item;
 
+    private final ObservableList<ExternalFileTypeItemViewModel> fileTypes;
     private EditExternalFileTypeViewModel viewModel;
 
-    public EditExternalFileTypeEntryDialog(ExternalFileTypeItemViewModel item, String dialogTitle) {
-        this.item = item;
+    private final ControlsFxVisualizer visualizer = new ControlsFxVisualizer();
 
+    public EditExternalFileTypeEntryDialog(ExternalFileTypeItemViewModel item, String dialogTitle, ObservableList<ExternalFileTypeItemViewModel> fileTypes) {
+        this.item = item;
+        this.fileTypes = fileTypes;
         this.setTitle(dialogTitle);
 
         ViewLoader.view(this)
                   .load()
                   .setAsDialogPane(this);
 
+        getDialogPane().getButtonTypes().setAll(ButtonType.OK, ButtonType.CANCEL);
+
+        final Button confirmDialogButton = (Button) getDialogPane().lookupButton(ButtonType.OK);
+        confirmDialogButton.disableProperty().bind(viewModel.validationStatus().validProperty().not());
         this.setResultConverter(button -> {
             if (button == ButtonType.OK) {
                 viewModel.storeSettings();
@@ -57,19 +67,26 @@ public EditExternalFileTypeEntryDialog(ExternalFileTypeItemViewModel item, Strin
 
     @FXML
     public void initialize() {
-        viewModel = new EditExternalFileTypeViewModel(item);
+        visualizer.setDecoration(new IconValidationDecorator());
+
+        viewModel = new EditExternalFileTypeViewModel(item, fileTypes);
 
         icon.setGraphic(viewModel.getIcon());
 
         defaultApplication.selectedProperty().bindBidirectional(viewModel.defaultApplicationSelectedProperty());
         customApplication.selectedProperty().bindBidirectional(viewModel.customApplicationSelectedProperty());
         selectedApplication.disableProperty().bind(viewModel.defaultApplicationSelectedProperty());
         btnBrowse.disableProperty().bind(viewModel.defaultApplicationSelectedProperty());
-
         extension.textProperty().bindBidirectional(viewModel.extensionProperty());
         name.textProperty().bindBidirectional(viewModel.nameProperty());
         mimeType.textProperty().bindBidirectional(viewModel.mimeTypeProperty());
         selectedApplication.textProperty().bindBidirectional(viewModel.selectedApplicationProperty());
+
+        Platform.runLater(() -> {
+            visualizer.initVisualization(viewModel.extensionValidation(), extension, true);
+            visualizer.initVisualization(viewModel.nameValidation(), name, true);
+            visualizer.initVisualization(viewModel.mimeTypeValidation(), mimeType, true);
+        });
     }
 
     @FXML

diff --git a/...main/java/org/jabref/gui/preferences/externalfiletypes/EditExternalFileTypeViewModel.java b/...main/java/org/jabref/gui/preferences/externalfiletypes/EditExternalFileTypeViewModel.java
@@ -4,21 +4,41 @@
 import javafx.beans.property.SimpleBooleanProperty;
 import javafx.beans.property.SimpleStringProperty;
 import javafx.beans.property.StringProperty;
+import javafx.collections.ObservableList;
 import javafx.scene.Node;
 
+import org.jabref.logic.l10n.Localization;
+import org.jabref.model.strings.StringUtil;
+
+import de.saxsys.mvvmfx.utils.validation.CompositeValidator;
+import de.saxsys.mvvmfx.utils.validation.FunctionBasedValidator;
+import de.saxsys.mvvmfx.utils.validation.ValidationMessage;
+import de.saxsys.mvvmfx.utils.validation.ValidationStatus;
+import de.saxsys.mvvmfx.utils.validation.Validator;
+
 public class EditExternalFileTypeViewModel {
     private final ExternalFileTypeItemViewModel fileTypeViewModel;
-
     private final StringProperty nameProperty = new SimpleStringProperty("");
     private final StringProperty mimeTypeProperty = new SimpleStringProperty("");
     private final StringProperty extensionProperty = new SimpleStringProperty("");
     private final StringProperty selectedApplicationProperty = new SimpleStringProperty("");
     private final BooleanProperty defaultApplicationSelectedProperty = new SimpleBooleanProperty(false);
     private final BooleanProperty customApplicationSelectedProperty = new SimpleBooleanProperty(false);
-
-    public EditExternalFileTypeViewModel(ExternalFileTypeItemViewModel fileTypeViewModel) {
+    private final ObservableList<ExternalFileTypeItemViewModel> fileTypes;
+    private final String originalExtension;
+    private final String originalName;
+    private final String originalMimeType;
+    private CompositeValidator extensionValidator;
+    private CompositeValidator nameValidator;
+    private CompositeValidator mimeTypeValidator;
+    private CompositeValidator validator;
+
+    public EditExternalFileTypeViewModel(ExternalFileTypeItemViewModel fileTypeViewModel, ObservableList<ExternalFileTypeItemViewModel> fileTypes) {
         this.fileTypeViewModel = fileTypeViewModel;
-
+        this.fileTypes = fileTypes;
+        this.originalExtension = fileTypeViewModel.extensionProperty().getValue();
+        this.originalName = fileTypeViewModel.nameProperty().getValue();
+        this.originalMimeType = fileTypeViewModel.mimetypeProperty().getValue();
         extensionProperty.setValue(fileTypeViewModel.extensionProperty().getValue());
         nameProperty.setValue(fileTypeViewModel.nameProperty().getValue());
         mimeTypeProperty.setValue(fileTypeViewModel.mimetypeProperty().getValue());
@@ -29,6 +49,92 @@ public EditExternalFileTypeViewModel(ExternalFileTypeItemViewModel fileTypeViewM
             customApplicationSelectedProperty.setValue(true);
             selectedApplicationProperty.setValue(fileTypeViewModel.applicationProperty().getValue());
         }
+
+        setupValidation();
+    }
+
+    private void setupValidation() {
+        validator = new CompositeValidator();
+        extensionValidator = new CompositeValidator();
+
+        Validator extensionisNotBlankValidator = new FunctionBasedValidator<>(
+                extensionProperty,
+                StringUtil::isNotBlank,
+                ValidationMessage.error(Localization.lang("Please enter a name for the extension."))
+        );
+        Validator sameExtensionValidator = new FunctionBasedValidator<>(
+                extensionProperty,
+                extension -> {
+                    for (ExternalFileTypeItemViewModel fileTypeItem : fileTypes) {
+                        if (extension.equalsIgnoreCase(fileTypeItem.extensionProperty().get()) && !extension.equalsIgnoreCase(originalExtension)) {
+                            return false;
+                        }
+                    }
+                    return true;
+                },
+                ValidationMessage.error(Localization.lang("There already exists an external file type with the same extension"))
+        );
+        extensionValidator.addValidators(sameExtensionValidator, extensionisNotBlankValidator);
+
+        nameValidator = new CompositeValidator();
+        Validator sameNameValidator = new FunctionBasedValidator<>(
+                nameProperty,
+                name -> {
+                    for (ExternalFileTypeItemViewModel fileTypeItem : fileTypes) {
+                        if (name.equalsIgnoreCase(fileTypeItem.nameProperty().get()) && !name.equalsIgnoreCase(originalName)) {
+                            return false;
+                        }
+                    }
+                    return true;
+                },
+                ValidationMessage.error(Localization.lang("There already exists an external file type with the same name"))
+        );
+
+        Validator nameIsNotBlankValidator = new FunctionBasedValidator<>(
+                nameProperty,
+                StringUtil::isNotBlank,
+                ValidationMessage.error(Localization.lang("Please enter a name."))
+        );
+        nameValidator.addValidators(sameNameValidator, nameIsNotBlankValidator);
+
+        mimeTypeValidator = new CompositeValidator();
+        Validator mimeTypeIsNotBlankValidator = new FunctionBasedValidator<>(
+                mimeTypeProperty,
+                StringUtil::isNotBlank,
+                ValidationMessage.error(Localization.lang("Please enter a name for the MIME type."))
+        );
+
+        Validator sameMimeTypeValidator = new FunctionBasedValidator<>(
+                mimeTypeProperty,
+                mimeType -> {
+                    for (ExternalFileTypeItemViewModel fileTypeItem : fileTypes) {
+                        if (mimeType.equalsIgnoreCase(fileTypeItem.mimetypeProperty().get()) && !mimeType.equalsIgnoreCase(originalMimeType)) {
+                            return false;
+                        }
+                    }
+                    return true;
+                },
+                ValidationMessage.error(Localization.lang("There already exists an external file type with the same MIME type"))
+        );
+        mimeTypeValidator.addValidators(sameMimeTypeValidator, mimeTypeIsNotBlankValidator);
+
+        validator.addValidators(extensionValidator, sameExtensionValidator, nameValidator, sameNameValidator, mimeTypeValidator, sameMimeTypeValidator);
+    }
+
+    public ValidationStatus validationStatus() {
+        return validator.getValidationStatus();
+    }
+
+    public ValidationStatus extensionValidation() {
+        return extensionValidator.getValidationStatus();
+    }
+
+    public ValidationStatus mimeTypeValidation() {
+        return mimeTypeValidator.getValidationStatus();
+    }
+
+    public ValidationStatus nameValidation() {
+        return nameValidator.getValidationStatus();
     }
 
     public Node getIcon() {
@@ -59,6 +165,10 @@ public BooleanProperty customApplicationSelectedProperty() {
         return customApplicationSelectedProperty;
     }
 
+    public BooleanProperty validExtensionTypeProperty() {
+        return defaultApplicationSelectedProperty;
+    }
+
     public void storeSettings() {
         fileTypeViewModel.nameProperty().setValue(nameProperty.getValue().trim());
         fileTypeViewModel.mimetypeProperty().setValue(mimeTypeProperty.getValue().trim());

diff --git a/src/main/java/org/jabref/gui/preferences/externalfiletypes/ExternalFileTypesTab.java b/src/main/java/org/jabref/gui/preferences/externalfiletypes/ExternalFileTypesTab.java
@@ -78,19 +78,27 @@ public void initialize() {
                 .install(fileTypesTableIconColumn);
         new ValueTableCellFactory<ExternalFileTypeItemViewModel, Boolean>()
                 .withGraphic(none -> IconTheme.JabRefIcons.EDIT.getGraphicNode())
-                .withOnMouseClickedEvent((type, none) -> event -> viewModel.edit(type))
+                .withOnMouseClickedEvent((type, none) -> event -> editType(type))
                 .install(fileTypesTableEditColumn);
         new ValueTableCellFactory<ExternalFileTypeItemViewModel, Boolean>()
                 .withGraphic(none -> IconTheme.JabRefIcons.DELETE_ENTRY.getGraphicNode())
                 .withOnMouseClickedEvent((type, none) -> event -> viewModel.remove(type))
                 .install(fileTypesTableDeleteColumn);
     }
 
+    private void editType(ExternalFileTypeItemViewModel type) {
+        if (viewModel.edit(type)) {
+            fileTypesTable.getSelectionModel().selectLast();
+            fileTypesTable.scrollTo(viewModel.getFileTypes().size() - 1);
+        }
+    }
+
     @FXML
     private void addNewType() {
-        viewModel.addNewType();
-        fileTypesTable.getSelectionModel().selectLast();
-        fileTypesTable.scrollTo(viewModel.getFileTypes().size() - 1);
+        if (viewModel.addNewType()) {
+            fileTypesTable.getSelectionModel().selectLast();
+            fileTypesTable.scrollTo(viewModel.getFileTypes().size() - 1);
+        }
     }
 
     @FXML