[Sugar-devel] [FEATURE] Remove Presence Service

[Tomeu Vizoso]

On Mon, Aug 16, 2010 at 23:33, Marco Pesenti Gritti <marco at marcopg.org> wrote:
> I don't have enough time for a very detailed review and I don't really
> know telepathy, but here a few comments. Mostly nitpicks, it looks
> great in general.
>
> +        BuddyIcon.__init__(self, buddy=get_owner_instance(), size=size)
>
> Maybe assign to self._buddy here and reuse later to pass to the menu?

Done.

> +        if not self._buddy.props.present or \
> +                not self._buddy.props.current_activity:
>
> I would align the not :)

Done.

> +        name = self._get_new_icon_name(self._buddy.props.current_activity)
>
> Do we still need to use .props? I thought at some point gobject add
> some magic to be able to just use properties.

Done, though I'm a bit scared of name clashes.

> +        p_text = glib.markup_escape_text(self._model.bundle.get_name())
> +        p_icon = Icon(file=self._model.bundle.get_icon(),
>
> Not your fault but I hate abbreviating like this... It's not
> immediately clear what the variable refers to.

I really hate it as well, but I have refrained myself to fixing
several of these things because then the patch would have doubled in
size (at least!). I think we should have a pylint rule that warns
about all identifiers with less than 4 chars.

> +        item.show()
> +        self._invite_to_item[invite] = item
>
> I'd /n here to make the two blocks separate.

Done

> +    def set_present(self, present):
> +        self._present = present
> +
> +    present = gobject.property(type=bool, default=False, getter=is_present,
> +                               setter=set_present)
>
> I still think we should move away from GObject for non UI stuff :)

Agreed, but as above I think it should be a cleanup goal rather than
having feature patches fixing it bit by bit.

> +            if service.startswith('org.freedesktop.Telepathy.Connection.'):
> +                path = '/%s' % service.replace('.', '/')
> +                Connection(service, path, bus,
> +                           ready_handler=self.__connection_ready_cb)
>
> I don't know enough about telepathy, but the path guessing here looks weird.

It's weird, but actually correct :) The connection service name is
part of the API. see
http://telepathy.freedesktop.org/spec/org.freedesktop.Telepathy.Connection.html#description

> +        logging.debug('__got_dispatch_operation_cb')
> +        dispatch_operation_path = kwargs['dispatch_operation_path']
>
> Nitpicking again... In several places, I think it would be clearer to
> separate the logging in its own block.

Yeah, logging is a big issue with this patch because it's really
verbose but also very useful when debugging because of the difficulty
of reproducing all situations as the code is asynchronous. I have
agreed with Simon in removing most of it once things stabilize.

> +        if connection_path == '/':
> +            return
>
> Why are we ignoring this? Unless it's obvious to someone that
> understands telepathy, a comment would be useful here.

It's a DBus convention for object paths, equivalent to None. Will add a comment.

> +        #self._start_listening()
>
> Leftover?

Yup, we don't need to start listening again. Removed.

> +        if handle.invited:
> +            wait_loop = gobject.MainLoop()
> +            self._client_handler = _ClientHandler(
> +                    self.get_bundle_id(),
> +                    partial(self.__got_channel_cb, wait_loop))
> +            # The current API requires that self.shared_activity is set before
> +            # exiting from __init__, so we wait until we have got the shared
> +            # activity.
> +            wait_loop.run()
>
> Ouch, quite an hack :) I'd open a bug and reference it here, it should
> go away at some point.

Done http://bugs.sugarlabs.org/ticket/2168

> +            # Cannot call datastore.write async for creates:
> +            # https://dev.laptop.org/ticket/3071
> +            datastore.write(self._jobject)
>
> Update the bug reference to sugarlabs.org while you are changing this?

Done

> +        if handle.object_id is None and create_jobject:
> +            logging.debug('Creating a jobject.')
> +            self._jobject = datastore.create()
> +            title = _('%s Activity') % get_bundle_name()
> +            self._jobject.metadata['title'] = title
> +            self.set_title(self._jobject.metadata['title'])
> +            self._jobject.metadata['title_set_by_user'] = '0'
> +            self._jobject.metadata['activity'] = self.get_bundle_id()
> +            self._jobject.metadata['activity_id'] = self.get_id()
> +            self._jobject.metadata['keep'] = '0'
> +            self._jobject.metadata['preview'] = ''
> +            self._jobject.metadata['share-scope'] = SCOPE_PRIVATE
> +            if self.shared_activity is not None:
> +                icon_color = self.shared_activity.props.color
> +            else:
> +                client = gconf.client_get_default()
> +                icon_color = client.get_string('/desktop/sugar/user/color')
> +            self._jobject.metadata['icon-color'] = icon_color
>
> Separate blocks while you are at it :) It's really hard to read.

Split all this to a _create_journal_object() method.

> +++ b/src/sugar/presence/util.py
>
> Maybe a more specific name for this module? connection or something...

Done.

Thanks a lot for taking the time!

Tomeu

> _______________________________________________
> Sugar-devel mailing list
> Sugar-devel at lists.sugarlabs.org
> http://lists.sugarlabs.org/listinfo/sugar-devel
>