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

[Tomeu Vizoso]

On Sat, Sep 4, 2010 at 22:20, Aleksey Lim <alsroot at member.fsf.org> wrote:
> 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.

This is really bad and should be fixed inside GConf (though I think
anyway Sugar should be able to deal with incorrect gconf values). That
said, it may be more practical to deal with this problem when we move
to GSettigns/dconf, though that really depends on how many resources
are put into this issue.

Regards,

Tomeu

>> 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
> _______________________________________________
> Sugar-devel mailing list
> Sugar-devel at lists.sugarlabs.org
> http://lists.sugarlabs.org/listinfo/sugar-devel
>