From 3867072c7e43c9729cf917d8d4c44ae3689c8447 Mon Sep 17 00:00:00 2001 From: Houssem Nasri Date: Sat, 10 Sep 2022 21:58:52 +0100 Subject: [PATCH 1/4] [GSOC22] - D - Test the Three Way Merge UI (#9069) * Rename isIsFieldsMerged to isFieldsMerged * [WIP] Test FieldRowViewModel * Remove FieldRowController.java * Move fields merge and unmerge logic to FieldRowViewModel for better testability * Fix tests * Test the mergeFields method * Fix tests * Test unmergeFields method * Add test case for mergeFields * Create BibEntry instance using withField() * Ignore missing or surplus spaces when merging groups * Test GroupMerger * Extract regex into a field of type Pattern * Improve groups splitting performance * Delete AbstractCell.java * Rename 'NO_ROW_INDEX' to 'HEADER_ROW' - Because NO_ROW_INDEX is passed to ThreeWayMergeCell for header cells only * Extract logic from ThreeWayMergeCell to a view model * Use CSS to style cell padding * Test ThreeWayMergeCellViewModel * Don't override setUserData() and getUserData() - Setting user data was useful before when FieldRow depended on it to figure out the value of the selected cell, but now it has a different implementation * Create FieldValueCellViewModel * Refactor OpenExternalLinkAction * Refactor ThreeWayMergeViewModel * Test ThreeWayMergeViewModel * Checkstyle * Disable a test * Update FieldRowViewModelTest.java Co-authored-by: Carl Christian Snethlage <50491877+calixtus@users.noreply.github.com> --- .../newmergedialog/FieldRowView.java | 82 +------ .../newmergedialog/FieldRowViewModel.java | 73 +++++- .../newmergedialog/ThreeWayMergeView.css | 4 + .../newmergedialog/ThreeWayMergeView.java | 4 +- .../ThreeWayMergeViewModel.java | 33 +-- .../newmergedialog/cell/AbstractCell.java | 82 ------- .../newmergedialog/cell/FieldValueCell.java | 51 ++-- .../cell/FieldValueCellViewModel.java | 55 +++++ .../newmergedialog/cell/HeaderCell.java | 2 +- .../cell/OpenExternalLinkAction.java | 30 +-- .../cell/ThreeWayMergeCell.java | 69 ++---- .../cell/ThreeWayMergeCellViewModel.java | 71 ++++++ .../fieldsmerger/GroupMerger.java | 4 +- .../mergeentries/FieldRowViewModelTest.java | 226 ++++++++++++++++++ .../gui/mergeentries/GroupMergerTest.java | 41 ++++ .../ThreeWayMergeCellViewModelTest.java | 95 ++++++++ .../ThreeWayMergeViewModelTest.java | 76 ++++++ 17 files changed, 707 insertions(+), 291 deletions(-) delete mode 100644 src/main/java/org/jabref/gui/mergeentries/newmergedialog/cell/AbstractCell.java create mode 100644 src/main/java/org/jabref/gui/mergeentries/newmergedialog/cell/FieldValueCellViewModel.java create mode 100644 src/main/java/org/jabref/gui/mergeentries/newmergedialog/cell/ThreeWayMergeCellViewModel.java create mode 100644 src/test/java/org/jabref/gui/mergeentries/FieldRowViewModelTest.java create mode 100644 src/test/java/org/jabref/gui/mergeentries/GroupMergerTest.java create mode 100644 src/test/java/org/jabref/gui/mergeentries/ThreeWayMergeCellViewModelTest.java create mode 100644 src/test/java/org/jabref/gui/mergeentries/ThreeWayMergeViewModelTest.java diff --git a/src/main/java/org/jabref/gui/mergeentries/newmergedialog/FieldRowView.java b/src/main/java/org/jabref/gui/mergeentries/newmergedialog/FieldRowView.java index 9b50e95817d..e6e1cb56f6a 100644 --- a/src/main/java/org/jabref/gui/mergeentries/newmergedialog/FieldRowView.java +++ b/src/main/java/org/jabref/gui/mergeentries/newmergedialog/FieldRowView.java @@ -1,24 +1,17 @@ package org.jabref.gui.mergeentries.newmergedialog; -import javax.swing.undo.AbstractUndoableEdit; -import javax.swing.undo.CannotRedoException; -import javax.swing.undo.CannotUndoException; -import javax.swing.undo.CompoundEdit; - import javafx.beans.property.BooleanProperty; import javafx.beans.property.ReadOnlyStringProperty; import javafx.beans.property.SimpleBooleanProperty; import javafx.scene.control.ToggleGroup; import javafx.scene.layout.GridPane; -import org.jabref.gui.actions.SimpleCommand; import org.jabref.gui.mergeentries.newmergedialog.cell.FieldNameCell; import org.jabref.gui.mergeentries.newmergedialog.cell.FieldValueCell; import org.jabref.gui.mergeentries.newmergedialog.cell.MergedFieldCell; import org.jabref.gui.mergeentries.newmergedialog.cell.sidebuttons.ToggleMergeUnmergeButton; import org.jabref.gui.mergeentries.newmergedialog.diffhighlighter.SplitDiffHighlighter; import org.jabref.gui.mergeentries.newmergedialog.diffhighlighter.UnifiedDiffHighlighter; -import org.jabref.gui.mergeentries.newmergedialog.fieldsmerger.FieldMerger; import org.jabref.gui.mergeentries.newmergedialog.fieldsmerger.FieldMergerFactory; import org.jabref.gui.mergeentries.newmergedialog.toolbar.ThreeWayMergeToolbar; import org.jabref.model.entry.BibEntry; @@ -48,10 +41,8 @@ public class FieldRowView { private final ToggleGroup toggleGroup = new ToggleGroup(); - private final CompoundEdit fieldsMergedEdit = new CompoundEdit(); - public FieldRowView(Field field, BibEntry leftEntry, BibEntry rightEntry, BibEntry mergedEntry, FieldMergerFactory fieldMergerFactory, int rowIndex) { - viewModel = new FieldRowViewModel(field, leftEntry, rightEntry, mergedEntry); + viewModel = new FieldRowViewModel(field, leftEntry, rightEntry, mergedEntry, fieldMergerFactory); fieldNameCell = new FieldNameCell(field.getDisplayName(), rowIndex); leftValueCell = new FieldValueCell(viewModel.getLeftFieldValue(), rowIndex); @@ -66,9 +57,9 @@ public FieldRowView(Field field, BibEntry leftEntry, BibEntry rightEntry, BibEnt EasyBind.listen(toggleMergeUnmergeButton.fieldStateProperty(), ((observableValue, old, fieldState) -> { LOGGER.debug("Field merge state is {} for field {}", fieldState, field); if (fieldState == ToggleMergeUnmergeButton.FieldState.MERGED) { - new MergeCommand(fieldMergerFactory.create(field)).execute(); + viewModel.mergeFields(); } else { - new UnmergeCommand().execute(); + viewModel.unmergeFields(); } })); } @@ -188,71 +179,4 @@ public void hideDiff() { getRightValueCell().getStyleClassedLabel().clearStyle(0, rightValueLength); getRightValueCell().getStyleClassedLabel().replaceText(viewModel.getRightFieldValue()); } - - public class MergeCommand extends SimpleCommand { - private final FieldMerger fieldMerger; - - public MergeCommand(FieldMerger fieldMerger) { - this.fieldMerger = fieldMerger; - - this.executable.bind(viewModel.hasEqualLeftAndRightBinding().not()); - } - - @Override - public void execute() { - assert !viewModel.getLeftFieldValue().equals(viewModel.getRightFieldValue()); - - String oldLeftFieldValue = viewModel.getLeftFieldValue(); - String oldRightFieldValue = viewModel.getRightFieldValue(); - - String mergedFields = fieldMerger.merge(viewModel.getLeftFieldValue(), viewModel.getRightFieldValue()); - viewModel.setLeftFieldValue(mergedFields); - viewModel.setRightFieldValue(mergedFields); - - if (fieldsMergedEdit.canRedo()) { - fieldsMergedEdit.redo(); - } else { - fieldsMergedEdit.addEdit(new MergeFieldsUndo(oldLeftFieldValue, oldRightFieldValue, mergedFields)); - fieldsMergedEdit.end(); - } - } - } - - public class UnmergeCommand extends SimpleCommand { - - public UnmergeCommand() { } - - @Override - public void execute() { - if (fieldsMergedEdit.canUndo()) { - fieldsMergedEdit.undo(); - } - } - } - - class MergeFieldsUndo extends AbstractUndoableEdit { - private final String oldLeft; - private final String oldRight; - private final String mergedFields; - - MergeFieldsUndo(String oldLeft, String oldRight, String mergedFields) { - this.oldLeft = oldLeft; - this.oldRight = oldRight; - this.mergedFields = mergedFields; - } - - @Override - public void undo() throws CannotUndoException { - super.undo(); - viewModel.setLeftFieldValue(oldLeft); - viewModel.setRightFieldValue(oldRight); - } - - @Override - public void redo() throws CannotRedoException { - super.redo(); - viewModel.setLeftFieldValue(mergedFields); - viewModel.setRightFieldValue(mergedFields); - } - } } diff --git a/src/main/java/org/jabref/gui/mergeentries/newmergedialog/FieldRowViewModel.java b/src/main/java/org/jabref/gui/mergeentries/newmergedialog/FieldRowViewModel.java index 06356de4177..46886d9bee1 100644 --- a/src/main/java/org/jabref/gui/mergeentries/newmergedialog/FieldRowViewModel.java +++ b/src/main/java/org/jabref/gui/mergeentries/newmergedialog/FieldRowViewModel.java @@ -1,5 +1,10 @@ package org.jabref.gui.mergeentries.newmergedialog; +import javax.swing.undo.AbstractUndoableEdit; +import javax.swing.undo.CannotRedoException; +import javax.swing.undo.CannotUndoException; +import javax.swing.undo.CompoundEdit; + import javafx.beans.binding.Bindings; import javafx.beans.binding.BooleanBinding; import javafx.beans.property.BooleanProperty; @@ -9,6 +14,8 @@ import javafx.beans.property.SimpleStringProperty; import javafx.beans.property.StringProperty; +import org.jabref.gui.mergeentries.newmergedialog.fieldsmerger.FieldMerger; +import org.jabref.gui.mergeentries.newmergedialog.fieldsmerger.FieldMergerFactory; import org.jabref.model.entry.BibEntry; import org.jabref.model.entry.field.Field; import org.jabref.model.entry.field.InternalField; @@ -49,11 +56,16 @@ public enum Selection { private final BooleanBinding hasEqualLeftAndRight; - public FieldRowViewModel(Field field, BibEntry leftEntry, BibEntry rightEntry, BibEntry mergedEntry) { + private final FieldMergerFactory fieldMergerFactory; + + private final CompoundEdit fieldsMergedEdit = new CompoundEdit(); + + public FieldRowViewModel(Field field, BibEntry leftEntry, BibEntry rightEntry, BibEntry mergedEntry, FieldMergerFactory fieldMergerFactory) { this.field = field; this.leftEntry = leftEntry; this.rightEntry = rightEntry; this.mergedEntry = mergedEntry; + this.fieldMergerFactory = fieldMergerFactory; if (field.equals(InternalField.TYPE_HEADER)) { setLeftFieldValue(leftEntry.getType().getDisplayName()); @@ -125,7 +137,7 @@ public void selectLeftValue() { } public void selectRightValue() { - if (isIsFieldsMerged()) { + if (isFieldsMerged()) { selectLeftValue(); } else { setSelection(Selection.RIGHT); @@ -148,8 +160,33 @@ public String getMergedFieldValue() { return mergedFieldValue.get(); } - public void merge() { - setIsFieldsMerged(true); + public void mergeFields() { + assert !hasEqualLeftAndRightValues(); + + if (!FieldMergerFactory.canMerge(field)) { + throw new UnsupportedOperationException(); + } + + String oldLeftFieldValue = getLeftFieldValue(); + String oldRightFieldValue = getRightFieldValue(); + + FieldMerger fieldMerger = fieldMergerFactory.create(field); + String mergedFields = fieldMerger.merge(getLeftFieldValue(), getRightFieldValue()); + setLeftFieldValue(mergedFields); + setRightFieldValue(mergedFields); + + if (fieldsMergedEdit.canRedo()) { + fieldsMergedEdit.redo(); + } else { + fieldsMergedEdit.addEdit(new MergeFieldsUndo(oldLeftFieldValue, oldRightFieldValue, mergedFields)); + fieldsMergedEdit.end(); + } + } + + public void unmergeFields() { + if (fieldsMergedEdit.canUndo()) { + fieldsMergedEdit.undo(); + } } public BooleanBinding hasEqualLeftAndRightBinding() { @@ -168,7 +205,7 @@ public Selection getSelection() { return selectionProperty().get(); } - public boolean isIsFieldsMerged() { + public boolean isFieldsMerged() { return isFieldsMerged.get(); } @@ -219,4 +256,30 @@ public BibEntry getRightEntry() { public BibEntry getMergedEntry() { return mergedEntry; } + + class MergeFieldsUndo extends AbstractUndoableEdit { + private final String oldLeft; + private final String oldRight; + private final String mergedFields; + + MergeFieldsUndo(String oldLeft, String oldRight, String mergedFields) { + this.oldLeft = oldLeft; + this.oldRight = oldRight; + this.mergedFields = mergedFields; + } + + @Override + public void undo() throws CannotUndoException { + super.undo(); + setLeftFieldValue(oldLeft); + setRightFieldValue(oldRight); + } + + @Override + public void redo() throws CannotRedoException { + super.redo(); + setLeftFieldValue(mergedFields); + setRightFieldValue(mergedFields); + } + } } diff --git a/src/main/java/org/jabref/gui/mergeentries/newmergedialog/ThreeWayMergeView.css b/src/main/java/org/jabref/gui/mergeentries/newmergedialog/ThreeWayMergeView.css index ac73d7593f0..52b87f36949 100644 --- a/src/main/java/org/jabref/gui/mergeentries/newmergedialog/ThreeWayMergeView.css +++ b/src/main/java/org/jabref/gui/mergeentries/newmergedialog/ThreeWayMergeView.css @@ -35,6 +35,10 @@ -fx-background-color: -jr-row-even-background; } +.field-cell { + -fx-padding: 8; +} + .merge-toolbox { -fx-background-color: -jr-menu-background; } diff --git a/src/main/java/org/jabref/gui/mergeentries/newmergedialog/ThreeWayMergeView.java b/src/main/java/org/jabref/gui/mergeentries/newmergedialog/ThreeWayMergeView.java index 06f6861d751..f9c34886c86 100644 --- a/src/main/java/org/jabref/gui/mergeentries/newmergedialog/ThreeWayMergeView.java +++ b/src/main/java/org/jabref/gui/mergeentries/newmergedialog/ThreeWayMergeView.java @@ -106,7 +106,7 @@ private void initializeColumnConstraints() { private void initializeMergeGridPane() { mergeGridPane.getColumnConstraints().addAll(fieldNameColumnConstraints, leftEntryColumnConstraints, rightEntryColumnConstraints, mergedEntryColumnConstraints); - for (int fieldIndex = 0; fieldIndex < viewModel.allFieldsSize(); fieldIndex++) { + for (int fieldIndex = 0; fieldIndex < viewModel.numberOfVisibleFields(); fieldIndex++) { addRow(fieldIndex); mergeGridPane.getRowConstraints().add(new RowConstraints()); @@ -114,7 +114,7 @@ private void initializeMergeGridPane() { } private Field getFieldAtIndex(int index) { - return viewModel.allFields().get(index); + return viewModel.getVisibleFields().get(index); } private void addRow(int fieldIndex) { diff --git a/src/main/java/org/jabref/gui/mergeentries/newmergedialog/ThreeWayMergeViewModel.java b/src/main/java/org/jabref/gui/mergeentries/newmergedialog/ThreeWayMergeViewModel.java index 5314a725875..8df57827673 100644 --- a/src/main/java/org/jabref/gui/mergeentries/newmergedialog/ThreeWayMergeViewModel.java +++ b/src/main/java/org/jabref/gui/mergeentries/newmergedialog/ThreeWayMergeViewModel.java @@ -1,9 +1,10 @@ package org.jabref.gui.mergeentries.newmergedialog; import java.util.Comparator; -import java.util.HashSet; import java.util.Objects; import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.Stream; import javafx.beans.property.ObjectProperty; import javafx.beans.property.SimpleObjectProperty; @@ -18,7 +19,7 @@ import org.jabref.model.entry.field.FieldFactory; import org.jabref.model.entry.field.InternalField; -class ThreeWayMergeViewModel extends AbstractViewModel { +public class ThreeWayMergeViewModel extends AbstractViewModel { private final ObjectProperty leftEntry = new SimpleObjectProperty<>(); private final ObjectProperty rightEntry = new SimpleObjectProperty<>(); @@ -26,7 +27,7 @@ class ThreeWayMergeViewModel extends AbstractViewModel { private final StringProperty leftHeader = new SimpleStringProperty(); private final StringProperty rightHeader = new SimpleStringProperty(); - private final ObservableList allFields = FXCollections.observableArrayList(); + private final ObservableList visibleFields = FXCollections.observableArrayList(); public ThreeWayMergeViewModel(BibEntry leftEntry, BibEntry rightEntry, String leftHeader, String rightHeader) { Objects.requireNonNull(leftEntry, "Left entry is required"); @@ -41,9 +42,9 @@ public ThreeWayMergeViewModel(BibEntry leftEntry, BibEntry rightEntry, String le mergedEntry.set(new BibEntry()); - Set leftAndRightFieldsUnion = new HashSet<>(leftEntry.getFields()); - leftAndRightFieldsUnion.addAll(rightEntry.getFields()); - setAllFields(leftAndRightFieldsUnion); + setVisibleFields(Stream.concat( + leftEntry.getFields().stream(), + rightEntry.getFields().stream()).collect(Collectors.toSet())); } public StringProperty leftHeaderProperty() { @@ -90,26 +91,26 @@ public BibEntry getMergedEntry() { return mergedEntry.get(); } - public ObservableList allFields() { - return allFields; + public ObservableList getVisibleFields() { + return visibleFields; } /** * Convince method to determine the total number of fields in the union of the left and right fields. */ - public int allFieldsSize() { - return allFields.size(); + public int numberOfVisibleFields() { + return visibleFields.size(); } - private void setAllFields(Set fields) { - allFields.clear(); - allFields.addAll(fields); + private void setVisibleFields(Set fields) { + visibleFields.clear(); + visibleFields.addAll(fields); // Don't show internal fields. See org.jabref.model.entry.field.InternalField - allFields.removeIf(FieldFactory::isInternalField); + visibleFields.removeIf(FieldFactory::isInternalField); - allFields.sort(Comparator.comparing(Field::getName)); + visibleFields.sort(Comparator.comparing(Field::getName)); // Add the entry type field as the first field to display - allFields.add(0, InternalField.TYPE_HEADER); + visibleFields.add(0, InternalField.TYPE_HEADER); } } diff --git a/src/main/java/org/jabref/gui/mergeentries/newmergedialog/cell/AbstractCell.java b/src/main/java/org/jabref/gui/mergeentries/newmergedialog/cell/AbstractCell.java deleted file mode 100644 index ffb1f536986..00000000000 --- a/src/main/java/org/jabref/gui/mergeentries/newmergedialog/cell/AbstractCell.java +++ /dev/null @@ -1,82 +0,0 @@ -package org.jabref.gui.mergeentries.newmergedialog.cell; - -import javafx.beans.property.BooleanProperty; -import javafx.beans.property.BooleanPropertyBase; -import javafx.beans.property.SimpleStringProperty; -import javafx.beans.property.StringProperty; -import javafx.css.PseudoClass; -import javafx.geometry.Insets; -import javafx.scene.layout.HBox; - -/** - * - */ -public abstract class AbstractCell extends HBox { - public static final String ODD_PSEUDO_CLASS = "odd"; - public static final String EVEN_PSEUDO_CLASS = "even"; - public static final int NO_ROW_NUMBER = -1; - private static final String DEFAULT_STYLE_CLASS = "field-cell"; - private final StringProperty text = new SimpleStringProperty(); - private final BooleanProperty odd = new BooleanPropertyBase() { - @Override - public Object getBean() { - return AbstractCell.this; - } - - @Override - public String getName() { - return "odd"; - } - - @Override - protected void invalidated() { - pseudoClassStateChanged(PseudoClass.getPseudoClass(ODD_PSEUDO_CLASS), get()); - pseudoClassStateChanged(PseudoClass.getPseudoClass(EVEN_PSEUDO_CLASS), !get()); - } - }; - - private final BooleanProperty even = new BooleanPropertyBase() { - @Override - public Object getBean() { - return AbstractCell.this; - } - - @Override - public String getName() { - return "even"; - } - - @Override - protected void invalidated() { - pseudoClassStateChanged(PseudoClass.getPseudoClass(EVEN_PSEUDO_CLASS), get()); - pseudoClassStateChanged(PseudoClass.getPseudoClass(ODD_PSEUDO_CLASS), !get()); - } - }; - - public AbstractCell(String text, int rowIndex) { - getStyleClass().add(DEFAULT_STYLE_CLASS); - if (rowIndex != NO_ROW_NUMBER) { - if (rowIndex % 2 == 1) { - odd.setValue(true); - } else { - even.setValue(true); - } - } - - setPadding(new Insets(8)); - - setText(text); - } - - public String getText() { - return textProperty().get(); - } - - public StringProperty textProperty() { - return text; - } - - public void setText(String text) { - textProperty().set(text); - } -} diff --git a/src/main/java/org/jabref/gui/mergeentries/newmergedialog/cell/FieldValueCell.java b/src/main/java/org/jabref/gui/mergeentries/newmergedialog/cell/FieldValueCell.java index 04fda36975d..4d3c1e57fd8 100644 --- a/src/main/java/org/jabref/gui/mergeentries/newmergedialog/cell/FieldValueCell.java +++ b/src/main/java/org/jabref/gui/mergeentries/newmergedialog/cell/FieldValueCell.java @@ -1,9 +1,7 @@ package org.jabref.gui.mergeentries.newmergedialog.cell; import javafx.beans.property.BooleanProperty; -import javafx.beans.property.BooleanPropertyBase; import javafx.beans.property.ObjectProperty; -import javafx.beans.property.SimpleObjectProperty; import javafx.css.PseudoClass; import javafx.geometry.Insets; import javafx.geometry.Pos; @@ -47,36 +45,26 @@ public class FieldValueCell extends ThreeWayMergeCell implements Toggle { public static final String SELECTION_BOX_STYLE_CLASS = "selection-box"; private static final PseudoClass SELECTED_PSEUDO_CLASS = PseudoClass.getPseudoClass("selected"); - private final ObjectProperty toggleGroup = new SimpleObjectProperty<>(); + private final StyleClassedTextArea label = new StyleClassedTextArea(); private final ActionFactory factory = new ActionFactory(Globals.getKeyPrefs()); private final VirtualizedScrollPane scrollPane = new VirtualizedScrollPane<>(label); HBox labelBox = new HBox(scrollPane); - private final BooleanProperty selected = new BooleanPropertyBase() { - @Override - public Object getBean() { - return FieldValueCell.class; - } - - @Override - public String getName() { - return "selected"; - } - - @Override - protected void invalidated() { - pseudoClassStateChanged(SELECTED_PSEUDO_CLASS, get()); - getToggleGroup().selectToggle(FieldValueCell.this); - } - }; private final HBox selectionBox = new HBox(); private final HBox actionsContainer = new HBox(); + private final FieldValueCellViewModel viewModel; public FieldValueCell(String text, int rowIndex) { super(text, rowIndex); + viewModel = new FieldValueCellViewModel(text); + EasyBind.listen(viewModel.selectedProperty(), (observable, old, isSelected) -> { + pseudoClassStateChanged(SELECTED_PSEUDO_CLASS, isSelected); + getToggleGroup().selectToggle(FieldValueCell.this); + }); + viewModel.fieldValueProperty().bind(textProperty()); initialize(); } @@ -86,7 +74,6 @@ private void initialize() { initializeLabel(); initializeSelectionBox(); initializeActions(); - textProperty().addListener(invalidated -> setUserData(getText())); setOnMouseClicked(e -> { if (!isDisabled()) { setSelected(true); @@ -180,42 +167,32 @@ private void preventTextSelectionViaMouseEvents() { @Override public ToggleGroup getToggleGroup() { - return toggleGroupProperty().get(); + return viewModel.getToggleGroup(); } @Override public void setToggleGroup(ToggleGroup toggleGroup) { - toggleGroupProperty().set(toggleGroup); + viewModel.setToggleGroup(toggleGroup); } @Override public ObjectProperty toggleGroupProperty() { - return toggleGroup; + return viewModel.toggleGroupProperty(); } @Override public boolean isSelected() { - return selectedProperty().get(); + return viewModel.isSelected(); } @Override public void setSelected(boolean selected) { - selectedProperty().set(selected); + viewModel.setSelected(selected); } @Override public BooleanProperty selectedProperty() { - return selected; - } - - @Override - public void setUserData(Object value) { - super.setText((String) value); - } - - @Override - public Object getUserData() { - return super.getText(); + return viewModel.selectedProperty(); } public StyleClassedTextArea getStyleClassedLabel() { diff --git a/src/main/java/org/jabref/gui/mergeentries/newmergedialog/cell/FieldValueCellViewModel.java b/src/main/java/org/jabref/gui/mergeentries/newmergedialog/cell/FieldValueCellViewModel.java new file mode 100644 index 00000000000..7587d1e261b --- /dev/null +++ b/src/main/java/org/jabref/gui/mergeentries/newmergedialog/cell/FieldValueCellViewModel.java @@ -0,0 +1,55 @@ +package org.jabref.gui.mergeentries.newmergedialog.cell; + +import javafx.beans.property.BooleanProperty; +import javafx.beans.property.ObjectProperty; +import javafx.beans.property.SimpleBooleanProperty; +import javafx.beans.property.SimpleObjectProperty; +import javafx.beans.property.SimpleStringProperty; +import javafx.beans.property.StringProperty; +import javafx.scene.control.ToggleGroup; + +public class FieldValueCellViewModel { + private final StringProperty fieldValue = new SimpleStringProperty(); + private final BooleanProperty selected = new SimpleBooleanProperty(FieldValueCell.class, "selected"); + private final ObjectProperty toggleGroup = new SimpleObjectProperty<>(); + + public FieldValueCellViewModel(String text) { + setFieldValue(text); + } + + public String getFieldValue() { + return fieldValue.get(); + } + + public StringProperty fieldValueProperty() { + return fieldValue; + } + + public void setFieldValue(String fieldValue) { + this.fieldValue.set(fieldValue); + } + + public boolean isSelected() { + return selected.get(); + } + + public BooleanProperty selectedProperty() { + return selected; + } + + public void setSelected(boolean selected) { + this.selected.set(selected); + } + + public ToggleGroup getToggleGroup() { + return toggleGroup.get(); + } + + public ObjectProperty toggleGroupProperty() { + return toggleGroup; + } + + public void setToggleGroup(ToggleGroup toggleGroup) { + this.toggleGroup.set(toggleGroup); + } +} diff --git a/src/main/java/org/jabref/gui/mergeentries/newmergedialog/cell/HeaderCell.java b/src/main/java/org/jabref/gui/mergeentries/newmergedialog/cell/HeaderCell.java index 76d3fceff0a..70ca433229a 100644 --- a/src/main/java/org/jabref/gui/mergeentries/newmergedialog/cell/HeaderCell.java +++ b/src/main/java/org/jabref/gui/mergeentries/newmergedialog/cell/HeaderCell.java @@ -11,7 +11,7 @@ public class HeaderCell extends ThreeWayMergeCell { private final Label label = new Label(); public HeaderCell(String text) { - super(text, ThreeWayMergeCell.NO_ROW_NUMBER); + super(text, ThreeWayMergeCell.HEADER_ROW); initialize(); } diff --git a/src/main/java/org/jabref/gui/mergeentries/newmergedialog/cell/OpenExternalLinkAction.java b/src/main/java/org/jabref/gui/mergeentries/newmergedialog/cell/OpenExternalLinkAction.java index 1b5087b413a..0d0a2e08230 100644 --- a/src/main/java/org/jabref/gui/mergeentries/newmergedialog/cell/OpenExternalLinkAction.java +++ b/src/main/java/org/jabref/gui/mergeentries/newmergedialog/cell/OpenExternalLinkAction.java @@ -6,13 +6,16 @@ import org.jabref.gui.actions.SimpleCommand; import org.jabref.gui.desktop.JabRefDesktop; import org.jabref.model.entry.identifier.DOI; -import org.jabref.model.strings.StringUtil; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * A command for opening DOIs and URLs. This was created primarily for simplifying {@link FieldValueCell}. - * */ + */ public class OpenExternalLinkAction extends SimpleCommand { private final String urlOrDoi; + private final Logger LOGGER = LoggerFactory.getLogger(OpenExternalLinkAction.class); public OpenExternalLinkAction(String urlOrDoi) { this.urlOrDoi = urlOrDoi; @@ -20,22 +23,21 @@ public OpenExternalLinkAction(String urlOrDoi) { @Override public void execute() { - if (StringUtil.isBlank(urlOrDoi)) { - return; - } - try { - String url; if (DOI.isValid(urlOrDoi)) { - url = DOI.parse(urlOrDoi).flatMap(DOI::getExternalURI).map(URI::toString).orElse(""); + JabRefDesktop.openBrowser( + DOI.parse(urlOrDoi) + .flatMap(DOI::getExternalURI) + .map(URI::toString) + .orElse("") + ); } else { - url = urlOrDoi; + JabRefDesktop.openBrowser( + urlOrDoi + ); } - - JabRefDesktop.openBrowser(url); - } catch ( - IOException ex) { - // TODO: Do something + } catch (IOException e) { + LOGGER.warn("Cannot open the given external link '{}'", urlOrDoi, e); } } } diff --git a/src/main/java/org/jabref/gui/mergeentries/newmergedialog/cell/ThreeWayMergeCell.java b/src/main/java/org/jabref/gui/mergeentries/newmergedialog/cell/ThreeWayMergeCell.java index c3496fcecb6..a7afc813698 100644 --- a/src/main/java/org/jabref/gui/mergeentries/newmergedialog/cell/ThreeWayMergeCell.java +++ b/src/main/java/org/jabref/gui/mergeentries/newmergedialog/cell/ThreeWayMergeCell.java @@ -1,82 +1,43 @@ package org.jabref.gui.mergeentries.newmergedialog.cell; -import javafx.beans.property.BooleanProperty; -import javafx.beans.property.BooleanPropertyBase; -import javafx.beans.property.SimpleStringProperty; import javafx.beans.property.StringProperty; import javafx.css.PseudoClass; -import javafx.geometry.Insets; import javafx.scene.layout.HBox; +import com.tobiasdiez.easybind.EasyBind; + /** * */ public abstract class ThreeWayMergeCell extends HBox { public static final String ODD_PSEUDO_CLASS = "odd"; public static final String EVEN_PSEUDO_CLASS = "even"; - public static final int NO_ROW_NUMBER = -1; + public static final int HEADER_ROW = -1; private static final String DEFAULT_STYLE_CLASS = "field-cell"; - private final StringProperty text = new SimpleStringProperty(); - private final BooleanProperty odd = new BooleanPropertyBase() { - @Override - public Object getBean() { - return ThreeWayMergeCell.this; - } - - @Override - public String getName() { - return "odd"; - } - - @Override - protected void invalidated() { - pseudoClassStateChanged(PseudoClass.getPseudoClass(ODD_PSEUDO_CLASS), get()); - pseudoClassStateChanged(PseudoClass.getPseudoClass(EVEN_PSEUDO_CLASS), !get()); - } - }; - - private final BooleanProperty even = new BooleanPropertyBase() { - @Override - public Object getBean() { - return ThreeWayMergeCell.this; - } - @Override - public String getName() { - return "even"; - } - - @Override - protected void invalidated() { - pseudoClassStateChanged(PseudoClass.getPseudoClass(EVEN_PSEUDO_CLASS), get()); - pseudoClassStateChanged(PseudoClass.getPseudoClass(ODD_PSEUDO_CLASS), !get()); - } - }; + private final ThreeWayMergeCellViewModel viewModel; public ThreeWayMergeCell(String text, int rowIndex) { getStyleClass().add(DEFAULT_STYLE_CLASS); - if (rowIndex != NO_ROW_NUMBER) { - if (rowIndex % 2 == 1) { - odd.setValue(true); - } else { - even.setValue(true); - } - } - - setPadding(new Insets(8)); - - setText(text); + viewModel = new ThreeWayMergeCellViewModel(text, rowIndex); + + EasyBind.subscribe(viewModel.oddProperty(), isOdd -> { + pseudoClassStateChanged(PseudoClass.getPseudoClass(ODD_PSEUDO_CLASS), isOdd); + }); + EasyBind.subscribe(viewModel.evenProperty(), isEven -> { + pseudoClassStateChanged(PseudoClass.getPseudoClass(EVEN_PSEUDO_CLASS), isEven); + }); } public String getText() { - return textProperty().get(); + return viewModel.getText(); } public StringProperty textProperty() { - return text; + return viewModel.textProperty(); } public void setText(String text) { - textProperty().set(text); + viewModel.setText(text); } } diff --git a/src/main/java/org/jabref/gui/mergeentries/newmergedialog/cell/ThreeWayMergeCellViewModel.java b/src/main/java/org/jabref/gui/mergeentries/newmergedialog/cell/ThreeWayMergeCellViewModel.java new file mode 100644 index 00000000000..e5674a60251 --- /dev/null +++ b/src/main/java/org/jabref/gui/mergeentries/newmergedialog/cell/ThreeWayMergeCellViewModel.java @@ -0,0 +1,71 @@ +package org.jabref.gui.mergeentries.newmergedialog.cell; + +import javafx.beans.property.BooleanProperty; +import javafx.beans.property.SimpleBooleanProperty; +import javafx.beans.property.SimpleStringProperty; +import javafx.beans.property.StringProperty; + +import com.tobiasdiez.easybind.EasyBind; + +import static org.jabref.gui.mergeentries.newmergedialog.cell.ThreeWayMergeCell.HEADER_ROW; + +public class ThreeWayMergeCellViewModel { + private final StringProperty text = new SimpleStringProperty(); + private final BooleanProperty odd = new SimpleBooleanProperty(ThreeWayMergeCell.class, "odd"); + private final BooleanProperty even = new SimpleBooleanProperty(ThreeWayMergeCell.class, "even"); + + public ThreeWayMergeCellViewModel(String text, int rowIndex) { + setText(text); + if (rowIndex != HEADER_ROW) { + if (rowIndex % 2 == 1) { + odd.setValue(true); + } else { + even.setValue(true); + } + } + + EasyBind.subscribe(odd, isOdd -> { + setEven(!isOdd); + }); + + EasyBind.subscribe(even, isEven -> { + setOdd(!isEven); + }); + } + + public String getText() { + return text.get(); + } + + public StringProperty textProperty() { + return text; + } + + public void setText(String text) { + this.text.set(text); + } + + public boolean isOdd() { + return odd.get(); + } + + public BooleanProperty oddProperty() { + return odd; + } + + public void setOdd(boolean odd) { + this.odd.set(odd); + } + + public boolean isEven() { + return even.get(); + } + + public BooleanProperty evenProperty() { + return even; + } + + public void setEven(boolean even) { + this.even.set(even); + } +} diff --git a/src/main/java/org/jabref/gui/mergeentries/newmergedialog/fieldsmerger/GroupMerger.java b/src/main/java/org/jabref/gui/mergeentries/newmergedialog/fieldsmerger/GroupMerger.java index fdbbddbb268..2b892fcd31a 100644 --- a/src/main/java/org/jabref/gui/mergeentries/newmergedialog/fieldsmerger/GroupMerger.java +++ b/src/main/java/org/jabref/gui/mergeentries/newmergedialog/fieldsmerger/GroupMerger.java @@ -1,6 +1,7 @@ package org.jabref.gui.mergeentries.newmergedialog.fieldsmerger; import java.util.Arrays; +import java.util.regex.Pattern; import java.util.stream.Collectors; import org.jabref.model.entry.field.StandardField; @@ -11,6 +12,7 @@ * */ public class GroupMerger implements FieldMerger { public static final String GROUPS_SEPARATOR = ", "; + public static final Pattern GROUPS_SEPARATOR_REGEX = Pattern.compile("\s*,\s*"); @Override public String merge(String groupsA, String groupsB) { @@ -21,7 +23,7 @@ public String merge(String groupsA, String groupsB) { } else if (StringUtil.isBlank(groupsB)) { return groupsA; } else { - return Arrays.stream((groupsA + GROUPS_SEPARATOR + groupsB).split(GROUPS_SEPARATOR)) + return Arrays.stream(GROUPS_SEPARATOR_REGEX.split(groupsA + GROUPS_SEPARATOR + groupsB)) .distinct() .collect(Collectors.joining(GROUPS_SEPARATOR)); } diff --git a/src/test/java/org/jabref/gui/mergeentries/FieldRowViewModelTest.java b/src/test/java/org/jabref/gui/mergeentries/FieldRowViewModelTest.java new file mode 100644 index 00000000000..dee615603ad --- /dev/null +++ b/src/test/java/org/jabref/gui/mergeentries/FieldRowViewModelTest.java @@ -0,0 +1,226 @@ +package org.jabref.gui.mergeentries; + +import java.util.Optional; + +import org.jabref.gui.groups.GroupsPreferences; +import org.jabref.gui.mergeentries.newmergedialog.FieldRowViewModel; +import org.jabref.gui.mergeentries.newmergedialog.fieldsmerger.FieldMergerFactory; +import org.jabref.model.entry.BibEntry; +import org.jabref.model.entry.Month; +import org.jabref.model.entry.field.Field; +import org.jabref.model.entry.field.StandardField; +import org.jabref.model.entry.types.StandardEntryType; +import org.jabref.preferences.PreferencesService; + +import org.jbibtex.ParseException; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Disabled; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +public class FieldRowViewModelTest { + + BibEntry leftEntry; + BibEntry rightEntry; + + BibEntry extraEntry; + + BibEntry mergedEntry; + + FieldRowViewModel viewModel; + + FieldMergerFactory fieldMergerFactory; + + PreferencesService mockedPrefs = mock(PreferencesService.class); + GroupsPreferences mockedGroupsPrefs = mock(GroupsPreferences.class); + + @BeforeEach + public void setup() throws ParseException { + leftEntry = new BibEntry(StandardEntryType.Article) + .withCitationKey("LajnDiezScheinEtAl2012") + .withField(StandardField.AUTHOR, "Lajn, A and Diez, T and Schein, F and Frenzel, H and von Wenckstern, H and Grundmann, M") + .withField(StandardField.TITLE, "Light and temperature stability of fully transparent ZnO-based inverter circuits") + .withField(StandardField.NUMBER, "4") + .withField(StandardField.PAGES, "515--517") + .withField(StandardField.VOLUME, "32") + .withField(StandardField.GROUPS, "Skimmed") + .withField(StandardField.JOURNAL, "Electron Device Letters, IEEE") + .withField(StandardField.PUBLISHER, "IEEE") + .withField(StandardField.YEAR, "2012"); + + rightEntry = new BibEntry(StandardEntryType.Book) + .withCitationKey("KolbLenhardWirtz2012") + .withField(StandardField.AUTHOR, "Stefan Kolb and Guido Wirtz") + .withField(StandardField.BOOKTITLE, "Proceedings of the 5\\textsuperscript{th} {IEEE} International Conference on Service-Oriented Computing and Applications {(SOCA'12)}, Taipei, Taiwan") + .withField(StandardField.TITLE, "{Bridging the Heterogeneity of Orchestrations - A Petri Net-based Integration of BPEL and Windows Workflow}") + .withField(StandardField.ORGANIZATION, "IEEE") + .withField(StandardField.PAGES, "1--8") + .withField(StandardField.ADDRESS, "Oxford, United Kingdom") + .withField(StandardField.GROUPS, "By rating, Skimmed") + .withMonth(Month.DECEMBER) + .withField(StandardField.KEYWORDS, "a, b, c") + .withField(StandardField.YEAR, "2012"); + + extraEntry = new BibEntry(StandardEntryType.InProceedings) + .withCitationKey("BoopalGarridoGustafsson2013") + .withField(StandardField.AUTHOR, "Padma Prasad Boopal and Mario Garrido and Oscar Gustafsson") + .withField(StandardField.BOOKTITLE, "2013 {IEEE} International Symposium on Circuits and Systems (ISCAS2013), Beijing, China, May 19-23, 2013") + .withField(StandardField.TITLE, "A reconfigurable {FFT} architecture for variable-length and multi-streaming {OFDM} standards") + .withField(StandardField.KEYWORDS, "b, c, a") + .withField(StandardField.YEAR, "2013"); + + mergedEntry = new BibEntry(); + fieldMergerFactory = new FieldMergerFactory(mockedPrefs); + + when(mockedPrefs.getGroupsPreferences()).thenReturn(mockedGroupsPrefs); + when(mockedGroupsPrefs.getKeywordSeparator()).thenReturn(','); + } + + @Test + public void selectNonEmptyValueShouldSelectLeftFieldValueIfItIsNotEmpty() { + var numberFieldViewModel = createViewModelForField(StandardField.NUMBER); + numberFieldViewModel.selectNonEmptyValue(); + assertEquals(FieldRowViewModel.Selection.LEFT, numberFieldViewModel.getSelection()); + + var authorFieldViewModel = createViewModelForField(StandardField.AUTHOR); + authorFieldViewModel.selectNonEmptyValue(); + assertEquals(FieldRowViewModel.Selection.LEFT, authorFieldViewModel.getSelection()); + } + + @Test + public void selectNonEmptyValueShouldSelectRightFieldValueIfLeftValueIsEmpty() { + var monthFieldViewModel = createViewModelForField(StandardField.MONTH); + monthFieldViewModel.selectNonEmptyValue(); + assertEquals(FieldRowViewModel.Selection.RIGHT, monthFieldViewModel.getSelection()); + + var addressFieldViewModel = createViewModelForField(StandardField.ADDRESS); + addressFieldViewModel.selectNonEmptyValue(); + assertEquals(FieldRowViewModel.Selection.RIGHT, addressFieldViewModel.getSelection()); + } + + @Test + public void hasEqualLeftAndRightValuesShouldReturnFalseIfOneOfTheValuesIsEmpty() { + var monthFieldViewModel = createViewModelForField(StandardField.MONTH); + monthFieldViewModel.selectNonEmptyValue(); + assertFalse(monthFieldViewModel.hasEqualLeftAndRightValues()); + } + + @Test + public void hasEqualLeftAndRightValuesShouldReturnTrueIfLeftAndRightAreEqual() { + var yearFieldViewModel = createViewModelForField(StandardField.YEAR); + yearFieldViewModel.selectNonEmptyValue(); + assertTrue(yearFieldViewModel.hasEqualLeftAndRightValues()); + } + + @Test + @Disabled("This test is kept as a reminder to implement a different comparison logic based on the given field.") + public void hasEqualLeftAndRightValuesShouldReturnTrueIfKeywordsAreEqual() { + FieldRowViewModel keywordsField = new FieldRowViewModel(StandardField.KEYWORDS, rightEntry, extraEntry, mergedEntry, fieldMergerFactory); + assertTrue(keywordsField.hasEqualLeftAndRightValues()); + } + + @Test + public void selectLeftValueShouldBeCorrect() { + var monthFieldViewModel = createViewModelForField(StandardField.MONTH); + monthFieldViewModel.selectLeftValue(); + assertEquals(FieldRowViewModel.Selection.LEFT, monthFieldViewModel.getSelection()); + assertEquals(Optional.of(""), Optional.ofNullable(monthFieldViewModel.getMergedFieldValue())); + + monthFieldViewModel.selectRightValue(); + monthFieldViewModel.selectLeftValue(); + assertEquals(FieldRowViewModel.Selection.LEFT, monthFieldViewModel.getSelection()); + assertEquals(Optional.of(""), Optional.of(monthFieldViewModel.getMergedFieldValue())); + } + + @Test + public void selectRightValueShouldBeCorrect() { + var monthFieldViewModel = createViewModelForField(StandardField.MONTH); + monthFieldViewModel.selectRightValue(); + assertEquals(FieldRowViewModel.Selection.RIGHT, monthFieldViewModel.getSelection()); + assertEquals(rightEntry.getField(StandardField.MONTH), Optional.of(monthFieldViewModel.getMergedFieldValue())); + + monthFieldViewModel.selectLeftValue(); + monthFieldViewModel.selectRightValue(); + assertEquals(FieldRowViewModel.Selection.RIGHT, monthFieldViewModel.getSelection()); + assertEquals(rightEntry.getField(StandardField.MONTH), Optional.of(monthFieldViewModel.getMergedFieldValue())); + } + + @Test + public void isFieldsMergedShouldReturnTrueIfLeftAndRightValuesAreEqual() { + var yearFieldViewModel = createViewModelForField(StandardField.YEAR); + assertTrue(yearFieldViewModel.isFieldsMerged()); + } + + @Test + public void isFieldsMergedShouldReturnFalseIfLeftAndRightValuesAreNotEqual() { + var monthFieldViewModel = createViewModelForField(StandardField.MONTH); + assertFalse(monthFieldViewModel.isFieldsMerged()); + + var addressFieldViewModel = createViewModelForField(StandardField.ADDRESS); + assertFalse(addressFieldViewModel.isFieldsMerged()); + + var authorFieldViewModel = createViewModelForField(StandardField.AUTHOR); + assertFalse(authorFieldViewModel.isFieldsMerged()); + } + + @Test + public void mergeFieldsShouldResultInLeftAndRightValuesBeingEqual() { + var groupsField = createViewModelForField(StandardField.GROUPS); + groupsField.mergeFields(); + assertEquals(groupsField.getLeftFieldValue(), groupsField.getRightFieldValue()); + } + + @Test + public void mergeFieldsShouldBeCorrectEvenWhenOnOfTheValuesIsEmpty() { + var keywordsField = createViewModelForField(StandardField.KEYWORDS); + keywordsField.mergeFields(); + assertEquals(keywordsField.getLeftFieldValue(), keywordsField.getRightFieldValue()); + } + + @Test + public void mergeFieldsShouldThrowUnsupportedOperationExceptionIfTheGivenFieldCanBeMerged() { + var authorField = createViewModelForField(StandardField.AUTHOR); + assertThrows(UnsupportedOperationException.class, authorField::mergeFields); + } + + @Test + public void mergeFieldsShouldSelectLeftFieldValue() { + var groupsField = createViewModelForField(StandardField.GROUPS); + groupsField.mergeFields(); + + assertEquals(FieldRowViewModel.Selection.LEFT, groupsField.getSelection()); + } + + @Test + public void unmergeFieldsShouldBeCorrect() { + var groupsField = createViewModelForField(StandardField.GROUPS); + var oldLeftGroups = groupsField.getLeftFieldValue(); + var oldRightGroups = groupsField.getRightFieldValue(); + groupsField.mergeFields(); + groupsField.unmergeFields(); + + assertEquals(oldLeftGroups, groupsField.getLeftFieldValue()); + assertEquals(oldRightGroups, groupsField.getRightFieldValue()); + } + + @Test + public void unmergeFieldsShouldDoNothingIfFieldsAreNotMerged() { + var groupsField = createViewModelForField(StandardField.GROUPS); + var oldLeftGroups = groupsField.getLeftFieldValue(); + var oldRightGroups = groupsField.getRightFieldValue(); + groupsField.unmergeFields(); + + assertEquals(oldLeftGroups, groupsField.getLeftFieldValue()); + assertEquals(oldRightGroups, groupsField.getRightFieldValue()); + } + + public FieldRowViewModel createViewModelForField(Field field) { + return new FieldRowViewModel(field, leftEntry, rightEntry, mergedEntry, fieldMergerFactory); + } +} diff --git a/src/test/java/org/jabref/gui/mergeentries/GroupMergerTest.java b/src/test/java/org/jabref/gui/mergeentries/GroupMergerTest.java new file mode 100644 index 00000000000..4f1c6824062 --- /dev/null +++ b/src/test/java/org/jabref/gui/mergeentries/GroupMergerTest.java @@ -0,0 +1,41 @@ +package org.jabref.gui.mergeentries; + +import java.util.stream.Stream; + +import org.jabref.gui.mergeentries.newmergedialog.fieldsmerger.GroupMerger; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +public class GroupMergerTest { + + GroupMerger groupMerger; + + @BeforeEach + void setup() { + this.groupMerger = new GroupMerger(); + } + + private static Stream mergeShouldMergeGroupsCorrectly() { + return Stream.of( + Arguments.of("a", "b", "a, b"), + Arguments.of("a", "", "a"), + Arguments.of("", "", ""), + Arguments.of("", "b", "b"), + Arguments.of("a, b", "c", "a, b, c"), + Arguments.of("a, b, c", "c", "a, b, c"), + Arguments.of("a, b", "c, d", "a, b, c, d"), + Arguments.of("a, b, c", "b, z", "a, b, c, z") + ); + } + + @ParameterizedTest + @MethodSource + public void mergeShouldMergeGroupsCorrectly(String groupsA, String groupsB, String expected) { + assertEquals(expected, groupMerger.merge(groupsA, groupsB)); + } +} diff --git a/src/test/java/org/jabref/gui/mergeentries/ThreeWayMergeCellViewModelTest.java b/src/test/java/org/jabref/gui/mergeentries/ThreeWayMergeCellViewModelTest.java new file mode 100644 index 00000000000..929e0e56db0 --- /dev/null +++ b/src/test/java/org/jabref/gui/mergeentries/ThreeWayMergeCellViewModelTest.java @@ -0,0 +1,95 @@ +package org.jabref.gui.mergeentries; + +import java.util.stream.Stream; + +import org.jabref.gui.mergeentries.newmergedialog.cell.ThreeWayMergeCellViewModel; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +public class ThreeWayMergeCellViewModelTest { + + ThreeWayMergeCellViewModel viewModel; + + @BeforeEach + void setup() { + viewModel = new ThreeWayMergeCellViewModel("Hello", 0); + } + + private static Stream testOddEvenLogic() { + return Stream.of( + Arguments.of(true, false, true, false), + Arguments.of(false, false, true, false), + Arguments.of(true, true, false, true), + Arguments.of(false, true, false, true) + ); + } + + private static Stream isEvenShouldReturnTrueIfRowIndexIsEven() { + return Stream.of( + Arguments.of(0, true), + Arguments.of(100, true), + Arguments.of(1, false), + Arguments.of(2, true), + Arguments.of(9999, false), + Arguments.of(Integer.MAX_VALUE, false) + ); + } + + private static Stream isOddShouldReturnTrueIfRowIndexIsOdd() { + return Stream.of( + Arguments.of(1, true), + Arguments.of(101, true), + Arguments.of(3, true), + Arguments.of(7777, true), + Arguments.of(9999, true), + Arguments.of(Integer.MAX_VALUE, true), + Arguments.of(0, false) + ); + } + + private static Stream getTextAndSetTextShouldBeConsistent() { + return Stream.of( + Arguments.of(""), + Arguments.of("Hello"), + Arguments.of("World"), + Arguments.of("" + null), + Arguments.of("Hello, World"), + Arguments.of("عربي") + ); + } + + @ParameterizedTest + @MethodSource + void testOddEvenLogic(boolean setIsOdd, boolean setIsEven, boolean expectedIsOdd, boolean expectedIsEven) { + viewModel.setOdd(setIsOdd); + viewModel.setEven(setIsEven); + assertEquals(expectedIsOdd, viewModel.isOdd()); + assertEquals(expectedIsEven, viewModel.isEven()); + } + + @ParameterizedTest + @MethodSource + void isEvenShouldReturnTrueIfRowIndexIsEven(int rowIndex, boolean expected) { + ThreeWayMergeCellViewModel newViewModel = new ThreeWayMergeCellViewModel("", rowIndex); + assertEquals(expected, newViewModel.isEven()); + } + + @ParameterizedTest + @MethodSource + void isOddShouldReturnTrueIfRowIndexIsOdd(int rowIndex, boolean expected) { + ThreeWayMergeCellViewModel newViewModel = new ThreeWayMergeCellViewModel("", rowIndex); + assertEquals(expected, newViewModel.isOdd()); + } + + @ParameterizedTest + @MethodSource + void getTextAndSetTextShouldBeConsistent(String setText) { + viewModel.setText(setText); + assertEquals(viewModel.getText(), setText); + } +} diff --git a/src/test/java/org/jabref/gui/mergeentries/ThreeWayMergeViewModelTest.java b/src/test/java/org/jabref/gui/mergeentries/ThreeWayMergeViewModelTest.java new file mode 100644 index 00000000000..fed09e4e0dc --- /dev/null +++ b/src/test/java/org/jabref/gui/mergeentries/ThreeWayMergeViewModelTest.java @@ -0,0 +1,76 @@ +package org.jabref.gui.mergeentries; + +import java.util.HashSet; +import java.util.List; + +import org.jabref.gui.mergeentries.newmergedialog.ThreeWayMergeViewModel; +import org.jabref.model.entry.BibEntry; +import org.jabref.model.entry.field.Field; +import org.jabref.model.entry.field.FieldFactory; +import org.jabref.model.entry.field.InternalField; +import org.jabref.model.entry.field.StandardField; +import org.jabref.model.entry.field.UnknownField; +import org.jabref.model.entry.types.StandardEntryType; + +import com.google.common.collect.Comparators; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +public class ThreeWayMergeViewModelTest { + + ThreeWayMergeViewModel viewModel; + BibEntry leftEntry; + BibEntry rightEntry; + List visibleFields; + + @BeforeEach + void setup() { + leftEntry = new BibEntry(StandardEntryType.Article) + .withField(StandardField.AUTHOR, "Erik G. Larsson and Oscar Gustafsson") + .withField(StandardField.TITLE, "The Impact of Dynamic Voltage and Frequency Scaling on Multicore {DSP} Algorithm Design [Exploratory {DSP]}") + .withField(StandardField.NUMBER, "1") + .withField(new UnknownField("custom"), "1.2.3"); + + rightEntry = new BibEntry(StandardEntryType.InProceedings) + .withField(StandardField.AUTHOR, "Henrik Ohlsson and Oscar Gustafsson and Lars Wanhammar") + .withField(StandardField.TITLE, "Arithmetic transformations for increased maximal sample rate of bit-parallel bireciprocal lattice wave digital filters") + .withField(StandardField.BOOKTITLE, "Proceedings of the 2001 International Symposium on Circuits and Systems, {ISCAS} 2001, Sydney, Australia, May 6-9, 2001") + .withField(StandardField.NUMBER, "2"); + viewModel = new ThreeWayMergeViewModel(leftEntry, rightEntry, "left", "right"); + visibleFields = viewModel.getVisibleFields(); + } + + @Test + void getVisibleFieldsShouldReturnASortedListOfFieldsWithEntryTypeAtTheHeadOfTheList() { + List names = visibleFields.stream().map(Field::getName).skip(1).toList(); + Comparators.isInOrder(names, String::compareTo); + } + + @Test + void getVisibleFieldsShouldNotHaveDuplicates() { + assertEquals(new HashSet<>(visibleFields).size(), viewModel.numberOfVisibleFields()); + } + + @Test + void getVisibleFieldsShouldHaveEntryTypeFieldAtTheHeadOfTheList() { + assertEquals(InternalField.TYPE_HEADER, visibleFields.get(0)); + } + + @Test + void getVisibleFieldsShouldContainAllNonInternalFieldsInRightAndLeftEntry() { + assertTrue(visibleFields.containsAll(leftEntry.getFields().stream().filter(this::isNotInternalField).toList())); + assertTrue(visibleFields.containsAll(rightEntry.getFields().stream().filter(this::isNotInternalField).toList())); + } + + @Test + void getVisibleFieldsShouldIncludeCustomFields() { + assertTrue(visibleFields.contains(new UnknownField("custom"))); + } + + private boolean isNotInternalField(Field field) { + return !FieldFactory.isInternalField(field); + } +} From 21fd2e2882d4ec1cda229a73240d1b203975f484 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 12 Sep 2022 16:18:13 +0200 Subject: [PATCH 2/4] Bump mockito-core from 4.7.0 to 4.8.0 (#9151) Bumps [mockito-core](https://github.com/mockito/mockito) from 4.7.0 to 4.8.0. - [Release notes](https://github.com/mockito/mockito/releases) - [Commits](https://github.com/mockito/mockito/compare/v4.7.0...v4.8.0) --- updated-dependencies: - dependency-name: org.mockito:mockito-core dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build.gradle b/build.gradle index 20b2f3f921c..1af0ffb4ce8 100644 --- a/build.gradle +++ b/build.gradle @@ -207,7 +207,7 @@ dependencies { testImplementation 'org.junit.jupiter:junit-jupiter:5.9.0' testImplementation 'org.junit.platform:junit-platform-launcher:1.9.0' - testImplementation 'org.mockito:mockito-core:4.7.0' + testImplementation 'org.mockito:mockito-core:4.8.0' testImplementation 'org.xmlunit:xmlunit-core:2.9.0' testImplementation 'org.xmlunit:xmlunit-matchers:2.9.0' testRuntimeOnly 'com.tngtech.archunit:archunit-junit5-engine:0.23.1' From 6b4ecfeeb88486e810d177791107c1c55d91193a Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 12 Sep 2022 16:37:17 +0200 Subject: [PATCH 3/4] Bump org.eclipse.jgit from 6.2.0.202206071550-r to 6.3.0.202209071007-r (#9152) Bumps org.eclipse.jgit from 6.2.0.202206071550-r to 6.3.0.202209071007-r. --- updated-dependencies: - dependency-name: org.eclipse.jgit:org.eclipse.jgit dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- build.gradle | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build.gradle b/build.gradle index 1af0ffb4ce8..752bfe0dfb8 100644 --- a/build.gradle +++ b/build.gradle @@ -141,7 +141,7 @@ dependencies { antlr4 'org.antlr:antlr4:4.9.3' implementation 'org.antlr:antlr4-runtime:4.9.3' - implementation group: 'org.eclipse.jgit', name: 'org.eclipse.jgit', version: '6.2.0.202206071550-r' + implementation group: 'org.eclipse.jgit', name: 'org.eclipse.jgit', version: '6.3.0.202209071007-r' implementation group: 'com.fasterxml.jackson.dataformat', name: 'jackson-dataformat-yaml', version: '2.13.4' implementation group: 'com.fasterxml.jackson.datatype', name: 'jackson-datatype-jsr310', version: '2.13.4' From 9604f7d078cc5c77cdd2717cc896f506d57e713c Mon Sep 17 00:00:00 2001 From: Houssem Nasri Date: Mon, 12 Sep 2022 15:38:01 +0100 Subject: [PATCH 4/4] Fix missing CSS for some dialogs (#9150) * Load css for the 'Merge PDF metadata' dialog * Load css for the 'Search results from open libraries' dialog * Update CHANGELOG.md * Update CHANGELOG.md --- CHANGELOG.md | 1 + src/main/java/org/jabref/gui/JabRefFrame.java | 2 +- .../org/jabref/gui/fieldeditors/LinkedFileViewModel.java | 3 ++- src/main/java/org/jabref/gui/search/GlobalSearchBar.java | 7 +++++-- src/main/resources/l10n/JabRef_en.properties | 1 + 5 files changed, 10 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fb6b2aa76dd..251b6ec7e30 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -50,6 +50,7 @@ Note that this project **does not** adhere to [Semantic Versioning](http://semve - We fixed an issue where title case didn't capitalize words after en-dash characters and skip capitalization of conjunctions that comes after en-dash characters. [#9068](https://github.com/JabRef/jabref/pull/9068),[#9142](https://github.com/JabRef/jabref/pull/9142) - We fixed an issue where JabRef would not exit when a connection to a LibreOffice document was established previously and the document is still open. [#9075](https://github.com/JabRef/jabref/issues/9075) - We fixed an issue about selecting the save order in the preferences. [#9175](https://github.com/JabRef/jabref/issues/9147) +- We fixed an issue where the CSS styles are missing in some dialogs. [#9150](https://github.com/JabRef/jabref/pull/9150) ### Removed diff --git a/src/main/java/org/jabref/gui/JabRefFrame.java b/src/main/java/org/jabref/gui/JabRefFrame.java index b931773824d..dd2bf48db16 100644 --- a/src/main/java/org/jabref/gui/JabRefFrame.java +++ b/src/main/java/org/jabref/gui/JabRefFrame.java @@ -190,7 +190,7 @@ public JabRefFrame(Stage mainStage) { this.themeManager = Globals.getThemeManager(); this.dialogService = new JabRefDialogService(mainStage, this, themeManager); this.undoManager = Globals.undoManager; - this.globalSearchBar = new GlobalSearchBar(this, stateManager, prefs, undoManager); + this.globalSearchBar = new GlobalSearchBar(this, stateManager, prefs, undoManager, dialogService); this.pushToApplicationCommand = new PushToApplicationCommand(stateManager, dialogService, prefs); this.fileHistory = new FileHistoryMenu(prefs, dialogService, getOpenDatabaseAction()); this.taskExecutor = Globals.TASK_EXECUTOR; diff --git a/src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java b/src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java index 65d5c505f75..1b0c9f6da8f 100644 --- a/src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java +++ b/src/main/java/org/jabref/gui/fieldeditors/LinkedFileViewModel.java @@ -557,6 +557,7 @@ public ValidationStatus fileExistsValidationStatus() { public void parsePdfMetadataAndShowMergeDialog() { linkedFile.findIn(databaseContext, preferences.getFilePreferences()).ifPresent(filePath -> { MultiMergeEntriesView dialog = new MultiMergeEntriesView(preferences, taskExecutor); + dialog.setTitle(Localization.lang("Merge PDF metadata")); dialog.addSource(Localization.lang("Entry"), entry); dialog.addSource(Localization.lang("Verbatim"), wrapImporterToSupplier(new PdfVerbatimBibTextImporter(preferences.getImportFormatPreferences()), filePath)); dialog.addSource(Localization.lang("Embedded"), wrapImporterToSupplier(new PdfEmbeddedBibFileImporter(preferences.getImportFormatPreferences()), filePath)); @@ -565,7 +566,7 @@ public void parsePdfMetadataAndShowMergeDialog() { } dialog.addSource(Localization.lang("XMP metadata"), wrapImporterToSupplier(new PdfXmpImporter(preferences.getXmpPreferences()), filePath)); dialog.addSource(Localization.lang("Content"), wrapImporterToSupplier(new PdfContentImporter(preferences.getImportFormatPreferences()), filePath)); - dialog.showAndWait().ifPresent(newEntry -> { + dialogService.showCustomDialogAndWait(dialog).ifPresent(newEntry -> { databaseContext.getDatabase().removeEntry(entry); databaseContext.getDatabase().insertEntry(newEntry); }); diff --git a/src/main/java/org/jabref/gui/search/GlobalSearchBar.java b/src/main/java/org/jabref/gui/search/GlobalSearchBar.java index d42bcd34cb7..d37c5b2657f 100644 --- a/src/main/java/org/jabref/gui/search/GlobalSearchBar.java +++ b/src/main/java/org/jabref/gui/search/GlobalSearchBar.java @@ -39,6 +39,7 @@ import javafx.scene.text.TextFlow; import org.jabref.gui.ClipBoardManager; +import org.jabref.gui.DialogService; import org.jabref.gui.Globals; import org.jabref.gui.JabRefFrame; import org.jabref.gui.StateManager; @@ -101,16 +102,18 @@ public class GlobalSearchBar extends HBox { private final UndoManager undoManager; private final SearchPreferences searchPreferences; + private final DialogService dialogService; private final BooleanProperty globalSearchActive = new SimpleBooleanProperty(false); private GlobalSearchResultDialog globalSearchResultDialog; - public GlobalSearchBar(JabRefFrame frame, StateManager stateManager, PreferencesService preferencesService, CountingUndoManager undoManager) { + public GlobalSearchBar(JabRefFrame frame, StateManager stateManager, PreferencesService preferencesService, CountingUndoManager undoManager, DialogService dialogService) { super(); this.stateManager = stateManager; this.preferencesService = preferencesService; this.searchPreferences = preferencesService.getSearchPreferences(); this.undoManager = undoManager; + this.dialogService = dialogService; searchField.disableProperty().bind(needsDatabase(stateManager).not()); @@ -240,7 +243,7 @@ private void initSearchModifierButtons() { globalSearchActive.setValue(true); globalSearchResultDialog = new GlobalSearchResultDialog(undoManager); performSearch(); - globalSearchResultDialog.showAndWait(); + dialogService.showCustomDialogAndWait(globalSearchResultDialog); globalSearchActive.setValue(false); }); } diff --git a/src/main/resources/l10n/JabRef_en.properties b/src/main/resources/l10n/JabRef_en.properties index c9e469a00f8..12093e83f81 100644 --- a/src/main/resources/l10n/JabRef_en.properties +++ b/src/main/resources/l10n/JabRef_en.properties @@ -2448,6 +2448,7 @@ Symmetric\ word\ by\ word=Symmetric word by word Verbatim=Verbatim Word\ by\ word=Word by word Could\ not\ extract\ Metadata\ from\:\ %0=Could not extract Metadata from: %0 +Merge\ PDF\ metadata=Merge PDF metadata Add\ certificate=Add certificate Serial\ number=Serial number