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

[Morgan Collett]

On Mon, May 26, 2008 at 12:10 PM, Tomeu Vizoso <tomeu at tomeuvizoso.net> wrote:
> Hi,
>
>         self._dict = {}
> +        self._private_invites = {}
>
> _private_invites doesn't seem to be used in the patch.

Right.

>         owner.connect('joined-activity', self._owner_joined_cb)
> +        # FIXME need equivalent for ^ for private invite
>
> Having a ticket number may be a good way of tracking this?
>
> -    def add_invite(self, issuer, bundle_id, activity_id):
> +    def add_invite(self, bundle_id, activity_id):
>
> We know that one of these days (perhaps for the August release), we'll
> need a version number to identify unequivocally a bundle. Would make
> sense to add it now to the API even if it's not used yet?

I'll make a note of that - but I don't want to add unrelated things to
this patch...

> +        invite = Invite(bundle_id,
> +                        private_connection=private_connection)
>
> This could fit in one line.
>
> +        self._dict.pop(invite.get_private_connection())
>
> if you are not going to use the result of pop(), then I think using
> 'del' would be clearer.
>
> +        invite = self._dict.get(private_connection)
>
> Why not self._dict[private_connection]?
>
>         tp_channel = json.write([str(bus_name), str(connection),
>                                  str(channel)])
>
> Instead of converting to str every value, why not using simplejson? It
> will accept subclasses of the base types. python-json doesn't seem to
> be very actively maintained right now.

OK, I couldn't remember which of the json modules was the winner :)

> +        if channel_type == CHANNEL_TYPE_TEXT:
> +            bundle_id = 'org.laptop.Chat'
> +        else:
> +            bundle_id = 'org.laptop.VideoChat'
>
> What are the plans for the future? Could we drop these hardcoded values somehow?

I'm open to suggestions... at the moment, Chat is the only sugarized
activity supporting XMPP but it's possible the overlay chat can handle
this type of connection in the future. We would still need a way to
launch a VideoChat (or equivalent) activity.

Perhaps at the time when we have multiple activities that could be
launched, we could add a Control Panel option?

> +    def start_activity_with_uri(self, activity_type, uri):
>
> What's the format of private_connection and how is it an URI? I was
> expecting activityfactory.create_with_uri() to be dropped at some
> point. Why cannot be used an activity_id like in normal invites?

This is something I would especially like input on. With an XMPP
connection from a non Sugar XMPP client, PS sends the
private-invitation signal but there is no shared activity. The whole
point of supporting this is that shared activities have a room (MUC)
on the server (in the case of gabble, but salut has an equivalent)
whereas now this will support 1-1 XMPP chat with no room. So there's
no activity_id.

Therefore, I needed a way to pass the Telepathy channel to the
instance of Chat. It's not a URI at all, just an arbitrary string
(actually multiple values encoded with json). The values we need to
get to Chat are, for example:

bus_name: "org.freedesktop.Telepathy.Connection.gabble.jabber.a088b096fcea6d858fc0cfbfe8879f67817b6aca_40your_2ehost_2ename_2fTelepathy"

connection: "/org/freedesktop/Telepathy/Connection/gabble/jabber/a088b096fcea6d858fc0cfbfe8879f67817b6aca_40your_2ehost_2ename_2fTelepathy"

Channel: "/org/freedesktop/Telepathy/Connection/gabble/jabber/a088b096fcea6d858fc0cfbfe8879f67817b6aca_40your_2ehost_2ename_2fTelepathy/ImChannel4"

So, should I find some other piece of metadata to use, and create an
equivalent of start_activity_with_uri that passes the parameter(s)
there? Any other ways of passing these values to a newly-instantiated
activity?

> +from sugar import activity, profile
>
> I think this should be two lines.

OK

> +        if invite.get_activity_id():
> +            # shared activity
> +            mesh = shellmodel.get_instance().get_mesh()
> +            activity_model = mesh.get_activity(invite.get_activity_id())
> +            self._activity_model = activity_model
> +            self._bundle_id = activity_model.get_bundle_id()
> +        else:
> +            # private invite to 1-1 connection
> +            self._private_connection = invite.get_private_connection()
> +            self._bundle_id = invite.get_bundle_id()
>
> Suggestion: what about having ActivityInvite and PrivateInvite both
> inheriting from BaseInvite?

I had it like that at one point, but decided to combine them for
simplicity since ActivityInvite had almost nothing in common with
PrivateInvite. The parameters we need to pass are completely
different... Perhaps the code is less readable now though, I'll take
another look.

> +            if not activity_model:
> +                return
>
> Should this raise an exception instead? Or at least log an error/warning?

If you are invited to a shared activity, and the activity disappears
from Mesh view before you can join it (you were slow, the others
played and left) then we can't join anything.

> Sorry about the nipicks!

Oh, those are welcome - but it's the bigger issues that I'd like more
input on :)

Morgan