Zeitgeist Framework

Merge lp:~seif/zeitgeist/fix-693861 into lp:zeitgeist/0.1

  1. fix-693861
  2. Merge into 0.8-python
Proposed by Seif Lotfy
Status: Rejected
Rejected by: Markus Korn
Proposed branch: lp:~seif/zeitgeist/fix-693861
Merge into: lp:zeitgeist/0.1
Diff against target: 183 lines (+71/-13)
5 files modified
_zeitgeist/engine/extension.py (+22/-13)
_zeitgeist/engine/remote.py (+3/-0)
test/engine-extension-test.py (+14/-0)
test/remote-test.py (+17/-0)
zeitgeist/client.py (+15/-0)
To merge this branch: bzr merge lp:~seif/zeitgeist/fix-693861
Reviewer Review Type Date Requested Status
Markus Korn Needs Information
Review via email: mp+44683@code.launchpad.net

Description of the change

Added a get_extensions method to the API since it is very useful for client applications to know if any specific extensions are loaded or not

To post a comment you must log in.
Revision history for this message
Markus Korn (thekorn) wrote :

Hi Seif. It's great to see you working on this. Have you considered making this an property and expose this property as a readonly property over the properties interface? IMHO this interface is perfect for this case.

review: Needs Information
Revision history for this message
Seif Lotfy (seif) wrote :

Hi Markus,
In fact I did considered doing it as a property. But seeing that an extensions property is already in place:
---
@property
def extensions(self):
 return self.__extensions
---
which is used to internally in the main to call the extensions and apply the hooks etc...
So the only way get this right is to call the new property something else like: extension_names.
What do you think?

Revision history for this message
Markus Korn (thekorn) wrote :

> Hi Markus,
> In fact I did considered doing it as a property. But seeing that an extensions
> property is already in place:
> ---
> @property
> def extensions(self):
> return self.__extensions
> ---
> which is used to internally in the main to call the extensions and apply the
> hooks etc...
> So the only way get this right is to call the new property something else
> like: extension_names.
> What do you think?

Hmm, I was talking about DBus properties, when I would work on this bug I would do it like: http://paste.ubuntu.com/547489/
(includes tests and client wrapper)

Revision history for this message
Seif Lotfy (seif) wrote :

I love it. However I would like to add a method to main.py:

get_extension_names(self):
    return [unicode(extension.__class__.__name__) for extension in self.engine.extensions]

Lets say I would like to write an extension that depends on another being running. It will take off the load (being developer friendly) of doing:

if "Extension1" in [unicode(extension.__class__.__name__) for extension in self.engine.extensions]:

and let it look like

if "Extension1" in [extension for extension in self.get_extensions(]:

What do you think?

lp:~seif/zeitgeist/fix-693861 updated
1651. By Seif Lotfy

apply markus's patch plus my changes

Revision history for this message
Seif Lotfy (seif) wrote :

I applied your patch with my set of tiny changes for what I proposed in the previous comment. Feel free to criticize

Revision history for this message
Markus Korn (thekorn) wrote :

I think the method to get a list of loaded extension names should be a method of ExtensionCollection. Also we should add a function which gives us the extension name, so we don't have obj.__class__.__name__ in various places (btw. this would make changing the naming convention easier)

I addressed both points at http://paste.ubuntu.com/547526/ (diff against lp:zeitgeist)

Revision history for this message
Seif Lotfy (seif) wrote :

Ok this seems much better thanks alot. Yet I have a question. Why a get_extension_name and not a name property for each extension that returns self.__class__.name
I tried to modify your patch to do so but for some reason make check fails while triggering by hand works like charm. http://paste.ubuntu.com/547533/

Revision history for this message
Markus Korn (thekorn) wrote :

I don't like a name property on each extension, because I think we don't want to loose control over the namings of the extensions. by adding a property the extensions author can simply overwrite it and return random stuff, which migth break on our side (e.g. names including points).

Revision history for this message
Seif Lotfy (seif) wrote :

OK I agree with you again.
Please feel free to push ur patch directly into trunk unless you want someone else's opinion.

lp:~seif/zeitgeist/fix-693861 updated
1652. By Seif Lotfy

apply markus's patch to add a dbus property exposing the extensions loaded by the engine

Revision history for this message
Markus Korn (thekorn) wrote :

Closing this MP since I directly pushed my patch to lp:zeitgeist.

Unmerged revisions

1652. By Seif Lotfy

apply markus's patch to add a dbus property exposing the extensions loaded by the engine

1651. By Seif Lotfy

apply markus's patch plus my changes

1650. By Seif Lotfy

added get_extension method to the DBus and client APIs

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file '_zeitgeist/engine/extension.py'
2--- _zeitgeist/engine/extension.py 2010-12-18 00:08:50 +0000
3+++ _zeitgeist/engine/extension.py 2010-12-25 19:06:29 +0000
4@@ -32,6 +32,17 @@
5 return issubclass(obj, cls)
6 except TypeError:
7 return False
8+
9+def get_extension_name(extension):
10+ """ We are using the name of the Extension-class as extension's unique
11+ name, later we might want to prepend modul or package names
12+ """
13+ if safe_issubclass(extension, Extension):
14+ return extension.__name__
15+ elif isinstance(extension, Extension):
16+ return extension.__class__.__name__
17+ else:
18+ raise TypeError
19
20 class Extension(object):
21 """ Base class for all extensions
22@@ -234,19 +245,20 @@
23 return "%s(%r)" %(self.__class__.__name__, sorted(self.__methods.keys()))
24
25 def load(self, extension):
26- log.debug("Loading extension '%s'" % extension.__name__)
27 if not issubclass(extension, Extension):
28 raise TypeError("Unable to load %r, all extensions must be "
29 "subclasses of %r" % (extension, Extension))
30+ ext_name = get_extension_name(extension)
31+ log.debug("Loading extension '%s'" %ext_name)
32 try:
33 obj = extension(self.__engine)
34 except Exception:
35- log.exception("Failed loading the '%s' extension" % extension.__name__)
36+ log.exception("Failed loading the '%s' extension" %ext_name)
37 return False
38
39 for method in obj.PUBLIC_METHODS:
40 self._register_method(method, getattr(obj, method))
41- self.__extensions[obj.__class__.__name__] = obj
42+ self.__extensions[ext_name] = obj
43
44 def unload(self, extension=None):
45 """
46@@ -260,17 +272,11 @@
47
48 # We need to clone the key list to avoid concurrent
49 # modification of the extension dict
50- for ext_name in list(self.__extensions.iterkeys()):
51- self.unload(self.__extensions[ext_name])
52+ for ext in list(self):
53+ self.unload(ext)
54 else:
55- log.debug("Unloading extension '%s'" \
56- % extension.__class__.__name__)
57- if safe_issubclass(extension, Extension):
58- ext_name = extension.__name__
59- elif isinstance(extension, Extension):
60- ext_name = extension.__class__.__name__
61- else:
62- raise TypeError
63+ ext_name = get_extension_name(extension)
64+ log.debug("Unloading extension '%s'" %ext_name)
65 obj = self.__extensions[ext_name]
66 for method in obj.PUBLIC_METHODS:
67 del self.methods[method]
68@@ -343,3 +349,6 @@
69
70 def __iter__(self):
71 return self.__extensions.itervalues()
72+
73+ def iter_names(self):
74+ return self.__extensions.iterkeys()
75
76=== modified file '_zeitgeist/engine/remote.py'
77--- _zeitgeist/engine/remote.py 2010-11-01 08:25:01 +0000
78+++ _zeitgeist/engine/remote.py 2010-12-25 19:06:29 +0000
79@@ -43,6 +43,7 @@
80 """
81 _dbus_properties = {
82 "version": property(lambda self: (0, 6, 0)),
83+ "extensions": property(lambda self: list(self._engine.extensions.iter_names())),
84 }
85
86 # Initialization
87@@ -348,6 +349,8 @@
88 self._engine.close()
89 if self._mainloop:
90 self._mainloop.quit()
91+ # remove the interface from all busses (in our case from the session bus)
92+ self.remove_from_connection()
93
94 # Properties interface
95
96
97=== modified file 'test/engine-extension-test.py'
98--- test/engine-extension-test.py 2010-09-21 09:17:41 +0000
99+++ test/engine-extension-test.py 2010-12-25 19:06:29 +0000
100@@ -140,6 +140,20 @@
101 self.assertEqual(len(filter(lambda x: x is not None, events)), 1)
102
103
104+class TestExtensionsCollection(_engineTestClass):
105+
106+ def testIterNames(self):
107+ SampleExtension1 = type("SampleExtension1", (Extension,), dict(Extension.__dict__))
108+ SampleExtension2 = type("SampleExtension2", (Extension,), dict(Extension.__dict__))
109+
110+ self.engine.extensions.load(SampleExtension1)
111+ self.assertEquals(list(self.engine.extensions.iter_names()), ["SampleExtension1"])
112+
113+ self.engine.extensions.load(SampleExtension2)
114+ self.assertEquals(
115+ sorted(self.engine.extensions.iter_names()),
116+ ["SampleExtension1", "SampleExtension2"]
117+ )
118
119
120 if __name__ == "__main__":
121
122=== modified file 'test/remote-test.py'
123--- test/remote-test.py 2010-09-22 18:44:16 +0000
124+++ test/remote-test.py 2010-12-25 19:06:29 +0000
125@@ -353,6 +353,23 @@
126 interface.Quit()
127 self.assertEquals(interface._engine.is_closed(), True)
128
129+
130+class ZeitgeistRemotePropertiesTest(testutils.RemoteTestCase):
131+
132+ def __init__(self, methodName):
133+ super(ZeitgeistRemotePropertiesTest, self).__init__(methodName)
134+
135+ def testExtensions(self):
136+ self.assertEquals(
137+ sorted(self.client.get_extensions()),
138+ ["Blacklist", "DataSourceRegistry"]
139+ )
140+ self.assertEquals(
141+ sorted(self.client._iface.extensions()),
142+ ["Blacklist", "DataSourceRegistry"]
143+ )
144+
145+
146 class ZeitgeistDaemonTest(unittest.TestCase):
147
148 def setUp(self):
149
150=== modified file 'zeitgeist/client.py'
151--- zeitgeist/client.py 2010-12-14 22:02:57 +0000
152+++ zeitgeist/client.py 2010-12-25 19:06:29 +0000
153@@ -147,6 +147,14 @@
154 dbus_interface=dbus.PROPERTIES_IFACE)(self.INTERFACE_NAME,
155 "version"))
156
157+ def extensions(self):
158+ """Returns active extensions"""
159+ dbus_interface = self.__shared_state["dbus_interface"]
160+ return dbus_interface._disconnection_safe(lambda:
161+ dbus_interface.proxy.get_dbus_method("Get",
162+ dbus_interface=dbus.PROPERTIES_IFACE)(self.INTERFACE_NAME,
163+ "extensions"))
164+
165 def get_extension(cls, name, path, busname=None):
166 """ Returns an interface to the given extension.
167
168@@ -327,8 +335,15 @@
169 "Reply handler not callable, found %s" % reply_handler)
170 return self._void_reply_handler
171
172+ # Properties
173+
174 def get_version(self):
175 return [int(i) for i in self._iface.version()]
176+
177+ def get_extensions(self):
178+ return [unicode(i) for i in self._iface.extensions()]
179+
180+ # Methods
181
182 def insert_event (self, event, ids_reply_handler=None, error_handler=None):
183 """

Subscribers

People subscribed via source and target branches