[Code-review] review: Guillaume's buddy-view-rebased branch
Guillaume Desmottes
guillaume.desmottes at collabora.co.uk
Wed Jul 2 09:56:27 EDT 2008
Le mercredi 02 juillet 2008 à 14:02 +0100, Dafydd Harries a écrit :
> Nice branch: the weak dict is quite elegant and the tests are thorough. Just a
> few minor things to change.
>
Thanks.
> > 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?
>
changed.
> > @@ -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/?
agree. done.
> > @@ -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:
> ...
done.
> > +
> > + 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.
>
fixed too.
> > @@ -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]
>
Done.
> > @@ -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?
Humm because I forgot this method exist. :)
I moved to view iteration code to add_buddy.
> > 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.
done.
> > + 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.
>
right.
> > + 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.
The current form is OK IMHO.
Thanks for the review.
G.
More information about the Code-review
mailing list