[sugar] code reviews

[Simon Schampijer]

Hi Tomeu,

I found and have fixes two issues regarding the redesign:

a) the devices in the tray does draw the palette but not the black 
border around the icon like they do in the Friendstray for example

b) when you add more than max items to the clipboard tray not the first 
but the currently added gets deleted. get_children() seems to have 
another order for gtk.Containers than for hippo ones.

Best,
    Simon


Tomeu Vizoso wrote:
> 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
> _______________________________________________
> Sugar mailing list
> Sugar at lists.laptop.org
> http://lists.laptop.org/listinfo/sugar

-------------- next part --------------
A non-text attachment was scrubbed...
Name: max_items_clipboard.patch
Type: text/x-patch
Size: 595 bytes
Desc: not available
Url : http://lists.laptop.org/pipermail/sugar/attachments/20080325/5ca793a4/attachment.bin 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: device_tray.patch
Type: text/x-patch
Size: 1508 bytes
Desc: not available
Url : http://lists.laptop.org/pipermail/sugar/attachments/20080325/5ca793a4/attachment-0001.bin