Merge lp:~manishsinha/zeitgeist/new-blacklist-implementation into lp:zeitgeist/0.1

[Manish Sinha (मनीष सिन्हा)]

Implements the new Blacklist API as per http://wiki.zeitgeist-project.com/Zeitgeist_Blacklist

* All methods implemented
* Add signals implemented
* Unit Tests updated accordingly

[Mikkel Kamstrup Erlandsen (kamstrup)]

35 + PUBLIC_METHODS = ["add_blacklist", "get_blacklist"]

Maybe add remove_blacklist here as well?

75 + def add_blacklist(self, event_id, event_template):
85 + def remove_blacklist(self, event_id):
104 + def AddTemplate(self, event_id, event_template):

Should the first arg not be named blacklist_id or?

143 + def RemoveTemplates(self, event_id):

Again, arg should be blacklist_id? Also is it not RemoveTemplate (ie. not plural)

review: Needs Fixing

[lp:~manishsinha/zeitgeist/new-blacklist-implementation]

1703. By Manish Sinha (मनीष सिन्हा) on 2011-04-06

Updated as per Mikkel's review at https://code.launchpad.net/~manishsinha/zeitgeist/new-blacklist-implementation/+merge/56469

* Added remove_template in PUBLIC_METHODS
* Renamed RemoveTemplates to RemoveTemplate
* Changed all the occourance of event_id with blacklist_id

[Mikkel Kamstrup Erlandsen (kamstrup)]

Ok, I am happy now. Top notch work Manish! And thanks for persevering this long haul :-)

(I think we should clarify the DBus protocol in the docstring a bit so other languages can implement it, but that can come later)

review: Approve

[lp:~manishsinha/zeitgeist/new-blacklist-implementation]

1704. By Manish Sinha (मनीष सिन्हा) on 2011-04-06

Updated docstrings for blacklist extension

1705. By Manish Sinha (मनीष सिन्हा) on 2011-04-06

Storing blacklists as Event object instead of simple objects to reduce time taken for each comparison

1706. By Manish Sinha (मनीष सिन्हा) on 2011-04-07

Merged with lp:zeitgeist

1707. By Manish Sinha (मनीष सिन्हा) on 2011-04-07

Merged from lp:zeitgeist

1708. By Manish Sinha (मनीष सिन्हा) on 2011-04-07

Merge fromlp:zeitgeist for 3rd time

[Markus Korn (thekorn)]

I'm approving too, thanks for working on this manish.
With one small note: if some user already has some blacklists defined in the old pickle format this blacklists will simply ignored by this change. But Seif, manish and me agreed on IRC that this is ok, we should just make sure to tell our users about it once this gets released (add a comment about it to NEWS)

review: Approve

=== modified file '_zeitgeist/engine/extensions/blacklist.py'
--- _zeitgeist/engine/extensions/blacklist.py	2011-01-17 15:54:47 +0000
+++ _zeitgeist/engine/extensions/blacklist.py	2011-04-07 08:42:07 +0000
@@ -3,6 +3,7 @@
 # Zeitgeist
 #
 # Copyright © 2009 Mikkel Kamstrup Erlandsen <[e-mail]>
+#           © 2011 Manish Sinha <[e-mail]>
 #
 # This program is free software: you can redistribute it and/or modify
 # it under the terms of the GNU Lesser General Public License as published by
@@ -18,7 +19,7 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import os
-import pickle
+import json
 import dbus
 import dbus.service
 from xdg import BaseDirectory
@@ -30,7 +31,7 @@
 
 log = logging.getLogger("zeitgeist.blacklist")
 
-CONFIG_FILE = os.path.join(constants.DATA_PATH, "blacklist.pickle")
+CONFIG_FILE = os.path.join(constants.DATA_PATH, "blacklist.json")
 BLACKLIST_DBUS_OBJECT_PATH = "/org/gnome/zeitgeist/blacklist"
 BLACKLIST_DBUS_INTERFACE = "org.gnome.zeitgeist.Blacklist"
 
@@ -47,7 +48,7 @@
 	:const:`/org/gnome/zeitgeist/blacklist` under the bus name
 	:const:`org.gnome.zeitgeist.Engine`.
 	"""
-	PUBLIC_METHODS = ["set_blacklist", "get_blacklist"]
+	PUBLIC_METHODS = ["add_blacklist", "remove_blacklist", "get_blacklist"]
 	
 	def __init__ (self, engine):		
 		Extension.__init__(self, engine)
@@ -55,61 +56,119 @@
 		                             BLACKLIST_DBUS_OBJECT_PATH)
 		if os.path.exists(CONFIG_FILE):
 			try:
-				raw_blacklist = pickle.load(file(CONFIG_FILE))
-				self._blacklist = map(Event, raw_blacklist)
+				pcl_file = open(CONFIG_FILE, "r")
+				self._blacklist = {}
+				blacklist = json.load(pcl_file)
+				[self._blacklist.setdefault(key, Event(blacklist[key])) \
+				                    for key in blacklist]
 				log.debug("Loaded blacklist config from %s"
 				          % CONFIG_FILE)
 			except Exception, e:
 				log.warn("Failed to load blacklist config file %s: %s"\
 				         % (CONFIG_FILE, e))
-				self._blacklist = []
+				self._blacklist = {}
 		else:
 			log.debug("No existing blacklist config found")
-			self._blacklist = []
+			self._blacklist = {}
 	
 	def pre_insert_event(self, event, sender):
-		for tmpl in self._blacklist:
+		for tmpl in self._blacklist.itervalues():
 			if event.matches_template(tmpl): return None
 		return event
 	
 	# PUBLIC
-	def set_blacklist(self, event_templates):
-		self._blacklist = event_templates
-		map(Event._make_dbus_sendable, self._blacklist)
-		
-		out = file(CONFIG_FILE, "w")
-		pickle.dump(map(Event.get_plain, self._blacklist), out)		
-		out.close()
-		log.debug("Blacklist updated: %s" % self._blacklist)
+	def add_blacklist(self, blacklist_id, event_template):
+		Event._make_dbus_sendable(event_template)
+		self._blacklist[blacklist_id] = Event(event_template)
+		
+		out = file(CONFIG_FILE, "w")
+		json.dump(self._blacklist, out)		
+		out.close()
+		log.debug("Blacklist added: %s" % self._blacklist)
+	
+	# PUBLIC
+	def remove_blacklist(self, blacklist_id):
+		event_template = self._blacklist[blacklist_id]
+		del self._blacklist[blacklist_id]
+		
+		out = file(CONFIG_FILE, "w")
+		json.dump(self._blacklist, out)		
+		out.close()
+		log.debug("Blacklist deleted: %s" % self._blacklist)
+		
+		return event_template
 	
 	# PUBLIC
 	def get_blacklist(self):
 		return self._blacklist
 	
 	@dbus.service.method(BLACKLIST_DBUS_INTERFACE,
-	                     in_signature="a("+constants.SIG_EVENT+")")
-	def SetBlacklist(self, event_templates):
+	                     in_signature="s("+constants.SIG_EVENT+")")
+	def AddTemplate(self, blacklist_id, event_template):
 		"""
-		Set the blacklist to :const:`event_templates`. Events
+		Set the blacklist to :const:`event_template`. Events
 		matching any these templates will be blocked from insertion
 		into the log. It is still possible to find and look up events
 		matching the blacklist which was inserted before the blacklist
 		banned them.
 		
-		:param event_templates: A list of
+		:param blacklist_id: A string identifier for a blacklist template
+		
+		:param event_template: An object of
 		    :class:`Events <zeitgeist.datamodel.Event>`
 		"""
-		tmp = map(Event, event_templates)
-		self.set_blacklist(tmp)
+		tmp = Event(event_template)
+		self.add_blacklist(blacklist_id, tmp)
+		self.TemplateAdded(blacklist_id, event_template)
 		
 	@dbus.service.method(BLACKLIST_DBUS_INTERFACE,
 	                     in_signature="",
-	                     out_signature="a("+constants.SIG_EVENT+")")
-	def GetBlacklist(self):
+	                     out_signature="a{s("+constants.SIG_EVENT+")}")
+	def GetTemplates(self):
 		"""
-		Get the current blacklist templates.
+		Get the current list of blacklist templates.
 		
-		:returns: A list of
-		    :class:`Events <zeitgeist.datamodel.Event>`
+		:returns: An dictionary of { string ,
+		    :class:`Events <zeitgeist.datamodel.Event>` }
 		"""
 		return self.get_blacklist()
+	
+	@dbus.service.method(BLACKLIST_DBUS_INTERFACE,
+	                     in_signature="s",
+	                     out_signature="")
+	def RemoveTemplate(self, blacklist_id):
+		"""
+		Remove a blacklist template which is identified by the 
+		        blacklist_id provided
+		
+		:param blacklist_id: A string identifier for a blacklist template
+		
+		"""
+		try:
+			event_template = self.remove_blacklist(blacklist_id)
+			self.TemplateRemoved(blacklist_id, event_template)
+		except KeyError:
+			log.debug("Blacklist %s not found " % blacklist_id)
+	
+	@dbus.service.signal(BLACKLIST_DBUS_INTERFACE,
+	                      signature="s("+constants.SIG_EVENT+")")
+	def TemplateAdded(self, blacklist_id, event_template):
+		"""
+		Raised when a template is added
+		
+		:param blacklist_id: The Id of the Blacklist template used for
+		    setting the Blacklist
+		:param event_template: The blacklist template which was set
+		"""
+		pass
+
+	@dbus.service.signal(BLACKLIST_DBUS_INTERFACE,
+	                      signature="s("+constants.SIG_EVENT+")")
+	def TemplateRemoved(self, blacklist_id, event_template):
+		"""
+		Raised when a template is deleted
+		
+		:param blacklist_id: The Id of the Blacklist template which was deleted
+		:param event_template: The blacklist template which was deleted 
+		"""
+		pass

=== modified file 'test/blacklist-test.py'
--- test/blacklist-test.py	2010-09-22 19:21:19 +0000
+++ test/blacklist-test.py	2011-04-07 08:42:07 +0000
@@ -29,18 +29,19 @@
 		self.blacklist = dbus.Interface(obj, "org.gnome.zeitgeist.Blacklist")
 	
 	def testClear(self):
-		self.blacklist.SetBlacklist([])
-		empty = self.blacklist.GetBlacklist()
-		self.assertEquals(empty, [])
+		allTemplates = self.blacklist.GetTemplates()
+		[self.blacklist.RemoveTemplate(key) for key in allTemplates.iterkeys()]
+		newAllTemplates = self.blacklist.GetTemplates()
+		self.assertEquals(len(newAllTemplates), 0)
 		
 	def testSetOne(self):
 		orig = Event.new_for_values(interpretation=Interpretation.ACCESS_EVENT,
 		                            subject_uri="http://nothingtoseehere.gov")
-		self.blacklist.SetBlacklist([orig])
-		result = map(Event, self.blacklist.GetBlacklist())
+		self.blacklist.AddTemplate("Foobar", orig)
+		res = self.blacklist.GetTemplates()
 		
-		self.assertEquals(len(result), 1)
-		result = result[0]
+		self.assertEquals(len(res), 1)
+		result = Event(res["Foobar"])
 		self.assertEquals(result.manifestation, "")
 		self.assertEquals(result.interpretation, Interpretation.ACCESS_EVENT)
 		self.assertEquals(len(result.subjects), 1)
@@ -49,18 +50,17 @@
 	
 	def testApplyBlacklist(self):
 		self.testSetOne()
-		ev = Event()
-		ev.interpretation = Interpretation.ACCESS_EVENT
+		ev = Event.new_for_values(interpretation=Interpretation.ACCESS_EVENT,
+		                            subject_uri="http://nothingtoseehere.gov")
 		ev.manifestation = Manifestation.USER_ACTIVITY
 		ev.actor = "app.//foo.desktop"
-		subj = ev.append_subject()
-		subj.uri = "http://nothingtoseehere.gov"
+
 		inserted_ids = self.insertEventsAndWait([ev])
 		self.assertEquals(1, len(inserted_ids))
-		self.assertEquals(0, inserted_ids[0])
+		self.assertEquals(0, int(inserted_ids[0]))
 		
 		# Now change the event to pass the blacklist
-		subj.uri = "htpp://totallyvaliduri.com"
+		ev.get_subjects()[0].uri = "htpp://totallyvaliduri.com"
 		inserted_ids = self.insertEventsAndWait([ev])
 		self.assertEquals(1, len(inserted_ids))
 		self.assertTrue(0 != inserted_ids[0])
@@ -73,9 +73,10 @@
 		del self.blacklist
 		iface = ZeitgeistDBusInterface()
 		blacklist = iface.get_extension("Blacklist", "blacklist")
-		blacklist.SetBlacklist([])
-		empty = blacklist.GetBlacklist()
-		self.assertEquals(empty, [])
+		allTemplates = blacklist.GetTemplates()
+		[blacklist.RemoveTemplate(key) for key in allTemplates.iterkeys()]
+		newAllTemplates = blacklist.GetTemplates()
+		self.assertEquals(len(newAllTemplates), 0)
 
 if __name__ == "__main__":
 	unittest.main()