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

[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.

>      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)


>      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.

>  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. :)

>      for i in range(1, len(sys.argv)):
>          if sys.argv[i] == '-v':
>              del sys.argv[i]
> diff --git a/tools/fixme.sh b/tools/fixme.sh
> new file mode 100644
> index 0000000..acf1653
> --- /dev/null
> +++ b/tools/fixme.sh
> @@ -0,0 +1,2 @@
> +#!/bin/sh
> +egrep --color -rI '# [A-Z]{3,}' gadget


Funny :)


	G.