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

[Martin Dengler]

[apart from some feedback on your first two comments I can get a new
patch out later today -- so can you focus on those sections please?]

On Thu, Apr 17, 2008 at 12:00:48AM -0400, Eben Eliason wrote:
> 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.

The purpose of this variable (lvl) is twofold:

1) Read things once: it's not threadsafe to access self._level twice
and expect to read the same value.  Yes, python doesn't have the same
thread support as other languages, and maybe self._level doesn't
change very often at all, but why write code that once in a while, for
one or two people, is going to make the text show "50%" and the slider
show "49%"?  It'd be messy, and it's easy to avoid by just reading the
_level once;

2) once I started getting a little (yes, overly) paraoid by #1 (I
originally wrote the code just as you've recommended, FWIW :)), I
realized some would think it's a bit nicer to not have multiple reads
for the same value, and that a shorter variable name would make the
code indentation a bit nicer.  This point is admittedly very minor,
but adds a tiny bit of weight to #1.

The purpose of status_text and secondary_text is different: rather
than set them in each branch of the conditional, or (I think worse,
but IIUC you prefer), do something with it "inline" in each branch, I
just mutate it in the (few) branches of the conditional as appropriate
and then do something, once, with it.

Compare these structures:

1.
sometext = ''
if a:
   ...
elif b:
   if b1:
   ...
else:
   sometext = 'foo'
dosomething(sometext)

...is, all other things being equals, better than (less duplication):

2.
if a:
   sometext = ''
   ...
elif b:
   sometext = ''
   if b1:
   ...
else:
   sometext = 'foo'
dosomething(sometext)

...and probably even "more better" (ugh, but perhaps you know what I
mean) than:

3.
if a:
   dosomething('')
   ...
elif b:
   dosomething('')
   if b1:
   ...
else:
   dosomething('foo')

I think #3 is particularly bad because you now make a maintainer do
this:

if a:
   dosomething('')
   dosomethingelse('')
   ...
elif b:
   dosomething('')
   dosomethingelse(')  <-- haha look...found this bug when proofreading!
   if b1:
   ...
else:
   dosomething('foo')
   dosomethingelse('foo')

rather than just:

sometext = ''
if a:
   ...
elif b:
   if b1:
   ...
else:
   sometext = 'foo'
dosomething(sometext)
dosomethingelse(sometext)

Am I overlooking some other consideration(s)?

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

I could add the badge and the notification, sure.  Do you want that in
a separate patch?  Adding a badge is trivial (you want the
emblem-warning.svg, I guess, not emblem-notification.svg or anything
forthcoming, right?).  It'd be a bit quicker if I could do the
notification in a separate patch, I think, as I'm not sure how to do
that yet.

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

Ok -- your design had the "3 hours remaining", so I probably shouldn't
have played with your design without asking you first.  I'll change it
to just H:mm (as you also request with your code below).

> Also, I'd use a more descriptive variable name like
> time_left; guess_text is somewhat ambiguous, apart from the context.

Sure -- I wanted the variable name to scream "this code knows that
it's not doing a good job of calculating the time remaining", but of
course it's a very tiny thing to change.

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

Cool!  Will do.

> >  +                    mins_leftover = minutes_left % 60
> 
> I would personally just overwrite the minutes_left variable here
> instead of creating a new one.

Yeah, I was close to doing that too and felt for some reason it was a
bit sloppy, but happy to have confirmation that it's preferable!

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

Exactly.  I think we've given enough people enough time to object, and
I haven't heard any (though have received some excellent suggestions
as to where The Right Way might be being implemented), and - as I
noted in my patch/commit comment - the algorithm is close to what a
non-techy human might do, so at least isn't going to be wrong in a
surprising (disappointing) way.  So let's just do it.  I've been using
it for a few days now and it's not offensive (I don't find myself
thinking "ooooh, I know that's wrong and it's wrong by...let's
see...just working it out..." etc. etc.).

> - Eben

Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.laptop.org/pipermail/devel/attachments/20080417/bb4e3568/attachment.sig>