[sugar] [review] controlpanel - modularize model

[Simon Schampijer]

Marco Pesenti Gritti wrote:
> On Sat, May 10, 2008 at 9:38 PM, Simon Schampijer <simon at schampijer.de> wrote:
>> Hello,
>>
>> http://dev.laptop.org/~erikos/001_controlpanel.patch
>> is the full patch.
>>
>> And you can see the splited patches (the last two) here:
>> http://dev.laptop.org/git?p=users/erikos/sugar;a=shortlog;h=master
> 
> Very high level review, I'll not be able to do a detailed one until
> I'm back from vacation... perhaps Tomeu can take this over.
> 

Thanks for the review!

> -sys.path.insert(0, '@prefix@/share/sugar/shell')
> +path = '@prefix@/share/sugar/shell'
> +
> +sys.path.insert(0, path)
> 
> I'd rather add shell_path to config.py.in

http://dev.laptop.org/git?p=users/erikos/sugar;a=commit;h=db6ddf7ad60fc53e0160b87a0d870832fd5558d8

> +src/controlpanel/icons/Makefile
> 
> We already setting up an icons path for the shell:
> 
> src/main.py:    icons_path = os.path.join(config.data_path, 'icons')
> 
> So I'd just put these icons in sugar/data/icons and install them in
> data_path/icons

http://dev.laptop.org/git?p=users/erikos/sugar;a=commit;h=7fe2c92880b8390a43ddc8f719e961c378e9d1b0

> diff --git a/src/controlpanel/controltoolbar.py
> b/src/controlpanel/controltoolbar.py
> 
> control in there sounds redundant, I'd just make it toolbar.py
> 
> +class DetailView(gtk.VBox):
> 
> DetailView sounds wrong to me. Maybe PanelView or PanelPage?

I used SectionView:
http://dev.laptop.org/git?p=users/erikos/sugar;a=commit;h=3b090dfa6e2083ad461997cd3ea4d43ed0ed0d09

> +    __gsignals__ = {
> +        'valid-section': (gobject.SIGNAL_RUN_FIRST,
> +                          gobject.TYPE_NONE,
> +                          ([bool]))
> +    }
> 
> Any reason this is not a property?

The reason is that with the current code I have to signal the validity of the 
section to the ControlPanel class. The controlpanel object owns the 
sectiontoolbar object. The 'ok' icon changes it's sensitivity when the section 
is not valid.

> +class InlineAlert(gtk.EventBox):
> 
> Don't we have already an Alert widget in the toolkit? How is this different?

Inline alerts are different from the other alerts beause they are no dialogs, 
they only inform about a current event, so subclassing from alert would not be 
right in my opinion. I suggest that we add the inlinealert to the same file 
though. Or we create a trimmed down alert class where we subclass from.

> ---
> 
> A couple of more general notes:
> 
> At some point we will start using gconf (or similars) and a python
> model will be unnecessary for those preferences. Also they will
> support change notification and the view will have to listen for
> changes. It's probably fine for python models to *not* support change
> notifications.
> 
> The undo stuff should be made more generic. A possible approach is to
> split out the code which initialize the widgets to the current values
> to a setup() method. Wrap the model module inside a Model object.
> Model proxies the get_something/set_something method to the module,
> and remembers the initial values. DetailView.undo() calls the
> Model.undo()(which resets preferences to the inital values) and then
> DetailView.setup().
> 
> Marco
> 

Ok, I will look into it.

Thanks,
    Simon