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

[Morgan Collett]

On Fri, Jun 13, 2008 at 14:56, Tomeu Vizoso <tomeu at tomeuvizoso.net> wrote:
> 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.

Thanks.

I've pushed the changes for the issues below to a new patch:
http://dev.laptop.org/git?p=users/morgan/sugar;a=commitdiff;h=0b821f770d51647f3d41131027db6c4345501a45

> #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?

Fixed in a subsequent patch:
http://dev.laptop.org/git?p=users/morgan/sugar;a=commitdiff;h=1eff46e05bf56ae49304c3233fe3f8988c8e284e#patch2

>
> +        tp_channel = json.write([str(bus_name), str(connection),
> +                                 str(channel)])
>
> If you use simpljson instead, you won't need to cast to str.

Already switched to simplejson:
http://dev.laptop.org/git?p=users/morgan/sugar;a=commitdiff;h=1eff46e05bf56ae49304c3233fe3f8988c8e284e#patch2

I have now removed the 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?

Will discuss in a separate mail.

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

Good idea, I'll change to that.

>
> +        tp_channel = simplejson.dumps([str(bus_name), str(connection),
> +                                       str(channel)])
>
> No need to cast when using simplejson

Right - same as above. Fixed.

>
> +        self._activity_model = activity_model = None
>
> This is only needed in the else block below.

Fixed in http://dev.laptop.org/git?p=users/morgan/sugar;a=commitdiff;h=0383581b2789fa7a8d0f8eb99da6068eb67b5500

>
> +        if activity_model:
>
> Better to check if it isn't None?

Fixed.

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

Removed when I refactored InviteButton to BaseInviteButton,
ActivityInviteButton and PrivateInviteButton (and same for
InvitePalette) in
http://dev.laptop.org/git?p=users/morgan/sugar;a=commitdiff;h=0383581b2789fa7a8d0f8eb99da6068eb67b5500

> 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

Ah, great idea. Done.

Regards
Morgan