[sugar] frame redesign

[Tomeu Vizoso]

On Wed, Mar 19, 2008 at 5:05 PM, Simon Schampijer <simon at schampijer.de> wrote:
> Hi Tomeu, Eben,
>
>  I went through the last commits until 'Remove activities.defaults, not
>  used any more'. I will go through the rest as well. Here are my thoughts:
>
>  *** Pulse activity icons in the ring during launch
>  commit 3670fda59103c32fcbe8b28a9f8e77a75ac15d31
>
>  pylint:
>   - src/model/homemodel.py: Unused import dbus (maybe you can remove
>  that when you fix something in that file)

Done

>   - 139: Line too long (81/80)
>   - 198: Line too long (81/80)

Do we care about one char more?

>  _get_color() could be a function since it does not access the actual
>  object. Probably ok in this case.

I like to group code in classes when it makes sense only in that
class. Everybody agree? A later commit makes the method use
self._level, though.

>  W:211:ActivityIcon: Use of "property" on an old style class
>  W:215:ActivityIcon: Use of "property" on an old style class

False positive from pylint, ActivityIcon is a new style class as
inherits from CanvasIcon.

>  Why do we 'wrap' the get_version method here with the property construct?

Because the code is shorter and leaves more room for in the future
changing the implementation without breaking API compatibility. I
think we want to do this in general? Not expose setters nor getters
but properties?

>  *** Made background colors of zoom spheres white, for consistency
>  commit c5b776bcf27b4f6e638fac3658437a3c194e7ddd
>   - src/view/BuddyIcon.py
>       - no semicolon

Done

>  *** Fixed scale and placement of a single icon in the ring
>  commit ef75cabea665c436fc40cee887462931462d6bbf
>   - what do the '**' instead of '*' mean ?

It dereferences twice. It's some pygtk magic for getting the rest of
the named args as a dict.

>  *** Store favorite activities in the profile dir.
>  commit 3b5131e4b16eb5a83278de3787a5b1f4763ae566
>   - no need to import os.path as well we import os already

Done

>  *** General Errors
>  src.view.home.activitiesring:
>  E: 75:ActivitiesRing.__activity_added_cb: Undefined variable 'info'
>      - should probably be activity_info

Done

>  src.view.home.activitieslist
>  W: 20: Unused import gtk

Done

>  * General Thoughts
>  What is the advantage from simplejson over json. The home page says:
>  simplejson is API compatible with json-py, but is more easily
>  extensible, has better unicode support, is probably also faster, and is
>  MIT licensed (json-py is LGPL/GPL). Should we recommend people using
>  simplejson?

My main problem with json is that it requires the data types to be the
base types in python. So if you want to encode data from dbus, you'll
have to convert all dbus.String to str. So perhaps we should go always
with simplejson?

Thanks a lot for the review,

Tomeu