[sugar] code reviews
Tue Mar 25 12:15:04 EDT 2008
On Tue, Mar 25, 2008 at 5:09 PM, Eben Eliason <eben.eliason at gmail.com> wrote:
> > hope you don't mind if I review some code of yours ;)
> Please do. =)
> > * 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.
> Well, that would make more sense, huh?
Had to go to the hippo-canvas code to figure out... :/
> > + 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?
> Oops. I meant to go back and change that. I did it right in the
> ActivityPalette class ;)
> > + # 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.
> I'd like confirmation on that one. I know Simon pushed a fix which
> would allow the passing of style.STANDARD_ICON_SIZE instead of
> gtk.ICON_SIZE_LARGE_TOOLBAR. I figured I could later push a change
> which fixes all of these at once. However, I think that the inability
> to initialize an icon with a size is an independent problem. Perhaps
> I'm wrong, or perhaps there is a fix for that as well. Simon?
> The lack of spaces result from it's previous place as a keyword arg to
> the constructor. My bad.
> > + 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,
> I wasn't aware that you could pass keyword args in place of regular
> arguments. Those two None values remain there for backward compatible
> API and nothing more. I opted to switch to the new property API in
> all places where I modified the creation of a Palette.
Yes, but the reader of that code doesn't know all that ;)
> > * 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.
> Fair enough. I thought about changing them, but I decided not to in
> order to keep the diff small. There are many instances of single
> letter variables in this file, and I thought that it would be best to
> perform a later commit which does nothing but address that issue
Sure, that's a good reason to let things as they are.
> Thanks for the review! After hearing back on some of these points
> I'll be happy to make the suggested changes.
More information about the Sugar-devel