Fix #1500: Renaming of explicit groups changes entries accordingly

CHANGELOG.md

@@ -34,6 +34,7 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `#
 - Fixed [#1507](https://github.com/JabRef/jabref/issues/1507): Keywords are now separated by the delimiter specified in the preferences
 - Fixed [#1484](https://github.com/JabRef/jabref/issues/1484): HTML export handles some UTF characters wrong
 - Fixed [#1534](https://github.com/JabRef/jabref/issues/1534): "Mark entries imported into database" does not work correctly
+- Fixed [#1500](https://github.com/JabRef/jabref/issues/1500): Renaming of explicit groups now changes entries accordingly
 - Fixed issue where field changes were not undoable if the time stamp was updated on editing
 
 ### Removed

src/main/java/net/sf/jabref/gui/groups/AutoGroupDialog.java

@@ -93,7 +93,7 @@ public void actionPerformed(ActionEvent e) {
                 dispose();
 
                 try {
-                    GroupTreeNode autoGroupsRoot = new GroupTreeNode(
+                    GroupTreeNode autoGroupsRoot = GroupTreeNode.fromGroup(
                             new ExplicitGroup(Localization.lang("Automatically created groups"), GroupHierarchyType.INCLUDING));
                     Set<String> hs;
                     String fieldText = field.getText();
@@ -121,7 +121,7 @@ public void actionPerformed(ActionEvent e) {
                     for (String keyword : hs) {
                         KeywordGroup group = new KeywordGroup(keyword, fieldText, keyword, false, false,
                                 GroupHierarchyType.INDEPENDENT);
-                        autoGroupsRoot.addChild(new GroupTreeNode(group));
+                        autoGroupsRoot.addChild(GroupTreeNode.fromGroup(group));
                     }
 
                     autoGroupsRoot.moveTo(m_groupsRoot.getNode());

src/main/java/net/sf/jabref/gui/groups/GroupSelector.java

@@ -72,6 +72,7 @@
 import net.sf.jabref.gui.worker.AbstractWorker;
 import net.sf.jabref.logic.groups.AbstractGroup;
 import net.sf.jabref.logic.groups.AllEntriesGroup;
+import net.sf.jabref.logic.groups.EntriesGroupChange;
 import net.sf.jabref.logic.groups.GroupTreeNode;
 import net.sf.jabref.logic.groups.MoveGroupChange;
 import net.sf.jabref.logic.l10n.Localization;
@@ -353,7 +354,7 @@ public void stateChanged(ChangeEvent e) {
                 KeyStroke.getKeyStroke(KeyEvent.VK_RIGHT, KeyEvent.CTRL_MASK));
 
 
-        setGroups(new GroupTreeNode(new AllEntriesGroup()));
+        setGroups(GroupTreeNode.fromGroup(new AllEntriesGroup()));
     }
 
     private void definePopup() {
@@ -787,10 +788,25 @@ public void actionPerformed(ActionEvent e) {
             gd.setVisible(true);
             if (gd.okPressed()) {
                 AbstractGroup newGroup = gd.getResultingGroup();
-                AbstractUndoableEdit undoAddPreviousEntries = gd.getUndoForAddPreviousEntries();
+
+                int i = JOptionPane.showConfirmDialog(panel.frame(),
+                        Localization.lang("Assign the original group's entries to this group?"),
+                        Localization.lang("Change of Grouping Method"),
+                        JOptionPane.YES_NO_OPTION, JOptionPane.QUESTION_MESSAGE);
+                boolean keepPreviousAssignments = i == JOptionPane.YES_OPTION &&
+                        WarnAssignmentSideEffects.warnAssignmentSideEffects(newGroup, panel.frame());
+
+                AbstractUndoableEdit undoAddPreviousEntries = null;
                 UndoableModifyGroup undo = new UndoableModifyGroup(GroupSelector.this, groupsRoot, node, newGroup);
-                node.getNode().setGroup(newGroup);
+                Optional<EntriesGroupChange> addChange = node.getNode().setGroup(newGroup, keepPreviousAssignments,
+                        panel.getDatabase().getEntries());
+                if (addChange.isPresent()) {
+                    undoAddPreviousEntries = UndoableChangeEntriesOfGroup.getUndoableEdit(null, addChange.get());
+                }
+
+                groupsTreeModel.reload();
                 revalidateGroups(node);
+
                 // Store undo information.
                 if (undoAddPreviousEntries == null) {
                     panel.getUndoManager().addEdit(undo);
@@ -821,7 +837,7 @@ public void actionPerformed(ActionEvent e) {
                 return; // ignore
             }
             final AbstractGroup newGroup = gd.getResultingGroup();
-            final GroupTreeNode newNode = new GroupTreeNode(newGroup);
+            final GroupTreeNode newNode = GroupTreeNode.fromGroup(newGroup);
             final GroupTreeNodeViewModel node = getNodeToUse();
             if (node == null) {
                 groupsRoot.getNode().addChild(newNode);
@@ -852,7 +868,7 @@ public void actionPerformed(ActionEvent e) {
                 return; // ignore
             }
             final AbstractGroup newGroup = gd.getResultingGroup();
-            final GroupTreeNode newNode = new GroupTreeNode(newGroup);
+            final GroupTreeNode newNode = GroupTreeNode.fromGroup(newGroup);
             final GroupTreeNodeViewModel node = getNodeToUse();
             node.getNode().addChild(newNode);
             UndoableAddOrRemoveGroup undo = new UndoableAddOrRemoveGroup(groupsRoot,
@@ -1202,7 +1218,7 @@ public void setActiveBasePanel(BasePanel panel) {
         }
         MetaData metaData = panel.getBibDatabaseContext().getMetaData();
         if (metaData.getGroups() == null) {
-            GroupTreeNode newGroupsRoot = new GroupTreeNode(new AllEntriesGroup());
+            GroupTreeNode newGroupsRoot = GroupTreeNode.fromGroup(new AllEntriesGroup());
             metaData.setGroups(newGroupsRoot);
             setGroups(newGroupsRoot);
         } else {

src/main/java/net/sf/jabref/gui/groups/GroupTreeNodeViewModel.java

@@ -333,7 +333,7 @@ public boolean isAllEntriesGroup() {
     }
 
     public void addNewGroup(AbstractGroup newGroup, CountingUndoManager undoManager) {
-        GroupTreeNode newNode = new GroupTreeNode(newGroup);
+        GroupTreeNode newNode = GroupTreeNode.fromGroup(newGroup);
         this.getNode().addChild(newNode);
 
         UndoableAddOrRemoveGroup undo = new UndoableAddOrRemoveGroup(this,

src/main/java/net/sf/jabref/gui/groups/UndoableAddOrRemoveGroup.java

@@ -71,7 +71,7 @@ public UndoableAddOrRemoveGroup(GroupTreeNodeViewModel groupsRoot,
         // storing a backup of the whole subtree is not required when children
         // are kept
         m_subtreeBackup = editType != UndoableAddOrRemoveGroup.REMOVE_NODE_KEEP_CHILDREN ? editedNode.getNode()
-                .copySubtree() : new GroupTreeNode(editedNode.getNode().getGroup().deepCopy());
+                .copySubtree() : GroupTreeNode.fromGroup(editedNode.getNode().getGroup().deepCopy());
         // remember path to edited node. this cannot be stored as a reference,
         // because the reference itself might change. the method below is more
         // robust.

src/main/java/net/sf/jabref/logic/groups/GroupTreeNode.java

@@ -18,6 +18,8 @@
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Objects;
+import java.util.Optional;
+import java.util.stream.Collectors;
 
 import net.sf.jabref.importer.fileformat.ParseException;
 import net.sf.jabref.logic.search.SearchMatcher;
@@ -38,11 +40,15 @@ public class GroupTreeNode extends TreeNode<GroupTreeNode> {
      *
      * @param group the group underlying this node
      */
-    public GroupTreeNode(AbstractGroup group) {
+    private GroupTreeNode(AbstractGroup group) {
         super(GroupTreeNode.class);
         setGroup(group);
     }
 
+    public static GroupTreeNode fromGroup(AbstractGroup group) {
+        return new GroupTreeNode(group);
+    }
+
     /**
      * Returns the group underlying this node.
      *
@@ -55,10 +61,37 @@ public AbstractGroup getGroup() {
     /**
      * Associates the specified group with this node.
      *
-     * @param group the new group (has to be non-null)
+     * @param newGroup the new group (has to be non-null)
      */
-    public void setGroup(AbstractGroup group) {
-        this.group = Objects.requireNonNull(group);
+    @Deprecated // use other overload
+    public void setGroup(AbstractGroup newGroup) {
+        this.group = Objects.requireNonNull(newGroup);
+    }
+
+    /**
+     * Associates the specified group with this node while also providing the possibility to modify previous matched
+     * entries so that they are now matched by the new group.
+     *
+     * @param newGroup the new group (has to be non-null)
+     * @param shouldKeepPreviousAssignments specifies whether previous matched entries should be carried over
+     * @param entriesInDatabase list of entries in the database
+     */
+    public Optional<EntriesGroupChange> setGroup(AbstractGroup newGroup, boolean shouldKeepPreviousAssignments,
+            List<BibEntry> entriesInDatabase) {
+        AbstractGroup oldGroup = getGroup();
+        setGroup(newGroup);
+
+        // Keep assignments from previous group
+        if (shouldKeepPreviousAssignments && newGroup.supportsAdd()) {
+            List<BibEntry> entriesMatchedByOldGroup = entriesInDatabase.stream().filter(oldGroup::isMatch)
+                    .collect(Collectors.toList());
+            if (oldGroup instanceof ExplicitGroup && newGroup instanceof ExplicitGroup) {
+                // Rename of explicit group, so remove old group assignment
+                oldGroup.remove(entriesMatchedByOldGroup);
+            }
+            return newGroup.add(entriesMatchedByOldGroup);
+        }
+        return Optional.empty();
     }
 
     /**
@@ -200,7 +233,7 @@ public String getName() {
     }
 
     public GroupTreeNode addSubgroup(AbstractGroup group) {
-        GroupTreeNode child = new GroupTreeNode(group);
+        GroupTreeNode child = GroupTreeNode.fromGroup(group);
         addChild(child);
         return child;
     }
@@ -212,7 +245,7 @@ public String toString() {
 
     @Override
     public GroupTreeNode copyNode() {
-        return new GroupTreeNode(group);
+        return GroupTreeNode.fromGroup(group);
     }
 
     public static GroupTreeNode parse(List<String> orderedData) throws ParseException {

src/main/java/net/sf/jabref/logic/groups/GroupsParser.java

@@ -43,7 +43,7 @@ public static GroupTreeNode importGroups(List<String> orderedData) throws ParseE
             }
             int level = Integer.parseInt(string.substring(0, spaceIndex));
             AbstractGroup group = AbstractGroup.fromString(string.substring(spaceIndex + 1));
-            GroupTreeNode newNode = new GroupTreeNode(group);
+            GroupTreeNode newNode = GroupTreeNode.fromGroup(group);
             if (cursor == null) {
                 // create new root
                 cursor = newNode;

src/main/java/net/sf/jabref/sql/importer/DatabaseImporter.java

@@ -212,7 +212,7 @@ private void importGroupsTree(MetaData metaData, Map<String, BibEntry> entries,
             final String database_id) throws SQLException {
         Map<String, GroupTreeNode> groups = new HashMap<>();
         LinkedHashMap<GroupTreeNode, String> parentIds = new LinkedHashMap<>();
-        GroupTreeNode rootNode = new GroupTreeNode(new AllEntriesGroup());
+        GroupTreeNode rootNode = GroupTreeNode.fromGroup(new AllEntriesGroup());
 
         String query = SQLUtil.queryAllFromTable("groups WHERE database_id='" + database_id + "' ORDER BY groups_id");
         try (Statement statement = conn.createStatement();
@@ -253,7 +253,7 @@ private void importGroupsTree(MetaData metaData, Map<String, BibEntry> entries,
                 }
 
                 if (group != null) {
-                    GroupTreeNode node = new GroupTreeNode(group);
+                    GroupTreeNode node = GroupTreeNode.fromGroup(group);
                     parentIds.put(node, rsGroups.getString("parent_id"));
                     groups.put(rsGroups.getString("groups_id"), node);
                 }

src/test/java/net/sf/jabref/exporter/BibDatabaseWriterTest.java

@@ -196,7 +196,7 @@ public void writeMetadataAndEncoding() throws IOException {
 
     @Test
     public void writeGroups() throws IOException, ParseException {
-        GroupTreeNode groupRoot = new GroupTreeNode(new AllEntriesGroup());
+        GroupTreeNode groupRoot = GroupTreeNode.fromGroup(new AllEntriesGroup());
         groupRoot.addSubgroup(new ExplicitGroup("test", GroupHierarchyType.INCLUDING));
         metaData.setGroups(groupRoot);
 
@@ -215,8 +215,8 @@ public void writeGroups() throws IOException, ParseException {
     public void writeGroupsAndEncoding() throws IOException, ParseException {
         SavePreferences preferences = new SavePreferences().withEncoding(Charsets.US_ASCII);
 
-        GroupTreeNode groupRoot = new GroupTreeNode(new AllEntriesGroup());
-        groupRoot.addChild(new GroupTreeNode(new ExplicitGroup("test", GroupHierarchyType.INCLUDING)));
+        GroupTreeNode groupRoot = GroupTreeNode.fromGroup(new AllEntriesGroup());
+        groupRoot.addChild(GroupTreeNode.fromGroup(new ExplicitGroup("test", GroupHierarchyType.INCLUDING)));
         metaData.setGroups(groupRoot);
 
         databaseWriter.writePartOfDatabase(stringWriter, bibtexContext, Collections.emptyList(), preferences);

src/test/java/net/sf/jabref/importer/ConvertLegacyExplicitGroupsTest.java

@@ -39,7 +39,7 @@ public void setUp() throws Exception {
 
     @Test
     public void performActionWritesGroupMembershipInEntry() throws Exception {
-        ParserResult parserResult = generateParserResult(entry, new GroupTreeNode(group));
+        ParserResult parserResult = generateParserResult(entry, GroupTreeNode.fromGroup(group));
 
         action.performAction(basePanel, parserResult);
 
@@ -48,7 +48,7 @@ public void performActionWritesGroupMembershipInEntry() throws Exception {
 
     @Test
     public void performActionClearsLegacyKeys() throws Exception {
-        ParserResult parserResult = generateParserResult(entry, new GroupTreeNode(group));
+        ParserResult parserResult = generateParserResult(entry, GroupTreeNode.fromGroup(group));
 
         action.performAction(basePanel, parserResult);
 
@@ -57,7 +57,7 @@ public void performActionClearsLegacyKeys() throws Exception {
 
     @Test
     public void performActionWritesGroupMembershipInEntryForComplexGroupTree() throws Exception {
-        GroupTreeNode root = new GroupTreeNode(new AllEntriesGroup());
+        GroupTreeNode root = GroupTreeNode.fromGroup(new AllEntriesGroup());
         root.addSubgroup(new ExplicitGroup("TestGroup2", GroupHierarchyType.INCLUDING));
         root.addSubgroup(group);
         ParserResult parserResult = generateParserResult(entry, root);
@@ -69,7 +69,7 @@ public void performActionWritesGroupMembershipInEntryForComplexGroupTree() throw
 
     @Test
     public void isActionNecessaryReturnsTrueIfGroupContainsLegacyKeys() throws Exception {
-        ParserResult parserResult = generateParserResult(entry, new GroupTreeNode(group));
+        ParserResult parserResult = generateParserResult(entry, GroupTreeNode.fromGroup(group));
 
         assertTrue(action.isActionNecessary(parserResult));
     }