[Sugar-devel] [PATCH sugar-toolkit] presence: use RoomConfig1 to configure channel properties (#3629)

[Sascha Silbe]

Daniel Drake <dsd at laptop.org> writes:

[...]
> However, as of Telepathy-0.24.0, these properties have gone away.
>
>     http://telepathy.freedesktop.org/spec/Channel_Type_Text.html
>     Changed in 0.24.0. This interface used to have a bunch of clunky
>     Telepathy.Properties. They have been removed in favour of D-Bus
>     properties on the Room2, Subject2 and RoomConfig1 interfaces.

Ah, reading the quote carefully, it's clear that the version number
applies to the specification. It would be nice to adjust the
introductory sentence to reflect that (e.g. "as of Telepathy
specification 0.24.0").


[...]
[src/sugar/presence/activity.py]
> @@ -43,6 +43,7 @@ from sugar.presence.buddy import Buddy
>  
>  CONN_INTERFACE_ACTIVITY_PROPERTIES = 'org.laptop.Telepathy.ActivityProperties'
>  CONN_INTERFACE_BUDDY_INFO = 'org.laptop.Telepathy.BuddyInfo'
> +CONN_INTERFACE_ROOM_CONFIG = 'org.freedesktop.Telepathy.Channel.Interface.RoomConfig1'

This line is too long. According to the PEP-8 indentation
recommendations [1], I would suggest:

CONN_INTERFACE_ROOM_CONFIG = \
    'org.freedesktop.Telepathy.Channel.Interface.RoomConfig1'

> @@ -675,13 +676,47 @@ class _JoinCommand(_BaseCommand):
>              self_handle = self._global_self_handle
>  
>          if self_handle in added:

Since there's nothing else in this function that gets run if the
condition is False, it would make the code easier to read if we
converted this to guardian style. I.e.:

         if self_handle not in added:
            return


> -            if PROPERTIES_INTERFACE not in self.text_channel:
> -                self._finished = True
> -                self.emit('finished', None)
> -            else:
> +            # Use RoomConfig1 to configure the text channel. If this
> +            # doesn't exist, fall-back on old-style PROPERTIES_INTERFACE.
> +            if CONN_INTERFACE_ROOM_CONFIG in self.text_channel:
> +                self.__update_room_config()
> +            elif PROPERTIES_INTERFACE in self.text_channel:
>                  self.text_channel[PROPERTIES_INTERFACE].ListProperties(
>                      reply_handler=self.__list_properties_cb,
>                      error_handler=self.__error_handler_cb)
> +            else:
> +                self._finished = True
> +                self.emit('finished', None)

It seems the if/elif blocks and the else block now handle cases that are
different in nature. While if block vs. elif block distinguishes between
the versions of Telepathy in use, if/elif blocks vs. else block
seems to distinguish between different stages of the protocol. That's
fine, but a comment explaining the latter check would make this code
block easier to understand.


> +    def __update_room_config(self):
> +        # FIXME: invite-only ought to be set on private activities; but
> +        # since only the owner can change invite-only, that would break
> +        # activity scope changes.
> +        props = {
> +            # otherwise buddy resolution breaks
> +            'Anonymous': False,
> +            # anyone who knows about the channel can join
> +            'InviteOnly': False,
> +            # vanish when there are no members
> +            'Persistent': False,
> +            # don't appear in server room lists
> +            'Private': True,
> +        }

This part seems to be a direct copy of the corresponding block in
__list_properties_cb(). How about factoring it out into a class
constant?


[...]
> +    def __room_cfg_error_cb(self, error):
> +        # If RoomConfig update fails, it's probably because we don't have
> +        # permission (e.g. we are not the session initiator). Thats OK -
> +        # ignore the failure and carry on.
> +        self._finished = True
> +        self.emit('finished', None)

Would it be possible to check the error and log it if it doesn't match
our expectations (i.e. permission denied)?


Otherwise the patch is looking good, thanks!

Sascha

[1] http://www.python.org/dev/peps/pep-0008/#id12
-- 
http://sascha.silbe.org/
http://www.infra-silbe.de/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 489 bytes
Desc: not available
URL: <http://lists.sugarlabs.org/archive/sugar-devel/attachments/20120703/781a8c92/attachment.pgp>