[sugar] [review] controlpanel - modularize model
Simon Schampijer
simon at schampijer.de
Mon May 12 11:52:12 EDT 2008
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
More information about the Sugar
mailing list