[sugar] code reviews

[Tomeu Vizoso]

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.
>
>  Indeed.
>
>
>  >  +    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
>  instead.

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.

Tomeu