[Sugar-devel] Review of patch submitted by Martin for bug #2087

[Sascha Silbe]

Excerpts from Kandarp Kaushik's message of Sat Sep 04 16:16:57 +0200 2010:

[data/sugar.schemas.in]
> +      <locale name="C">
> +        <short>Protected Activities Bundle Indentifiers</short>

Typo. Also a bit hard to understand. Something along the lines of
"Bundle IDs of protected activities" might be better.

> +        <long>Users will not be allowed to erase these
> +        activities through the list view.</long>

[src/jarabe/desktop/activitieslist.py]
> +        if activity_info.is_user_activity() and \
> +            not registry.is_activity_protected(self._bundle_id):
> +                menu_item = MenuItem(_('Erase'), 'list-remove')
> +                menu_item.connect('activate', self.__erase_activate_cb)
> +                self.menu.append(menu_item)
> +                menu_item.show()
> +
> +                if not os.access(activity_info.get_path(), os.W_OK):
> +                    menu_item.props.sensitive = False

This is getting convoluted. Please factor it out and turn your condition
into one or several guards [1]. E.g.:

    def _add_erase_option(self, activity_info):
        if not activity_info.is_user_activity():
            return
        if registry.is_activity_protected(self._bundle_id):
            return
        # show menu item etc.

A suitable docstring is left as an exercise for the reader. ;)


[src/jarabe/model/bundleregistry.py]
> +        client = gconf.client_get_default()
> +        self._protected_activities =
> client.get_list('/desktop/sugar/protected_activities',
> +                                                      gconf.VALUE_STRING)

I have a feeling this will break badly if the GConf value is not set.
See #1147 [2] for a similar case.

And please don't indent this much for a continuation. Personally I'd use
regular indentation (i.e. four spaces); maybe Tomeu can state his
preference so we can agree on some guideline.


PS: Please use git-send-email or some equivalent method of sending
    patches. Your message has had at least one line of the patch
    wrapped; I would have been unable to try out the patch (because
    it wouldn't apply).

Sascha

[1] http://en.wikipedia.org/wiki/Guard_(computing)
[2] https://bugs.sugarlabs.org/ticket/1147
--
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: 490 bytes
Desc: not available
Url : http://lists.sugarlabs.org/archive/sugar-devel/attachments/20100904/09744b58/attachment.pgp