On Tue, Mar 25, 2008 at 5:09 PM, Eben Eliason <[EMAIL PROTECTED]> 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 _______________________________________________ Sugar mailing list [email protected] http://lists.laptop.org/listinfo/sugar
