[sugar] control panel review

[Simon Schampijer]

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?

Thanks,
    Simon


Tomeu Vizoso wrote:
> Hi,
> 
> yesterday had a bunch of hours in the train, so decided to give a look
> to Simon's work on the control panel. I think he has done a great work
> and would love to see it ASAP on the builds.
> 
> Please take in account that most of the comments are style nitpicks of
> me that other people may not share.
> 
> cmd.py
> 
>     '''Print the help to the screen'''
> s/the/
> 
>     print _('To apply your changes you have to restart sugar.\n' +
>             'Hit ctrl+alt+erase on the keyboard to trigger a restart.')
> Should we tell to close first the activities so no data is lost?

Hmmm not sure I would like to handle this with session-management which we will
have seen i hope.

>     names = os.listdir(os.path.join(config.shell_path, '/'.join(subpath)))
> s/names/file_names
> 
>     modules = []
>     for name in names:
> s/name/file_name
> 
>         if name.endswith('.py') and name != '__init__.py':
>             tmp = name.strip('.py')
> s/tmp/module_name
> 
>             mod = __import__('.'.join(subpath) + '.' +
> s/mod/module
>

Done.

>     except getopt.GetoptError:
>         cmd_help()
>         sys.exit(2)
> 
>     if not opts:
>         cmd_help()
>         sys.exit()
> Why exit(2) and exit()? Shouldn't be the same exit code?

Made it the same exit code. From the docs it says: "Unix programs generally use 2
for command line syntax errors and 1 for all other kind of errors." So 2 as status
error code makes sense here.


>     for opt, key in opts:
> s/opt/options, etc

Done.

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

>                     if note == 'RESTART':
> Would be better a numeric constant?

Done.

>                         print _("sugar-control-panel: %s"
>                                 % detail)
> May be easier to read in a single line?

yup done.

> gui.py
> 
> _ = lambda msg: gettext.dgettext('sugar', msg)
> This is only needed in sugar-toolkit. In sugar it is already set.

Done.

>     def _update_accept_focus(self):
>         self.window.set_accept_focus(True)
> Do we need this method?

No we don't need it - removed.

>             self._table.attach(gridwidget, column, column+1, row, row+1)
> Should be spaces around the + operator.
> 
>             if column == 5:
> Should 5 be a constant? _MAX_COLUMNS?

yes.

>                 if query in key or query in key.upper() \
>                         or query in key.capitalize():
> What about just if query.lower() in key.lower(): ?

+1

>         class_pointer =  self._options[option]['view']
> Not sure what class_pointer is, maybe view_constructor would be a better name?

named it view_class

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

> 
>         subpath = ['controlpanel', 'view']
>         names = os.listdir(os.path.join(config.shell_path,
> '/'.join(subpath)))
>         for name in names:
> What about:
>         modules_path = os.path.join(config.shell_path, 'controlpanel', 'view')
>         for file_name in os.listdir(modules_path):

I use the subpath in another call again so I left it like that.

>                 class_pointer = getattr(mod, tmp[0].capitalize()
>                                         + tmp[1:], None)
> Can we depend on this algorithm to determine the name of the class inside the
> module? Isn't any other option clearer and more safe?

I made the class-name another constant and added logging for failures.

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

>     def __accept_clicked_cb(self, widget, data=None):
> data=None is not needed.

+1

>             alert.add_button(gtk.RESPONSE_CANCEL, _('Cancel'), cancel_icon)
> Would make sense to display the string 'Cancel changes' instead? At first I
> thought it meant 'Cancel restart'.

Sounds good.

>     def __select_option_cb(self, button, event, option=None):
> Can option be ever None?

Yup took that out.

>     def __stop_clicked_cb(self, widget, data=None):
> No need for data=None.

+1

>         self._section_toolbar.accept_button.set_sensitive( \
>                 section_view.props.valid_section)
> May be clearer:
>         section_is_valid = section_view.props.valid_section
>         self._section_toolbar.accept_button.set_sensitive(section_is_valid)

ok.

> class ModuleWrapper(object):
> If this class represents one module, why not just call it 'Module'? Many classes
> are wrappers around concepts, calling all them Wrapper is not practical.

I think it makes sense for this class to use Wrapper since this is really what it
is. But I renamed it to ModelWrapper.

>     def __init__(self, mod):
>         self._mod = mod
> s/mod/module?
> 
> class _GridWidget(gtk.EventBox):
>     __gtype_name__ = "SugarGridWidget"
> What about calling it _ModuleIcon instead?

Named it SectionIcon.

>         self._options = {}
>         self._setup()
> Why not:
>         self._options = self._get_options()
> 
>         self.icon_name = None
>         self.pixel_size = style.GRID_CELL_SIZE
>         self.xo_color = None
>         self.title = 'No Title'
> These variables back properties. It is not clear whether should be accessed as
> gobject properties or as python properties. I would make them private
> so it's clear
> that should only be used as gobject properties.

Ok.

> 
> inlinealert.py
> 
> class InlineAlert(gtk.EventBox):
> But the normal alerts are inline as well. This name doesn't characterize well
> what this class is about. What about NotificationAlert or InformativeAlert, etc?

We will refactor the whole alert API soon. And then we make this a bit clearer.

> Why EventBox and not HBox or Bin?

changed to be a HBox.

>         self._msg = None
> s/msg/message, etc

the alert api is like that so I leave it that way.

> sectionview.py
> 
> _ = lambda msg: gettext.dgettext('sugar', msg)
> Not needed.
> 
>         self.valid_section = True
> Should be private? s/valid_section/is_valid?

done.

>         self.restart = False
> What this means? Perhaps s/restart/needs_restart?

ok.

>         self.restart_alerts = []
> I'm having trouble understanding what this serves for. Maybe a pydoc comment
> would clarify it?

This holds a list of alerts that are visible in a section. You changed for example
nick and color in the aboutme section. This information is stored until you quit
the control panel.

>         self._restart_msg = _('Changes require a sugar restart to take effect.')
> Private member not used.

done.

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

>     def __init__(self, model, alerts):
> What about refactoring some of this code in methods like _setup_nick(),
> _setup_color(), etc?

Ok done.

> view/aboutxo.py
> 
>     def __init__(self, model=None, alerts=None):
> Might make sense to split the constructor in _setup_identity(), etc?

Done.

>         not_available = _('Not available')
> Maybe should be a private constant at the module level?
> 
>         serial_no = self._read_file('/ofw/serial-number')
> Why is this in the view and not in the model?

Moved the logic to the model and added a cmd option to read the aboutxo information 
as well.

> view/frame.py
> 
>         self._delay_valid = True
> s/delay_valid/is_delay_valid?

done.

>         # (value, lower, upper, step_increment, page_increment, page_size)
>         adj = gtk.Adjustment(100, 0, 1000, 100, 100, 0)
> Cannot be used named args?

+1

>         state = ('off', 'on')[widget.get_active()]
> Should be constants?
> 
>     def __delay_changed_cb(self, widget, data=None):
> s/widget/scale?

done.

>         self._delay_sid = gobject.timeout_add(1000,
> Should the timeout be a constant common to all sections?

Yes. Made that constant available in sectionview.

> view/language.py
> 
>         self._tvcolumn = gtk.TreeViewColumn(_('Language'))
> s/tvcolumn/language_column?

Yup changed.

>         self.cell = gtk.CellRendererText()
> Why is this public?

There was no reason - changed.

>             if lang.split('.')[0] in row[0].split('.')[0]:
> Can this be made more clear? Perhaps by storing some value(s) in intermediate
> variable(s)?

done.

>                 "cursor-changed", self.__langchanged_cd)
> s/__langchanged_cd/__lang_changed_cb

done.

>             if key in row[1] or key.capitalize() in row[1]:
> What about:
>             if key.lower() in row[1].lower():

done.

>         if self._model.get_language == self._store.get_value(row[1], 0):
> self._model.get_language?

Outch thanks!

> 
> view/network.py, view/timezone.py
> 
> No new suggestions.
> 
> model/aboutme.py
> 
>     pro = profile.get_profile()
> What about importing only get_profile() from the profile module? In that way
> we could use profile instead of just prof.

Hmmm I would have to import it twice in that case since I use the module directly
as well profile.get_nick_name() - so I did not change it.

>     color = get_color().to_string()
>     tmp = color.split(',')
> 
>     stroke = None
>     fill = None
>     for color in _COLORS:
>         for hue in _COLORS[color]:
>             if _COLORS[color][hue] == tmp[0]:
>                 stroke = (color, hue)
>             if _COLORS[color][hue] == tmp[1]:
>                 fill = (color, hue)
> Suggestion to make it clearer:
>     color_string = get_color().to_string()
>     stroke, fill = color.split(',')
> 
>     stroke_tuple = None
>     fill_tuple = None
>     for base_color in _COLORS:
>         for hue in _COLORS[base_color]:
>             html_color = _COLORS[base_color][hue]
>             if stroke == html_color:
>                 stroke_tuple = (color, hue)
>             if fill == html_color:
>                 fill_tuple = (color, hue)
> 
>         print 'stroke:   color=%s hue=%s' % (stroke[0], stroke[1])
> Shouldn't be translatable?

done.

> def set_color(stroke, fill, modstroke='medium', modfill='medium'):
> s/modstroke/stroke_modifier, etc

done.

> model/frame.py
> 
> def set_delay(value):
> s/value/delay

+1

> def get_top_active():
> What about get_top_is_warm_edge? Not sure what could be explicit enough :/

I changed the whole part. We have now an option for hot_corners_delay and one for 
warm_edges_delay.

> model/language.py
> 
>     try:
>         for line in lines:
> Perhaps the try should be inside the for?

I took the try stuff out completely. Did not make sense what it is meant to be caught.

>                 loc = line.lstrip('locale:')
> s/loc/locale, etc

yup

>         fd.write('LANG="en_US.utf8"\n')
> constant?

done.

>         for i in range(len(lines)):
> wouldn't work this below?:
>         for line in lines:

Yup.

>         print(_("Could not access %s. Create standard settings.") % path)
> Translatable strings shouldn't be duplicated. Maybe is better to add a private
> method?

I changed the to not contain a variable and moved the string to a constant.

>     for lang in languages:
> instead:
>     for language, territory, locale in languages:

Done.

> 
> model/network.py
> 
>     if state in (0,1):
> space after comma

+1


> def get_all_timezones(fn='/usr/share/zoneinfo/zone.tab'):
> constant?

Yup.

>     """Set the system timezone
>     server : e.g. 'America/Los_Angeles'
>     """
> server?

:) Changed.

> Thanks,
> 
> Tomeu