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
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)
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
[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/branch 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
