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

[Martin Dengler]

On Wed, May 14, 2008 at 12:14:57PM +0200, Tomeu Vizoso wrote:
> Hi, sorry for the delay.

Same here - was on holiday.

> 
> On Tue, May 13, 2008 at 12:43 AM, Martin Dengler
> <martin at martindengler.com> wrote:
> >  +    def get_mute(self):
> 
> You use 'muted' instead of 'mute' below, which one is more correct?

I prefer 'muted', because 'muted' sounds more like a predicate/boolean
to me and 'mute' sounds more like a verb.  (consider 'get_stopped()'
vs. 'get_stop').  Changed get_mute() to get_muted().

> >  +        if not self._mixer or not self._master:
> >  +            logging.error('Cannot get the mute status')
> >  +            return False
> 
> Shouldn't we return True if there's no way to get the volume?

I thought we should assume we're not muted if we don't know what the
volume is, but I think I see your point.  I don't think the return
value is sensible in this scenario, though, so perhaps we should
raise()?  I don't have strong feelings, so I've changed to return
True.

> >  diff --git a/src/model/devices/Makefile.am b/src/model/devices/Makefile.am
> >  index 5440eeb..3124144 100644
> >  --- a/src/model/devices/Makefile.am
> >  +++ b/src/model/devices/Makefile.am
> >  @@ -5,4 +5,6 @@ sugar_PYTHON =                  \
> >         __init__.py             \
> >         device.py               \
> >         devicesmodel.py         \
> >  -       battery.py
> >  +       battery.py              \
> >  +       speaker.py
> 
> Just as a comment, the convention is to sort files alphabetically (it
> was already wrong).

Gotcha - I didn't want to reorder but as I'm changing the battery.py
line anyway I'll reorder it.

> >  @@ -54,6 +55,13 @@ class DevicesModel(gobject.GObject):
> >
> >          for udi in hal_manager.FindDeviceByCapability('battery'):
> >              self.add_device(battery.Device(udi))
> >
> >  +        try:
> >
> > +            self.add_device(speaker.Device())
> >  +        except Exception, speaker_fail_msg:
> >  +            logging.error("could not initialize speaker device: %s" %
> >  +                          speaker_fail_msg)
> >  +            logging.debug(traceback.format_exc())
> 
> I would add the speaker device in the constructor, instead of in
> _observe_hal_manager().

It should be in the constructor, I agree.  I'm sure I put it there
but...must've slipped a method somehow.  Scary.  Moved.

> Perhaps we should add try..except blocks to all the add_device calls?
> Not in this patch, though.

Yes, possibly.  It's the Device-subclass __init__ and the .py file
import itself that we have to worry about, though, I think (consider
that the logical extension of what you've said would be to just put a
try...except in add_device()).  Something to think about in a later
patch, I agree.

> >  +    def get_type(self):
> >  +        return 'speaker'
> 
> Maybe should be a constant at the module level?

Every Device-subclass implements this method in a similar form.  I'm
not a huge fan of this function - __name__ should be enough.
AFAICS, it's just a way of helping deviceview.py with the fact that
model & view implementations are split at a high level
(src/{model,view}/...):
http://dev.laptop.org/git?p=sugar;a=blob;f=src/view/devices/deviceview.py;h=90ebbf55413925f537a9e9e900d3828bb4f28bac;hb=HEAD
.

Assuming you prefer the consistency I'll leave this as-is, but I'm not
at all bothered about changing it.

> >  +    def do_get_property(self, pspec):
> >  +        if pspec.name == "level":
> >  +            return self._get_level()
> >  +        elif pspec.name == "muted":
> >  +            return self._get_muted()
> 
> See above comment about muted vs mute.

ibid/fixed.

> >  +        self._adjustment = gtk.Adjustment(
> >  +            self._model.props.level, 0.0, 101.0, 1, 1, 1)
> 
> Not the most common of breaking an arg list, but I don't particularly care.

I don't know much gtk so the Adjustment positional args mean little to
me - I have to look them up each time I care - so far about twice :).
I've looked them up a third time and tried a bit more sensical split.

> Thanks!

Thank you!

> Tomeu

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/20080530/b65f065d/attachment.pgp