diff --git a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java index 670130757ad..57f17315ad9 100644 --- a/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java +++ b/src/main/java/org/jabref/gui/groups/GroupTreeViewModel.java @@ -30,8 +30,13 @@ import org.jabref.model.database.BibDatabaseContext; import org.jabref.model.entry.BibEntry; import org.jabref.model.groups.AbstractGroup; +import org.jabref.model.groups.AutomaticKeywordGroup; +import org.jabref.model.groups.AutomaticPersonsGroup; import org.jabref.model.groups.ExplicitGroup; import org.jabref.model.groups.GroupTreeNode; +import org.jabref.model.groups.RegexKeywordGroup; +import org.jabref.model.groups.SearchGroup; +import org.jabref.model.groups.TexGroup; import org.jabref.model.groups.WordKeywordGroup; import org.jabref.model.metadata.MetaData; import org.jabref.preferences.PreferencesService; @@ -145,14 +150,12 @@ private void onActiveDatabaseChanged(Optional newDatabase) { } else { rootGroup.setValue(null); } - currentDatabase = newDatabase; } /** * Opens "New Group Dialog" and add the resulting group to the specified group */ - public void addNewSubgroup(GroupNodeViewModel parent, GroupDialogHeader groupDialogHeader) { currentDatabase.ifPresent(database -> { Optional newGroup = dialogService.showCustomDialogAndWait(new GroupDialogView( @@ -182,20 +185,121 @@ private void writeGroupChangesToMetaData() { currentDatabase.ifPresent(database -> database.getMetaData().setGroups(rootGroup.get().getGroupNode())); } + private boolean isGroupTypeEqual(AbstractGroup oldGroup, AbstractGroup newGroup) { + return oldGroup.getClass().equals(newGroup.getClass()); + } + + /** + * Check if it is necessary to show a group modified, reassign entry dialog
+ * Group name change is handled separately + * + * @param oldGroup Original Group + * @param newGroup Edited group + * @return true if just trivial modifications (e.g. color or description) or the relevant group properties are equal, false otherwise + */ + boolean onlyMinorChanges(AbstractGroup oldGroup, AbstractGroup newGroup) { + // we need to use getclass here because we have different subclass inheritance e.g. ExplicitGroup is a subclass of WordKeyWordGroup + if (oldGroup.getClass() == WordKeywordGroup.class) { + WordKeywordGroup oldWordKeywordGroup = (WordKeywordGroup) oldGroup; + WordKeywordGroup newWordKeywordGroup = (WordKeywordGroup) newGroup; + + return Objects.equals(oldWordKeywordGroup.getSearchField().getName(), newWordKeywordGroup.getSearchField().getName()) + && Objects.equals(oldWordKeywordGroup.getSearchExpression(), newWordKeywordGroup.getSearchExpression()) + && Objects.equals(oldWordKeywordGroup.isCaseSensitive(), newWordKeywordGroup.isCaseSensitive()); + } else if (oldGroup.getClass() == RegexKeywordGroup.class) { + RegexKeywordGroup oldRegexKeywordGroup = (RegexKeywordGroup) oldGroup; + RegexKeywordGroup newRegexKeywordGroup = (RegexKeywordGroup) newGroup; + + return Objects.equals(oldRegexKeywordGroup.getSearchField().getName(), newRegexKeywordGroup.getSearchField().getName()) + && Objects.equals(oldRegexKeywordGroup.getSearchExpression(), newRegexKeywordGroup.getSearchExpression()) + && Objects.equals(oldRegexKeywordGroup.isCaseSensitive(), newRegexKeywordGroup.isCaseSensitive()); + } else if ((oldGroup.getClass() == SearchGroup.class)) { + SearchGroup oldSearchGroup = (SearchGroup) oldGroup; + SearchGroup newSearchGroup = (SearchGroup) newGroup; + + return Objects.equals(oldSearchGroup.getSearchExpression(), newSearchGroup.getSearchExpression()) + && Objects.equals(oldSearchGroup.getSearchFlags(), newSearchGroup.getSearchFlags()); + } else if (oldGroup.getClass() == AutomaticKeywordGroup.class) { + AutomaticKeywordGroup oldAutomaticKeywordGroup = (AutomaticKeywordGroup) oldGroup; + AutomaticKeywordGroup newAutomaticKeywordGroup = (AutomaticKeywordGroup) oldGroup; + + return Objects.equals(oldAutomaticKeywordGroup.getKeywordDelimiter(), newAutomaticKeywordGroup.getKeywordDelimiter()) + && Objects.equals(oldAutomaticKeywordGroup.getKeywordHierarchicalDelimiter(), newAutomaticKeywordGroup.getKeywordHierarchicalDelimiter()) + && Objects.equals(oldAutomaticKeywordGroup.getField().getName(), newAutomaticKeywordGroup.getField().getName()); + } else if (oldGroup.getClass() == AutomaticPersonsGroup.class) { + AutomaticPersonsGroup oldAutomaticPersonsGroup = (AutomaticPersonsGroup) oldGroup; + AutomaticPersonsGroup newAutomaticPersonsGroup = (AutomaticPersonsGroup) newGroup; + + return Objects.equals(oldAutomaticPersonsGroup.getField().getName(), newAutomaticPersonsGroup.getField().getName()); + } else if (oldGroup.getClass() == TexGroup.class) { + TexGroup oldTexGroup = (TexGroup) oldGroup; + TexGroup newTexGroup = (TexGroup) newGroup; + return Objects.equals(oldTexGroup.getFilePath().toString(), newTexGroup.getFilePath().toString()); + } + return true; + } + /** * Opens "Edit Group Dialog" and changes the given group to the edited one. */ public void editGroup(GroupNodeViewModel oldGroup) { currentDatabase.ifPresent(database -> { Optional newGroup = dialogService.showCustomDialogAndWait(new GroupDialogView( - dialogService, - database, - preferences, - oldGroup.getGroupNode().getGroup(), - GroupDialogHeader.SUBGROUP)); - + dialogService, + database, + preferences, + oldGroup.getGroupNode().getGroup(), + GroupDialogHeader.SUBGROUP)); newGroup.ifPresent(group -> { - // TODO: Keep assignments + + AbstractGroup oldGroupDef = oldGroup.getGroupNode().getGroup(); + String oldGroupName = oldGroupDef.getName(); + + boolean groupTypeEqual = isGroupTypeEqual(oldGroupDef, group); + boolean onlyMinorModifications = groupTypeEqual && onlyMinorChanges(oldGroupDef, group); + + // dialog already warns us about this if the new group is named like another existing group + // We need to check if only the name changed as this is relevant for the entry's group field + if (groupTypeEqual && !group.getName().equals(oldGroupName) && onlyMinorModifications) { + int groupsWithSameName = 0; + Optional databaseRootGroup = currentDatabase.get().getMetaData().getGroups(); + if (databaseRootGroup.isPresent()) { + // we need to check the old name for duplicates. If the new group name occurs more than once, it won't matter + groupsWithSameName = databaseRootGroup.get().findChildrenSatisfying(g -> g.getName().equals(oldGroupName)).size(); + } + boolean removePreviousAssignments = true; + // We found more than 2 groups, so we cannot simply remove old assignment + if (groupsWithSameName >= 2) { + removePreviousAssignments = false; + } + + oldGroup.getGroupNode().setGroup( + group, + true, + removePreviousAssignments, + database.getEntries()); + + dialogService.notify(Localization.lang("Modified group \"%0\".", group.getName())); + writeGroupChangesToMetaData(); + // This is ugly but we have no proper update mechanism in place to propagate the changes, so redraw everything + refresh(); + return; + } + + if (groupTypeEqual && onlyMinorChanges(oldGroup.getGroupNode().getGroup(), group)) { + oldGroup.getGroupNode().setGroup( + group, + true, + true, + database.getEntries()); + + writeGroupChangesToMetaData(); + refresh(); + return; + } + + // Major modifications + String content = Localization.lang("Assign the original group's entries to this group?"); ButtonType keepAssignments = new ButtonType(Localization.lang("Assign"), ButtonBar.ButtonData.YES); ButtonType removeAssignments = new ButtonType(Localization.lang("Do not assign"), ButtonBar.ButtonData.NO); @@ -203,17 +307,16 @@ public void editGroup(GroupNodeViewModel oldGroup) { if (newGroup.get().getClass() == WordKeywordGroup.class) { content = content + "\n\n" + - Localization.lang("(Note: If original entries lack keywords to qualify for the new group configuration, confirming here will add them)"); + Localization.lang("(Note: If original entries lack keywords to qualify for the new group configuration, confirming here will add them)"); } Optional previousAssignments = dialogService.showCustomButtonDialogAndWait(Alert.AlertType.WARNING, - Localization.lang("Change of Grouping Method"), - content, - keepAssignments, - removeAssignments, - cancel); - // WarnAssignmentSideEffects.warnAssignmentSideEffects(newGroup, panel.frame()); + Localization.lang("Change of Grouping Method"), + content, + keepAssignments, + removeAssignments, + cancel); boolean removePreviousAssignments = (oldGroup.getGroupNode().getGroup() instanceof ExplicitGroup) - && (group instanceof ExplicitGroup); + && (group instanceof ExplicitGroup); int groupsWithSameName = 0; Optional databaseRootGroup = currentDatabase.get().getMetaData().getGroups(); @@ -221,22 +324,24 @@ public void editGroup(GroupNodeViewModel oldGroup) { String name = oldGroup.getGroupNode().getGroup().getName(); groupsWithSameName = databaseRootGroup.get().findChildrenSatisfying(g -> g.getName().equals(name)).size(); } + // okay we found more than 2 groups with the same name + // If we only found one we can still do it if (groupsWithSameName >= 2) { removePreviousAssignments = false; } if (previousAssignments.isPresent() && (previousAssignments.get().getButtonData() == ButtonBar.ButtonData.YES)) { oldGroup.getGroupNode().setGroup( - group, - true, - removePreviousAssignments, - database.getEntries()); + group, + true, + removePreviousAssignments, + database.getEntries()); } else if (previousAssignments.isPresent() && (previousAssignments.get().getButtonData() == ButtonBar.ButtonData.NO)) { oldGroup.getGroupNode().setGroup( - group, - false, - removePreviousAssignments, - database.getEntries()); + group, + false, + removePreviousAssignments, + database.getEntries()); } else if (previousAssignments.isPresent() && (previousAssignments.get().getButtonData() == ButtonBar.ButtonData.CANCEL_CLOSE)) { return; } @@ -259,10 +364,8 @@ public void editGroup(GroupNodeViewModel oldGroup) { // if (!addChange.isEmpty()) { // undoAddPreviousEntries = UndoableChangeEntriesOfGroup.getUndoableEdit(null, addChange); // } - dialogService.notify(Localization.lang("Modified group \"%0\".", group.getName())); writeGroupChangesToMetaData(); - // This is ugly but we have no proper update mechanism in place to propagate the changes, so redraw everything refresh(); }); diff --git a/src/main/java/org/jabref/model/groups/GroupTreeNode.java b/src/main/java/org/jabref/model/groups/GroupTreeNode.java index 6cb8b15d722..efe4d233fee 100644 --- a/src/main/java/org/jabref/model/groups/GroupTreeNode.java +++ b/src/main/java/org/jabref/model/groups/GroupTreeNode.java @@ -72,17 +72,17 @@ public List setGroup(AbstractGroup newGroup, boolean shouldKeepPrev group = Objects.requireNonNull(newGroup); List changes = new ArrayList<>(); - boolean shouldRemove = shouldRemovePreviousAssignments && (oldGroup instanceof GroupEntryChanger); - boolean shouldAdd = shouldKeepPreviousAssignments && (newGroup instanceof GroupEntryChanger); - if (shouldAdd || shouldRemove) { + boolean shouldRemoveFromOldGroup = shouldRemovePreviousAssignments && (oldGroup instanceof GroupEntryChanger); + boolean shouldAddToNewGroup = shouldKeepPreviousAssignments && (newGroup instanceof GroupEntryChanger); + if (shouldAddToNewGroup || shouldRemoveFromOldGroup) { List entriesMatchedByOldGroup = entriesInDatabase.stream().filter(oldGroup::isMatch) .collect(Collectors.toList()); - if (shouldRemove) { + if (shouldRemoveFromOldGroup) { GroupEntryChanger entryChanger = (GroupEntryChanger) oldGroup; changes.addAll(entryChanger.remove(entriesMatchedByOldGroup)); } - if (shouldAdd) { + if (shouldAddToNewGroup) { GroupEntryChanger entryChanger = (GroupEntryChanger) newGroup; changes.addAll(entryChanger.add(entriesMatchedByOldGroup)); } diff --git a/src/test/java/org/jabref/gui/groups/GroupTreeViewModelTest.java b/src/test/java/org/jabref/gui/groups/GroupTreeViewModelTest.java index c96d5262396..96ba2684e5d 100644 --- a/src/test/java/org/jabref/gui/groups/GroupTreeViewModelTest.java +++ b/src/test/java/org/jabref/gui/groups/GroupTreeViewModelTest.java @@ -12,6 +12,7 @@ import org.jabref.model.database.BibDatabaseContext; import org.jabref.model.entry.BibEntry; import org.jabref.model.entry.field.StandardField; +import org.jabref.model.groups.AbstractGroup; import org.jabref.model.groups.AllEntriesGroup; import org.jabref.model.groups.ExplicitGroup; import org.jabref.model.groups.GroupHierarchyType; @@ -20,17 +21,22 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.mockito.Answers; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; class GroupTreeViewModelTest { + private StateManager stateManager; private GroupTreeViewModel groupTree; private BibDatabaseContext databaseContext; private TaskExecutor taskExecutor; private PreferencesService preferencesService; + private DialogService dialogService; @BeforeEach void setUp() { @@ -39,12 +45,13 @@ void setUp() { stateManager.activeDatabaseProperty().setValue(Optional.of(databaseContext)); taskExecutor = new CurrentThreadTaskExecutor(); preferencesService = mock(PreferencesService.class); + dialogService = mock(DialogService.class, Answers.RETURNS_DEEP_STUBS); + when(preferencesService.getGroupsPreferences()).thenReturn(new GroupsPreferences( - GroupViewMode.UNION, - true, - true, - new SimpleObjectProperty<>(',') - )); + GroupViewMode.UNION, + true, + true, + new SimpleObjectProperty<>(','))); groupTree = new GroupTreeViewModel(stateManager, mock(DialogService.class), preferencesService, taskExecutor, new CustomLocalDragboard()); } @@ -85,4 +92,51 @@ void keywordGroupsAreNotRemovedFromEntriesOnDelete() { assertEquals(groupName, entry.getField(StandardField.KEYWORDS).get()); } + + @Test + void shouldNotShowDialogWhenGroupNameChanges() { + AbstractGroup oldGroup = new ExplicitGroup("group", GroupHierarchyType.INDEPENDENT, ','); + AbstractGroup newGroup = new ExplicitGroup("newGroupName", GroupHierarchyType.INDEPENDENT, ','); + BibEntry entry = new BibEntry(); + databaseContext.getDatabase().insertEntry(entry); + + GroupTreeViewModel model = new GroupTreeViewModel(stateManager, dialogService, preferencesService, taskExecutor, new CustomLocalDragboard()); + assertTrue(model.onlyMinorChanges(oldGroup, newGroup)); + } + + @Test + void shouldNotShowDialogWhenGroupsAreEqual() { + AbstractGroup oldGroup = new WordKeywordGroup("group", GroupHierarchyType.INCLUDING, StandardField.KEYWORDS, "keywordTest", true, ',', true); + AbstractGroup newGroup = new WordKeywordGroup("group", GroupHierarchyType.INCLUDING, StandardField.KEYWORDS, "keywordTest", true, ',', true); + + BibEntry entry = new BibEntry(); + databaseContext.getDatabase().insertEntry(entry); + + GroupTreeViewModel model = new GroupTreeViewModel(stateManager, dialogService, preferencesService, taskExecutor, new CustomLocalDragboard()); + assertTrue(model.onlyMinorChanges(oldGroup, newGroup)); + } + + @Test + void shouldShowDialogWhenKeywordDiffers() { + AbstractGroup oldGroup = new WordKeywordGroup("group", GroupHierarchyType.INCLUDING, StandardField.KEYWORDS, "keywordTest", true, ',', true); + AbstractGroup newGroup = new WordKeywordGroup("group", GroupHierarchyType.INCLUDING, StandardField.KEYWORDS, "keywordChanged", true, ',', true); + + BibEntry entry = new BibEntry(); + databaseContext.getDatabase().insertEntry(entry); + + GroupTreeViewModel model = new GroupTreeViewModel(stateManager, dialogService, preferencesService, taskExecutor, new CustomLocalDragboard()); + assertFalse(model.onlyMinorChanges(oldGroup, newGroup)); + } + + @Test + void shouldShowDialogWhenCaseSensitivyDiffers() { + AbstractGroup oldGroup = new WordKeywordGroup("group", GroupHierarchyType.INCLUDING, StandardField.KEYWORDS, "keywordTest", false, ',', true); + AbstractGroup newGroup = new WordKeywordGroup("group", GroupHierarchyType.INCLUDING, StandardField.KEYWORDS, "keywordChanged", true, ',', true); + + BibEntry entry = new BibEntry(); + databaseContext.getDatabase().insertEntry(entry); + + GroupTreeViewModel model = new GroupTreeViewModel(stateManager, dialogService, preferencesService, taskExecutor, new CustomLocalDragboard()); + assertFalse(model.onlyMinorChanges(oldGroup, newGroup)); + } }