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

[Tomeu Vizoso]

Some comments on top of Sascha's excellent review:

On Fri, Jul 2, 2010 at 12:03, Sascha Silbe
<sascha-ml-ui-sugar-devel at silbe.org> wrote:
> Excerpts from anishmangal2002's message of Thu Jul 01 15:03:16 +0000 2010:
>
>> +        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)

We should check timeout_add() doesn't add a reference to self. I used
to at least some time ago and it would mean leaking palette instances
and timers. It may have been made to use weak references in the mean
time, not sure.

If that's the case, we need to remove the timer on popdown and create
a new one on popup.

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

We use lowercase_separated_by_underscores for method names, see
"Naming Conventions" in http://www.python.org/dev/peps/pep-0008/ .

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

Also, lowercase_separated_by_underscores for all variables.

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

True, but keep in mind the potential issue with leaking Palette instances.

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

We try to avoid multiple statements in a single line, so lines need to be split.

A few more nitpicks:

+from gettext import gettext as _
+
+import logging

logging should be grouped with gettext because both are in the python
std library.

+from sugar.graphics import style
+from jarabe.frame.frameinvoker import FrameWidgetInvoker

The jarabe import should be in a separate group because is part of the
currently running program.

+        icon_name = 'computer'

You may want to make it a constant similar to FRAME_POSITION_RELATIVE.

+        self.updating = False
+        self.proc_stat_old = []
+        self.proc_stat_new = []

This should be private (leading underscore).

+            self._cpu_text.set_label('CPU free: %d%%' % cpu_free)

We don't want to translate this string?

+        vbox.pack_start(self._cpu_text, padding=10)

The padding shouldn't be hardcoded to a number of pixels, see the
constants in style.py.

+        statFile.close()

Maybe use try..finally?

+        for i in range(len(timeList)) :

for i in time_list: doesn't work?

+        return 100 * (int(free)+int(buffers)+int(cached)) / int(total)

Operators should have spaces around.

Great patch!

Thanks,

Tomeu