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

Eben Eliason eben.eliason at gmail.com
Fri Apr 25 13:21:50 EDT 2008


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?

>  +        if len(nonzero_vols) != 0 and len(nonzero_vols) != len(volumes):
>  +            volumes = nonzero_vols

The second condition seems unnecessary.  If they are the same length,
they are the same, and performing the reassignment is not an issue.
Also, the first comparison would read more clearly if it used a
greater than comparison, instead of inequality, since the length
cannot be negative.

>  +        #sometimes alsa fails to set all channels' volume, so try a few times

What's the rate of failure here?  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?  For that matter, why do we need to mess with a bunch
of channels at all? (Pardon my ignorance...feel free to ignore these
questions entirely.)

>  +        TrayIcon.__init__(self, icon_name=_ICON_NAME,
>  +                          xo_color=profile.get_color())

You can align this with "self, ..."

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

>  +        self._mute_item = MenuItem(_('Unmute'))

Tomeu will whine here because you use this string in two places.  You
could instead initialize it with a null string and call _update_info
to set the label appropriately.  For some reason, it seem that you
cannot simply pass None (or leave out the argument completely), but
must instead pass '' (null string) instead.

>  +        self.set_content(vbox)

Any need to put this down below the MenuItem creation?  It seems oddly
detatched from the other VBox stuff.

>  +            mute_item_text = 'Unmute'
...
>  +            mute_item_text = 'Mute'

These should be localized!

- Eben


More information about the Sugar mailing list