[sugar] control panel review

[Tomeu Vizoso]

On Tue, Jun 3, 2008 at 11:42 PM, Simon Schampijer <simon at schampijer.de> wrote:
> Hi Tomeu,
>
> thanks very much for your detailed review - it was very useful! I am fine
> with most of your comments and changed the code accordingly. I made inline
> comments for the items.
>
> So I would re-base my changes on latest HEAD now and push if you are ok?

Ok with me. In the future, I hope we can land things like this in
small pieces, I think it plays better with the review process. A few
remaining comments:

>>            for module in modules:
>>                method = getattr(module, 'set_' + key, None)
>>                if method:
>>                    print method.__doc__
>>                    sys.exit()
>>            print _("sugar-control-panel: key=%s not an available option"
>>                    % key)
>> What if different modules have the same method?
>
> Do we really have this issue? I think/hope we don't have more than one
> module which
> has for example the method set_color.

Yes, I agree that the model structure requires that modules don't have
duplicated methods, but what will happen when someone codes a new
module and strange things happen? Can we make this error more clear?
Maybe keep iterating and displaying a warning if there's another
method with the same name in other module?

>>            {'optionname': {'view', 'model', 'button', 'keywords', 'icon'}
>> }
>> In python, would be {'optionname': ['view', 'model', 'button',
>> 'keywords', 'icon'] }
>
> It is a dictionary options with the keys 'optionname' which has the keys
> 'view',
> 'model'...

You meant the values 'view', 'model', ...?

>>> {'optionname': {'view', 'model', 'button', 'keywords', 'icon'} }
  File "<stdin>", line 1
    {'optionname': {'view', 'model', 'button', 'keywords', 'icon'} }
                          ^
SyntaxError: invalid syntax

>>                if tmp in self._options:
>> This means we cannot have one section just with command line support?
>
> Oh we can have one section just with command line support. You do so by
> providing
> only the module and not the view class.

But _get_options() will only setup in self._options options that are
present on a view module, right? Btw, if the function is called
_get_something, it should probably return an array instead of setting
a private member variable.

>> view/aboutme.py
>>
>> class EventIcon(gtk.EventBox):
>> What's the reason of existence of this class?
>
> I could have used a hippo.CanvasIcon but I thought we wanted to get rid of
> hippo code where possible. Maybe we should move it to the toolkit so it can
> be used in other places as well?

What has CanvasIcon to do here? Icon is not enough?

Thanks,

Tomeu