[Code-review] review of Daf's view gadget branch

[Dafydd Harries]

Ar 24/06/2008 am 12:57, ysgrifennodd Guillaume Desmottes:
> Review of
> https://dev.laptop.org/git?p=users/daf/gadget;a=shortlog;h=view
> 
> 
> 
> > diff --git a/gadget/component.py b/gadget/component.py
> > index 2d42555..8e16ccc 100644
> > --- a/gadget/component.py
> > +++ b/gadget/component.py
> > @@ -116,8 +116,13 @@ class ActivityViewHandler(object):
> >          self.gadget.send(message)
> >  
> >      def activity_gone(self, activity):
> > -        # FIXME: send remove message
> > -        pass
> > +        message = domish.Element((None, 'message'))
> > +        added = message.addElement((ns.ACTIVITY, 'removed'))
> > +        added['id'] = self.id
> > +        activity_elem = added.addElement((ns.ACTIVITY, 'activity'))
> > +        activity_elem['id'] = str(activity.id)
> > +        activity_elem['room'] = activity.room
> > +        self.gadget.send(message)
> >  
> 
> Humm this message doesn't have any 'to' and 'from' attribute.
> We should set self.contact as 'to' attribute and we currently use
> "gadget.parent.jabberId" to find Gadget's jid.
> 
> Same for activity_found.

Right.

> >      def member_added(self, activity, member):
> >          # FIXME: send add message
> > diff --git a/gadget/roster.py b/gadget/roster.py
> > index ff1ae4b..334699e 100644
> > --- a/gadget/roster.py
> > +++ b/gadget/roster.py
> > @@ -2,7 +2,7 @@ import os.path
> >  import twisted.words.protocols.jabber.jid
> >  from twisted.python.filepath import FilePath
> >  
> > -class GadgetRoster:
> > +class GadgetRoster(object):
> 
> What's the rational of this? (just wondering)

Subclassing "object" makes the class a new-style class. In most cases, it
doesn't make a difference, but old-style classes can make things weird
sometimes to it's better to always use new-style clasess. Here's some
background information:

  http://docs.python.org/ref/node33.html

> >      def __init__(self, path):
> >          # FIXME: maybe we could use SQLite?
> >          self._path = path
> > diff --git a/gadget/test_activity_query.py
> b/gadget/test_activity_query.py
> > index e29847b..0c9c066 100644
> > --- a/gadget/test_activity_query.py
> > +++ b/gadget/test_activity_query.py
> > @@ -1,5 +1,5 @@
> >  
> > -from twisted.internet import defer
> > +from twisted.internet import defer, reactor
> >  from twisted.words.protocols.jabber.client import IQ
> >  
> >  from model import Activity
> > @@ -331,9 +331,9 @@ class ActivityViewByProperties(TestCase):
> >          model = self.service.model
> >          model.activity_update(1, {'color': ('str', 'blue')})
> >  
> > -        self.server.addOnetimeObserver('//message', self.message)
> > +        self.server.addOnetimeObserver('//message', self.message1)
> >  
> > -    def message(self, stanza):
> > +    def message1(self, stanza):
> >          added = stanza.firstChildElement()
> >          assert added.name == 'added'
> >          assert added['id'] == 'query1'
> > @@ -342,6 +342,19 @@ class ActivityViewByProperties(TestCase):
> >          assert activity['id'] == '1'
> >          assert activity['room'] == 'foo'
> >  
> > +        self.server.addOnetimeObserver('//message', self.message2)
> > +        model = self.service.model
> > +        model.activity_remove(1)
> > +
> > +    def message2(self, stanza):
> > +        added = stanza.firstChildElement()
> > +        assert added.name == 'removed'
> > +        assert added['id'] == 'query1'
> > +        activity = added.firstChildElement()
> > +        assert activity.name == 'activity'
> > +        assert activity['id'] == '1'
> > +        assert activity['room'] == 'foo'
> > +
> >          self.done.callback(None)
> >  
> 
> We should check message's to and from attributes.

Right.

> >  class ActivityIqRandomQuery(TestCase):
> > diff --git a/gadget/test_component.py b/gadget/test_component.py
> > index b9ce4c1..80b0b63 100644
> > --- a/gadget/test_component.py
> > +++ b/gadget/test_component.py
> > @@ -120,18 +120,15 @@ class SubscriptionTest(TestCase):
> >      def presence2(self, stanza):
> >          assert stanza['type'] == 'subscribe'
> >  
> > -        #presence = domish.Element((ns.CLIENT, 'presence'))
> > -        #presence['from'] = 'bob at foo.org'
> > -        #presence['to'] = 'gadget.foo.org'
> > -        #presence['type'] = 'subscribed'
> > -        #self.server.send(presence)
> > -        #reactor.callLater(0, self.later)
> > -
> > -        # FIXME: presence2 is called twice if we send this stanza
> > -        self.done.callback(None)
> > +        presence = domish.Element((ns.CLIENT, 'presence'))
> > +        presence['from'] = 'bob at foo.org'
> > +        presence['to'] = 'gadget.foo.org'
> > +        presence['type'] = 'subscribed'
> > +        self.server.send(presence)
> > +        reactor.callLater(0, self.later)
> >  
> >      def later(self):
> > -        assert self.roster.jids == set(['bob at foo.org'])
> > +        assert self.roster == set(['bob at foo.org'])
> >          self.done.callback(None)
> >  
> >  class ProbePresenceTest(TestCase):
> > diff --git a/test.py b/test.py
> > index c83473b..81608c4 100755
> > --- a/test.py
> > +++ b/test.py
> > @@ -54,6 +54,21 @@ if __name__ == '__main__':
> >      # hack!
> >      unittest.TestCase.shortDescription = shortDescription
> >  
> > +    # Patch in fix from
> http://twistedmatrix.com/trac/changeset/23418.
> > +    from twisted.words.xish.utility import EventDispatcher
> > +
> > +    def _addObserver(self, onetime, event, observerfn, priority,
> *args,
> > +            **kwargs):
> > +        if self._dispatchDepth > 0:
> > +            self._updateQueue.append(lambda:
> self._addObserver(onetime, event,
> > +                observerfn, priority, *args, **kwargs))
> > +
> > +        return self._oldAddObserver(onetime, event, observerfn,
> priority,
> > +            *args, **kwargs)
> > +
> > +    EventDispatcher._oldAddObserver = EventDispatcher._addObserver
> > +    EventDispatcher._addObserver = _addObserver
> > +
> 
> 
> Crack but I'll be really happy to see this bug fixed. :)

Seems it's not fixed in Twisted 8.1.0; perhaps in the next version.

-- 
Dafydd