[Sugar-devel] [PATCH sugar-toolkit] Handle DBUS tubes in the activity telepathy client, part of OLPC #10738

[Simon Schampijer]

On 04/16/2011 01:05 PM, Sascha Silbe wrote:
> Excerpts from Simon Schampijer's message of Thu Apr 14 19:43:05 +0200 2011:
>
>> This adds the handling of DBUS tube channels to the
>> 'HandlerChannelFilter' in activity's telepathy client.
>
> It would be nice to have some rationale for the change. Why didn't we do
> this before and in what way is the new design better? Are there any
> compatibility issues for activities that don't use sugar-toolkit (e.g.
> because they're not written in Python)?

I discussed these changes in detail with Tomeu and he was happy about them.

> [src/sugar/activity/activity.py]
>> @@ -65,9 +65,11 @@ import cjson
>>   from telepathy.server import DBusProperties
>>   from telepathy.interfaces import CHANNEL, \
>>                                    CHANNEL_TYPE_TEXT, \
>> +                                 CHANNEL_TYPE_DBUS_TUBE, \
>>                                    CLIENT, \
>>                                    CLIENT_HANDLER
>
> This can be shortened to:
>
> from telepathy.interfaces import CHANNEL, CHANNEL_TYPE_DBUS_TUBE, \
>                                   CHANNEL_TYPE_TEXT, CLIENT, CLIENT_HANDLER

Not relevant for the patch and the rest of the code in this area has the 
same style.

>> @@ -927,15 +930,23 @@ class _ClientHandler(dbus.service.Object, DBusProperties):
>>             })
>>
>>       def __get_filters_cb(self):
>> -        logging.debug('__get_filters_cb')
>> -        filters = {
>> +        logging.debug('__get_filters_cb Activity')
>> +        filter_text = {
>>               CHANNEL + '.ChannelType': CHANNEL_TYPE_TEXT,
>>               CHANNEL + '.TargetHandleType': CONNECTION_HANDLE_TYPE_CONTACT,
>>               }
>> -        filter_dict = dbus.Dictionary(filters, signature='sv')
>> -        logging.debug('__get_filters_cb %r', dbus.Array([filter_dict],
>> -                                                        signature='a{sv}'))
>> -        return dbus.Array([filter_dict], signature='a{sv}')
>> +        filter_dict = dbus.Dictionary(filter_text, signature='sv')
>> +        filters = dbus.Array([filter_dict], signature='a{sv}')
>> +
>> +        filter_tube = {
>> +            CHANNEL + '.ChannelType': CHANNEL_TYPE_DBUS_TUBE,
>> +            CHANNEL + '.TargetHandleType': CONNECTION_HANDLE_TYPE_ROOM,
>> +            }
>> +        filter_dict = dbus.Dictionary(filter_tube, signature='sv')
>> +        filters.append(filter_dict)
>> +
>> +        logging.debug('__get_filters_cb Activity filters=%r', filters)
>> +        return filters
>
> Like for the sugar patch, we should be able to get rid of the explicit
> conversions, making the code terser (and thus easier to grok).

We did solve this one in the other thread.

>> @@ -946,7 +957,12 @@ class _ClientHandler(dbus.service.Object, DBusProperties):
>>                   user_action_time, handler_info)
>>           try:
>>               for channel in channels:
>> -                self._got_channel_cb(connection, channel[0])
>> +                channel_properties = channel[1]
>
> How about:
>
>              for object_path, properties in channels:

Done.

> or:
>
>              for channel_object_path, channel_properties in channels:
>
>> +                channel_type = channel_properties[CHANNEL + '.ChannelType']
>> +                handle_type = channel_properties[CHANNEL + '.TargetHandleType']
>
>> +                if channel_type == CHANNEL_TYPE_TEXT and \
>> +                        handle_type == CONNECTION_HANDLE_TYPE_ROOM:
>> +                    self._got_channel_cb(connection, channel[0])
>
> PEP8:
>
>                  if channel_type == CHANNEL_TYPE_TEXT and \
>                     handle_type == CONNECTION_HANDLE_TYPE_ROOM:
>                      self._got_channel_cb(connection, channel[0])

Done.

>>           except Exception, e:
>>               logging.exception(e)
>
> logging.exception already includes the exception, we should instead use
> some more informative message including the actual values (esp.
> channels) that were passed in. Since it was part of the old code, we can
> address it in a separate patch if you prefer.

Yes, can be done in another patch.

Regards,
    Simon