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

[Eben Eliason]

I'll /try/ to keep my comments mostly to the UI and leave the code
review for others.  I'll also fail at that to some extent, since I
have curiosities about bits and pieces.

On Thu, Apr 24, 2008 at 7:33 AM, Martin Dengler
<martin at martindengler.com> wrote:

>  +        return self._master.flags & gst.interfaces.MIXER_TRACK_MUTE \
>  +                 == gst.interfaces.MIXER_TRACK_MUTE

If MIXER_TRACK_MUTE is a bit mask, isn't the equality check redundant?

>  +        #sometimes we get a spurious zero from one/more channel(s)
>  +        #we could just pick the first channel's volume, but this converges
>  +        #sometimes alsa fails to set all channels' volume, so try a few times

That's all fairly ugly, huh?  Oh well.

>  +        badge_name = None
...
>  +        self.icon.props.badge_name = badge_name

What's the logic for leaving this in?  Is there a circumstance in
which you think we may later add a badge, because at the moment this
variable is never used.

>  +    def _mute_item_text(self):
...
>  +        return _(mute_item_text)

Defining this in a function seems to work OK here, but I think I also
want icons on the menu item, which also depends on the current state.
As such, it may actually be cleaner to have an _update function of
some kind which handles both text and image together, setting them
directly instead of returning a value.  I suppose you could also have
separate functions for _mute_item_text and _mute_item_icon, as
well....I'll let the others decide.

Let's use the dialog-ok and dialog-cancel icons for now, which will
match the current mockups for the mic and camera.  We can change them
easily later if we need to.

>  +        mute_item_text = self._model.props.muted and 'Unmute' or 'Mute'

This is a tricky ternary stand-in.  Very clever.  Is it clear enough for others?

>  +        mute_item_text += '...'

This is a bad habit that everyone seems to get into.  Cut this line.
Every menu item takes an action when clicked, and that's implicit,
making the ellipsis effectively redundant. The ellipsis should only be
used when (a) the text represents a "state of flux", eg. in the AP
palettes /while/ "Connecting..." (not on the "Connect" menu item
itself, which initiates it) or (b) The action of the menu item itself
reveals a dialog which requires further feedback before actually
initiating a "real" action.

- Eben