[Sugar-devel] Review of patch submitted by Martin for bug #2087
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 . E.g.:
def _add_erase_option(self, activity_info):
if not activity_info.is_user_activity():
# show menu item etc.
A suitable docstring is left as an exercise for the reader. ;)
> + client = gconf.client_get_default()
> + self._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.
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).
-------------- next part --------------
A non-text attachment was scrubbed...
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