[sugar] [PATCH] Refactor invites for 1-1 Chat (#6298)
I extended the Invites to handle connections for 1-1 connections. This
should also handle incoming streaming media connections for VideoChat
(when that activity has the necessary bits in place).
It depends on Guillaume's patches in http://dev.laptop.org/ticket/6298
for presence-service, sugar, sugar-toolkit and chat-activity.
I will get this patch up in my git repo, as soon as I can figure out
why it won't allow me to push (non-fast forward).
It works in jhbuild with incoming XMPP chat giving an invite for Chat,
which launches Chat. The only thing not working yet is removing the
invite when you click on it.
Review please...
Morgan
commit a856fc40557fb1efc6e029088d49f74564d4675c
Author: Morgan Collett <[EMAIL PROTECTED]>
Date: Fri May 23 16:47:58 2008 +0200
#6298: Refactor invites to handle 1-1 XMPP connections
diff --git a/src/model/Invites.py b/src/model/Invites.py
index 9ffab44..2a4c9f2 100644
--- a/src/model/Invites.py
+++ b/src/model/Invites.py
@@ -18,10 +18,11 @@ import gobject
from sugar.presence import presenceservice
class Invite:
-def __init__(self, issuer, bundle_id, activity_id):
-self._issuer = issuer
+def __init__(self, bundle_id, activity_id=None,
+ private_connection=None):
self._activity_id = activity_id
self._bundle_id = bundle_id
+self._private_connection = private_connection
def get_activity_id(self):
return self._activity_id
@@ -29,6 +30,11 @@ class Invite:
def get_bundle_id(self):
return self._bundle_id
+def get_private_connection(self):
+"""Connection info from private invitation"""
+return self._private_connection
+
+
class Invites(gobject.GObject):
__gsignals__ = {
'invite-added': (gobject.SIGNAL_RUN_FIRST,
@@ -41,30 +47,52 @@ class Invites(gobject.GObject):
gobject.GObject.__init__(self)
self._dict = {}
+self._private_invites = {}
ps = presenceservice.get_instance()
owner = ps.get_owner()
owner.connect('joined-activity', self._owner_joined_cb)
+# FIXME need equivalent for ^ for private invite
-def add_invite(self, issuer, bundle_id, activity_id):
+def add_invite(self, bundle_id, activity_id):
if activity_id in self._dict:
# there is no point to add more than one time
# an invite for the same activity
return
-invite = Invite(issuer, bundle_id, activity_id)
+invite = Invite(bundle_id, activity_id=activity_id)
self._dict[activity_id] = invite
self.emit('invite-added', invite)
+def add_private_invite(self, private_connection, bundle_id):
+if private_connection in self._dict:
+# there is no point to add more than one invite for the
+# same incoming connection
+return
+
+invite = Invite(bundle_id,
+private_connection=private_connection)
+self._dict[private_connection] = invite
+self.emit('invite-added', invite)
+
def remove_invite(self, invite):
self._dict.pop(invite.get_activity_id())
self.emit('invite-removed', invite)
+def remove_private_invite(self, invite):
+self._dict.pop(invite.get_private_connection())
+self.emit('invite-removed', invite)
+
def remove_activity(self, activity_id):
invite = self._dict.get(activity_id)
if invite is not None:
self.remove_invite(invite)
+def remove_private_connection(self, private_connection):
+invite = self._dict.get(private_connection)
+if invite is not None:
+self.remove_private_invite(invite)
+
def _owner_joined_cb(self, owner, activity):
self.remove_activity(activity.props.id)
diff --git a/src/model/Owner.py b/src/model/Owner.py
index 7affb83..1643c6f 100644
--- a/src/model/Owner.py
+++ b/src/model/Owner.py
@@ -17,6 +17,7 @@
import gobject
import os
+import json
from telepathy.interfaces import CHANNEL_TYPE_TEXT
@@ -81,7 +82,7 @@ class ShellOwner(gobject.GObject):
return self._nick
def _activity_invitation_cb(self, pservice, activity, buddy, message):
-self._invites.add_invite(buddy, activity.props.type,
+self._invites.add_invite(activity.props.type,
activity.props.id)
def _private_invitation_cb(self, pservice, bus_name, connection,
@@ -91,14 +92,13 @@ class ShellOwner(gobject.GObject):
This is a connection by a non-Sugar XMPP client, so
launch Chat with the Telepathy connection and channel.
"""
-import json
-from sugar import activity
-from sugar.activity import activityfactory
+if channel_type == CHANNEL_TYPE_TEXT:
+bundle_id = 'org.laptop.Chat'
+else:
+bundle_id = 'org.laptop.VideoChat'
tp_channel = json.write([str(bus_name), str(connection),
Re: [sugar] [PATCH] Refactor invites for 1-1 Chat (#6298)
On Fri, May 23, 2008 at 05:12:29PM +0200, Morgan Collett wrote: > I will get this patch up in my git repo, as soon as I can figure out > why it won't allow me to push (non-fast forward). Git is warning you about divergence: there are patches on the branch you're pushing into that you have not yet 'seen' since they aren't in the history of the branch you're pushing. If you know what you're doing, you can force the push with the '-f' flag. If you screw up, check the reflogs in .git/logs/refs/heads/ in the remote repo. (The .git might be absent if it's a bare repo). If you want to see the patches that you're missing, run git remote update gitk --all and look for remote branches. Finally, to incorporate the changes, you can merge or rebase. Let me know if more detail is required, Michael ___ Sugar mailing list Sugar@lists.laptop.org http://lists.laptop.org/listinfo/sugar
Re: [sugar] [PATCH] Refactor invites for 1-1 Chat (#6298)
On Mon, May 26, 2008 at 5:09 PM, Tomeu Vizoso <[EMAIL PROTECTED]> wrote: > On Mon, May 26, 2008 at 4:01 PM, Morgan Collett > <[EMAIL PROTECTED]> wrote: >> On Mon, May 26, 2008 at 12:10 PM, Tomeu Vizoso <[EMAIL PROTECTED]> 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 ___ Sugar mailin
Re: [sugar] [PATCH] Refactor invites for 1-1 Chat (#6298)
Le mercredi 04 juin 2008 à 17:08 +0200, Morgan Collett a écrit : > The main code to review is at: > http://dev.laptop.org/git?p=users/morgan/sugar;a=shortlog;h=6298 (3 > most recent patches). As bundle_id is passed to both constructor, you could move it to BaseInvite.__init__ Didn't read code carefully but InviteButton and InvitePalette still contain lot of: if shared: ... else: # private Maybe it would be worth to abstract these 2 classes too if possible? G. ___ Sugar mailing list Sugar@lists.laptop.org http://lists.laptop.org/listinfo/sugar
Re: [sugar] [PATCH] Refactor invites for 1-1 Chat (#6298)
On Wed, Jun 4, 2008 at 17:25, Guillaume Desmottes <[EMAIL PROTECTED]> wrote: > Le mercredi 04 juin 2008 à 17:08 +0200, Morgan Collett a écrit : >> The main code to review is at: >> http://dev.laptop.org/git?p=users/morgan/sugar;a=shortlog;h=6298 (3 >> most recent patches). > > As bundle_id is passed to both constructor, you could move it to > BaseInvite.__init__ > > > Didn't read code carefully but InviteButton and InvitePalette still > contain lot of: > > if shared: > ... > else: > # private > > > Maybe it would be worth to abstract these 2 classes too if possible? Done, in the patch http://dev.laptop.org/git?p=users/morgan/sugar;a=commitdiff;h=0383581b2789fa7a8d0f8eb99da6068eb67b5500 Sugar Team, please can I have further review: The main code to review is at: http://dev.laptop.org/git?p=users/morgan/sugar;a=shortlog;h=6298 (4 most recent patches). 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? Related changes are at: http://dev.laptop.org/git?p=users/morgan/presence-service;a=shortlog;h=6298 - Guillaume's change, r+ from me - Waiting on the above sugar-toolkit patch approval http://dev.laptop.org/git?p=users/morgan/chat-activity;a=shortlog;h=6298 - r+ from me for all Guillaume's changes Regards Morgan ___ Sugar mailing list Sugar@lists.laptop.org http://lists.laptop.org/listinfo/sugar
Re: [sugar] [PATCH] Refactor invites for 1-1 Chat (#6298)
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.
#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
___
Sugar mailing list
Sugar@lists.laptop.org
http://lists.laptop.org/listinfo/sugar
Re: [sugar] [PATCH] Refactor invites for 1-1 Chat (#6298)
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
Sugar@lists.laptop.org
http://lists.laptop.org/listinfo/sugar
Re: [sugar] [PATCH] Refactor invites for 1-1 Chat (#6298)
On Fri, Jun 13, 2008 at 14:56, Tomeu Vizoso <[EMAIL PROTECTED]> wrote:
>
> #6298: Launch Chat for 1-1 XMPP chat
>
> +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?
What I need is to pass a string to the activity, in the metadata. It's
not a URI (actually it's the json-encoded strings for a telepathy
channel) but create_with_uri was the closest to what I need.
I'm happy to change to something similar which isn't create_with_uri,
but I don't know what you would prefer.
Regards
Morgan
___
Sugar mailing list
Sugar@lists.laptop.org
http://lists.laptop.org/listinfo/sugar
Re: [sugar] [PATCH] Refactor invites for 1-1 Chat (#6298)
r+ ;)
On Fri, Jun 13, 2008 at 3:41 PM, Morgan Collett
<[EMAIL PROTECTED]> wrote:
> On Fri, Jun 13, 2008 at 14:56, Tomeu Vizoso <[EMAIL PROTECTED]> wrote:
>>
>> #6298: Launch Chat for 1-1 XMPP chat
>>
>> +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?
>
> What I need is to pass a string to the activity, in the metadata. It's
> not a URI (actually it's the json-encoded strings for a telepathy
> channel) but create_with_uri was the closest to what I need.
>
> I'm happy to change to something similar which isn't create_with_uri,
> but I don't know what you would prefer.
>
> Regards
> Morgan
>
___
Sugar mailing list
Sugar@lists.laptop.org
http://lists.laptop.org/listinfo/sugar
