[sugar] [PATCH] Refactor invites for 1-1 Chat (#6298)

[Tomeu Vizoso]

On Tue, Jun 10, 2008 at 5:21 PM, Morgan Collett
<morgan.collett at gmail.com> wrote:
> http://dev.laptop.org/git?p=users/morgan/sugar-toolkit;a=shortlog;h=6298
> - Guillaume's change, r+ from me
> - Can I push this to sugar-toolkit?

Think so.

#6298: Launch Chat for 1-1 XMPP chat

+        import json
+        from sugar import activity
+        from sugar.activity import activityfactory

Why not having the imports at the top?

+        tp_channel = json.write([str(bus_name), str(connection),
+                                 str(channel)])

If you use simpljson instead, you won't need to cast to str.

+            activityfactory.create_with_uri('org.laptop.Chat', tp_channel)

Marco, I thought we wanted to deprecate create_with_uri()? Do you have
a better idea here?

6298: Refactor invites to handle 1-1 XMPP connections

+        Note: self_activity_id is set to None to differentiate between
+            PrivateInvites and ActivityInvites

What about having _activity_id only in ActivityInvite and use
isinstance of hasattr to differentiate if needed?

+        tp_channel = simplejson.dumps([str(bus_name), str(connection),
+                                       str(channel)])

No need to cast when using simplejson

+        self._activity_model = activity_model = None

This is only needed in the else block below.

+        if activity_model:

Better to check if it isn't None?

+        if activity_model:
+            # shared activity
...
         else:
+            # private invite: displays with owner's colors

I suggest to use a boolean variable with a sensible name instead of
inline comments.

6298: Subclass InviteButton

-        menu_item.connect('activate', self.__join_activate_cb)
+        menu_item.connect('activate', self._join_activate_cb)

Better use __ for signal handlers, so we avoid name clashes in
subclasses. People can lose lots of time because of this.

+    def _join_activate_cb(self, menu_item):
+        raise NotImplementedError

Oh, I see now. Overriding signal handlers is not something that you'll
see in the code very often. What about:

+    def __join_activate_cb(self, menu_item):
+        self.join()
+
+    def join(self):
+        raise NotImplementedError

Sorry about the delay,

Tomeu