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

[Morgan Collett]

On Mon, May 26, 2008 at 5:09 PM, Tomeu Vizoso <tomeu at tomeuvizoso.net> wrote:
> On Mon, May 26, 2008 at 4:01 PM, Morgan Collett
> <morgan.collett at gmail.com> wrote:
>> On Mon, May 26, 2008 at 12:10 PM, Tomeu Vizoso <tomeu at tomeuvizoso.net> wrote:
>>
>>> +        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?
>
> Rather than that, what about activities stating which kind of
> invitations can accept? Then the user could choose one of several
> activities from the invitation palette.
>
> But this looks like too much complexity for the August release.
>
>>> +    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.
>
> Hmm, I don't quite see why the presence of a MUC should affect having
> an activity_id or a private connection. Seems to me like an
> implementation detail surfacing too high on the API. There's no way a
> private connection could be exposed as a privately-shared activity?
>
>> 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?
>
> No other way that I know.
>
>>> +        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.
>
> Yes, if they have nothing in common, being two classes look cleaner ;)

I have split it out into ActivityInvite and PrivateInvite as you recommended.

I've updated the patches and pushed to the repos below. I think I have
dealt with all the review comments so far, so please take another
look. I have tested it and fixed the problem where the invites weren't
removed. In testing on my branch it is working well for me.

The main code to review is at:
http://dev.laptop.org/git?p=users/morgan/sugar;a=shortlog;h=6298 (3
most recent patches).

Related changes are at:
http://dev.laptop.org/git?p=users/morgan/sugar-toolkit;a=shortlog;h=6298
- Guillaume's change, r+ from me
http://dev.laptop.org/git?p=users/morgan/presence-service;a=shortlog;h=6298
- Guillaume's change, r+ from me
http://dev.laptop.org/git?p=users/morgan/chat-activity;a=shortlog;h=6298
- r+ from me for all Guillaume's changes

Regards
Morgan