[Sugar-devel] [PATCH sugar] Fix invitations from a non sugar client (empathy), part of OLPC #10814

[Sascha Silbe]

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/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 500 bytes
Desc: not available
URL: <http://lists.sugarlabs.org/archive/sugar-devel/attachments/20110416/673d3f56/attachment-0001.pgp>