[sugar] [PATCH] Add speaker device and icon by default
Martin Dengler
martin
Fri May 30 09:45:18 EDT 2008
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
More information about the Sugar-devel
mailing list