From d18ce55fa75ae67de5c2e3a0f221e247c0cc68d4 Mon Sep 17 00:00:00 2001 From: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com> Date: Fri, 8 May 2020 22:25:13 +0200 Subject: [PATCH] UI consistency - BibTexStringEditorDialog rework (#6287) --- src/main/java/org/jabref/gui/Base.css | 5 + .../jabref/gui/entryeditor/EntryEditor.css | 4 - .../metadata/BibtexStringEditorDialog.fxml | 72 ++++++++------ .../BibtexStringEditorDialogView.java | 97 +++++++++++-------- .../BibtexStringEditorDialogViewModel.java | 76 ++++++++------- ....java => BibtexStringEditorItemModel.java} | 8 +- .../gui/util/ViewModelListCellFactory.java | 21 ++-- ...extFieldTableCellVisualizationFactory.java | 79 ++++++++++++--- src/main/resources/l10n/JabRef_en.properties | 3 +- 9 files changed, 231 insertions(+), 134 deletions(-) rename src/main/java/org/jabref/gui/metadata/{BibtexStringViewModel.java => BibtexStringEditorItemModel.java} (93%) diff --git a/src/main/java/org/jabref/gui/Base.css b/src/main/java/org/jabref/gui/Base.css index c136404b276..77e618a818a 100644 --- a/src/main/java/org/jabref/gui/Base.css +++ b/src/main/java/org/jabref/gui/Base.css @@ -613,6 +613,11 @@ -fx-text-fill: -jr-black; } +.table-cell:invalid, +.list-cell:invalid { + -fx-background-color: -jr-warn; +} + .combo-box-base { -fx-background-color: -fx-outer-border, -fx-control-inner-background; -fx-background-insets: 0, 1; diff --git a/src/main/java/org/jabref/gui/entryeditor/EntryEditor.css b/src/main/java/org/jabref/gui/entryeditor/EntryEditor.css index 61981cde6cb..e53c688a081 100644 --- a/src/main/java/org/jabref/gui/entryeditor/EntryEditor.css +++ b/src/main/java/org/jabref/gui/entryeditor/EntryEditor.css @@ -86,10 +86,6 @@ -fx-background-color: -jr-error; } -.list-cell:invalid { - -fx-background-color: -jr-warn; -} - .code-area .context-menu { -fx-font-family: sans-serif; } diff --git a/src/main/java/org/jabref/gui/metadata/BibtexStringEditorDialog.fxml b/src/main/java/org/jabref/gui/metadata/BibtexStringEditorDialog.fxml index 728897bd5db..477358e2290 100644 --- a/src/main/java/org/jabref/gui/metadata/BibtexStringEditorDialog.fxml +++ b/src/main/java/org/jabref/gui/metadata/BibtexStringEditorDialog.fxml @@ -7,31 +7,49 @@ - - - - - - - - - - - - - - - - + + + + + + + diff --git a/src/main/java/org/jabref/gui/metadata/BibtexStringEditorDialogView.java b/src/main/java/org/jabref/gui/metadata/BibtexStringEditorDialogView.java index 58dc75d6e23..a758ac1a005 100644 --- a/src/main/java/org/jabref/gui/metadata/BibtexStringEditorDialogView.java +++ b/src/main/java/org/jabref/gui/metadata/BibtexStringEditorDialogView.java @@ -1,8 +1,9 @@ package org.jabref.gui.metadata; +import java.util.Optional; + import javax.inject.Inject; -import javafx.event.ActionEvent; import javafx.fxml.FXML; import javafx.scene.control.Button; import javafx.scene.control.ButtonType; @@ -13,34 +14,30 @@ import javafx.util.converter.DefaultStringConverter; import org.jabref.gui.DialogService; -import org.jabref.gui.icon.IconTheme.JabRefIcons; +import org.jabref.gui.icon.IconTheme; import org.jabref.gui.util.BaseDialog; -import org.jabref.gui.util.IconValidationDecorator; +import org.jabref.gui.util.ValueTableCellFactory; import org.jabref.gui.util.ViewModelTextFieldTableCellVisualizationFactory; import org.jabref.logic.l10n.Localization; import org.jabref.model.database.BibDatabase; import com.airhacks.afterburner.views.ViewLoader; -import de.saxsys.mvvmfx.utils.validation.visualization.ControlsFxVisualizer; public class BibtexStringEditorDialogView extends BaseDialog { - @FXML private Button btnNewString; - @FXML private Button btnRemove; - @FXML private Button btnHelp; + @FXML private TableView stringsList; + @FXML private TableColumn labelColumn; + @FXML private TableColumn contentColumn; + @FXML private TableColumn actionsColumn; + @FXML private Button addStringButton; @FXML private ButtonType saveButton; - @FXML private TableView tblStrings; - @FXML private TableColumn colLabel; - @FXML private TableColumn colContent; - - private final ControlsFxVisualizer visualizer = new ControlsFxVisualizer(); private final BibtexStringEditorDialogViewModel viewModel; @Inject private DialogService dialogService; public BibtexStringEditorDialogView(BibDatabase database) { - viewModel = new BibtexStringEditorDialogViewModel(database); + this.viewModel = new BibtexStringEditorDialogViewModel(database); ViewLoader.view(this) .load() @@ -62,53 +59,67 @@ public BibtexStringEditorDialogView(BibDatabase database) { @FXML private void initialize() { - visualizer.setDecoration(new IconValidationDecorator()); - - btnHelp.setGraphic(JabRefIcons.HELP.getGraphicNode()); - btnHelp.setTooltip(new Tooltip(Localization.lang("Open Help page"))); + addStringButton.setTooltip(new Tooltip(Localization.lang("New string"))); - btnNewString.setTooltip(new Tooltip(Localization.lang("New string"))); - btnRemove.setTooltip(new Tooltip(Localization.lang("Remove selected strings"))); + labelColumn.setSortable(true); + labelColumn.setReorderable(false); - colLabel.setCellValueFactory(cellData -> cellData.getValue().labelProperty()); - new ViewModelTextFieldTableCellVisualizationFactory().withValidation(BibtexStringViewModel::labelValidation, visualizer).install(colLabel, new DefaultStringConverter()); + labelColumn.setCellValueFactory(cellData -> cellData.getValue().labelProperty()); + new ViewModelTextFieldTableCellVisualizationFactory() + .withValidation(BibtexStringEditorItemModel::labelValidation) + .install(labelColumn, new DefaultStringConverter()); + labelColumn.setOnEditCommit((CellEditEvent cellEvent) -> { - colContent.setCellValueFactory(cellData -> cellData.getValue().contentProperty()); - new ViewModelTextFieldTableCellVisualizationFactory().withValidation(BibtexStringViewModel::contentValidation, visualizer).install(colContent, new DefaultStringConverter()); + BibtexStringEditorItemModel cellItem = cellEvent.getTableView() + .getItems() + .get(cellEvent.getTablePosition().getRow()); - colLabel.setOnEditCommit((CellEditEvent cell) -> { + Optional existingItem = viewModel.labelAlreadyExists(cellEvent.getNewValue()); - String newLabelValue = cell.getNewValue(); - if (cell.getTableView().getItems().stream().anyMatch(strs -> strs.labelProperty().get().equals(newLabelValue))) { + if (existingItem.isPresent() && !existingItem.get().equals(cellItem)) { + dialogService.showErrorDialogAndWait(Localization.lang( + "A string with the label '%0' already exists.", + cellEvent.getNewValue())); - dialogService.showErrorDialogAndWait(Localization.lang("A string with the label '%0' already exists.", newLabelValue)); - cell.getRowValue().setLabel(""); + cellItem.setLabel(cellEvent.getOldValue()); } else { - cell.getRowValue().setLabel(cell.getNewValue()); + cellItem.setLabel(cellEvent.getNewValue()); } - }); - colContent.setOnEditCommit((CellEditEvent cell) -> { - cell.getRowValue().setContent(cell.getNewValue()); - }); - tblStrings.itemsProperty().bindBidirectional(viewModel.allStringsProperty()); - tblStrings.setEditable(true); + cellEvent.getTableView().refresh(); + }); - viewModel.seletedItemProperty().bind(tblStrings.getSelectionModel().selectedItemProperty()); + contentColumn.setSortable(true); + contentColumn.setReorderable(false); + contentColumn.setCellValueFactory(cellData -> cellData.getValue().contentProperty()); + new ViewModelTextFieldTableCellVisualizationFactory() + .withValidation(BibtexStringEditorItemModel::contentValidation) + .install(contentColumn, new DefaultStringConverter()); + contentColumn.setOnEditCommit((CellEditEvent cell) -> + cell.getRowValue().setContent(cell.getNewValue())); + + actionsColumn.setSortable(false); + actionsColumn.setReorderable(false); + actionsColumn.setCellValueFactory(cellData -> cellData.getValue().labelProperty()); + new ValueTableCellFactory() + .withGraphic(label -> IconTheme.JabRefIcons.DELETE_ENTRY.getGraphicNode()) + .withTooltip(label -> Localization.lang("Remove string %0", label)) + .withOnMouseClickedEvent(item -> evt -> + viewModel.removeString(stringsList.getFocusModel().getFocusedItem())) + .install(actionsColumn); + + stringsList.itemsProperty().bindBidirectional(viewModel.stringsListProperty()); + stringsList.setEditable(true); } @FXML - private void addString(ActionEvent event) { + private void addString() { viewModel.addNewString(); + stringsList.edit(stringsList.getItems().size() - 1, labelColumn); } @FXML - private void openHelp(ActionEvent event) { + private void openHelp() { viewModel.openHelpPage(); } - - @FXML - private void removeString(ActionEvent event) { - viewModel.removeString(); - } } diff --git a/src/main/java/org/jabref/gui/metadata/BibtexStringEditorDialogViewModel.java b/src/main/java/org/jabref/gui/metadata/BibtexStringEditorDialogViewModel.java index 12f8508e195..4ede8938062 100644 --- a/src/main/java/org/jabref/gui/metadata/BibtexStringEditorDialogViewModel.java +++ b/src/main/java/org/jabref/gui/metadata/BibtexStringEditorDialogViewModel.java @@ -1,15 +1,12 @@ package org.jabref.gui.metadata; -import java.util.List; -import java.util.Set; +import java.util.Optional; import java.util.stream.Collectors; import javafx.beans.property.BooleanProperty; import javafx.beans.property.ListProperty; -import javafx.beans.property.ObjectProperty; import javafx.beans.property.SimpleBooleanProperty; import javafx.beans.property.SimpleListProperty; -import javafx.beans.property.SimpleObjectProperty; import javafx.beans.value.ObservableValue; import javafx.collections.FXCollections; import javafx.collections.ObservableList; @@ -24,9 +21,11 @@ import org.fxmisc.easybind.EasyBind; public class BibtexStringEditorDialogViewModel extends AbstractViewModel { + private static final String NEW_STRING_LABEL = "NewString"; // must not contain spaces + + private final ListProperty stringsListProperty = + new SimpleListProperty<>(FXCollections.observableArrayList()); - private final ListProperty allStrings = new SimpleListProperty<>(FXCollections.observableArrayList()); - private final ObjectProperty selectedItemProperty = new SimpleObjectProperty<>(); private final BibDatabase bibDatabase; private final BooleanProperty validProperty = new SimpleBooleanProperty(); @@ -34,57 +33,68 @@ public BibtexStringEditorDialogViewModel(BibDatabase bibDatabase) { this.bibDatabase = bibDatabase; addAllStringsFromDB(); - ObservableList> allValidProperty = EasyBind.map(allStringsProperty(), BibtexStringViewModel::combinedValidationValidProperty); + ObservableList> allValidProperty = + EasyBind.map(stringsListProperty(), BibtexStringEditorItemModel::combinedValidationValidProperty); validProperty.bind(EasyBind.combine(allValidProperty, stream -> stream.allMatch(valid -> valid))); } private void addAllStringsFromDB() { - - Set strings = bibDatabase.getStringValues() - .stream() - .sorted(new BibtexStringComparator(false)) - .map(this::convertFromBibTexString) - .collect(Collectors.toSet()); - allStrings.addAll(strings); - } - - public ListProperty allStringsProperty() { - return this.allStrings; + stringsListProperty.addAll(bibDatabase.getStringValues().stream() + .sorted(new BibtexStringComparator(false)) + .map(this::convertFromBibTexString) + .collect(Collectors.toSet())); } public void addNewString() { - allStrings.add(new BibtexStringViewModel("", "")); + BibtexStringEditorItemModel newItem; + if (labelAlreadyExists(NEW_STRING_LABEL).isPresent()) { + int i = 1; + while (labelAlreadyExists(NEW_STRING_LABEL + i).isPresent()) { + i++; + } + newItem = new BibtexStringEditorItemModel(NEW_STRING_LABEL + i, ""); + } else { + newItem = new BibtexStringEditorItemModel(NEW_STRING_LABEL, ""); + } + + stringsListProperty.add(newItem); } - public void removeString() { - BibtexStringViewModel toBeRemoved = selectedItemProperty.getValue(); - allStrings.remove(toBeRemoved); + public void removeString(BibtexStringEditorItemModel item) { + stringsListProperty.remove(item); } - private BibtexStringViewModel convertFromBibTexString(BibtexString bibtexString) { - return new BibtexStringViewModel(bibtexString.getName(), bibtexString.getContent()); - } - - public ObjectProperty seletedItemProperty() { - return this.selectedItemProperty; + private BibtexStringEditorItemModel convertFromBibTexString(BibtexString bibtexString) { + return new BibtexStringEditorItemModel(bibtexString.getName(), bibtexString.getContent()); } public void save() { - List stringsToAdd = allStrings.stream().map(this::fromBibtexStringViewModel).collect(Collectors.toList()); - bibDatabase.setStrings(stringsToAdd); + bibDatabase.setStrings(stringsListProperty.stream() + .map(this::fromBibtexStringViewModel) + .collect(Collectors.toList())); } - private BibtexString fromBibtexStringViewModel(BibtexStringViewModel viewModel) { + private BibtexString fromBibtexStringViewModel(BibtexStringEditorItemModel viewModel) { String label = viewModel.labelProperty().getValue(); String content = viewModel.contentProperty().getValue(); return new BibtexString(label, content); } - public BooleanProperty validProperty() { - return validProperty; + public Optional labelAlreadyExists(String label) { + return stringsListProperty.stream() + .filter(item -> item.labelProperty().getValue().equals(label)) + .findFirst(); } public void openHelpPage() { HelpAction.openHelpPage(HelpFile.STRING_EDITOR); } + + public ListProperty stringsListProperty() { + return stringsListProperty; + } + + public BooleanProperty validProperty() { + return validProperty; + } } diff --git a/src/main/java/org/jabref/gui/metadata/BibtexStringViewModel.java b/src/main/java/org/jabref/gui/metadata/BibtexStringEditorItemModel.java similarity index 93% rename from src/main/java/org/jabref/gui/metadata/BibtexStringViewModel.java rename to src/main/java/org/jabref/gui/metadata/BibtexStringEditorItemModel.java index c8dd02e52a6..00ba787ce42 100644 --- a/src/main/java/org/jabref/gui/metadata/BibtexStringViewModel.java +++ b/src/main/java/org/jabref/gui/metadata/BibtexStringEditorItemModel.java @@ -14,7 +14,7 @@ import de.saxsys.mvvmfx.utils.validation.ValidationStatus; import de.saxsys.mvvmfx.utils.validation.Validator; -public class BibtexStringViewModel { +public class BibtexStringEditorItemModel { private final static Pattern IS_NUMBER = Pattern.compile("-?\\d+(\\.\\d+)?"); @@ -25,12 +25,12 @@ public class BibtexStringViewModel { private final Validator contentValidator; private final CompositeValidator combinedValidator; - public BibtexStringViewModel(String label, String content) { + public BibtexStringEditorItemModel(String label, String content) { this.labelProperty.setValue(label); this.contentProperty.setValue(content); - labelValidator = new FunctionBasedValidator<>(this.labelProperty, BibtexStringViewModel::validateLabel); - contentValidator = new FunctionBasedValidator<>(this.contentProperty, BibtexStringViewModel::validateContent); + labelValidator = new FunctionBasedValidator<>(this.labelProperty, BibtexStringEditorItemModel::validateLabel); + contentValidator = new FunctionBasedValidator<>(this.contentProperty, BibtexStringEditorItemModel::validateContent); combinedValidator = new CompositeValidator(labelValidator, contentValidator); } diff --git a/src/main/java/org/jabref/gui/util/ViewModelListCellFactory.java b/src/main/java/org/jabref/gui/util/ViewModelListCellFactory.java index 59743c49248..2e9be5c1ae1 100644 --- a/src/main/java/org/jabref/gui/util/ViewModelListCellFactory.java +++ b/src/main/java/org/jabref/gui/util/ViewModelListCellFactory.java @@ -154,7 +154,7 @@ public ListCell call(ListView param) { return new ListCell<>() { - List subscriptions = new ArrayList<>(); + final List subscriptions = new ArrayList<>(); @Override protected void updateItem(T item, boolean empty) { @@ -170,6 +170,7 @@ protected void updateItem(T item, boolean empty) { setGraphic(null); setOnMouseClicked(null); setTooltip(null); + pseudoClassStateChanged(INVALID_PSEUDO_CLASS, false); } else { if (toText != null) { setText(toText.call(viewModel)); @@ -206,14 +207,20 @@ protected void updateItem(T item, boolean empty) { } for (Map.Entry>> pseudoClassWithCondition : pseudoClasses.entrySet()) { ObservableValue condition = pseudoClassWithCondition.getValue().call(viewModel); - Subscription subscription = BindingsHelper.includePseudoClassWhen(this, pseudoClassWithCondition.getKey(), condition); - subscriptions.add(subscription); + subscriptions.add(BindingsHelper.includePseudoClassWhen( + this, + pseudoClassWithCondition.getKey(), + condition)); } if (validationStatusProperty != null) { - validationStatusProperty.call(viewModel).getHighestMessage().ifPresent(message -> { - setTooltip(new Tooltip(message.getMessage())); - subscriptions.add(BindingsHelper.includePseudoClassWhen(this, INVALID_PSEUDO_CLASS, validationStatusProperty.call(viewModel).validProperty().not())); - }); + validationStatusProperty.call(viewModel) + .getHighestMessage() + .ifPresent(message -> setTooltip(new Tooltip(message.getMessage()))); + + subscriptions.add(BindingsHelper.includePseudoClassWhen( + this, + INVALID_PSEUDO_CLASS, + validationStatusProperty.call(viewModel).validProperty().not())); } } } diff --git a/src/main/java/org/jabref/gui/util/ViewModelTextFieldTableCellVisualizationFactory.java b/src/main/java/org/jabref/gui/util/ViewModelTextFieldTableCellVisualizationFactory.java index fcf93a067b0..e042404213a 100644 --- a/src/main/java/org/jabref/gui/util/ViewModelTextFieldTableCellVisualizationFactory.java +++ b/src/main/java/org/jabref/gui/util/ViewModelTextFieldTableCellVisualizationFactory.java @@ -1,25 +1,33 @@ package org.jabref.gui.util; +import java.util.ArrayList; +import java.util.List; +import java.util.Optional; import java.util.function.Function; +import javafx.application.Platform; +import javafx.css.PseudoClass; import javafx.scene.control.TableCell; import javafx.scene.control.TableColumn; +import javafx.scene.control.TextField; +import javafx.scene.control.Tooltip; import javafx.scene.control.cell.TextFieldTableCell; +import javafx.scene.layout.HBox; import javafx.util.Callback; import javafx.util.StringConverter; import de.saxsys.mvvmfx.utils.validation.ValidationStatus; -import de.saxsys.mvvmfx.utils.validation.visualization.ControlsFxVisualizer; +import org.fxmisc.easybind.Subscription; public class ViewModelTextFieldTableCellVisualizationFactory implements Callback, TableCell> { - private ControlsFxVisualizer visualizer; + private static final PseudoClass INVALID_PSEUDO_CLASS = PseudoClass.getPseudoClass("invalid"); + private Function validationStatusProperty; private StringConverter stringConverter; - public ViewModelTextFieldTableCellVisualizationFactory withValidation(Function validationStatusProperty, ControlsFxVisualizer visualizer) { + public ViewModelTextFieldTableCellVisualizationFactory withValidation(Function validationStatusProperty) { this.validationStatusProperty = validationStatusProperty; - this.visualizer = visualizer; return this; } @@ -30,25 +38,68 @@ public void install(TableColumn column, StringConverter stringConverter @Override public TextFieldTableCell call(TableColumn param) { - return new TextFieldTableCell(stringConverter) { + return new TextFieldTableCell<>(stringConverter) { + + final List subscriptions = new ArrayList<>(); + + @Override + public void startEdit() { + super.startEdit(); + + // The textfield is lazily created and not already present when a TableCell is created. + lookupTextField().ifPresent(textField -> Platform.runLater(() -> { + textField.requestFocus(); + textField.selectAll(); + })); + } + + /** + * As 'textfield' is a private member of TextFieldTableCell we need need to get to it through the backdoor. + * + * @return The TextField containing the editable content of the TableCell + */ + private Optional lookupTextField() { + if (getGraphic() instanceof TextField) { + return Optional.of((TextField) getGraphic()); + } else { + // Could be an HBox with some graphic and a TextField if a graphic is specified for the TableCell + if (getGraphic() instanceof HBox) { + HBox hbox = (HBox) getGraphic(); + if ((hbox.getChildren().size() > 1) && hbox.getChildren().get(1) instanceof TextField) { + return Optional.of((TextField) hbox.getChildren().get(1)); + } + } + return Optional.empty(); + } + } @Override public void updateItem(T item, boolean empty) { super.updateItem(item, empty); - if (!empty && (getTableRow() != null)) { - Object rowItem = getTableRow().getItem(); + subscriptions.forEach(Subscription::unsubscribe); + subscriptions.clear(); - if (rowItem != null) { - S vm = (S) rowItem; - if ((visualizer != null) && (validationStatusProperty != null)) { - visualizer.initVisualization(validationStatusProperty.apply(vm), this); - } + if (empty || getTableRow() == null || getTableRow().getItem() == null) { + setText(null); + setGraphic(null); + setOnMouseClicked(null); + setTooltip(null); + pseudoClassStateChanged(INVALID_PSEUDO_CLASS, false); + } else { + S viewModel = getTableRow().getItem(); + if (validationStatusProperty != null) { + validationStatusProperty.apply(viewModel) + .getHighestMessage() + .ifPresent(message -> setTooltip(new Tooltip(message.getMessage()))); + + subscriptions.add(BindingsHelper.includePseudoClassWhen( + this, + INVALID_PSEUDO_CLASS, + validationStatusProperty.apply(viewModel).validProperty().not())); } } } }; - } - } diff --git a/src/main/resources/l10n/JabRef_en.properties b/src/main/resources/l10n/JabRef_en.properties index b20516b6a12..eebbfc6bc94 100644 --- a/src/main/resources/l10n/JabRef_en.properties +++ b/src/main/resources/l10n/JabRef_en.properties @@ -696,7 +696,7 @@ Remove\ link=Remove link Remove\ old\ entry=Remove old entry -Remove\ selected\ strings=Remove selected strings +Remove\ string\ %0=Remove string %0 Removed\ group\ "%0".=Removed group "%0". @@ -1903,7 +1903,6 @@ Selected\ items\:=Selected items: Download\ linked\ online\ files=Download linked online files Select\ the\ entries\ to\ be\ imported\:=Select the entries to be imported\: Add\ new\ String=Add new String -Remove\ selected\ Strings=Remove selected Strings Must\ not\ be\ empty\!=Must not be empty\! Open\ Help\ page=Open Help page Add\ new\ field\ name=Add new field name