[sugar] control panel review

[Tomeu Vizoso]

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?

    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

    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?

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

            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?

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

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

gui.py

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

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

            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?

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

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

            {'optionname': {'view', 'model', 'button', 'keywords', 'icon'} }
In python, would be {'optionname': ['view', 'model', 'button',
'keywords', 'icon'] }

        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):

                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?

                if tmp in self._options:
This means we cannot have one section just with command line support?

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

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

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

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

        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)

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.

    def __init__(self, mod):
        self._mod = mod
s/mod/module?

class _GridWidget(gtk.EventBox):
    __gtype_name__ = "SugarGridWidget"
What about calling it _ModuleIcon instead?

        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.


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?

Why EventBox and not HBox or Bin?

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


sectionview.py

_ = lambda msg: gettext.dgettext('sugar', msg)
Not needed.

        self.valid_section = True
Should be private? s/valid_section/is_valid?

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

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

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


view/aboutme.py

class EventIcon(gtk.EventBox):
What's the reason of existence of this class?

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


view/aboutxo.py

    def __init__(self, model=None, alerts=None):
Might make sense to split the constructor in _setup_identity(), etc?

        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?


view/frame.py

        self._delay_valid = True
s/delay_valid/is_delay_valid?

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

        state = ('off', 'on')[widget.get_active()]
Should be constants?

    def __delay_changed_cb(self, widget, data=None):
s/widget/scale?

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


view/language.py

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

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

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

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

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

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


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.

    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?

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


model/frame.py

def set_delay(value):
s/value/delay

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


model/language.py

    try:
        for line in lines:
Perhaps the try should be inside the for?

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

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

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

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

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


model/network.py

    if state in (0,1):
space after comma

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

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

Thanks,

Tomeu