On Fri, Jun 13, 2008 at 14:56, Tomeu Vizoso <[EMAIL PROTECTED]> wrote: > On Tue, Jun 10, 2008 at 5:21 PM, Morgan Collett > <[EMAIL PROTECTED]> 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 _______________________________________________ Sugar mailing list [email protected] http://lists.laptop.org/listinfo/sugar
