Re: [sugar] [PATCH] Add speaker device and icon by default
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
[EMAIL PROTECTED] 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
pgpeYmngRZ2Hb.pgp
Description: PGP signature
___
Sugar mailing list
Sugar@lists.laptop.org
http://lists.laptop.org/listinfo/sugar
Re: [sugar] [PATCH] Add speaker device and icon by default
Hi, sorry for the delay.
On Tue, May 13, 2008 at 12:43 AM, Martin Dengler
[EMAIL PROTECTED] wrote:
+def get_mute(self):
You use 'muted' instead of 'mute' below, which one is more correct?
+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?
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).
@@ -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().
Perhaps we should add try..except blocks to all the add_device calls?
Not in this patch, though.
+def get_type(self):
+return 'speaker'
Maybe should be a constant at the module level?
+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.
+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.
Thanks!
Tomeu
___
Sugar mailing list
Sugar@lists.laptop.org
http://lists.laptop.org/listinfo/sugar
Re: [sugar] [PATCH] Add speaker device and icon by default
On Tue, May 06, 2008 at 05:40:30PM +0100, Martin Dengler wrote: On Tue, May 06, 2008 at 04:39:30PM +0200, Tomeu Vizoso wrote: On 5/4/08, Martin Dengler [EMAIL PROTECTED] wrote: On Tue, Apr 29, 2008 at 02:12:58PM +0200, Tomeu Vizoso wrote: I think that Michael has spotted here a wonderful opportunity for making Sugar more robust, thanks to your patch. ... I haven't forgotten this thread, just been unable to work on it. After working on it a bit more I think the right thing to do is just the small change Tomeu suggested. I would really like in the future to submit a patch that allows one to place a few files in a devices.d/ subdir - like model.py, view.py, icon.svg, etc. - and then the devicesmodel.py code could try to load each model.py appropriately, and so forth, but right now most of our devices really are device brokers (hal-dbus, networkmanager) and such a setup doesn't mesh well with them. Obviously in this patch would be the do the initialization lazily and safely-type of code that would make m_stone a lot happier. But in the meantime the two/three lines Tomeu suggested will still make things safer without adding lots of code; I will resubmit the patch and hopefully that will make everyone a bit happier. Thanks, Tomeu Martin Martin pgp19PvPR6OJ3.pgp Description: PGP signature ___ Sugar mailing list Sugar@lists.laptop.org http://lists.laptop.org/listinfo/sugar
Re: [sugar] [PATCH] Add speaker device and icon by default
On Tue, May 06, 2008 at 04:39:30PM +0200, Tomeu Vizoso wrote:
On 5/4/08, Martin Dengler [EMAIL PROTECTED] wrote:
On Tue, Apr 29, 2008 at 02:12:58PM +0200, Tomeu Vizoso wrote:
I think that Michael has spotted here a wonderful opportunity for
making Sugar more robust, thanks to your patch.
...
I haven't forgotten this thread, just been unable to work on it.
After an hour of messing about with some ideas/code today, my current
approach is a bit more involved than just a try/except but nothing too
dramatic (just a small yak to shave):
- refactor sugar/model/devices/device{,smodel}.py to make
adding/removing device.Device subclasses safe
... this begs one to do the next step or be left with many many
try/excepts and very ugly code (or just one try/except around
speaker.py, which kind of doesn't improve matters that much - though
it's very simple code and I'm happy to move on to other things if
people would accept that :) )
- refactor sugar/model/devices/device{,semodel}.py to make discovering
device python files safe using metaclasses (see
http://blogs.gnome.org/jamesh/2008/02/12/python-metaclasses/ ); I
have considered the objections and alternatives including martian,
Zope's plugin framework design, and settled on this approach because
it's very simple, meets the very simple needs we have, and is very
little code
...this requires one to remove the networkmanager- and
halmanager-specific logic in devicesmodel.py:
- create a new networkmanager model class that will add_device() and
remove_device() using the same logic that used to be in
devicesmodel.py
- create a new halmanager model class in the same way
...and then we only need to:
- trivially update battery.py and speaker.py
- pretty trivially update network/mesh.py, wired.py, wireless.py;
these are only slightly more than trivial because devicesmodel.py
used to define some very network-specific variables that (IMO)
should be exposed by the networkmanager model (above) instead
This explanation is long-winded only because I wanted to allow people
to poke holes in the approach, not because it's a lot of work.
If it's not much work, can you provide a patch that gives an idea of
what you plan to do? No need to be the final patch right now.
Your ideas look interesting but I'm having a bit of difficulty in
visualizing how you would refactor things.
This is not a patch because I think it's easier to read due to the
volume of refactoring, and it's very very much incomplete and
unpolished (as I said it's an hour's work), but if you look at them in
order you'll get an idea of what I'm trying, hopefully:
http://pastebin.be/11020 - devicesmodel.py - note this is refactored
to be much simpler, and the major *new* feature is simply the
metaclass usage (I still have to make it import everything in the
model.devices subtree, but you see where it's going)
http://pastebin.be/11021 - device.py - again, much simpler now and the
main changes are 1) register with the metaclass; and 2) subclassers
shoudl implement realize() rather than __init__() to do the real work
http://pastebin.be/11022 - battery.py - example of the changes; they
are basically trivial (superclass, move __init__() to realize(), call
super in realize())
http://pastebin.be/11023 - networkmanagermodel.py - this is where a
lot of devicesmodel.py has gone, because this is really a meta
device that creates new devices.Device subclasses as NM tells us
about device appearance/disappearance. This is completely in flux and
definitely doesn't work, but at least the comments might show you
where I'm going (this is my bullet point #3 above)
Thanks,
Tomeu
Martin
pgp6oA04ymjJy.pgp
Description: PGP signature
___
Sugar mailing list
Sugar@lists.laptop.org
http://lists.laptop.org/listinfo/sugar
Re: [sugar] [PATCH] Add speaker device and icon by default
I'm not really sure about the value of this kind of try/except blocks. If the target is to avoid the shell exiting in any case, we could as well try/except the whole code executed before gtk.main(). Marco On Tue, Apr 29, 2008 at 2:12 PM, Tomeu Vizoso [EMAIL PROTECTED] wrote: On Tue, Apr 29, 2008 at 1:37 AM, Martin Dengler [EMAIL PROTECTED] wrote: On Mon, Apr 28, 2008 at 06:12:31PM -0400, Michael Stone wrote: On Mon, Apr 28, 2008 at 10:26:21PM +0100, Martin Dengler wrote: On Mon, Apr 28, 2008 at 01:08:04PM -0400, Michael Stone wrote: Can calls to self._mixer or self._master fail even when these attributes are not None? It doesn't appear a concern from the existing hardwaremanager.py code, and not in practice: I've tested checking/changing the volume in a few activities. I seem to recall having encountered situations where sugar startup would fail (in early versions of the QEMU image, before sound began working) as a result of unchecked use of sound hardware. I fixed the problem by commenting out volume control in the sugar shell. It was a particularly annoying problem because it was persistent, which meant that X entered an infinite reboot loop. Yes, that problem exists and my patch is no worse in this respect - I meant to make both points very explicit later in the email to which you replied. Given the clear implication that we should do better, I'll change my patch. If you, or marco, or anyone has an opinion about where the best place to introduce the real paranoia, please let me know. OTTOMH, given we obviously want to make Sugar not crash and (secondarily) minimize spamming of 'try:...except's, hardwaremanager.py's where I would introduce the bulk of the changes (rather than make speaker.py, randomactivity.py, presence-palette-that-wants-to-make-a-dinging-noise.py, etc. do this). If anyone thinks that's controversial let me know. The original author of the hardwaremanager.py trusted the gst classes just as much as I am... also keep in mind that I actually saw and worked around two failures (#6933, #6934) of exactly these attributes/implementations, In your opinion, did the original author of hardwaremanager.py (or authors of the clients of hardwaremanager.py?) exercise the degree of caution necessary to deliver a solid, reliable Sugar experience to our present audience? (I say present audience because I think that our audience has changed from one which consisted primarily of developers to one which consists primarily of semi-literate children.) As a rhetorical question I think I understand the point. As a non-rhetorical question, I'll decline to answer due to lack of context/familiarity with the conditions. Also, what happens if an exception is thrown by, say, Device.__init__()? Given the current state of error checking, sugar (the shell) will fail to start and matchbox will exit (I saw this during testing, and just now tried 'raise Exception(we love m_stone)' to confirm.) Is the exception properly recorded? I'm sorry, I will have to research the proper way to record such an exception. I don't know. Is it possible (easy?) to send the traceback upstream to us? Very interesting idea. Is there a plan for aggregating such data? cscott's FISL presenation had some (http-sourced?) usage graphs...is there a Send Microsoft a Report- (or We Share Your Pain :)) like server to which our code could send such reports? Do you want automated/staged trac submissions? The design/architecture/maintenance solution space is well beyond my time to explore. Lacking any leads therein I'm going to answer to your question as I know not this 'upstream', would be happy to use one, but have no resources to build one. Device.__init__() is four lines of code, and depends on util.unique_id() and gobject.GObject.__init__(). Device.__init__() sounds like the sort of thing that I expect will be overridden by subclasses in interesting ways and it sounds like the sort of thing that will break when people try to run Sugar on platforms we haven't tested pre-qualified. I think you're encouraging me to make Device.__init__() a bit safer, or add some comments, or something similar, rather than changing speaker.py's __init__(). It's going to get a bit hairy if we can't trust our superclass(es)'s constructor(s). Or perhaps you'd have me consider patching devices.py, to survive any of its devices' not initializing. If you / anyone has a preference for either approach, or how I structure the changes into one or more patches (this is part of the culture that I haven't picked up yet), please let me know.
Re: [sugar] [PATCH] Add speaker device and icon by default
The idea is that we have a complex system, and that value can be seen in having this system degrading gracefully when an unexpected error happens. Even more when is an area where kids are supposed to play with (adding device icons to the shell). Adding just one try..except block before gtk.main won't give us this graceful degradation. Tomeu On Tue, Apr 29, 2008 at 2:24 PM, Marco Pesenti Gritti [EMAIL PROTECTED] wrote: I'm not really sure about the value of this kind of try/except blocks. If the target is to avoid the shell exiting in any case, we could as well try/except the whole code executed before gtk.main(). Marco On Tue, Apr 29, 2008 at 2:12 PM, Tomeu Vizoso [EMAIL PROTECTED] wrote: On Tue, Apr 29, 2008 at 1:37 AM, Martin Dengler [EMAIL PROTECTED] wrote: On Mon, Apr 28, 2008 at 06:12:31PM -0400, Michael Stone wrote: On Mon, Apr 28, 2008 at 10:26:21PM +0100, Martin Dengler wrote: On Mon, Apr 28, 2008 at 01:08:04PM -0400, Michael Stone wrote: Can calls to self._mixer or self._master fail even when these attributes are not None? It doesn't appear a concern from the existing hardwaremanager.py code, and not in practice: I've tested checking/changing the volume in a few activities. I seem to recall having encountered situations where sugar startup would fail (in early versions of the QEMU image, before sound began working) as a result of unchecked use of sound hardware. I fixed the problem by commenting out volume control in the sugar shell. It was a particularly annoying problem because it was persistent, which meant that X entered an infinite reboot loop. Yes, that problem exists and my patch is no worse in this respect - I meant to make both points very explicit later in the email to which you replied. Given the clear implication that we should do better, I'll change my patch. If you, or marco, or anyone has an opinion about where the best place to introduce the real paranoia, please let me know. OTTOMH, given we obviously want to make Sugar not crash and (secondarily) minimize spamming of 'try:...except's, hardwaremanager.py's where I would introduce the bulk of the changes (rather than make speaker.py, randomactivity.py, presence-palette-that-wants-to-make-a-dinging-noise.py, etc. do this). If anyone thinks that's controversial let me know. The original author of the hardwaremanager.py trusted the gst classes just as much as I am... also keep in mind that I actually saw and worked around two failures (#6933, #6934) of exactly these attributes/implementations, In your opinion, did the original author of hardwaremanager.py (or authors of the clients of hardwaremanager.py?) exercise the degree of caution necessary to deliver a solid, reliable Sugar experience to our present audience? (I say present audience because I think that our audience has changed from one which consisted primarily of developers to one which consists primarily of semi-literate children.) As a rhetorical question I think I understand the point. As a non-rhetorical question, I'll decline to answer due to lack of context/familiarity with the conditions. Also, what happens if an exception is thrown by, say, Device.__init__()? Given the current state of error checking, sugar (the shell) will fail to start and matchbox will exit (I saw this during testing, and just now tried 'raise Exception(we love m_stone)' to confirm.) Is the exception properly recorded? I'm sorry, I will have to research the proper way to record such an exception. I don't know. Is it possible (easy?) to send the traceback upstream to us? Very interesting idea. Is there a plan for aggregating such data? cscott's FISL presenation had some (http-sourced?) usage graphs...is there a Send Microsoft a Report- (or We Share Your Pain :)) like server to which our code could send such reports? Do you want automated/staged trac submissions? The design/architecture/maintenance solution space is well beyond my time to explore. Lacking any leads therein I'm going to answer to your question as I know not this 'upstream', would be happy to use one, but have no resources to build one. Device.__init__() is four lines of code, and depends on util.unique_id() and gobject.GObject.__init__(). Device.__init__() sounds like the sort of thing that I expect will be overridden by subclasses in interesting ways and it sounds like the sort of thing that will break when people try to run Sugar on platforms we
Re: [sugar] [PATCH] Add speaker device and icon by default
On Fri, Apr 25, 2008 at 11:50 PM, Martin Dengler
[EMAIL PROTECTED] wrote:
+self.palette = SpeakerPalette(_('My Speakers'), model=model)
+self.set_palette(self.palette)
'set_palette' is the setter for the 'palette' property, so the second
line shouldn't be needed.
+model.connect('notify::level', self._speaker_status_changed_cb)
+model.connect('notify::muted', self._speaker_status_changed_cb)
+self.connect('expose-event', self._expose_event_cb)
Callbacks should start with two underscores in order to avoid name clashes.
The rest looks good to me.
Thanks,
Tomeu
___
Sugar mailing list
Sugar@lists.laptop.org
http://lists.laptop.org/listinfo/sugar
Re: [sugar] [PATCH] Add speaker device and icon by default
Can calls to self._mixer or self._master fail even when these attributes are not None? Also, what happens if an exception is thrown by, say, Device.__init__()? Thanks, Michael ___ Sugar mailing list Sugar@lists.laptop.org http://lists.laptop.org/listinfo/sugar
Re: [sugar] [PATCH] Add speaker device and icon by default
On Mon, Apr 28, 2008 at 10:26:21PM +0100, Martin Dengler wrote: On Mon, Apr 28, 2008 at 01:08:04PM -0400, Michael Stone wrote: Can calls to self._mixer or self._master fail even when these attributes are not None? It doesn't appear a concern from the existing hardwaremanager.py code, and not in practice: I've tested checking/changing the volume in a few activities. I seem to recall having encountered situations where sugar startup would fail (in early versions of the QEMU image, before sound began working) as a result of unchecked use of sound hardware. I fixed the problem by commenting out volume control in the sugar shell. It was a particularly annoying problem because it was persistent, which meant that X entered an infinite reboot loop. The original author of the hardwaremanager.py trusted the gst classes just as much as I am... also keep in mind that I actually saw and worked around two failures (#6933, #6934) of exactly these attributes/implementations, In your opinion, did the original author of hardwaremanager.py (or authors of the clients of hardwaremanager.py?) exercise the degree of caution necessary to deliver a solid, reliable Sugar experience to our present audience? (I say present audience because I think that our audience has changed from one which consisted primarily of developers to one which consists primarily of semi-literate children.) Also, what happens if an exception is thrown by, say, Device.__init__()? Given the current state of error checking, sugar (the shell) will fail to start and matchbox will exit (I saw this during testing, and just now tried 'raise Exception(we love m_stone)' to confirm.) Is the exception properly recorded? Is it possible (easy?) to send the traceback upstream to us? Device.__init__() is four lines of code, and depends on util.unique_id() and gobject.GObject.__init__(). Device.__init__() sounds like the sort of thing that I expect will be overridden by subclasses in interesting ways and it sounds like the sort of thing that will break when people try to run Sugar on platforms we haven't tested pre-qualified. and no other similar bit of code... does any more checking than this. Is this good? In particular, is it good that an exception that bubbles up through Device.__init__() causes X to enter an infinite restart loop? And please excuse me if I've missed a howler of a bug that you're socraticly/patiently trying to make me realize - just feel free to say outright what sucks and I'll fix it! You seem to be telling me that Sugar will crash if any of its device abstractions fails to initialize. That seems like a howler of a bug to me (though perhaps not one in your code). Is this desirable behavior? Is it intentional? Thanks for putting up me, Michael ___ Sugar mailing list Sugar@lists.laptop.org http://lists.laptop.org/listinfo/sugar
Re: [sugar] [PATCH] Add speaker device and icon by default
On Fri, Apr 25, 2008 at 1:56 PM, Eben Eliason [EMAIL PROTECTED] wrote: On Fri, Apr 25, 2008 at 1:46 PM, Martin Dengler self._mute_item.get_child().set_text(_(mute_item_text)) Oh, I missed that. I would wrap the localization around the string literal, so the connection is clear. (Yes, you have to use it twice instead of oncebut it was imported as _ for a reason. ;) ) Won't gettext fail to pick these up when generating the .pot file, the way it's written now? I was under the impression it just looks blindly through the source for _(quoted string) when building the translation files... Wade ___ Sugar mailing list Sugar@lists.laptop.org http://lists.laptop.org/listinfo/sugar
Re: [sugar] [PATCH] Add speaker device and icon by default
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
___
Sugar mailing list
Sugar@lists.laptop.org
http://lists.laptop.org/listinfo/sugar
Re: [sugar] [PATCH] Add speaker device and icon by default
On Fri, Apr 25, 2008 at 01:21:50PM -0400, Eben Eliason wrote:
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?
Probably not. The sequence elements are floats, so I figured why not.
+if len(nonzero_vols) != 0 and len(nonzero_vols) != len(volumes):
+volumes = nonzero_vols
The second condition seems unnecessary.
Possibly. We want to eliminate the nonzero volumes, but what if there
are no nonzero volumes? It's only unnecessary because sum([]) - used
later on - turns out to be zero (which I thought of now because it
wasn't 5am). So I could just replace the whole volumes = ... return
volumne ... with:
...
# sometimes we get a spurious zero from one/more channel(s)
nonzero_volumes = self._mixer.get_volume(self._master)
volume = sum(nonzero_volumes) / len(nonzero_volumes)
...
...and then it'd work. I'll do that.
+#sometimes alsa fails to set all channels' volume, so try a few
times
What's the rate of failure here?
About 25-50% if I hold down one of the arrow kesy while the slider has
focus and watch alsamixer's indication of the L+R channels' volume
from an ssh session into the laptop.
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?
Yes, because reading of zeros and the failure to set the volumes have
different solutions. The comment should better read
# sometimes alsa sets a channel's volume to zero rather than what we
tell it to
...and the problem is now sufficiently clear to justify the retry code.
+TrayIcon.__init__(self, icon_name=_ICON_NAME,
+ xo_color=profile.get_color())
You can align this with self, ...
Will do...code was cut pasted...
+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.
Again, cut pasted...will do.
+self._mute_item = MenuItem(_('Unmute'))
Tomeu will whine here because you use this string in two places.
I didn't like this either...I wasn't sure about initializing it to
None, and now you've clarified that, so I'll set it to ''.
+self.set_content(vbox)
Any need to put this down below the MenuItem creation? It seems oddly
detatched from the other VBox stuff.
Nope.
+mute_item_text = 'Unmute'
...
+mute_item_text = 'Mute'
These should be localized!
They are:
self._mute_item.get_child().set_text(_(mute_item_text))
- Eben
Martin
pgpKbLYPCrhhu.pgp
Description: PGP signature
___
Sugar mailing list
Sugar@lists.laptop.org
http://lists.laptop.org/listinfo/sugar
Re: [sugar] [PATCH] Add speaker device and icon by default
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 [EMAIL PROTECTED] 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 wellI'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 ___ Sugar mailing list Sugar@lists.laptop.org http://lists.laptop.org/listinfo/sugar
Re: [sugar] [PATCH] Add speaker device and icon by default
On 24.04.2008, at 18:01, Eben Eliason wrote: +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? It's an abuse of Python, IMHO. - Bert - ___ Sugar mailing list Sugar@lists.laptop.org http://lists.laptop.org/listinfo/sugar
Re: [sugar] [PATCH] Add speaker device and icon by default
On Thu, Apr 24, 2008 at 12:40 PM, Martin Dengler [EMAIL PROTECTED] wrote: 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 [EMAIL PROTECTED] 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. Hmmm, but you appear to be using it as such anyway. How else could the boolean be interpreted logically otherwise? (I may very well be missing something here.) It was pointed out to me that one reason for it would be to return a boolean type, instead of an int, which is a valid assessment (I'm too familiar with C). If that's the core reason, however, I might find simply casting to bool clearer. 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? Yup. +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. Good! I expect not to have to slap you again for this. :-P - Eben ___ Sugar mailing list Sugar@lists.laptop.org http://lists.laptop.org/listinfo/sugar
Re: [sugar] [PATCH] Add speaker device and icon by default
On Thu, Apr 24, 2008 at 12:47:11PM -0400, Eben Eliason wrote: On Thu, Apr 24, 2008 at 12:40 PM, Martin Dengler wrote: On Thu, Apr 24, 2008 at 12:01:52PM -0400, Eben Eliason wrote: On Thu, Apr 24, 2008 at 7:33 AM, Martin Dengler 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. Hmmm, but you appear to be using it as such anyway. How else could the boolean be interpreted logically otherwise? (I may very well be missing something here.) the GFlags class overrides the __and__ method...nice, eh? master.flags flags GST_MIXER_TRACK_OUTPUT | GST_MIXER_TRACK_MASTER of type GstMixerTrackFlags gst.interfaces.MIXER_TRACK_MUTE.__and__ method-wrapper '__and__' of MixerTrackFlags object at 0x817b52c type(gst.interfaces.MIXER_TRACK_MUTE) class 'gst.interfaces.MixerTrackFlags' type(gst.interfaces.MIXER_TRACK_MUTE gst.interfaces.MIXER_TRACK_MUTE) class 'gst.interfaces.MixerTrackFlags' - Eben Martin pgplSrcTMG9Dw.pgp Description: PGP signature ___ Sugar mailing list Sugar@lists.laptop.org http://lists.laptop.org/listinfo/sugar
