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

[Eben Eliason]

On Thu, Apr 17, 2008 at 6:19 AM, Martin Dengler
<martin at martindengler.com> wrote:
> [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;

You make a good case. =)

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

I think I'd disagree on the indentation, actually.  Clarity is always
better in my mind, and it doesn't look like you're in danger of going
past 80 chars.  I guess my biggest concern is that I keep reading
"absolute value of v" instead of "level" when I see it.  Heh.  Perhaps
you could even indicate your intent for the variable as described
above by calling it curr_level or something.

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

>  Am I overlooking some other consideration(s)?

No, not really.  You have good points here as well; as I said, I'm not
an authority!  I'd let others offer their opinions up on this style.
Clearly you win in maintainability.

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

Yeah, I think a separate patch is fine.

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

Well, we're talking about 3 different approaches here.  I actually do
like the natural language when I see "3 hours remaining" or "21
minutes remaining", but "3 hours and 21 minutes remaining" gets a bit
long, and I'm not sure it's any more clear than "3:21 remaining"
anyway.  Plus, in order to do it correctly, you need to handle plurals
and other messiness, so basically I went back on my word (my design)
in light of the actual implementation details.

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

Then compromise with time_left_guess or estimated_time_left.  Make the
variable read clearly without needing to trace the code in context.

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

I think that switching to a static "very little power remaining" at
the point at which you do calms some fears.  The closer you get to the
bottom, the more a little inaccuracy is going to become noticeable.

- Eben