Merge lp:~zeitgeist/zeitgeist/some-fixes into lp:~zeitgeist/zeitgeist/bluebird

[Seif Lotfy]

* moved parse_negation and parse_wildcard into the datamodel for unversal usage (not sure this is the right thing to do though)
* reduced duplication of check_field_match in datamodel by moving it outside the EVent and Subject classes into Zeitgeist namespace
* throw Errors if MOVE_EVENT invalid
* throw Error in find_related_uris
* remove fixed FIXMEs

[Michal Hruby (mhr3)]

[Merge line numbers as here on LP]

[15,29]: I don't like those methods being public in datamodel, it's an implementation detail.
[190,221,247,256]: Missing space before '('
[238]: It doesn't seem to be fixed. :P

Not really sure what to do about the first one, I'd rather have it inside Engine (public static) for the time being.

review: Needs Fixing

[lp:~zeitgeist/zeitgeist/some-fixes]

241. By Seif Lotfy on 2011-09-11

fixed missing space and added a FIXME

[Michal Hruby (mhr3)]

Oh and also [19]: it doesn't need any fix, it'll be working fine.

[lp:~zeitgeist/zeitgeist/some-fixes]

242. By Seif Lotfy on 2011-09-11

remove fixme

243. By Seif Lotfy on 2011-09-12

make bool check_field_match, parse_wildcard and parse_negation private

244. By Seif Lotfy on 2011-09-13

added whitespaces

245. By Seif Lotfy on 2011-09-13

implemented the pre_insert_events logic

=== modified file 'extensions/blacklist.vala'
--- extensions/blacklist.vala	2011-09-05 13:09:08 +0000
+++ extensions/blacklist.vala	2011-09-13 15:48:12 +0000
@@ -98,10 +98,9 @@
         public override void pre_insert_events (GenericArray<Event?> events,
             BusName? sender)
         {
-            // FIXME: do template matching...
-            // for event in events:
-            //     for tmpl in blacklist:
-            //         if event.matches_template(tmpl): event = null
+            for (int i=0; i < events.length; i++)
+                foreach (var tmpl in blacklist.get_values())
+                    if (events[i].matches_template(tmpl)) events[i] = null;
         }
 
         public void add_template (string template_id, Variant event_template)

=== modified file 'src/datamodel.vala'
--- src/datamodel.vala	2011-08-30 13:57:04 +0000
+++ src/datamodel.vala	2011-09-13 15:48:12 +0000
@@ -252,6 +252,68 @@
                              // must be immediately available to the user
         ANY             = 2  // The event subjects may or may not be available
     }
+    
+    // Used by get_where_clause_from_event_templates
+    /**
+     * Check if the value starts with the negation operator. If it does,
+     * remove the operator from the value and return true. Otherwise,
+     * return false.
+     */
+    private bool parse_negation (ref string val)
+    {
+        if (!val.has_prefix ("!"))
+            return false;
+        val = val.substring (1);
+        return true;
+    }
+    
+    // Used by get_where_clause_from_event_templates
+    /**
+     * Check if the value ends with the wildcard character. If it does,
+     * remove the wildcard character from the value and return true.
+     * Otherwise, return false.
+     */
+    private bool parse_wildcard (ref string val)
+    {
+        if (!val.has_suffix ("*"))
+            return false;
+        unowned uint8[] val_data = val.data;
+        val_data[val_data.length-1] = '\0';
+        return true;
+    }
+    
+    private bool check_field_match (string property,
+            string template_property, bool is_symbol = false,
+            bool can_wildcard = false)
+    {
+        var matches = false;
+        var temp_string = template_property;
+        var is_negated = parse_negation(ref temp_string);
+        var temp_property = template_property;
+        if (is_negated)
+            temp_property  = temp_property[1:template_property.length];
+
+        if (temp_property  == "") {
+            return true;
+        }
+        else if (temp_property  == property)
+        {
+            matches = true;
+        }
+        else if (is_symbol &&
+            Symbol.get_all_parents (property).index (temp_property ) > -1)
+        {
+            matches = true;
+        }
+        else if (can_wildcard && parse_wildcard(ref temp_string))
+        {
+            if (property.index_of (
+                    temp_property [0:temp_property.length-1]) > -1)
+                matches = true;
+        }
+        debug ("Checking matches for %s", temp_property);
+        return (is_negated) ? !matches : matches;
+    }
 
     public class Event : Object
     {
@@ -384,42 +446,14 @@
             }
        }
 
-        private bool check_field_match (string event_property,
-            string event_template_property, bool is_symbol = false,
-            bool can_wildcard = false)
-        {
-            var matches = false;
-
-            // FIXME: use common code!
-            var is_negated = (event_template_property[0] == '!');
-            var template_property = event_template_property;
-            if (is_negated)
-                template_property = template_property[1:template_property.length];
-
-            if (template_property == "") {
-                return true;
-            }
-            else if (template_property == event_property)
-            {
-                matches = true;
-            }
-            else if (is_symbol &&
-                Symbol.get_all_parents (event_property).index (template_property) > -1)
-            {
-                matches = true;
-            }
-            else if (can_wildcard && template_property.has_suffix("*")) // FIXME: use common code?
-            {
-                if (event_property.index_of (
-                        template_property[0:template_property.length-1]) > -1)
-                    matches = true;
-            }
-            debug ("Checking matches for %s", event_template_property);
-            return (is_negated) ? !matches : matches;
-        }
+        
 
         public bool matches_event (Event event)
         {
+            /**
+            Interpret *this* as the template an match *event* against it.
+            This method is the dual method of :meth:`matches_template`
+            */
             return event.matches_template (this);
         }
 
@@ -451,7 +485,6 @@
             if (!check_field_match (this.origin, template_event.origin, false, true))
                 return false;
 
-            //FIXME: Check for subject matching
             if (template_event.subjects.length == 0)
                 return true;
 
@@ -544,7 +577,7 @@
             if (subject_props >= 8)
                 current_uri = iter.next_value().get_string ();
             else
-                current_uri = ""; // FIXME: uri?
+                current_uri = uri;
         }
 
         public Variant to_variant ()
@@ -562,37 +595,12 @@
             return vb.end ();
         }
 
-        // FIXME: Why is this duplicated??? delete, delete, delete.
-        private bool check_field_match (string subj_property, string subj_template_property,
-             bool is_symbol = false, bool can_wildcard = false)
-        {
-            var matches = false;
-            var is_negated = (subj_template_property[0] == '!');
-
-            var template_property = subj_template_property;
-            if (is_negated)
-                template_property = template_property[1:template_property.length];
-
-            if (template_property == "")
-                return true;
-            else if (template_property == subj_property)
-                matches = true;
-            else if (is_symbol &&
-                Symbol.get_all_parents (subj_property).index (template_property) > -1)
-                matches = true;
-            else if (can_wildcard && template_property.has_suffix("*"))
-                if (subj_property.index_of(template_property[0:template_property.length-1]) > -1)
-                    matches = true;
-            if (is_negated){
-                matches = !matches;
-            }
-            debug("Checking matches for %s", subj_template_property);
-            return matches;
-        }
-
-        // FIXME: what's the point of this function?
         public bool matches_subject (Subject subject)
         {
+            /**
+            Interpret *this* as the template an match *subject* against it.
+            This method is the dual method of :meth:`matches_template`
+            */
             return subject.matches_template (this);
         }
 

=== modified file 'src/engine.vala'
--- src/engine.vala	2011-09-08 17:49:17 +0000
+++ src/engine.vala	2011-09-13 15:48:12 +0000
@@ -128,8 +128,8 @@
         }
         if (rc != Sqlite.DONE)
         {
-            warning ("Error: %d, %s\n", rc, db.errmsg ());
-            // FIXME: throw some error??
+            throw new EngineError.DATABASE_ERROR ("Error: %d, %s\n", 
+                rc, db.errmsg ());
         }
 
         var results = new GenericArray<Event?> ();
@@ -195,8 +195,6 @@
         //if (!where.may_have_results ())
         //    return new uint32[0];
 
-        // FIXME: IDs: SELECT DISTINCT / events: SELECT
-        // Is the former faster or can we just do the unique'ing on our side?
         string sql;
         if (distinct)
             sql = "SELECT DISTINCT id FROM event_view ";
@@ -377,7 +375,6 @@
         * Only URIs for subjects matching the indicated `result_event_templates`
         * and `result_storage_state` are returned.
         */
-        //FIXME: implement calculation
         if (result_type == ResultType.MOST_RECENT_EVENTS ||
             result_type == ResultType.LEAST_RECENT_EVENTS)
         {
@@ -388,11 +385,10 @@
             uint32[] ids = find_event_ids (time_range, event_templates,
                 storage_state, 0, ResultType.LEAST_RECENT_EVENTS);
 
-            // FIXME: If no results for the event_templates is found raise error
             if (event_templates.length > 0 && ids.length == 0)
             {
-                //throw new EngineError.INVALID_ARGUMENT(
-                //    "No results found for the event_templates");
+                throw new EngineError.INVALID_ARGUMENT (
+                    "No results found for the event_templates");
                 return new string[0];
             }
 
@@ -406,7 +402,6 @@
 
             // From here we create several graphs with the maximum depth of 2
             // and push all the nodes and vertices (events) in one pot together
-            // FIXME: the depth should be adaptable
 
             uint32[] pot = new uint32[ids.length + result_ids.length];
 
@@ -596,13 +591,18 @@
                 if (event.interpretation == ZG.MOVE_EVENT
                     && subject.uri == subject.current_uri)
                 {
-                    //FIXME: throw Error here
+                    throw new EngineError.INVALID_ARGUMENT ("Illegal event: unless
+                        event.interpretation is 'MOVE_EVENT' then subject.uri
+                        and subject.current_uri have to be the same");
                     return 0;
                 }
                 else if (event.interpretation != ZG.MOVE_EVENT
                     && subject.uri != subject.current_uri)
                 {
-                    //FIXME: throw Error here
+                    throw new EngineError.INVALID_ARGUMENT ("Redundant event: 
+                        event.interpretation indicates the uri has been moved 
+                        yet the subject.uri and subject.current_uri are 
+                        identical");
                     return 0;
                 }
 
@@ -634,7 +634,7 @@
 
         // FIXME: Should we add something just like TableLookup but with LRU
         //        for those? Or is embedding the query faster? Needs testing!
-
+        
         int rc;
         unowned Sqlite.Statement insert_stmt = database.event_insertion_stmt;
 
@@ -750,7 +750,6 @@
      */
     public void close ()
     {
-        // FIXME: unload extensions
         database.close();
     }
 
@@ -970,25 +969,6 @@
             return where;
     }
 
-    // FIXME: remove this
-    private static string[] NEGATION_SUPPORTED = {
-        "actor", "current_uri", "interpretation", "manifestation",
-        "mimetype", "origin", "uri" };
-
-    // Used by get_where_clause_from_event_templates
-    /**
-     * Check if the value starts with the negation operator. If it does,
-     * remove the operator from the value and return true. Otherwise,
-     * return false.
-     */
-    protected bool parse_negation (ref string val)
-    {
-        if (!val.has_prefix ("!"))
-            return false;
-        val = val.substring (1); // FIXME: fix for unicode
-        return true;
-    }
-
     // Used by get_where_clause_from_event_templates
     /**
      * If the value starts with the negation operator, throw an
@@ -1005,25 +985,6 @@
         throw new EngineError.INVALID_ARGUMENT (error_message);
     }
 
-    // FIXME: remove this
-    private static string[] WILDCARDS_SUPPORTED = {
-        "actor", "current_uri", "mimetype", "origin", "uri" };
-
-    // Used by get_where_clause_from_event_templates
-    /**
-     * Check if the value ends with the wildcard character. If it does,
-     * remove the wildcard character from the value and return true.
-     * Otherwise, return false.
-     */
-    protected bool parse_wildcard (ref string val)
-    {
-        if (!val.has_suffix ("*"))
-            return false;
-        unowned uint8[] val_data = val.data;
-        val_data[val_data.length-1] = '\0';
-        return true;
-    }
-
     // Used by get_where_clause_from_event_templates
     /**
      * If the value ends with the wildcard character, throw an error.
@@ -1049,10 +1010,20 @@
 
         WhereClause subwhere = new WhereClause(
             WhereClause.Type.OR, negated);
-        foreach (string uri in symbols)
+
+        if (symbols.length() == 1)
         {
             subwhere.add_match_condition (table_name,
-                lookup_table.get_id (uri));
+                lookup_table.get_id (_symbol));
+        }
+        else
+        {
+            string in_sql = "";
+            foreach (string uri in symbols)
+                in_sql += "%i,".printf(lookup_table.get_id (uri));
+            string sql = "%s %s IN (%s)".printf(table_name,
+                (negated) ? "NOT": "", in_sql[0:-1]);
+            subwhere.add(sql);
         }
         return subwhere;
     }