[sugar] code reviews

Tomeu Vizoso tomeu at tomeuvizoso.net
Tue Mar 25 05:21:58 EDT 2008


Hi Eben,

hope you don't mind if I review some code of yours ;)

* Colorize activities in Home view(s) on hover:

-        icon = CanvasIcon(size=style.STANDARD_ICON_SIZE, cache=True,
+        self.icon = CanvasIcon(size=style.STANDARD_ICON_SIZE, cache=True,

If icon is only used internally, then it is private and should start
with an underscore.

+    def __icon_hovering_changed_event_cb(self, icon, event):
+        if event:

What you call event should be a boolean called hovering.

+        self._profile = get_profile()

If the profile is not part of the state of the class, then I think we
shouldn't add a reference to it like that. Why don't you call
get_profile() each time it is needed?

+        # passing the icon_size property to the Icon contructor doesn't work
+        activity_icon.props.icon_size=gtk.ICON_SIZE_LARGE_TOOLBAR

Hmm, I think Simon pushed a fix for this some days ago? You may need
to sync your sugar-toolkit tree. Mind the spaces between operators and
operands.

+        Palette.__init__(self, None, None, primary_text=activity_info.name,

Please name the parameters that is not clear what they are, like this:

        Palette.__init__(self, label=None, accelerator=None,
primary_text=activity_info.name,

* Add icons to palettes of objects in Groups/Neighborhood:

-        p = palette.Palette(self._model.activity.props.name)

I know it isn't your code, but please use more complete names unless
they get really big.

Great work!

Thanks,

Tomeu


More information about the Sugar mailing list