[Sugar-devel] [PATCH] Add cpu and memory resource indicator to frame

[Sascha Silbe]

Excerpts from anishmangal2002's message of Thu Jul 01 15:03:16 +0000 2010:

[class DeviceView(TrayIcon)]

> +    def __init__(self):
> +        icon_name = 'computer'
> +
> +        client = gconf.client_get_default()
> +        color = XoColor(client.get_string('/desktop/sugar/user/color'))
> +        TrayIcon.__init__(self, icon_name=icon_name, xo_color=color)
I wonder if we should make the owner color the default for a TrayIcon.
(Nothing to be changed in your patch, just something for the core devs
to think about)

> +    def create_palette(self):
> +        palette = ResourcePalette(_('System Resources'))
Should "Resources" be lower case instead? (Question to native english
speakers)

> +    def __button_release_event_cb(self, widget, event):
> +        return True
> +
> +    def __expose_event_cb(self, *args):
> +        pass
Why do you connect these signals if you don't use them? Or is there
some side effect I'm missing?

[class ResourcePalette(Palette)]
> +    def __init__(self, primary_text):
[...]

> +        gobject.timeout_add(1000, self.__timer_cb)
The use of polling (i.e. a timer) seems to be necessary as /proc/stat
doesn't provide a better interface. (I.e. the way you do it is fine)

> +    def __getTimeList(self):
Single underscore should suffice. No one is going to subclass this, and
even then it's not something that needs protection from being overridden
in the subclass.

> +        statFile = file("/proc/stat", "r")
Tomeu will nag you about "" vs. '' ;)
(always use '' unless there's an apostrophe in the string itself).

> +        timeList = statFile.readline().split(" ")[2:6]
> +        statFile.close()
> +        for i in range(len(timeList)) :
> +            timeList[i] = int(timeList[i])
> +        return timeList

Personally I'd prefer:

        return [int(count)
            for count in file('/proc/stat').readline().split()[2:6]]

Tomeu might disagree as he tends to consider list comprehensions
(especially with more than one line) as harder to understand.

Please document what exactly __get_TimeList() does, including what the
returned values mean. (use a docstring)

> +    def __timer_cb(self):
> +        if self.updating:
> +            # Computing CPU usage statistics
If you factor this out into a separate function, you can turn this comment
into a DocString and elaborate on exactly what you are computing. I didn't
understand it even after looking up the syntax of /proc/stat.

> +            self.proc_stat_new = self.__getTimeList()
If you make all the other attributes private, you should make proc_stat_*
private as well.
You can probably even git rid of storing proc_stat_new in the instance
at all - it doesn't need to be accessible across iterations (= timer
callback invocations).

> +            for i in range(len(self.proc_stat_new)) :
> +                self.proc_stat_new[i] -= self.proc_stat_old[i]
A better set of variable names might help understanding what this does
and why it is necessary (/proc/stat entries are absolute times in
USER_HZ units). cpu_time_counts might be a start.

> +            cpu_free = (self.proc_stat_new[len(self.proc_stat_new) - 1] * 100.00 /
> +                    sum(self.proc_stat_new))

This equates to (iowait * 100) / (user + nice + system + idle + iowait),
so it's the percentage of time spent in iowait. I don't think it's a
useful figure for how busy (or idle) the CPU is.
If we decide not to split up iowait (it indicates permanent storage
activity, which we don't have an indicator for on XO-1) and idle,
we should at least add them:

        cpu_times_current = [new - old
            for new, old in zip(cpu_time_counts_new, self._cpu_time_counts)]
        user, nice, system, idle, iowait = cpu_times_current
        cpu_free = (idle + iowait) * 100 / sum(cpu_times_current)

BTW: the len(...) call is superfluous. In Python negative indices are
interpreted relative to the end of the list.

[...]
> +        self.proc_stat_old = self.__getTimeList()
This is a race condition (/proc/stat might have been updated since you
read it). Use the old return value instead.

> +    def __popup_cb(self, gobject):
> +        self.updating = True
> +
> +    def __popdown_cb(self, gobject):
> +        self.updating = False

Please stop the timer instead. Calculating values while we don't need them wastes energy and will prevent the CPU from entering low-power states (i.e. auto-suspend) in the future.

> +    def __percentage_memory_available(self):
> +
Please remove the extra empty line.

> +        for line in file('/proc/meminfo').readlines():
You don't need readlines() here, a file is an iterator itself (and will
return one line per iteration).

> +            name, value, unit = line.split()[:3]
> +            if 'MemTotal:' == name: total = value
> +            elif 'MemFree:' == name: free = value
> +            elif 'Buffers:' == name: buffers = value
> +            elif 'Cached:' == name: cached = value
> +            elif 'Active:' == name: break
> +
> +        return 100 * (int(free)+int(buffers)+int(cached)) / int(total)
If you move the int() calls to the if/elif block above, the last line is
easier to read.

> +def setup(tray):
> +    tray.add_device(DeviceView())
Please check for existance of /proc/stat. While I don't know of anyone
running Sugar on non-Linux kernels (e.g. Debian GNU/kFreeBSD [1]),
we should try to not break things if it can be (easily) avoided.

Sascha

[1] http://www.debian.org/ports/kfreebsd-gnu/
--
http://sascha.silbe.org/
http://www.infra-silbe.de/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
Url : http://lists.sugarlabs.org/archive/sugar-devel/attachments/20100702/7525b996/attachment-0001.pgp