Add warning message if group with same name is already present (#3077)

* Add warning message if group with same name is already present (or new group name contains keyword separator) * Update localization
JabRef · Aug 6, 2017 · ee4f252 · ee4f252
1 parent 973b127
commit ee4f252
Show file tree

Hide file tree

Showing 22 changed files with 102 additions and 8 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -12,7 +12,8 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `#
 
 ### Changed
 
-If fetched article is already in database the ImportInspectionDialog is started
+- If fetched article is already in database, then the entry merge dialog is shown.
+- An error message is now displayed if you try to create a group containing the keyword separator or if there is already a group with the same name. [#3075](https://github.com/JabRef/jabref/issues/3075) and [#1495](https://github.com/JabRef/jabref/issues/1495)
 
 ### Fixed
 
@@ -21,6 +22,7 @@ If fetched article is already in database the ImportInspectionDialog is started
 - We fixed the shortcut <kbd>Ctrl</kbd>+<kbd>F</kbd> for the search field.
 - We fixed an issue where the preferences could not be imported without a restart of JabRef [#3064](https://github.com/JabRef/jabref/issues/3064)
 - We fixed an issue where <kbd>DEL</kbd>, <kbd>Ctrl</kbd>+<kbd>C</kbd>, <kbd>Ctrl</kbd>+<kbd>V</kbd> and <kbd>Ctrl</kbd>+<kbd>A</kbd> in the search field triggered corresponding actions in the main table [#3067](https://github.com/JabRef/jabref/issues/3067)
+
 ### Removed
 
 

diff --git a/src/main/java/org/jabref/gui/groups/GroupDialog.java b/src/main/java/org/jabref/gui/groups/GroupDialog.java
@@ -45,6 +45,7 @@
 import org.jabref.model.groups.AutomaticPersonsGroup;
 import org.jabref.model.groups.ExplicitGroup;
 import org.jabref.model.groups.GroupHierarchyType;
+import org.jabref.model.groups.GroupTreeNode;
 import org.jabref.model.groups.RegexKeywordGroup;
 import org.jabref.model.groups.SearchGroup;
 import org.jabref.model.groups.WordKeywordGroup;
@@ -326,18 +327,45 @@ public void actionPerformed(ActionEvent e) {
         okButton.addActionListener(e -> {
                 isOkPressed = true;
             try {
+                String groupName = nameField.getText().trim();
                 if (explicitRadioButton.isSelected()) {
-                    resultingGroup = new ExplicitGroup(nameField.getText().trim(), getContext(),
-                            Globals.prefs.getKeywordDelimiter());
+                    Character keywordDelimiter = Globals.prefs.getKeywordDelimiter();
+                    if (groupName.contains(Character.toString(keywordDelimiter))) {
+                        jabrefFrame.showMessage(
+                                Localization.lang("The group name contains the keyword separator \"%0\" and thus probably does not work as expected.", Character.toString(keywordDelimiter)));
+                    }
+
+                    Optional<GroupTreeNode> rootGroup = jabrefFrame.getCurrentBasePanel().getBibDatabaseContext().getMetaData().getGroups();
+                    if (rootGroup.isPresent()) {
+                        int groupsWithSameName = rootGroup.get().findChildrenSatisfying(group -> group.getName().equals(groupName)).size();
+                        boolean warnAboutSameName = false;
+                        if (editedGroup == null && groupsWithSameName > 0) {
+                            // New group but there is already one group with the same name
+                            warnAboutSameName = true;
+                        }
+                        if (editedGroup != null && !editedGroup.getName().equals(groupName) && groupsWithSameName > 0) {
+                            // Edit group, changed name to something that is already present
+                            warnAboutSameName = true;
+                        }
+
+                        if (warnAboutSameName) {
+                            jabrefFrame.showMessage(
+                                    Localization.lang("There exists already a group with the same name.", Character.toString(keywordDelimiter)));
+                            return;
+                        }
+                    }
+
+                    resultingGroup = new ExplicitGroup(groupName, getContext(),
+                            keywordDelimiter);
                 } else if (keywordsRadioButton.isSelected()) {
                     // regex is correct, otherwise OK would have been disabled
                     // therefore I don't catch anything here
                     if (keywordGroupRegExp.isSelected()) {
-                        resultingGroup = new RegexKeywordGroup(nameField.getText().trim(), getContext(),
+                        resultingGroup = new RegexKeywordGroup(groupName, getContext(),
                                 keywordGroupSearchField.getText().trim(), keywordGroupSearchTerm.getText().trim(),
                                 keywordGroupCaseSensitive.isSelected());
                     } else {
-                        resultingGroup = new WordKeywordGroup(nameField.getText().trim(), getContext(),
+                        resultingGroup = new WordKeywordGroup(groupName, getContext(),
                                 keywordGroupSearchField.getText().trim(), keywordGroupSearchTerm.getText().trim(),
                                 keywordGroupCaseSensitive.isSelected(), Globals.prefs.getKeywordDelimiter(), false);
                     }
@@ -346,20 +374,20 @@ public void actionPerformed(ActionEvent e) {
                         // regex is correct, otherwise OK would have been
                         // disabled
                         // therefore I don't catch anything here
-                        resultingGroup = new SearchGroup(nameField.getText().trim(), getContext(), searchGroupSearchExpression.getText().trim(),
+                        resultingGroup = new SearchGroup(groupName, getContext(), searchGroupSearchExpression.getText().trim(),
                                 isCaseSensitive(), isRegex());
                     } catch (Exception e1) {
                         // should never happen
                     }
                 } else if (autoRadioButton.isSelected()) {
                     if (autoGroupKeywordsOption.isSelected()) {
                         resultingGroup = new AutomaticKeywordGroup(
-                                nameField.getText().trim(), getContext(),
+                                groupName, getContext(),
                                 autoGroupKeywordsField.getText().trim(),
                                 autoGroupKeywordsDeliminator.getText().charAt(0),
                                 autoGroupKeywordsHierarchicalDeliminator.getText().charAt(0));
                     } else {
-                        resultingGroup = new AutomaticPersonsGroup(nameField.getText().trim(), getContext(),
+                        resultingGroup = new AutomaticPersonsGroup(groupName, getContext(),
                                 autoGroupPersonsField.getText().trim());
                     }
                 }

diff --git a/src/main/java/org/jabref/model/TreeNode.java b/src/main/java/org/jabref/model/TreeNode.java
@@ -6,6 +6,7 @@
 import java.util.Objects;
 import java.util.Optional;
 import java.util.function.Consumer;
+import java.util.function.Predicate;
 
 import javafx.collections.FXCollections;
 import javafx.collections.ObservableList;
@@ -605,4 +606,21 @@ protected void notifyAboutDescendantChange(T source) {
             parent.notifyAboutDescendantChange(source);
         }
     }
+
+    /**
+     * Returns the group and any of its children in the tree satisfying the given condition.
+     */
+    public List<T> findChildrenSatisfying(Predicate<T> matcher) {
+        List<T> hits = new ArrayList<>();
+
+        if (matcher.test((T) this)) {
+            hits.add((T) this);
+        }
+
+        for (T child : getChildren()) {
+            hits.addAll(child.findChildrenSatisfying(matcher));
+        }
+
+        return hits;
+    }
 }
diff --git a/src/main/resources/l10n/JabRef_da.properties b/src/main/resources/l10n/JabRef_da.properties
@@ -2328,3 +2328,5 @@ Delete_the_selected_file_permanently_from_disk,_or_just_remove_the_file_from_the
 Delete_'%0'=
 Delete_from_disk=
 Remove_from_entry=
+The_group_name_contains_the_keyword_separator_"%0"_and_thus_probably_does_not_work_as_expected.=
+There_exists_already_a_group_with_the_same_name.=
diff --git a/src/main/resources/l10n/JabRef_de.properties b/src/main/resources/l10n/JabRef_de.properties
@@ -2328,3 +2328,5 @@ Delete_the_selected_file_permanently_from_disk,_or_just_remove_the_file_from_the
 Delete_'%0'=
 Delete_from_disk=
 Remove_from_entry=
+The_group_name_contains_the_keyword_separator_"%0"_and_thus_probably_does_not_work_as_expected.=
+There_exists_already_a_group_with_the_same_name.=
diff --git a/src/main/resources/l10n/JabRef_el.properties b/src/main/resources/l10n/JabRef_el.properties
@@ -2328,3 +2328,5 @@ Delete_the_selected_file_permanently_from_disk,_or_just_remove_the_file_from_the
 Delete_'%0'=
 Delete_from_disk=
 Remove_from_entry=
+The_group_name_contains_the_keyword_separator_"%0"_and_thus_probably_does_not_work_as_expected.=
+There_exists_already_a_group_with_the_same_name.=
diff --git a/src/main/resources/l10n/JabRef_en.properties b/src/main/resources/l10n/JabRef_en.properties
@@ -2328,3 +2328,5 @@ Delete_the_selected_file_permanently_from_disk,_or_just_remove_the_file_from_the
 Delete_'%0'=Delete_'%0'
 Delete_from_disk=Delete_from_disk
 Remove_from_entry=Remove_from_entry
+The_group_name_contains_the_keyword_separator_"%0"_and_thus_probably_does_not_work_as_expected.=The_group_name_contains_the_keyword_separator_"%0"_and_thus_probably_does_not_work_as_expected.
+There_exists_already_a_group_with_the_same_name.=There_exists_already_a_group_with_the_same_name.
diff --git a/src/main/resources/l10n/JabRef_es.properties b/src/main/resources/l10n/JabRef_es.properties
@@ -2328,3 +2328,5 @@ Delete_the_selected_file_permanently_from_disk,_or_just_remove_the_file_from_the
 Delete_'%0'=
 Delete_from_disk=
 Remove_from_entry=
+The_group_name_contains_the_keyword_separator_"%0"_and_thus_probably_does_not_work_as_expected.=
+There_exists_already_a_group_with_the_same_name.=
diff --git a/src/main/resources/l10n/JabRef_fa.properties b/src/main/resources/l10n/JabRef_fa.properties
@@ -2328,3 +2328,5 @@ Delete_the_selected_file_permanently_from_disk,_or_just_remove_the_file_from_the
 Delete_'%0'=
 Delete_from_disk=
 Remove_from_entry=
+The_group_name_contains_the_keyword_separator_"%0"_and_thus_probably_does_not_work_as_expected.=
+There_exists_already_a_group_with_the_same_name.=
diff --git a/src/main/resources/l10n/JabRef_fr.properties b/src/main/resources/l10n/JabRef_fr.properties
@@ -2328,3 +2328,5 @@ Delete_the_selected_file_permanently_from_disk,_or_just_remove_the_file_from_the
 Delete_'%0'=Supprimer_'%0'
 Delete_from_disk=Supprimer_du_disque
 Remove_from_entry=Effacer_de_l'entrée
+The_group_name_contains_the_keyword_separator_"%0"_and_thus_probably_does_not_work_as_expected.=
+There_exists_already_a_group_with_the_same_name.=
diff --git a/src/main/resources/l10n/JabRef_in.properties b/src/main/resources/l10n/JabRef_in.properties
@@ -2328,3 +2328,5 @@ Delete_the_selected_file_permanently_from_disk,_or_just_remove_the_file_from_the
 Delete_'%0'=
 Delete_from_disk=
 Remove_from_entry=
+The_group_name_contains_the_keyword_separator_"%0"_and_thus_probably_does_not_work_as_expected.=
+There_exists_already_a_group_with_the_same_name.=
diff --git a/src/main/resources/l10n/JabRef_it.properties b/src/main/resources/l10n/JabRef_it.properties
@@ -2328,3 +2328,5 @@ Delete_the_selected_file_permanently_from_disk,_or_just_remove_the_file_from_the
 Delete_'%0'=Cancella_'%0'
 Delete_from_disk=Cancella_dal_disco
 Remove_from_entry=Rimuovi_dalla_voce
+The_group_name_contains_the_keyword_separator_"%0"_and_thus_probably_does_not_work_as_expected.=
+There_exists_already_a_group_with_the_same_name.=
diff --git a/src/main/resources/l10n/JabRef_ja.properties b/src/main/resources/l10n/JabRef_ja.properties
@@ -2328,3 +2328,5 @@ Delete_the_selected_file_permanently_from_disk,_or_just_remove_the_file_from_the
 Delete_'%0'=
 Delete_from_disk=
 Remove_from_entry=
+The_group_name_contains_the_keyword_separator_"%0"_and_thus_probably_does_not_work_as_expected.=
+There_exists_already_a_group_with_the_same_name.=
diff --git a/src/main/resources/l10n/JabRef_nl.properties b/src/main/resources/l10n/JabRef_nl.properties
@@ -2328,3 +2328,5 @@ Delete_the_selected_file_permanently_from_disk,_or_just_remove_the_file_from_the
 Delete_'%0'=
 Delete_from_disk=
 Remove_from_entry=
+The_group_name_contains_the_keyword_separator_"%0"_and_thus_probably_does_not_work_as_expected.=
+There_exists_already_a_group_with_the_same_name.=
diff --git a/src/main/resources/l10n/JabRef_no.properties b/src/main/resources/l10n/JabRef_no.properties
@@ -2328,3 +2328,5 @@ Delete_the_selected_file_permanently_from_disk,_or_just_remove_the_file_from_the
 Delete_'%0'=
 Delete_from_disk=
 Remove_from_entry=
+The_group_name_contains_the_keyword_separator_"%0"_and_thus_probably_does_not_work_as_expected.=
+There_exists_already_a_group_with_the_same_name.=
diff --git a/src/main/resources/l10n/JabRef_pt_BR.properties b/src/main/resources/l10n/JabRef_pt_BR.properties
@@ -2328,3 +2328,5 @@ Delete_the_selected_file_permanently_from_disk,_or_just_remove_the_file_from_the
 Delete_'%0'=
 Delete_from_disk=
 Remove_from_entry=
+The_group_name_contains_the_keyword_separator_"%0"_and_thus_probably_does_not_work_as_expected.=
+There_exists_already_a_group_with_the_same_name.=
diff --git a/src/main/resources/l10n/JabRef_ru.properties b/src/main/resources/l10n/JabRef_ru.properties
@@ -2328,3 +2328,5 @@ Delete_the_selected_file_permanently_from_disk,_or_just_remove_the_file_from_the
 Delete_'%0'=
 Delete_from_disk=
 Remove_from_entry=
+The_group_name_contains_the_keyword_separator_"%0"_and_thus_probably_does_not_work_as_expected.=
+There_exists_already_a_group_with_the_same_name.=
diff --git a/src/main/resources/l10n/JabRef_sv.properties b/src/main/resources/l10n/JabRef_sv.properties
@@ -2328,3 +2328,5 @@ Delete_the_selected_file_permanently_from_disk,_or_just_remove_the_file_from_the
 Delete_'%0'=
 Delete_from_disk=
 Remove_from_entry=
+The_group_name_contains_the_keyword_separator_"%0"_and_thus_probably_does_not_work_as_expected.=
+There_exists_already_a_group_with_the_same_name.=
diff --git a/src/main/resources/l10n/JabRef_tr.properties b/src/main/resources/l10n/JabRef_tr.properties
@@ -2328,3 +2328,5 @@ Delete_the_selected_file_permanently_from_disk,_or_just_remove_the_file_from_the
 Delete_'%0'=
 Delete_from_disk=
 Remove_from_entry=
+The_group_name_contains_the_keyword_separator_"%0"_and_thus_probably_does_not_work_as_expected.=
+There_exists_already_a_group_with_the_same_name.=
diff --git a/src/main/resources/l10n/JabRef_vi.properties b/src/main/resources/l10n/JabRef_vi.properties
@@ -2328,3 +2328,5 @@ Delete_the_selected_file_permanently_from_disk,_or_just_remove_the_file_from_the
 Delete_'%0'=
 Delete_from_disk=
 Remove_from_entry=
+The_group_name_contains_the_keyword_separator_"%0"_and_thus_probably_does_not_work_as_expected.=
+There_exists_already_a_group_with_the_same_name.=
diff --git a/src/main/resources/l10n/JabRef_zh.properties b/src/main/resources/l10n/JabRef_zh.properties
@@ -2328,3 +2328,5 @@ Delete_the_selected_file_permanently_from_disk,_or_just_remove_the_file_from_the
 Delete_'%0'=
 Delete_from_disk=
 Remove_from_entry=
+The_group_name_contains_the_keyword_separator_"%0"_and_thus_probably_does_not_work_as_expected.=
+There_exists_already_a_group_with_the_same_name.=
diff --git a/src/test/java/org/jabref/model/TreeNodeTest.java b/src/test/java/org/jabref/model/TreeNodeTest.java
@@ -610,6 +610,16 @@ public void removeChildIndexSomewhereInTreeInvokesChangeEvent() {
         verify(subscriber).accept(node);
     }
 
+    @Test
+    public void findChildrenWithSameName() throws Exception {
+        TreeNodeTestData.TreeNodeMock root = new TreeNodeTestData.TreeNodeMock("A");
+        TreeNodeTestData.TreeNodeMock childB = root.addChild(new TreeNodeTestData.TreeNodeMock("B"));
+        TreeNodeTestData.TreeNodeMock node = childB.addChild(new TreeNodeTestData.TreeNodeMock("A"));
+        TreeNodeTestData.TreeNodeMock childA = root.addChild(new TreeNodeTestData.TreeNodeMock("A"));
+
+        assertEquals(Arrays.asList(root, node, childA), root.findChildrenSatisfying(treeNode -> treeNode.getName().equals("A")));
+    }
+
     private static class WrongTreeNodeImplementation extends TreeNode<TreeNodeTestData.TreeNodeMock> {
         // This class is a wrong derived class of TreeNode<T>
         // since it does not extends TreeNode<WrongTreeNodeImplementation>