[Sugar-devel] Review of patch submitted by Martin for bug #2087
tomeu at sugarlabs.org
Mon Sep 6 04:42:05 EDT 2010
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:
>> > + <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 . 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  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
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.
>> 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).
>>  http://en.wikipedia.org/wiki/Guard_(computing)
>>  https://bugs.sugarlabs.org/ticket/1147
>> Sugar-devel mailing list
>> Sugar-devel at lists.sugarlabs.org
> Sugar-devel mailing list
> Sugar-devel at lists.sugarlabs.org
More information about the Sugar-devel