[sugar] [PATCH] Add speaker device and icon by default

[Martin Dengler]

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.

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


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

> >  +        TrayIcon.__init__(self, icon_name=_ICON_NAME,
> >  +                          xo_color=profile.get_color())
> 
> You can align this with "self, ..."

Will do...code was cut & pasted...

> +class SpeakerPalette(Palette):
> >  +
> >  +    def __init__(self, primary_text, model):
> >  +        Palette.__init__(self, primary_text)
> 
> It would be better to pass primary text as a keyword arg, using the
> property API.  Passing the standard arg directly is deprecated.

Again, cut & pasted...will do.

> >  +        self._mute_item = MenuItem(_('Unmute'))
> 
> Tomeu will whine here because you use this string in two places.

I didn't like this either...I wasn't sure about initializing it to
None, and now you've clarified that, so I'll set it to ''.

> >  +        self.set_content(vbox)
> 
> Any need to put this down below the MenuItem creation?  It seems oddly
> detatched from the other VBox stuff.

Nope.

> >  +            mute_item_text = 'Unmute'
> ...
> >  +            mute_item_text = 'Mute'
> 
> These should be localized!

They are:

self._mute_item.get_child().set_text(_(mute_item_text))

> - 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/sugar/attachments/20080425/29cf4571/attachment.pgp