Fixed #803 dynamic group using free-form search returned wrong results

JabRef · Feb 17, 2016 · 3fc03ad · 3fc03ad
1 parent c895951
commit 3fc03ad
Show file tree

Hide file tree

Showing 9 changed files with 39 additions and 75 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -44,7 +44,7 @@ to [sourceforge feature requests](https://sourceforge.net/p/jabref/features/) by
 - Fixed [#710](https://github.com/JabRef/jabref/issues/710): Fixed quit behaviour under OSX
 - Merge from DOI now honors removed fields
 - Fixed [#778](https://github.com/JabRef/jabref/issues/778): Fixed NPE when exporting to .sql File
-
+- Fixed [#803](https://github.com/JabRef/jabref/issues/803): Fixed dynamically group, free-form search
 
 ### Removed
 - Fixed [#627](https://github.com/JabRef/jabref/issues/627): The pdf field is removed from the export formats, use the file field

diff --git a/src/main/java/net/sf/jabref/groups/structure/AbstractGroup.java b/src/main/java/net/sf/jabref/groups/structure/AbstractGroup.java
@@ -134,12 +134,6 @@ public final void setName(String name) {
      */
     public abstract AbstractUndoableEdit remove(List<BibEntry> entries);
 
-    /**
-     * @param query The search option to apply.
-     * @return true if this group contains the specified entry, false otherwise.
-     */
-    public abstract boolean contains(String query, BibEntry entry);
-
     /**
      * @return true if this group contains the specified entry, false otherwise.
      */

diff --git a/src/main/java/net/sf/jabref/groups/structure/AllEntriesGroup.java b/src/main/java/net/sf/jabref/groups/structure/AllEntriesGroup.java
@@ -90,11 +90,6 @@ public AbstractUndoableEdit remove(List<BibEntry> entries) {
         return null;
     }
 
-    @Override
-    public boolean contains(String query, BibEntry entry) {
-        return true; // contains everything
-    }
-
     @Override
     public AbstractGroup deepCopy() {
         return new AllEntriesGroup();

diff --git a/src/main/java/net/sf/jabref/groups/structure/ExplicitGroup.java b/src/main/java/net/sf/jabref/groups/structure/ExplicitGroup.java
@@ -92,7 +92,7 @@ public SearchRule getSearchRule() {
 
             @Override
             public boolean applyRule(String query, BibEntry bibEntry) {
-                return contains(query, bibEntry);
+                return contains(bibEntry);
             }
 
             @Override
@@ -151,11 +151,6 @@ public boolean contains(BibEntry entry) {
         return entries.contains(entry);
     }
 
-    @Override
-    public boolean contains(String query, BibEntry entry) {
-        return contains(entry);
-    }
-
     @Override
     public AbstractGroup deepCopy() {
         ExplicitGroup copy = new ExplicitGroup(name, context);
@@ -191,11 +186,7 @@ public boolean equals(Object o) {
                 return false;
             }
         }
-        if (!keys.isEmpty()) {
-            return false;
-        }
-        return other.name.equals(name)
-                && (other.getHierarchicalContext() == getHierarchicalContext());
+        return keys.isEmpty() && other.name.equals(name) && (other.getHierarchicalContext() == getHierarchicalContext());
     }
 
     /**

diff --git a/src/main/java/net/sf/jabref/groups/structure/KeywordGroup.java b/src/main/java/net/sf/jabref/groups/structure/KeywordGroup.java
@@ -133,7 +133,7 @@ public SearchRule getSearchRule() {
         return new SearchRule() {
             @Override
             public boolean applyRule(String query, BibEntry bibEntry) {
-                return contains(query, bibEntry);
+                return contains(bibEntry);
             }
 
             @Override
@@ -178,7 +178,7 @@ public AbstractUndoableEdit add(List<BibEntry> entries) {
                     Localization.lang("add entries to group"));
             boolean modified = false;
             for (BibEntry entry : entries) {
-                if (!getSearchRule().applyRule(SearchRule.NULL_QUERY, entry)) {
+                if (!contains(entry)) {
                     String oldContent = entry
                             .getField(searchField);
                     String pre = Globals.prefs.get(JabRefPreferences.GROUP_KEYWORD_SEPARATOR);
@@ -213,7 +213,7 @@ public AbstractUndoableEdit remove(List<BibEntry> entries) {
             NamedCompound ce = new NamedCompound(Localization.lang("remove from group"));
             boolean modified = false;
             for (BibEntry entry : entries) {
-                if (getSearchRule().applyRule(SearchRule.NULL_QUERY, entry)) {
+                if (contains(entry)) {
                     String oldContent = entry
                             .getField(searchField);
                     removeMatches(entry);
@@ -249,17 +249,6 @@ public boolean equals(Object o) {
                 && (getHierarchicalContext() == other.getHierarchicalContext());
     }
 
-    /*
-     * (non-Javadoc)
-     *
-     * @see net.sf.jabref.groups.structure.AbstractGroup#contains(java.util.Map,
-     *      net.sf.jabref.BibEntry)
-     */
-    @Override
-    public boolean contains(String query, BibEntry entry) {
-        return contains(entry);
-    }
-
     @Override
     public boolean contains(BibEntry entry) {
         if (!entry.hasField(searchField)) {

diff --git a/src/main/java/net/sf/jabref/groups/structure/SearchGroup.java b/src/main/java/net/sf/jabref/groups/structure/SearchGroup.java
@@ -15,6 +15,7 @@
 */
 package net.sf.jabref.groups.structure;
 
+import net.sf.jabref.logic.search.SearchQuery;
 import net.sf.jabref.model.database.BibDatabase;
 import net.sf.jabref.model.entry.BibEntry;
 import net.sf.jabref.Globals;
@@ -42,11 +43,7 @@ public class SearchGroup extends AbstractGroup {
 
     public static final String ID = "SearchGroup:";
 
-    private final String searchExpression;
-    private final boolean caseSensitive;
-    private final boolean regExp;
-
-    private final SearchRule searchRule;
+    private final SearchQuery query;
 
     private static final Log LOGGER = LogFactory.getLog(SearchGroup.class);
 
@@ -56,11 +53,8 @@ public class SearchGroup extends AbstractGroup {
      */
     public SearchGroup(String name, String searchExpression, boolean caseSensitive, boolean regExp, GroupHierarchyType context) {
         super(name, context);
-        this.searchExpression = searchExpression;
-        this.caseSensitive = caseSensitive;
-        this.regExp = regExp;
 
-        this.searchRule = SearchRules.getSearchRuleByQuery(searchExpression, caseSensitive, regExp);
+        this.query = new SearchQuery(searchExpression, caseSensitive, regExp);
     }
 
     /**
@@ -120,7 +114,7 @@ public String getTypeId() {
      */
     @Override
     public SearchRule getSearchRule() {
-        return this.searchRule;
+        return this.query.rule;
     }
 
     /**
@@ -131,13 +125,13 @@ public SearchRule getSearchRule() {
     public String toString() {
         return SearchGroup.ID + StringUtil.quote(name, AbstractGroup.SEPARATOR, AbstractGroup.QUOTE_CHAR) + AbstractGroup.SEPARATOR
                 + context.ordinal() + AbstractGroup.SEPARATOR
-                + StringUtil.quote(searchExpression, AbstractGroup.SEPARATOR, AbstractGroup.QUOTE_CHAR)
-                + AbstractGroup.SEPARATOR + StringUtil.booleanToBinaryString(caseSensitive) + AbstractGroup.SEPARATOR
-                + StringUtil.booleanToBinaryString(regExp) + AbstractGroup.SEPARATOR;
+                + StringUtil.quote(getSearchExpression(), AbstractGroup.SEPARATOR, AbstractGroup.QUOTE_CHAR)
+                + AbstractGroup.SEPARATOR + StringUtil.booleanToBinaryString(isCaseSensitive()) + AbstractGroup.SEPARATOR
+                + StringUtil.booleanToBinaryString(isRegExp()) + AbstractGroup.SEPARATOR;
     }
 
     public String getSearchExpression() {
-        return searchExpression;
+        return this.query.query;
     }
 
     @Override
@@ -169,33 +163,22 @@ public boolean equals(Object o) {
         }
         SearchGroup other = (SearchGroup) o;
         return name.equals(other.name)
-                && searchExpression.equals(other.searchExpression)
-                && (caseSensitive == other.caseSensitive)
-                && (regExp == other.regExp)
+                && this.getSearchExpression().equals(other.getSearchExpression())
+                && (this.isCaseSensitive() == other.isCaseSensitive())
+                && (isRegExp() == other.isRegExp())
                 && (getHierarchicalContext() == other.getHierarchicalContext());
     }
 
-    /*
-     * (non-Javadoc)
-     *
-     * @see net.sf.jabref.groups.structure.AbstractGroup#contains(java.util.Map,
-     *      net.sf.jabref.BibEntry)
-     */
-    @Override
-    public boolean contains(String searchOptions, BibEntry entry) {
-        return getSearchRule().applyRule(searchOptions, entry);
-    }
-
     @Override
     public boolean contains(BibEntry entry) {
-        return contains(SearchRule.DUMMY_QUERY, entry);
+        return this.query.isMatch(entry);
     }
 
     @Override
     public AbstractGroup deepCopy() {
         try {
-            return new SearchGroup(name, searchExpression, caseSensitive,
-                    regExp, context);
+            return new SearchGroup(getName(), getSearchExpression(), isCaseSensitive(),
+                    isRegExp(), getHierarchicalContext());
         } catch (Throwable t) {
             // this should never happen, because the constructor obviously
             // succeeded in creating _this_ instance!
@@ -206,11 +189,11 @@ public AbstractGroup deepCopy() {
     }
 
     public boolean isCaseSensitive() {
-        return caseSensitive;
+        return this.query.caseSensitive;
     }
 
     public boolean isRegExp() {
-        return regExp;
+        return this.query.regularExpression;
     }
 
     @Override
@@ -220,7 +203,7 @@ public boolean isDynamic() {
 
     @Override
     public String getDescription() {
-        return SearchDescribers.getSearchDescriberFor(searchRule, searchExpression).getDescription();
+        return this.query.description;
     }
 
     @Override
@@ -237,7 +220,7 @@ public String getShortDescription() {
         sb.append(" (");
         sb.append(Localization.lang("search expression"));
         sb.append(" <b>").
-                append(StringUtil.quoteForHTML(searchExpression)).append("</b>)");
+                append(StringUtil.quoteForHTML(getSearchExpression())).append("</b>)");
         switch (getHierarchicalContext()) {
         case INCLUDING:
             sb.append(", ").append(Localization.lang("includes subgroups"));

diff --git a/src/main/java/net/sf/jabref/logic/search/SearchQuery.java b/src/main/java/net/sf/jabref/logic/search/SearchQuery.java
@@ -11,7 +11,7 @@ public class SearchQuery {
     public final String query;
     public final boolean caseSensitive;
     public final boolean regularExpression;
-    private final SearchRule rule;
+    public final SearchRule rule;
     public final String description;
 
     public SearchQuery(String query, boolean caseSensitive, boolean regularExpression) {

diff --git a/src/main/java/net/sf/jabref/logic/search/SearchRule.java b/src/main/java/net/sf/jabref/logic/search/SearchRule.java
@@ -37,8 +37,6 @@ public interface SearchRule {
      * As I am not sure whether null could be substituted by "dummy" I leave everything as is.
      */
     String DUMMY_QUERY = "dummy";
-    String NULL_QUERY = null;
-
 
     boolean applyRule(String query, BibEntry bibEntry);
 

diff --git a/src/test/java/net/sf/jabref/groups/structure/SearchGroupTest.java b/src/test/java/net/sf/jabref/groups/structure/SearchGroupTest.java
@@ -1,11 +1,25 @@
 package net.sf.jabref.groups.structure;
 
+import net.sf.jabref.model.entry.BibEntry;
 import org.junit.Test;
 
 import static org.junit.Assert.*;
 
 public class SearchGroupTest {
 
+    @Test
+    public void testContains() {
+        SearchGroup group = new SearchGroup("myExplicitGroup", "review",
+                true, true, GroupHierarchyType.INDEPENDENT);
+        assertEquals("SearchGroup:myExplicitGroup;0;review;1;1;", group.toString());
+
+        BibEntry entry = new BibEntry();
+        assertFalse(group.contains(entry));
+
+        entry.addKeyword("review");
+        assertTrue(group.contains(entry));
+    }
+
     @Test
     public void testToStringSimple() {
         SearchGroup group = new SearchGroup("myExplicitGroup", "author=harrer",