Excerpts from Simon Schampijer's message of Thu Apr 14 19:42:35 +0200 2011:
[src/jarabe/model/invites.py]
> +class BaseInvite(object):
> + """Invitation to shared activity or private 1-1 Telepathy channel"""
> + def __init__(self, dispatch_operation_path, handle, handler):
> self.dispatch_operation_path = dispatch_operation_path
> self._handle = handle
> self._handler = handler
I know this is from the old code, but can we rename self._handler to
self._handler_name? With the previous name I'd assume it's a callback.
Alternatively we could introduce a docstring that explains the
parameters.
> + def _name_owner_changed_cb(self, name, old_owner, new_owner):
> + logging.debug('BasicInvite._name_owner_changed_cb %r %r %r', name,
> + new_owner, old_owner)
s/BasicInvite/BaseInvite/ I suppose.
> + if name == self._handler and new_owner and not old_owner:
> + self._call_handle_with()
> +
> + def _call_handle_with(self):
Both BaseInvite and Invites contain a method that calls HandleWith
(BaseInvite._call_handle_with() vs. Invites._handle_with()). They look
exactly the same to me. What's the difference? Do we need both?
It would be nice to at least give them the same name.
> +class ActivityInvite(BaseInvite):
> + """Invitation to a shared activity."""
> + def __init__(self, dispatch_operation_path, handle, handler,
> + activity_properties):
> + BaseInvite.__init__(self, dispatch_operation_path, handle, handler)
> +
> + if activity_properties is not None:
> + self._activity_properties = activity_properties
> + else:
> + self._activity_properties = {}
How about:
self._activity_properties = activity_properties or {}
Or can activity_properties contain a value that's evaluated as False,
but still semantically different from None / {}?
[class PrivateInvite]
> + def get_color(self):
> + client = gconf.client_get_default()
> + return XoColor(client.get_string('/desktop/sugar/user/color'))
I wonder whether we should move the gconf code to XoColor and indicate
we want the owner color via some parameter. Not as part of this patch,
of course.
> + def join(self):
> + logging.error('PrivateInvite.join handler %r', self._handler)
> +
> + registry = bundleregistry.get_registry()
> + bundle_id = self.get_bundle_id()
> + bundle = registry.get_bundle(bundle_id)
> + if bundle is None:
> + self._call_handle_with()
What exactly does the latter part do? Reject the invitation? Or pass it
on to a non-Sugar application?
> else:
> - logging.debug('__handle_with_reply_cb')
> + bus = dbus.SessionBus()
> + bus.add_signal_receiver(self._name_owner_changed_cb,
> + 'NameOwnerChanged',
> + 'org.freedesktop.DBus',
> + arg0=self._handler)
> + misc.launch(bundle, color=self.get_color(), invited=True,
> + uri=self._private_channel)
Making the "if bundle is None" check guarding style would get rid of one
level of indentation and one line continuation for the
add_signal_receiver, slightly improving code readability:
if bundle is None:
self._call_handle_with()
return
bus = dbus.SessionBus()
bus.add_signal_receiver(self._name_owner_changed_cb,
'NameOwnerChanged', 'org.freedesktop.DBus',
arg0=self._handler)
misc.launch(bundle, color=self.get_color(), invited=True,
uri=self._private_channel)
[Invites._dispatch_non_sugar_invitation()]
> if channel_type == CHANNEL_TYPE_CONTACT_LIST:
> self._handle_with(dispatch_operation_path, CLIENT + '.Sugar')
> elif channel_type == CHANNEL_TYPE_TEXT:
> handler = CLIENT + '.org.laptop.Chat'
We should make this configurable at some point, but not now (it would be
a feature, not a bug fix).
[src/jarabe/model/telepathyclient.py]
> @@ -63,13 +63,20 @@ class TelepathyClient(dbus.service.Object,
> DBusProperties):
> def __get_filters_cb(self):
> logging.debug('__get_filters_cb')
>
> - filt = {
> + activity_invitation = {
> CHANNEL + '.ChannelType': CHANNEL_TYPE_TEXT,
> CHANNEL + '.TargetHandleType': CONNECTION_HANDLE_TYPE_ROOM,
> }
> - filter_dict = dbus.Dictionary(filt, signature='sv')
> + filter_dict = dbus.Dictionary(activity_invitation, signature='sv')
> filters = dbus.Array([filter_dict], signature='a{sv}')
>
> + text_invitation = {
> + CHANNEL + '.ChannelType': CHANNEL_TYPE_TEXT,
> + CHANNEL + '.TargetHandleType': CONNECTION_HANDLE_TYPE_CONTACT,
> + }
> + filter_dict = dbus.Dictionary(text_invitation, signature='sv')
> + filters.append(filter_dict)
> +
Like above I'm wondering whether we need the explicit conversions:
def __get_filters_cb(self):
# We can handle Activity (ROOM) and plain text (CONTACT) invitations
return [{CHANNEL + '.ChannelType': CHANNEL_TYPE_TEXT,
CHANNEL + '.TargetHandleType': CONNECTION_HANDLE_TYPE_ROOM},
{CHANNEL + '.ChannelType': CHANNEL_TYPE_TEXT,
CHANNEL + '.TargetHandleType': CONNECTION_HANDLE_TYPE_CONTACT}]
Sascha
--
http://sascha.silbe.org/
http://www.infra-silbe.de/
signature.asc
Description: PGP signature
_______________________________________________ Sugar-devel mailing list Sugar-devel@lists.sugarlabs.org http://lists.sugarlabs.org/listinfo/sugar-devel
