[Code-review] review: Guillaume's buddy-view-rebased branch
Dafydd Harries
dafydd.harries at collabora.co.uk
Wed Jul 2 09:02:22 EDT 2008
Nice branch: the weak dict is quite elegant and the tests are thorough. Just a
few minor things to change.
> diff --git a/gadget/component.py b/gadget/component.py
> index d099472..97ced8a 100644
> --- a/gadget/component.py
> +++ b/gadget/component.py
> @@ -74,10 +74,16 @@ class ActivityViewHandler(object):
> self.contact = contact
> self.id = id
>
> + def _change_message(self):
> + return elem('message', to=self.contact, type='notice',
> + from_=self.gadget.parent.jabberId)(
> + elem('amp', xmlns=ns.AMP)(
> + elem('rule', condition='deliver-at', value='stored',
> + action='error')))
> +
Perhaps _make_change_message() would be more descriptive?
> @@ -118,7 +157,10 @@ class GadgetService(component.Service):
> self.pending_activities = {}
> # muc jid -> Activity
> self.joining_activities = {}
> + # from jid -> (view id -> ActivityViewHandler)
> self.activity_views = {}
> + # from jid -> (view id -> BuddyViewHandler)
> + self.buddy_views = {}
> self.iq_dispatch = {
> (ns.BUDDY, 'query'): self.iq_buddy_query,
> (ns.BUDDY, 'view'): self.iq_buddy_view,
Perhaps s/from jid/owner JID/?
> @@ -236,20 +304,58 @@ class GadgetService(component.Service):
> return result
>
> def iq_buddy_query(self, stanza):
> - return self.reply_to_buddy_query(stanza)
> + try:
> + terms = self.parse_buddy_query(stanza)
> + except ValueError, e:
> + return e.args[0]
> +
> + results = self.execute_buddy_query(terms)
> + return self.reply_to_buddy_query(stanza, results)
>
> def iq_buddy_view(self, stanza):
> + from_ = stanza['from']
> view_node = stanza.firstChildElement()
> + id = view_node.getAttribute('id')
>
> - if view_node.getAttribute('id') is None:
> + if id is None:
> # missing id attribute
> result = make_iq_error(stanza)
> error = result.firstChildElement()
> error.addElement((self.ns, "invalid-property"))
> return result
>
> - # TODO: create view
> - return self.reply_to_buddy_query(stanza)
> + views = self.buddy_views.get(from_, {})
> + if views == {}:
> + self.buddy_views[from_] = views
> + view = views.get(id)
> +
> + if view is not None:
> + result = make_iq_error(stanza)
> + error = result.firstChildElement()
> + error.addElement((self.ns, "already-exists"))
> + return error
I think this would be clearer:
views = self.buddy_views.get(from)
if views is not None and views.get(id) is not None:
...
> +
> + try:
> + terms = self.parse_buddy_query(stanza)
> + except ValueError, e:
> + return e.args[0]
> +
> + assert terms
> + term = terms[0]
> + handler = BuddyViewHandler(self, from_, id)
> +
> + if term[0] == 'search':
> + jid, props = term[1:]
> + # FIXME: Do we want to support jid search in views?
> + view = self.model.buddy_view(sys.maxint, props, handler)
> + elif term[0] == 'random':
> + max = term[1]
> + view = self.model.buddy_view(max, {}, handler)
> + else:
> + assert False
> +
> + views[id] = view
> + return self.reply_to_buddy_query(stanza, view.results)
>
> def parse_activity_query(self, stanza):
> _query = stanza.firstChildElement()
> @@ -385,6 +491,9 @@ class GadgetService(component.Service):
> return result
>
> views = self.activity_views.get(from_, {})
> + if views == {}:
> + self.activity_views[from_] = views
> +
> view = views.get(id)
>
> if view is not None:
And something similar here.
> @@ -417,7 +526,7 @@ class GadgetService(component.Service):
> else:
> assert False
>
> - self.activity_views[(from_, id)] = view
> + views[id] = view
> return self.reply_to_activity_query(stanza, view.results)
>
> def iq_disco_info(self, stanza):
> @@ -455,6 +564,12 @@ class GadgetService(component.Service):
> elif elem.uri == ns.PUBSUB_EVENT and elem.name == 'event':
> self.message_pubsub_notification(stanza, elem)
> return
> + elif elem.uri == ns.ACTIVITY and elem.name == 'close':
> + self.message_close_activity_view(stanza, elem)
> + return
> + elif elem.uri == ns.BUDDY and elem.name == 'close':
> + self.message_close_buddy_view(stanza, elem)
> + return
>
> elif type == 'groupchat':
> for elem in stanza.elem
I've changed this to use a dict in my 'dispatch' branch.
> @@ -462,7 +577,6 @@ class GadgetService(component.Service):
> self.message_activity_properties(stanza, elem)
> return
>
> - # FIXME: handle close query stanza
> log.msg('got unhandled <message>')
>
> def message_muc(self, stanza, x):
> @@ -607,7 +721,16 @@ class GadgetService(component.Service):
> except KeyError:
> pass
>
> - # FIXME: close any open views
> + # remove open views if any
> + try:
> + del self.buddy_views[jid]
> + except KeyError:
> + pass
> +
> + try:
> + del self.activity_views[jid]
> + except KeyError:
> + pass
Better:
if jid in self.buddy_views:
del self.buddy_views[jid]
if jid in self.activity_views:
del self.activity_views[jid]
> @@ -726,3 +847,41 @@ class GadgetService(component.Service):
> return
>
> self.model.activity_update(id, properties)
> +
> + def message_close_activity_view(self, stanza, close):
> + from_ = stanza['from']
> + id = close['id']
> + if not id:
> + log.msg('Ignoring activity view close stanza from %s without ID'
> + % from_)
> + return
> +
> + views = self.activity_views.get(from_, {})
> + if not views.has_key(id):
> + log.msg('Ignoring activity view close stanza from %s, unknown ID %s'
> + % (from_, id))
> + return
> +
> + del views[id]
> +
> + if len(views) == 0:
> + del self.activity_views[from_]
> +
> + def message_close_buddy_view(self, stanza, close):
> + from_ = stanza['from']
> + id = close['id']
> + if not id:
> + log.msg('Ignoring buddy view close stanza from %s without ID'
> + % from_)
> + return
> +
> + views = self.buddy_views.get(from_, {})
> + if not views.has_key(id):
> + log.msg('Ignoring buddy view close stanza from %s, unknown ID %s'
> + % (from_, id))
> + return
> +
> + del views[id]
> +
> + if len(views) == 0:
> + del self.buddy_views[from_]
> diff --git a/gadget/model.py b/gadget/model.py
> index d3c9da7..6fd917d 100644
> --- a/gadget/model.py
> +++ b/gadget/model.py
> @@ -1,4 +1,5 @@
> import random
> +import weakref
>
> class Activity(object):
> __slots__ = ('id', 'room', 'properties', 'members')
> @@ -63,11 +64,35 @@ class Buddy(object):
> self.jid = jid
> self.properties = properties
>
> +class View:
> + def __init__(self, model, size, test, handler):
> + self.model = model
> + self.size = size
> + self._test = test
> + self.handler = handler
> + self.results = []
> +
> + def test(self, obj):
> + return self._test(obj)
> +
> + def __iter__(self):
> + return iter(self.results)
> +
> +class ActivityView (View):
> + pass
> +
> +class BuddyView (View):
> + pass
> +
> class GadgetModel(object):
> def __init__(self):
> self.activities = {}
> self.buddies = {}
> - self.activity_views = []
> + # Store views in a weak dict so they are automatically removed when
> + # the component close them.
> + # We don't use the associated value.
> + self.activity_views = weakref.WeakKeyDictionary()
> + self.buddy_views = weakref.WeakKeyDictionary()
>
> def _property_match(self, obj, properties):
> "Check: for (k, v) in properties, (k, v) is in obj.properties."
> @@ -196,22 +221,8 @@ class GadgetModel(object):
> def test(activity):
> return self._property_match(activity, properties)
>
> - class ActivityView:
> - def __init__(self, model, size, test, handler):
> - self.model = model
> - self.size = size
> - self._test = test
> - self.handler = handler
> - self.results = []
> -
> - def test(self, activity):
> - return self._test(activity)
> -
> - def __iter__(self):
> - return iter(self.results)
> -
> view = ActivityView(self, size, test, handler)
> - self.activity_views.append(view)
> + self.activity_views[view] = True
> results = []
>
> for activity in self.activities.itervalues():
> @@ -232,12 +243,65 @@ class GadgetModel(object):
> self.add_buddy(buddy)
> return buddy
>
> + def buddy_view(self, size, properties, handler):
> + def test(activity):
> + return self._property_match(activity, properties)
> +
> + view = BuddyView(self, size, test, handler)
> + self.buddy_views[view] = True
> + results = []
> +
> + for buddy in self.buddies.itervalues():
> + if len(results) >= view.size:
> + break
> +
> + if view.test(buddy):
> + results.append(buddy)
> +
> + view.results = results
> + return view
> +
> + def buddy_add(self, jid, properties):
> + buddy = Buddy(jid, properties)
> + self.buddies[jid] = buddy
> +
> + for view in self.buddy_views:
> + # FIXME: check view size
> +
> + if view.test(buddy):
> + view.results.append(buddy)
> + view.handler.buddy_added(buddy)
> +
> + return buddy
> +
> +
Why not modify create_buddy to do this?
> def buddy_update(self, jid, properties):
> buddy = self.buddies[jid]
> buddy.properties = properties
>
> + for view in self.buddy_views:
> + in_ = buddy in view.results
> + test = view.test(buddy)
> +
> + if in_:
> + if not test:
> + view.results.remove(buddy)
> + view.handler.buddy_removed(buddy)
> + else:
> + # notify about the change
> + view.handler.buddy_changed(buddy)
> + elif test and not in_:
> + view.results.append(buddy)
> + view.handler.buddy_added(buddy)
> +
> def buddy_remove(self, jid):
> buddy = self.buddies[jid]
> +
> + for view in self.buddy_views:
> + if view.test(buddy):
> + view.results.remove(buddy)
> + view.handler.buddy_removed(buddy)
> +
> del self.buddies[jid]
>
> def buddy_by_id(self, jid):
> diff --git a/gadget/test_component.py b/gadget/test_component.py
> index 7e112b8..62e6b64 100644
> --- a/gadget/test_component.py
> +++ b/gadget/test_component.py
> @@ -710,3 +711,42 @@ class ActivityPropertiesSpoofTest(TestCase):
>
> self.done.callback(None)
>
> +class ViewOfflineTest(TestCase):
> + """test if views are automatically removed when their initator turns
> + offline"""
s/turns/goes/. Also, please capitalise this sentence and add a full stop to
this end.
> + def runTest(self):
> + self.done = defer.Deferred()
> + self.server.addOnetimeObserver('//iq', self.iq)
> + self.server.addOnetimeObserver('//iq', self.iq2)
> +
> + # request a buddy view
> + iq = elem_iq(self.server, 'get',
> + from_='bob at foo.org', to='gadget.bar.org')(
> + elem(ns.BUDDY, 'view', id='view1')(
> + elem('random', max='10')))
I think the second line doesn't need to be indented quite so much; a regular
four-space indent would do.
> + iq.send()
> +
> + def iq(self, stanza):
> + # request an activity view
> + iq = elem_iq(self.server, 'get',
> + from_='bob at foo.org', to='gadget.bar.org')(
> + elem(ns.ACTIVITY, 'view', id='view1')(
> + elem('random', max='10')))
> + iq.send()
> +
> + def iq2(self, stanza):
> + # Bob turns offline
> + presence = domish.Element((ns.CLIENT, 'presence'))
> + presence['from'] = 'bob at foo.org'
> + presence['to'] = 'gadget.bar.org'
> + presence['type'] = 'unavailable'
> + self.server.send(presence)
> +
> + reactor.callLater(0, self.later)
> +
> + def later(self):
> + assert len(self.service.activity_views) == 0
> + assert len(self.model.activity_views) == 0
> + assert len(self.service.buddy_views) == 0
> + assert len(self.model.buddy_views) == 0
> + self.done.callback(None)
> diff --git a/gadget/util.py b/gadget/util.py
> index 0a8c510..801cbdd 100644
> --- a/gadget/util.py
> +++ b/gadget/util.py
> @@ -70,7 +70,10 @@ def elem(a, b=None, **kw):
> elem = _elem((None, a))
>
> for k, v in kw.iteritems():
> - elem[k] = v
> + if k == 'from_':
> + elem['from'] = v
> + else:
> + elem[k] = v
>
> return elem
We could also do:
elem[k.rstrip('_')] = v
But I'm not sure if it's better.
--
Dafydd
More information about the Code-review
mailing list