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

[Simon Schampijer]

On 04/16/2011 12:24 AM, Sascha Silbe wrote:
> 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.

a callback would look like 'handler_cb', so... :)

>> +    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.

Done.

>> +        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.

They are the same basically. Made it 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 {}

Is not as clear to me. Sounds like a matter of taste.

> 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.

Yeah, might be an idea.

>> +    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?

http://telepathy.freedesktop.org/spec/Channel_Dispatch_Operation.html#Method:HandleWith

>>           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)
>

Done.

> [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:

For reference see 
http://permalink.gmane.org/gmane.comp.freedesktop.dbus/13936

Regards,
    Simon