Merge lp:~zeitgeist/zeitgeist/find_events into lp:zeitgeist/0.1

[Siegfried Gevatter]

[Siegfried Gevatter (rainct)]

FindEventIds is great for complex use cases, especially interfaces involving pagination (as we discussed in Bolzano), but for simple queries or uses like in the GNOME Activity Journal, where all data is requested at once (whatever the number of events there are), it's really only in the way, by forcing an additional SQL query and D-Bus request.

Considering that SQLite and D-Bus are what take most of the time in a request, we should avoid those as much as possible. This branch implements the few necessary changes in engine/ to implement this; additions to client.py can follow once it's merged.

[Mikkel Kamstrup Erlandsen (kamstrup)]

I am ok with this in theory, but I have a few comments regarding the code:

1. Can you please fully document FindEvents() with Sphinx markup?

2. I don't like calling our method find_eventids() and then returning Event instances... And using a method argument to control the return type seems non-standard to me. The engine is semi-public API because we have extensions so I think we should strive to keep it clean. I suggest we have to public methods find_events() and find_event_ids().

3. The docs for FindEventIds() should explain its purpose (large result sets - keeping ordering) and cross ref FIndEvents()

4. Can we have the ZeitgeistClient methods implemented and documented before the merge also?

review: Needs Fixing (code review)

[lp:~zeitgeist/zeitgeist/find_events]

1277. By Siegfried Gevatter on 2010-01-12

Expose FindEvents in the Python module.

1278. By Siegfried Gevatter on 2010-01-12

Update FindEvents to use the new engine methods instead of return_events=True.

1279. By Siegfried Gevatter on 2010-01-12

s/ids_reply_handler/events_reply_handler/ where appropriate

1280. By Siegfried Gevatter on 2010-01-12

Fix out_signature of FindEvents.

1281. By Siegfried Gevatter on 2010-01-12

More fixing: Simplify find_events results before sending
them over D-Bus.

[Siegfried Gevatter (rainct)]

I've fixed your concerns and pushed into lp:zeitgeist. Please tell me if there's anything else you can spot.

I've also updated the GNOME Activity Journal to make use of this new method.

=== modified file '_zeitgeist/engine/main.py'
--- _zeitgeist/engine/main.py	2010-01-12 17:12:47 +0000
+++ _zeitgeist/engine/main.py	2010-01-12 21:55:18 +0000
@@ -3,7 +3,7 @@
 # Zeitgeist
 #
 # Copyright © 2009 Mikkel Kamstrup Erlandsen <[e-mail]>
-# Copyright © 2009 Siegfried-Angel Gevatter Pujals <[e-mail]>
+# Copyright © 2009-2010 Siegfried-Angel Gevatter Pujals <[e-mail]>
 # Copyright © 2009 Seif Lotfy <[e-mail]>
 # Copyright © 2009 Markus Korn <[e-mail]>
 # Copyright © 2009 Alexander Gabriel <[e-mail]>
@@ -552,12 +552,16 @@
 		
 		return where
 	
-	def find_eventids(self, time_range, event_templates, storage_state,
-		max_events, order, return_events=False):
+	def _find_events(self, return_mode, time_range, event_templates,
+		storage_state, max_events, order):
 		"""
 		Accepts 'event_templates' as either a real list of Events or as
 		a list of tuples (event_data,subject_data) as we do in the
-		DBus API
+		DBus API.
+		
+		Return modes:
+		 - 0: IDs.
+		 - 1: Events.
 		"""
 		
 		t = time.time()
@@ -567,7 +571,7 @@
 		if not where.may_have_results():
 			return []
 		
-		if not return_events:
+		if return_mode == 0:
 			sql = "SELECT DISTINCT id FROM event_view"
 		else:
 			sql = "SELECT * FROM event_view"
@@ -587,13 +591,19 @@
 		
 		result = self._cursor.execute(sql, where.arguments).fetchall()
 		
-		if return_events:
+		if return_mode == 1:
 			return self.get_events(rows=result)
 		result = [row[0] for row in result]
 		
 		log.debug("Fetched %d event IDs in %fs" % (len(result), time.time()- t))
 		return result
 	
+	def find_eventids(self, *args):
+		return self._find_events(0, *args)
+	
+	def find_events(self, *args):
+		return self._find_events(1, *args)
+	
 	def find_related_uris(self, timerange, event_templates, result_event_templates,
 		result_storage_state):
 		"""

=== modified file '_zeitgeist/engine/remote.py'
--- _zeitgeist/engine/remote.py	2010-01-12 17:23:20 +0000
+++ _zeitgeist/engine/remote.py	2010-01-12 21:55:18 +0000
@@ -2,7 +2,7 @@
 
 # Zeitgeist
 #
-# Copyright © 2009 Siegfried-Angel Gevatter Pujals <[e-mail]>
+# Copyright © 2009-2010 Siegfried-Angel Gevatter Pujals <[e-mail]>
 # Copyright © 2009 Mikkel Kamstrup Erlandsen <[e-mail]>
 #
 # This program is free software: you can redistribute it and/or modify
@@ -53,31 +53,34 @@
 		self._mainloop = mainloop
 		self._notifications = MonitorManager()
 	
+	# Private methods
+	
+	def _make_events_sendable(self, events):
+		for event in events:
+			if event is not None:
+				event._make_dbus_sendable()
+		return [NULL_EVENT if event is None else event for event in events]
+	
 	# Reading stuff
 	
 	@dbus.service.method(constants.DBUS_INTERFACE,
 						in_signature="au", out_signature="a("+constants.SIG_EVENT+")")
 	def GetEvents(self, event_ids):
-		"""Get full event data for a set of event ids
+		"""Get full event data for a set of event IDs
 		
 		Each event which is not found in the event log is represented
 		by the `NULL_EVENT` struct in the resulting array.
 		
-		:param event_ids: An array of event ids. Fx. obtained by calling
+		:param event_ids: An array of event IDs. Fx. obtained by calling
 		    :meth:`FindEventIds`
 		:type event_ids: Array of unsigned 32 bit integers.
 		    DBus signature au
-		:returns: Full event data for all the requested ids. The
+		:returns: Full event data for all the requested IDs. The
 		   event data can be conveniently converted into a list of
 		   :class:`Event` instances by calling *events = map(Event.new_for_struct, result)*
 		:rtype: A list of serialized events. DBus signature a(asaasay).
 		"""
-		events = _engine.get_events(event_ids)
-		for event in events:
-			if event is not None:
-				event._make_dbus_sendable()
-		events = [NULL_EVENT if event is None else event for event in events]
-		return events
+		return self._make_events_sendable(_engine.get_events(event_ids))
 	
 	@dbus.service.method(constants.DBUS_INTERFACE,
 						in_signature="(xx)a("+constants.SIG_EVENT+")a("+constants.SIG_EVENT+")u",
@@ -123,11 +126,14 @@
 			result_event_templates, storage_state)
 	
 	@dbus.service.method(constants.DBUS_INTERFACE,
-						in_signature="(xx)a("+constants.SIG_EVENT+")uuu", out_signature="au")
+						in_signature="(xx)a("+constants.SIG_EVENT+")uuu",
+						out_signature="au")
 	def FindEventIds(self, time_range, event_templates, storage_state,
 			num_events, result_type):
-		"""Search for events matching a given set of templates and return theids of matching events.
-		Use :meth:`GetEvents` passing in the returned ids to look up
+		"""Search for events matching a given set of templates and return
+		the IDs of matching events.
+		
+		Use :meth:`GetEvents` passing in the returned IDs to look up
 		the full event data.
 		
 		The matching is done where unset fields in the templates
@@ -135,52 +141,101 @@
 		subject then events will match the template if any one of their
 		subjects match any one of the subject templates.
 		
-		:param time_range: two timestamps defining the timerange for
-		    the query. When using the Python bindings for Zeitgeist you
-		    may pass a :class:`TimeRange <zeitgeist.datamodel.TimeRange>`
-		    instance directly to this method
-		:type time_range: tuple of 64 bit integers. DBus signature (xx)
-		:param event_templates: An array of event templates which the
-		    returned events should match at least one of.
-		    When using the Python bindings for Zeitgeist you may pass
-		    a list of  :class:`Event <zeitgeist.datamodel.Event>`
-		    instances directly to this method.
-		:type event_templates: array of events. DBus signature a(asaasay)
-		:param storage_state: whether the item is currently known to be available. The list of possible values is enumerated in :class:`StorageState <zeitgeist.datamodel.StorageState>` class
-		:type storage_state: unsigned integer
-		:param num_events: maximal amount of returned events
-		:type num_events: unsigned integer
-		:param order: unsigned integer representing a :class:`result type <zeitgeist.datamodel.ResultType>`
-		:type order: unsigned integer
-		:returns: An array containing the ids of all matching events,
+		This method is intended for queries potentially returning a
+		large result set. It is especially useful in cases where only
+		a portion of the results are to be displayed at the same time
+		(eg., by using paging or dynamic scrollbars), as by holding a
+		list of IDs you keep a stable ordering and you can ask for the
+		details associated to them in hunks, when you need them. For queries
+		yielding a small amount of results, or where you need the information
+		about all results at once no matter how many of them there are,
+		see :meth:`FindEvents`.
+		
+		:param time_range: two timestamps defining the timerange for
+		    the query. When using the Python bindings for Zeitgeist you
+		    may pass a :class:`TimeRange <zeitgeist.datamodel.TimeRange>`
+		    instance directly to this method
+		:type time_range: tuple of 64 bit integers. DBus signature (xx)
+		:param event_templates: An array of event templates which the
+		    returned events should match at least one of.
+		    When using the Python bindings for Zeitgeist you may pass
+		    a list of  :class:`Event <zeitgeist.datamodel.Event>`
+		    instances directly to this method.
+		:type event_templates: array of events. DBus signature a(asaasay)
+		:param storage_state: whether the item is currently known to be
+		    available. The list of possible values is enumerated in
+		    :class:`StorageState <zeitgeist.datamodel.StorageState>` class
+		:type storage_state: unsigned integer
+		:param num_events: maximal amount of returned events
+		:type num_events: unsigned integer
+		:param order: unsigned integer representing
+		    a :class:`result type <zeitgeist.datamodel.ResultType>`
+		:type order: unsigned integer
+		:returns: Full event data for all the requested IDs, up to a maximum
+		    of *num_events* events, sorted and grouped as defined by the
+		    *result_type* parameter. The event data can be conveniently
+		    converted into a list of :class:`Event` instances by calling
+		    *events = map(Event.new_for_struct, result)*
+		:rtype: A list of serialized events. DBus signature a(asaasay).
+		"""
+		time_range = TimeRange(time_range[0], time_range[1])
+		event_templates = map(Event, event_templates)
+		return _engine.find_eventids(time_range, event_templates, storage_state,
+			num_events, result_type)
+
+	@dbus.service.method(constants.DBUS_INTERFACE,
+						in_signature="(xx)a("+constants.SIG_EVENT+")uuu",
+						out_signature="a("+constants.SIG_EVENT+")")
+	def FindEvents(self, time_range, event_templates, storage_state,
+			num_events, result_type):
+		"""Get events matching a given set of templates.
+		
+		The matching is done where unset fields in the templates
+		are treated as wildcards. If a template has more than one
+		subject then events will match the template if any one of their
+		subjects match any one of the subject templates.
+		
+		In case you need to do a query yielding a large (or unpredictable)
+		result set and you only want to show some of the results at the
+		same time (eg., by paging them), use :meth:`FindEventIds`.
+		
+		:param time_range: two timestamps defining the timerange for
+		    the query. When using the Python bindings for Zeitgeist you
+		    may pass a :class:`TimeRange <zeitgeist.datamodel.TimeRange>`
+		    instance directly to this method
+		:type time_range: tuple of 64 bit integers. DBus signature (xx)
+		:param event_templates: An array of event templates which the
+		    returned events should match at least one of.
+		    When using the Python bindings for Zeitgeist you may pass
+		    a list of  :class:`Event <zeitgeist.datamodel.Event>`
+		    instances directly to this method.
+		:type event_templates: array of events. DBus signature a(asaasay)
+		:param storage_state: whether the item is currently known to be
+		    available. The list of possible values is enumerated in
+		    :class:`StorageState <zeitgeist.datamodel.StorageState>` class
+		:type storage_state: unsigned integer
+		:param num_events: maximal amount of returned events
+		:type num_events: unsigned integer
+		:param order: unsigned integer representing
+		    a :class:`result type <zeitgeist.datamodel.ResultType>`
+		:type order: unsigned integer
+		:returns: An array containing the IDs of all matching events,
 		    up to a maximum of *num_events* events. Sorted and grouped
 		    as defined by the *result_type* parameter.
 		:rtype: Array of unsigned 32 bit integers
 		"""
 		time_range = TimeRange(time_range[0], time_range[1])
 		event_templates = map(Event, event_templates)
-		return _engine.find_eventids(time_range, event_templates, storage_state, num_events, result_type)
-
-	@dbus.service.method(constants.DBUS_INTERFACE,
-						in_signature="(xx)a("+constants.SIG_EVENT+")uuu", out_signature="au")
-	def FindEvents(self, time_range, event_templates, storage_state,
-			num_events, result_type):
-		"""Shorthand for GetEvents(FindEventIds()).
-		
-		Using this function is more efficient, if you don't need to deal with
-		large amounts of data in a paginated UI.
-		"""
-		time_range = TimeRange(time_range[0], time_range[1])
-		event_templates = map(Event, event_templates)
-		return _engine.find_eventids(time_range, event_templates, storage_state,
-			num_events, result_type, return_events=True)
+		return self._make_events_sendable(_engine.find_events(time_range,
+			event_templates, storage_state, num_events, result_type))
 
 	# Writing stuff
 	
 	@dbus.service.method(constants.DBUS_INTERFACE,
 						in_signature="a("+constants.SIG_EVENT+")", out_signature="au")
 	def InsertEvents(self, events):
-		"""Inserts events into the log. Returns an array containing the ids of the inserted events
+		"""Inserts events into the log. Returns an array containing the IDs
+		of the inserted events
 		
 		Each event which failed to be inserted into the log (either by
 		being blocked or because of an error) will be represented by `0`
@@ -197,9 +252,9 @@
 		    If you are using the Python bindings you may pass
 		    :class:`Event <zeitgeist.datamodel.Event>` instances
 		    directly to this method
-		:returns: An array containing the event ids of the inserted
+		:returns: An array containing the event IDs of the inserted
 		    events. In case any of the events where already logged,
-		    the id of the existing event will be returned. `0` as id
+		    the ID of the existing event will be returned. `0` as ID
 		    indicates a failed insert into the log.
 		:rtype: Array of unsigned 32 bits integers. DBus signature au.
 		"""
@@ -283,7 +338,9 @@
 	@dbus.service.method(constants.DBUS_INTERFACE,
 			in_signature="o(xx)a("+constants.SIG_EVENT+")", sender_keyword="owner")
 	def InstallMonitor(self, monitor_path, time_range, event_templates, owner=None):
-		"""Register a client side monitor object to receive callbacks when events matching *time_range* and *event_templates* are inserted or deleted.
+		"""Register a client side monitor object to receive callbacks when
+		events matching *time_range* and *event_templates* are inserted or
+		deleted.
 		
 		The monitor object must implement the interface :ref:`org.gnome.zeitgeist.Monitor <org_gnome_zeitgeist_Monitor>`
 		

=== modified file 'test/engine-test.py'
--- test/engine-test.py	2010-01-12 17:12:47 +0000
+++ test/engine-test.py	2010-01-12 21:55:18 +0000
@@ -273,8 +273,8 @@
 		import_events("test/data/five_events.js", self.engine)
 		event_template = Event.new_for_values(interpretation="stfu:OpenEvent",
 		    subjects=[Subject()])
-		events = self.engine.find_eventids((0, 0), [event_template],
-		    StorageState.Any, 0, 1, return_events=True)
+		events = self.engine.find_events((0, 0), [event_template],
+		    StorageState.Any, 0, 1)
 		self.assertEquals(2, len(events))
 		for event in events:
 			self.assertEqual(event.interpretation, "stfu:OpenEvent")

=== modified file 'zeitgeist/client.py'
--- zeitgeist/client.py	2010-01-10 21:10:16 +0000
+++ zeitgeist/client.py	2010-01-12 21:55:18 +0000
@@ -395,6 +395,16 @@
 		
 		The actual :class:`Events` can be looked up via the
 		:meth:`get_events()` method.
+		
+		This method is intended for queries potentially returning a
+		large result set. It is especially useful in cases where only
+		a portion of the results are to be displayed at the same time
+		(eg., by using paging or dynamic scrollbars), as by holding a
+		list of IDs you keep a stable ordering, and you can ask for
+		the details associated to them in hunks, when you need them. For
+		queries with a small amount of results, or where you need the
+		information about all results at once no matter how many of them
+		there are, see :meth:`find_events_for_templates`.
 		 
 		In case of errors a message will be printed on stderr, and
 		an empty result passed to ids_reply_handler.
@@ -444,28 +454,8 @@
 	def find_event_ids_for_template (self, event_template, ids_reply_handler,
 		**kwargs):
 		"""
-		Send a query matching a single Event-template to the
-		Zeitgeist event log. If the event template has more
-		than one subject the query will match if any one of the subject
-		templates match.
-		
-		The query will be done via an asynchronous DBus call and
-		this method will return immediately. The return value
-		will be passed to 'ids_reply_handler' as a list
-		of integer event ids. This list must be the sole argument for
-		the callback.
-		
-		The actual :class:`Events` can be looked up via the
-		:meth:`get_events` method.
-		 
-		In case of errors a message will be printed on stderr, and
-		an empty result passed to *ids_reply_handler*.
-		To override this default set the *error_handler* named argument
-		to a callable that takes a single exception as its sole
-		argument.
-		
-		In order to use this method there needs to be a mainloop
-		runnning. Both Qt and GLib mainloops are supported.
+		Alias for :meth:`find_event_ids_for_templates`, for use when only
+		one template is needed.
 		"""
 		self.find_event_ids_for_templates([event_template],
 						ids_reply_handler,
@@ -473,28 +463,12 @@
 	
 	def find_event_ids_for_values(self, ids_reply_handler, **kwargs):
 		"""
-		Send a query for events matching the keyword arguments passed
-		to this function. The allowed keywords are the same as the ones
-		allowed by
+		Alias for :meth:`find_event_ids_for_templates`, for when only
+		one template is needed. Instead of taking an already created
+		template, like :meth:`find_event_ids_for_template`, this method
+		will construct the template from the parameters it gets. The
+		allowed keywords are the same as the ones allowed by
 		:meth:`Event.new_for_values() <zeitgeist.datamodel.Event.new_for_values>`.
-		
-		The query will be done via an asynchronous DBus call and
-		this method will return immediately. The return value
-		will be passed to *ids_reply_handler* as a list
-		of integer event ids. This list must be the sole argument for
-		the callback.
-		
-		The actual :class:`Events` can be looked up via the
-		:meth:`get_events` method.
-		
-		In case of errors a message will be printed on stderr, and
-		an empty result passed to *ids_reply_handler*.
-		To override this default set the *error_handler* named argument
-		to a callable that takes a single exception as its sole
-		argument.
-		
-		In order to use this method there needs to be a mainloop
-		runnning. Both Qt and GLib mainloops are supported.
 		"""
 		ev = Event.new_for_values(**kwargs)
 		
@@ -502,6 +476,104 @@
 						ids_reply_handler,
 						**kwargs)
 	
+	def find_events_for_templates (self,
+					event_templates,
+					events_reply_handler,
+					timerange = None,
+					storage_state = StorageState.Any,
+					num_events = 20,
+					result_type = ResultType.MostRecentEvents,
+					error_handler=None):
+		"""
+		Send a query matching a collection of
+		:class:`Event <zeitgeist.datamodel.Event>` templates to the
+		Zeitgeist event log. The query will match if an event matches
+		any of the templates. If an event template has more
+		than one subject the query will match if any one of the subject
+		templates match.
+		
+		The query will be done via an asynchronous DBus call and
+		this method will return immediately. The return value
+		will be passed to 'events_reply_handler' as a list
+		of :class:`Event`s. This list must be the sole argument for
+		the callback.
+		
+		If you need to do a query yielding a large (or unpredictable)
+		result set and you only want to show some of the results at the
+		same time (eg., by paging them), consider using
+		:meth:`find_event_ids_for_templates`.
+		 
+		In case of errors a message will be printed on stderr, and
+		an empty result passed to events_reply_handler.
+		To override this default set the error_handler named argument
+		to a callable that takes a single exception as its sole
+		argument.
+		
+		In order to use this method there needs to be a mainloop
+		runnning. Both Qt and GLib mainloops are supported.
+		
+		:param event_templates: List or tuple of
+		    :class:`Event <zeitgeist.datamodel.Event>` instances
+		:param events_reply_handler: Callable taking a list of integers
+		:param timerange: A
+		    :class:`TimeRange <zeitgeist.datamodel.TimeRange>` instance
+		    that the events must have occured within. Defaults to
+		    :meth:`TimeRange.until_now()`.
+		:param storage_state: A value from the
+		    :class:`StorageState <zeitgeist.datamodel.StorageState>`
+		    enumeration. Defaults to :const:`StorageState.Any`
+		:param num_events: The number of events to return; default is 20
+		:param result_type: A value from the
+		    :class:`ResultType <zeitgeist.datamodel.ResultType>`
+		    enumeration. Defaults to ResultType.MostRecentEvent
+		:param error_handler: Callback to catch error messages.
+		        Read about the default behaviour above
+		"""
+		self._check_list_or_tuple(event_templates)
+		self._check_members(event_templates, Event)
+		
+		if not callable(events_reply_handler):
+			raise TypeError(
+				"Reply handler not callable, found %s" % events_reply_handler)
+		
+		if timerange is None:
+			timerange = TimeRange.until_now()
+		
+		self._iface.FindEvents(timerange,
+					event_templates,
+					storage_state,
+					num_events,
+					result_type,
+					reply_handler=lambda raw: events_reply_handler(
+						map(Event.new_for_struct, raw)),
+					error_handler=self._safe_error_handler(error_handler,
+						events_reply_handler, []))
+	
+	def find_events_for_template (self, event_template, events_reply_handler,
+		**kwargs):
+		"""
+		Alias for :meth:`find_events_for_templates`, for use when only
+		one template is needed.
+		"""
+		self.find_event_ids_for_templates([event_template],
+						events_reply_handler,
+						**kwargs)
+	
+	def find_events_for_values(self, events_reply_handler, **kwargs):
+		"""
+		Alias for :meth:`find_events_for_templates`, for when only
+		one template is needed. Instead of taking an already created
+		template, like :meth:`find_event_ids_for_template`, this method
+		will construct the template from the parameters it gets. The
+		allowed keywords are the same as the ones allowed by
+		:meth:`Event.new_for_values() <zeitgeist.datamodel.Event.new_for_values>`.
+		"""
+		ev = Event.new_for_values(**kwargs)
+		
+		self.find_events_for_templates([ev],
+						events_reply_handler,
+						**kwargs)
+	
 	def get_events (self, event_ids, events_reply_handler, error_handler=None):
 		"""
 		Look up a collection of :class:`Events <zeitgeist.datamodel.Event>`