[sugar] [PATCH] support battery-charge-state-dependent battery frame icon

[Eben Eliason]

So, I'm not the authoritative source on this, but I'll toss in my thoughts...

>  +        lvl = self._level
>  +        secondary_text = ''
>  +        status_text = '%s%%' % lvl

I would avoid declaring variables unless they really make serve a
purpose.  Here it seems to add a level of indirection to what's going
on, actually making it less clear.  For instance, lvl = self._level
just duplicates a private variable that already exists, which for that
matter is only used twice.  The same goes for status_text...it's only
used in one place.  The introduction of the secondary_text variable
also adds indirection, since you have to follow it through to its use
in the assignment at the end.  It would be clearer to do this
assignment in place, skipping the temp variable.

>  +            if lvl <= 15:
>  +                secondary_text = _('Very little power remaining')

This is probably the correct place to fire off a notification, and
perhaps even badge the icon with a warning badge.  Of course, all of
the necessary components aren't in place for this to work perfectly
just yet...not sure if it's worth adding at this point.  It would
depend, at a minimum on Eric Burns' forthcoming notification patch for
specifying the appropriate screen corner.

>  +                minutes = _('m')
>  +                hours = _('h')
>  +                #TODO: make this less of an wild/educated guess
>  +                minutes_left = int(lvl / 0.59)
>  +                if minutes_left < 60:
>  +                    guess_text = '%s%s' % (minutes_left, minutes)
>  +                else:
>  +                    hours_left = minutes_left / 60
>  +                    mins_leftover = minutes_left % 60
>  +                    guess_text = '%s%s%s%s' % (hours_left, hours,
>  +                                               mins_leftover, minutes)

I think I'd prefer the format "1:23 remaining", without the extra 'h'
and 'm' business.  Also, I'd use a more descriptive variable name like
time_left; guess_text is somewhat ambiguous, apart from the context.
In fact, I might forego that string completely in favor of calculating
hours_left and mins_left variables, and then directly doing:

self.props.secondary_text = '%d:%.2d %s' + (hours, mins, _('remaining'))

Note that we need to ensure the minutes string contains two digits
(.2), and also that we are passing integers (%d).

>  +                    mins_leftover = minutes_left % 60

I would personally just overwrite the minutes_left variable here
instead of creating a new one.

Of course, many of my changes depend on whether or not people think
that the guesswork employed here is worth implementing.

- Eben