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

[Martin Dengler]

On Thu, Apr 24, 2008 at 12:01:52PM -0400, Eben Eliason wrote:
> 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?

It's not a bitmask, IIUC.  So no, it's not redundant AFAICS.  But yes,
it'd read much more nicely without the equality check if it wasn't.

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

I know!

> >  +        badge_name = None
> ...
> >  +        self.icon.props.badge_name = badge_name
> 
> What's the logic for leaving this in?

Hmmm...I must've missed that pylint warning.  The variable goes.

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

Agree -- I just didn't have any icons, so no use for such a function
yet.

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

dialog-ok to go with the "Unmute" text, and -cancel to go with "Mute"?

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

I'll change to the proper ternary clause as recommended by bemasc.


> >  +        mute_item_text += '...'
> 
> This is a bad habit that everyone seems to get into.  Cut this line.

Sorry!  I know the rule(s) you cite, and must blame lack of sleep for
leaving this in.  I was cargo-culting "Disconnect..." from
network/wireless.py, I think.

> - 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/20080424/3da5ac11/attachment.pgp