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

Sascha Silbe sascha-ml-reply-to-2010-3 at silbe.org
Sat Sep 4 11:13:03 EDT 2010

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

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

> +        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():
        if registry.is_activity_protected(self._bundle_id):
        # show menu item etc.

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

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


[1] http://en.wikipedia.org/wiki/Guard_(computing)
[2] https://bugs.sugarlabs.org/ticket/1147
-------------- 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 

More information about the Sugar-devel mailing list