[sugar] control panel review

[Simon Schampijer]

Tomeu Vizoso wrote:
> 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?

Checking now if there is more than one option with the same name and print a 
warning message.

>>>            {'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

Just take it out since it leads to confusion.

>>>                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? 

But the command line option has it's own method to get the module information.
http://dev.laptop.org/git?p=users/erikos/sugar/.git;a=blob;f=src/controlpanel/cmd.py;h=c6f590668d798cea5cf38836aa7be0ba7ba3d53e;hb=fd0936e9d34a6517e00cb3151cd0f03ffb8a14c3#l43

Btw, if the function is called
> _get_something, it should probably return an array instead of setting
> a private member variable.

Ok, changed that.

>>> 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?

The sugar.icon.Icon is derived from gtk.Image to receive events you need to pack it 
in an EventBox.

gtk.Image is a "no window" widget (has no gtk.gdk.Window of its own), so by default 
does not receive events. If you want to receive events on the image, such as button 
clicks, place the image inside a gtk.EventBox, then connect to the event signals on 
the event box.

So I just keep it.

Best,
    Simon