On Fri, Apr 25, 2008 at 1:46 PM, Martin Dengler
<[EMAIL PROTECTED]> wrote:
> On Fri, Apr 25, 2008 at 01:21:50PM -0400, Eben Eliason wrote:
>  > Alright...I'll go at it again, sits it's been sitting around for a day... 
> =)
>  >
>  > >  +        nonzero_vols = [v for v in volumes if v > 0.0]
>  >
>  > Is it actually necessary to compare against a forced float (0.0) here?
>
>  Probably not.  The sequence elements are floats, so I figured why not.

It's not wrong.  I've just never seen it done. =) Obviously, 0 is
losslessly representable as both int and float.

>  > >  +        if len(nonzero_vols) != 0 and len(nonzero_vols) != 
> len(volumes):
>  > >  +            volumes = nonzero_vols
>  >
>  > The second condition seems unnecessary.
>
>  Possibly.  We want to eliminate the nonzero volumes, but what if there
>  are no nonzero volumes?  It's only unnecessary because sum([]) - used

If there are none at all, then the first condition short circuits
anyway, and we're in good shape.

>  later on - turns out to be zero (which I thought of now because it
>  wasn't 5am).  So I could just replace the whole volumes = ... return
>  volumne ... with:
>
>  ...
>  # sometimes we get a spurious zero from one/more channel(s)
>  nonzero_volumes = self._mixer.get_volume(self._master)
>  volume = sum(nonzero_volumes) / len(nonzero_volumes)
>  ...
>
>  ...and then it'd work.  I'll do that.

Sounds much nicer.

>  > >  +        #sometimes alsa fails to set all channels' volume, so try a 
> few times
>  >
>  > What's the rate of failure here?
>
>  About 25-50% if I hold down one of the arrow kesy while the slider has
>  focus and watch alsamixer's indication of the L+R channels' volume
>  from an ssh session into the laptop.
>
>  >  If we're always ignoring the zeros when reading the volume, does it
>  > matter if we do extra work to be sure to set them all?
>
>  Yes, because reading of zeros and the failure to set the volumes have
>  different solutions.  The comment should better read
>
>  # sometimes alsa sets a channel's volume to zero rather than what we
>   tell it to
>
>  ...and the problem is now sufficiently clear to justify the retry code.

Yuck.  And OK.


>  > >  +            mute_item_text = 'Unmute'
>  > ...
>  > >  +            mute_item_text = 'Mute'
>  >
>  > These should be localized!
>
>  They are:
>
>
>  self._mute_item.get_child().set_text(_(mute_item_text))

Oh, I missed that.  I would wrap the localization around the string
literal, so the connection is clear. (Yes, you have to use it twice
instead of once....but it was imported as _ for a reason. ;) )

- Eben
_______________________________________________
Sugar mailing list
Sugar@lists.laptop.org
http://lists.laptop.org/listinfo/sugar

Reply via email to