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

[Aleksey Lim]

On Sat, Sep 04, 2010 at 05:13:03PM +0200, Sascha Silbe wrote:
> 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.

I've found that there is no way to call has() method(all sugar code
calls get_* methods w/o any checks). If everything is installed, we
will get key value in any case (default from shcheme), but if
installation is inproper... I've managed to coredump python for unknow
keys.

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



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


-- 
Aleksey