[Sugar-devel] Review of patch submitted by Martin for bug #2087
alsroot at member.fsf.org
Sat Sep 4 16:20:23 EDT 2010
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
> 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
More information about the Sugar-devel