Fix #628: implement hierarchical keywords (#2703)

* Rename keywords to keyword chains * Parse hierarchical keywords * Automatic group creates a proper tree instead of a simple list for hierarchical keywords * Make hierarchical delimiter for automatic group configurable and write it to bib file * Adapt Changelog entry * Fix tests
JabRef · Apr 6, 2017 · 67e457c · 67e457c
1 parent 1f7bba8
commit 67e457c
Show file tree

Hide file tree

Showing 19 changed files with 449 additions and 125 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -50,7 +50,7 @@ We refer to [GitHub issues](https://github.com/JabRef/jabref/issues) by using `#
 - We added a few properties to a group:
     - Icon (with customizable color) that is shown in the groups panel (implements a [feature request in the forum](http://discourse.jabref.org/t/assign-colors-to-groups/321)).
     - Description text that is shown on mouse hover (implements old feature requests [489](https://sourceforge.net/p/jabref/feature-requests/489/) and [818](https://sourceforge.net/p/jabref/feature-requests/818/)
-- We introduced "automatic groups" that automatically create subgroups based on a certain criteria (e.g. a subgroup for every author or keyword). Implements [91](https://sourceforge.net/p/jabref/feature-requests/91/), [398](https://sourceforge.net/p/jabref/feature-requests/398/) and [#1173](https://github.com/JabRef/jabref/issues/1173).
+- We introduced "automatic groups" that automatically create subgroups based on a certain criteria (e.g. a subgroup for every author or keyword) and supports hierarchies. Implements [91](https://sourceforge.net/p/jabref/feature-requests/91/), [398](https://sourceforge.net/p/jabref/feature-requests/398/) and [#1173](https://github.com/JabRef/jabref/issues/1173) and [#628](https://github.com/JabRef/jabref/issues/628).
 - Expansion status of groups are saved across sessions. [#1428](https://github.com/JabRef/jabref/issues/1428)
 - We removed the ordinals-to-superscript formatter from the recommendations for biblatex save actions [#2596](https://github.com/JabRef/jabref/issues/2596)
 - The `Move linked files to default file directory`-Cleanup operation respects the `File directory pattern` setting 

diff --git a/src/main/java/org/jabref/gui/groups/GroupDialog.java b/src/main/java/org/jabref/gui/groups/GroupDialog.java
@@ -39,6 +39,7 @@
 import org.jabref.logic.l10n.Localization;
 import org.jabref.logic.search.SearchQuery;
 import org.jabref.model.entry.FieldName;
+import org.jabref.model.entry.Keyword;
 import org.jabref.model.groups.AbstractGroup;
 import org.jabref.model.groups.AutomaticKeywordGroup;
 import org.jabref.model.groups.AutomaticPersonsGroup;
@@ -100,6 +101,7 @@ class GroupDialog extends JDialog implements Dialog<AbstractGroup> {
             Localization.lang("Generate groups from keywords in a BibTeX field"));
     private final JTextField autoGroupKeywordsField = new JTextField(60);
     private final JTextField autoGroupKeywordsDeliminator = new JTextField(60);
+    private final JTextField autoGroupKeywordsHierarchicalDeliminator = new JTextField(60);
     private final JRadioButton autoGroupPersonsOption = new JRadioButton(
             Localization.lang("Generate groups for author last names"));
     private final JTextField autoGroupPersonsField = new JTextField(60);
@@ -183,22 +185,24 @@ public GroupDialog(JabRefFrame jabrefFrame, AbstractGroup editedGroup) {
         bg.add(autoGroupPersonsOption);
 
         FormLayout layoutAutoGroup = new FormLayout("left:20dlu, 4dlu, left:pref, 4dlu, fill:60dlu",
-                "p, 2dlu, p, 2dlu, p, 2dlu, p, 2dlu, p");
+                "p, 2dlu, p, 2dlu, p, p, 2dlu, p, 2dlu, p");
         FormBuilder builderAutoGroup = FormBuilder.create();
         builderAutoGroup.layout(layoutAutoGroup);
         builderAutoGroup.add(autoGroupKeywordsOption).xyw(1, 1, 5);
         builderAutoGroup.add(Localization.lang("Field to group by") + ":").xy(3, 3);
         builderAutoGroup.add(autoGroupKeywordsField).xy(5, 3);
         builderAutoGroup.add(Localization.lang("Use the following delimiter character(s):")).xy(3, 5);
         builderAutoGroup.add(autoGroupKeywordsDeliminator).xy(5, 5);
-        builderAutoGroup.add(autoGroupPersonsOption).xyw(1, 7, 5);
-        builderAutoGroup.add(Localization.lang("Field to group by") + ":").xy(3, 9);
-        builderAutoGroup.add(autoGroupPersonsField).xy(5, 9);
+        builderAutoGroup.add(autoGroupKeywordsHierarchicalDeliminator).xy(5, 6);
+        builderAutoGroup.add(autoGroupPersonsOption).xyw(1, 8, 5);
+        builderAutoGroup.add(Localization.lang("Field to group by") + ":").xy(3, 10);
+        builderAutoGroup.add(autoGroupPersonsField).xy(5, 10);
         optionsPanel.add(builderAutoGroup.build(), String.valueOf(GroupDialog.INDEX_AUTO_GROUP));
 
         autoGroupKeywordsOption.setSelected(true);
         autoGroupKeywordsField.setText(Globals.prefs.get(JabRefPreferences.GROUPS_DEFAULT_FIELD));
         autoGroupKeywordsDeliminator.setText(Globals.prefs.get(JabRefPreferences.KEYWORD_SEPARATOR));
+        autoGroupKeywordsHierarchicalDeliminator.setText(Keyword.DEFAULT_HIERARCHICAL_DELIMITER.toString());
         autoGroupPersonsField.setText(FieldName.AUTHOR);
 
         // ... for buttons panel
@@ -349,9 +353,11 @@ public void actionPerformed(ActionEvent e) {
                     }
                 } else if (autoRadioButton.isSelected()) {
                     if (autoGroupKeywordsOption.isSelected()) {
-                        resultingGroup = new AutomaticKeywordGroup(nameField.getText().trim(), getContext(),
+                        resultingGroup = new AutomaticKeywordGroup(
+                                nameField.getText().trim(), getContext(),
                                 autoGroupKeywordsField.getText().trim(),
-                                autoGroupKeywordsDeliminator.getText().charAt(0));
+                                autoGroupKeywordsDeliminator.getText().charAt(0),
+                                autoGroupKeywordsHierarchicalDeliminator.getText().charAt(0));
                     } else {
                         resultingGroup = new AutomaticPersonsGroup(nameField.getText().trim(), getContext(),
                                 autoGroupPersonsField.getText().trim());
@@ -429,7 +435,8 @@ public void actionPerformed(ActionEvent e) {
 
                 if (editedGroup.getClass() == AutomaticKeywordGroup.class) {
                     AutomaticKeywordGroup group = (AutomaticKeywordGroup) editedGroup;
-                    autoGroupKeywordsDeliminator.setText(group.getKeywordSeperator().toString());
+                    autoGroupKeywordsDeliminator.setText(group.getKeywordDelimiter().toString());
+                    autoGroupKeywordsHierarchicalDeliminator.setText(group.getKeywordHierarchicalDelimiter().toString());
                     autoGroupKeywordsField.setText(group.getField());
                 } else if (editedGroup.getClass() == AutomaticPersonsGroup.class) {
                     AutomaticPersonsGroup group = (AutomaticPersonsGroup) editedGroup;

diff --git a/src/main/java/org/jabref/gui/groups/GroupNodeViewModel.java b/src/main/java/org/jabref/gui/groups/GroupNodeViewModel.java
@@ -1,14 +1,9 @@
 package org.jabref.gui.groups;
 
 import java.util.List;
-import java.util.Map;
 import java.util.Objects;
 import java.util.Optional;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.function.Function;
-import java.util.function.Predicate;
 import java.util.stream.Collectors;
-import java.util.stream.Stream;
 
 import javafx.beans.binding.Bindings;
 import javafx.beans.binding.BooleanBinding;
@@ -67,16 +62,12 @@ public GroupNodeViewModel(BibDatabaseContext databaseContext, StateManager state
         if (groupNode.getGroup() instanceof AutomaticGroup) {
             AutomaticGroup automaticGroup = (AutomaticGroup) groupNode.getGroup();
 
-            // TODO: Update on changes to entry list (however: there is no flatMap and filter as observable TransformationLists)
-            children = databaseContext.getDatabase()
-                    .getEntries().stream()
-                    .flatMap(stream -> createSubgroups(automaticGroup, stream))
-                    .filter(distinctByKey(group -> group.getGroupNode().getName()))
+            children = automaticGroup.createSubgroups(databaseContext.getDatabase().getEntries()).stream()
+                    .map(this::toViewModel)
                     .sorted((group1, group2) -> group1.getDisplayName().compareToIgnoreCase(group2.getDisplayName()))
                     .collect(Collectors.toCollection(FXCollections::observableArrayList));
         } else {
-            children = EasyBind.map(groupNode.getChildren(),
-                    child -> new GroupNodeViewModel(databaseContext, stateManager, taskExecutor, child));
+            children = EasyBind.map(groupNode.getChildren(), this::toViewModel);
         }
         hasChildren = new SimpleBooleanProperty();
         hasChildren.bind(Bindings.isNotEmpty(children));
@@ -97,18 +88,12 @@ public GroupNodeViewModel(BibDatabaseContext databaseContext, StateManager state
         this(databaseContext, stateManager, taskExecutor, new GroupTreeNode(group));
     }
 
-    private static <T> Predicate<T> distinctByKey(Function<? super T, ?> keyExtractor) {
-        Map<Object,Boolean> seen = new ConcurrentHashMap<>();
-        return t -> seen.putIfAbsent(keyExtractor.apply(t), Boolean.TRUE) == null;
-    }
-
     static GroupNodeViewModel getAllEntriesGroup(BibDatabaseContext newDatabase, StateManager stateManager, TaskExecutor taskExecutor) {
         return new GroupNodeViewModel(newDatabase, stateManager, taskExecutor, DefaultGroupsFactory.getAllEntriesGroup());
     }
 
-    private Stream<GroupNodeViewModel> createSubgroups(AutomaticGroup automaticGroup, BibEntry entry) {
-        return automaticGroup.createSubgroups(entry).stream()
-                .map(child -> new GroupNodeViewModel(databaseContext, stateManager, taskExecutor, child));
+    private GroupNodeViewModel toViewModel(GroupTreeNode child) {
+        return new GroupNodeViewModel(databaseContext, stateManager, taskExecutor, child);
     }
 
     public List<FieldChange> addEntriesToGroup(List<BibEntry> entries) {
@@ -243,7 +228,7 @@ public String getPath() {
     }
 
     public Optional<GroupNodeViewModel> getChildByPath(String pathToSource) {
-        return groupNode.getChildByPath(pathToSource).map(child -> new GroupNodeViewModel(databaseContext, stateManager, taskExecutor, child));
+        return groupNode.getChildByPath(pathToSource).map(this::toViewModel);
     }
 
     /**

diff --git a/src/main/java/org/jabref/logic/exporter/GroupSerializer.java b/src/main/java/org/jabref/logic/exporter/GroupSerializer.java
@@ -152,7 +152,9 @@ private String serializeAutomaticKeywordGroup(AutomaticKeywordGroup group) {
         appendAutomaticGroupDetails(sb, group);
         sb.append(StringUtil.quote(group.getField(), MetadataSerializationConfiguration.GROUP_UNIT_SEPARATOR, MetadataSerializationConfiguration.GROUP_QUOTE_CHAR));
         sb.append(MetadataSerializationConfiguration.GROUP_UNIT_SEPARATOR);
-        sb.append(group.getKeywordSeperator());
+        sb.append(group.getKeywordDelimiter());
+        sb.append(MetadataSerializationConfiguration.GROUP_UNIT_SEPARATOR);
+        sb.append(group.getKeywordHierarchicalDelimiter());
         sb.append(MetadataSerializationConfiguration.GROUP_UNIT_SEPARATOR);
         appendGroupDetails(sb, group);
         return sb.toString();

diff --git a/src/main/java/org/jabref/logic/importer/util/GroupsParser.java b/src/main/java/org/jabref/logic/importer/util/GroupsParser.java
@@ -127,8 +127,9 @@ private static AbstractGroup automaticKeywordGroupFromString(String string) {
         String name = StringUtil.unquote(tok.nextToken(), MetadataSerializationConfiguration.GROUP_QUOTE_CHAR);
         GroupHierarchyType context = GroupHierarchyType.getByNumberOrDefault(Integer.parseInt(tok.nextToken()));
         String field = StringUtil.unquote(tok.nextToken(), MetadataSerializationConfiguration.GROUP_QUOTE_CHAR);
-        Character separator = tok.nextToken().charAt(0);
-        AutomaticKeywordGroup newGroup = new AutomaticKeywordGroup(name, context, field, separator);
+        Character delimiter = tok.nextToken().charAt(0);
+        Character hierarchicalDelimiter = tok.nextToken().charAt(0);
+        AutomaticKeywordGroup newGroup = new AutomaticKeywordGroup(name, context, field, delimiter, hierarchicalDelimiter);
         addGroupDetails(tok, newGroup);
         return newGroup;
     }

diff --git a/src/main/java/org/jabref/model/ChainNode.java b/src/main/java/org/jabref/model/ChainNode.java
@@ -58,35 +58,24 @@ public Optional<T> getParent() {
     }
 
     /**
-     * Returns this node's child or an empty Optional if this node has no child.
+     * Sets the parent node of this node.
+     * <p>
+     * This method does not set this node as the child of the new parent nor does it remove this node
+     * from the old parent. You should probably call {@link #moveTo(ChainNode)} to change the chain.
      *
-     * @return this node's child T, or an empty Optional if this node has no child
+     * @param parent the new parent
      */
-    public Optional<T> getChild() {
-        return Optional.ofNullable(child);
+    protected void setParent(T parent) {
+        this.parent = Objects.requireNonNull(parent);
     }
 
     /**
-     * Removes this node from its parent and makes it a child of the specified node.
-     * In this way the whole subchain based at this node is moved to the given node.
+     * Returns this node's child or an empty Optional if this node has no child.
      *
-     * @param target      the new parent
-     * @throws NullPointerException           if target is null
-     * @throws UnsupportedOperationException  if target is an descendant of this node
+     * @return this node's child T, or an empty Optional if this node has no child
      */
-    public void moveTo(T target) {
-        Objects.requireNonNull(target);
-
-        // Check that the target node is not an ancestor of this node, because this would create loops in the tree
-        if (this.isAncestorOf(target)) {
-            throw new UnsupportedOperationException("the target cannot be a descendant of this node");
-        }
-
-        // Remove from previous parent
-        getParent().ifPresent(oldParent -> oldParent.removeChild());
-
-        // Add as child
-        target.setChild((T) this);
+    public Optional<T> getChild() {
+        return Optional.ofNullable(child);
     }
 
     /**
@@ -110,15 +99,26 @@ public T setChild(T child) {
     }
 
     /**
-     * Sets the parent node of this node.
-     * <p>
-     * This method does not set this node as the child of the new parent nor does it remove this node
-     * from the old parent. You should probably call {@link #moveTo(ChainNode)} to change the chain.
+     * Removes this node from its parent and makes it a child of the specified node.
+     * In this way the whole subchain based at this node is moved to the given node.
      *
-     * @param parent the new parent
+     * @param target      the new parent
+     * @throws NullPointerException           if target is null
+     * @throws UnsupportedOperationException  if target is an descendant of this node
      */
-    protected void setParent(T parent) {
-        this.parent = Objects.requireNonNull(parent);
+    public void moveTo(T target) {
+        Objects.requireNonNull(target);
+
+        // Check that the target node is not an ancestor of this node, because this would create loops in the tree
+        if (this.isAncestorOf(target)) {
+            throw new UnsupportedOperationException("the target cannot be a descendant of this node");
+        }
+
+        // Remove from previous parent
+        getParent().ifPresent(oldParent -> oldParent.removeChild());
+
+        // Add as child
+        target.setChild((T) this);
     }
 
     /**
@@ -151,4 +151,16 @@ public boolean isAncestorOf(T anotherNode) {
             return child.isAncestorOf(anotherNode);
         }
     }
+
+    /**
+     * Adds the given node at the end of the chain.
+     * E.g., "A > B > C" + "D" -> "A > B > C > D".
+     */
+    public void addAtEnd(T node) {
+        if (child == null) {
+            setChild(node);
+        } else {
+            child.addAtEnd(node);
+        }
+    }
 }
diff --git a/src/main/java/org/jabref/model/database/BibDatabase.java b/src/main/java/org/jabref/model/database/BibDatabase.java
@@ -3,6 +3,7 @@
 import java.math.BigInteger;
 import java.security.SecureRandom;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
@@ -198,6 +199,10 @@ public synchronized boolean insertEntry(BibEntry entry, EntryEventSource eventSo
         return duplicationChecker.isDuplicateCiteKeyExisting(entry);
     }
 
+    public synchronized void insertEntries(BibEntry... entries) throws KeyCollisionException {
+        insertEntries(Arrays.asList(entries), EntryEventSource.LOCAL);
+    }
+
     public synchronized void insertEntries(List<BibEntry> entries) throws KeyCollisionException {
         insertEntries(entries, EntryEventSource.LOCAL);
     }