Check group type before showing dialog in edit group

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<BibDatabaseContext> 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<AbstractGroup> newGroup = dialogService.showCustomDialogAndWait(new GroupDialogView(
@@ -182,61 +185,163 @@ 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 <br>
+     * 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<AbstractGroup> 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<GroupTreeNode> 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);
                 ButtonType cancel = new ButtonType(Localization.lang("Cancel"), ButtonBar.ButtonData.CANCEL_CLOSE);
 
                 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<ButtonType> 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<GroupTreeNode> databaseRootGroup = currentDatabase.get().getMetaData().getGroups();
                 if (databaseRootGroup.isPresent()) {
                     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();
             });

src/main/java/org/jabref/model/groups/GroupTreeNode.java

@@ -72,17 +72,17 @@ public List<FieldChange> setGroup(AbstractGroup newGroup, boolean shouldKeepPrev
         group = Objects.requireNonNull(newGroup);
 
         List<FieldChange> 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<BibEntry> 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));
             }

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));
+    }
 }