<br><br><div class="gmail_quote">On Mon, Jan 23, 2012 at 1:08 PM, Sascha Silbe <span dir="ltr"><<a href="mailto:silbe@activitycentral.com">silbe@activitycentral.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Excerpts from godiard's message of 2012-01-17 22:49:31 +0100:<br>
<div class="im"><br>
> Added controls to pause/stop and addressed suggestions<br>
> in the review process.<br>
<br>
</div>Please put the changelog below the marker line, i.e. below this one:<br>
<br>
> ---<br>
<br>
Please add a description. Besides other things I'm missing a clear<br>
mention that the (currently undocumented) dependency on the OLPC Speech<br>
Server [10] gets replaced by a new dependency [1] on gst-plugins-espeak.<br>
The latter still isn't packaged for many distros, including Debian.<br>
Given that the OLPC Speech Server isn't packaged either, it's no reason<br>
to reject the patch, but it's definitely worth mentioning.<br>
<br></blockquote><div><br>Ok<br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
[data/<a href="http://sugar.schemas.in" target="_blank">sugar.schemas.in</a>]<br>
<div class="im">> + <schema><br>
> + <key>/schemas/desktop/sugar/speech/pitch</key><br>
> + <applyto>/desktop/sugar/speech/pitch</applyto><br>
> + <owner>sugar</owner><br>
> + <type>int</type><br>
> + <default>50</default><br>
> + <locale name="C"><br>
> + <short>Default pitch to the speech sugar service</short><br>
<br>
</div>Probably a typo: s/to/for/.<br>
<br>
As can be seen from the code, we retain user changes over a restart of<br>
Sugar, so we should drop the mention of "default".<br>
<div class="im"><br></div></blockquote><div><br>Ok <br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="im">
<br>
> + <long>Pitch value used by the speech service in Sugar,<br>
> + can be changed by the user, with controls in a icon<br>
> + in the frame</long><br>
<br>
</div>If we mention UI details here, we'd need to update them whenever the UI<br>
changes. Please state only what the setting affects.<br>
<br>
<br></blockquote><div><br>Ok<br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
[...]<br>
> + <key>/schemas/desktop/sugar/speech/rate</key><br>
[...]<br>
<br>
Same as above.<br>
<br>
<br>
[extensions/deviceicon/speech.py]<br>
<div class="im">> +from gettext import gettext as _<br>
> +import gconf<br>
<br>
</div>gconf isn't part of the standard Python libraries, so it should be moved<br>
to the following block of third-party module imports:<br>
<br>
> +import glib<br>
> +import gtk<br>
<br>
<br></blockquote><div><br>Ok<br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
[...]<br>
<div class="im">> +class SpeechDeviceView(TrayIcon):<br>
> +<br>
> + FRAME_POSITION_RELATIVE = 105<br>
<br>
</div>Please consider using a number that leaves us free to add more Device<br>
Icons in between without changing existing code. 150 might be a good<br>
choice.<br>
<div class="im"><br></div></blockquote><div><br>Ok<br> <br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="im">
<br>
> + def __init__(self):<br>
> + client = gconf.client_get_default()<br>
> + self._color = XoColor(client.get_string('/desktop/sugar/user/color'))<br>
> +<br>
> + TrayIcon.__init__(self, icon_name=_ICON_NAME, xo_color=self._color)<br>
> +<br>
> + self.set_palette_invoker(FrameWidgetInvoker(self))<br>
> +<br>
> + self._manager = speech.get_speech_manager()<br>
> +<br>
> + self.connect('expose-event', self.__expose_event_cb)<br>
> +<br>
> + self._icon_widget.connect('button-release-event',<br>
> + self.__button_release_event_cb)<br>
<br>
</div>Please don't spread the code this much using blank lines. In fact the<br>
above code would be at least as readable even without any blank line.<br>
<br></blockquote><div><br>This is funny, but ok :)<br> <br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
[...]<br>
<div class="im">> + def __button_release_event_cb(self, widget, event):<br>
> + if event.button != 1:<br>
> + return False<br>
<br>
</div>As the Palette doesn't have a default action, left-click should open the<br>
Palette, too.<br>
<div class="im"><br></div></blockquote><div><br>Good point.<br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="im">
<br>
> + def __expose_event_cb(self, *args):<br>
> + self._update_info()<br>
<br>
</div>Where does _update_info() get defined? What's the purpose of the expose<br>
callback?<br>
<div class="im"><br></div></blockquote><div><br>You are right, old code.<br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="im">
<br>
> +class SpeechPalette(Palette):<br>
> +<br>
> + def __init__(self, primary_text, manager):<br>
> + Palette.__init__(self, label=primary_text)<br>
> +<br>
> +<br>
> +<br>
<br>
</div>Please use pep8 and pylint [2] to check your code prior to submission<br>
[3,4].<br></blockquote><div><br>I have used it. If you see any problem tell me.<br><br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
[...]<br>
<div class="im">> + """<br>
> + self._play_item = MenuItem('Say selected text')<br>
> + self._play_icon = Icon(icon_name='player_play',<br>
> + icon_size=gtk.ICON_SIZE_MENU)<br>
> + self._pause_icon = Icon(icon_name='player_pause',<br>
> + icon_size=gtk.ICON_SIZE_MENU)<br>
> + self._play_item.set_image(self._play_icon)<br>
> + self.menu.append(self._play_item)<br>
> + self._play_item.show()<br>
> + self._play_item.connect('activate', self.__play_clicked_cb)<br>
> +<br>
> + self._stop_item = MenuItem('Stop talking')<br>
> + self._stop_icon = Icon(icon_name='player_stop',<br>
> + icon_size=gtk.ICON_SIZE_MENU)<br>
> + self._stop_item.set_image(self._stop_icon)<br>
> + self.menu.append(self._stop_item)<br>
> + self._stop_item.show()<br>
> + self._stop_item.connect('activate', self.__stop_clicked_cb)<br>
> + self._stop_item.set_sensitive(False)<br>
> + """<br>
<br>
</div>If you don't intend to execute code, please remove it entirely (applies<br>
to several other parts of your patch as well). Also docstrings are a<br>
particularly bad way of marking code to not get executed: without syntax<br>
highlighting (e.g. in emails and a lot of editors) it's hard to notice<br>
that the above, long code block doesn't get executed. Using comments<br>
makes it obvious as each line is marked individually. Some text editors<br>
even have shortcuts to comment in/out the currently selected code block<br>
to make this trivial.<br>
<br></blockquote><div><br>This code was commented because I proposed two different UI,<br>and wanted to do easy to other testers/reviewers try the two options.<br>I will remove the option with the MenuItem beacuse gtk3 limitations (see Simon mail)<br>
<br><br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
[...]<br>
<div class="im">> + pitch_step = 10<br>
> + self._adj_pitch = gtk.Adjustment(value=self._manager.get_pitch(),<br>
> + lower=self._manager.MIN_PITCH,<br>
> + upper=self._manager.MAX_PITCH,<br>
> + step_incr=pitch_step,<br>
> + page_incr=pitch_step,<br>
> + page_size=pitch_step)<br>
<br>
</div>Why do you disallow coarser steps when using the keyboard to change the<br>
value?<br>
<div class="im"><br></div></blockquote><div><br>Hmm, I thought would be easier, but may be is not a good option.<br><br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div class="im">
<br>
> + self._hscale_pitch = gtk.HScale(self._adj_pitch)<br>
> + self._hscale_pitch.set_digits(0)<br>
<br>
> + self._hscale_pitch.set_draw_value(False)<br>
<br>
</div>This is up to the Design Team to decide, but I'd imagine it to be useful<br>
to see the current value as a number, so the learner can reproducibly<br>
set the same value (both as before and as other learners).<br></blockquote><div><br>Good point, I will add it and then Design can can try it.<br><br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
[Rate adjustment]<br>
<br>
Same comments as for the pitch adjustment.<br>
<br>
<br>
[...]<br>
<div class="im">> + def __play_clicked_cb(self, widget):<br>
> + if self._manager.is_paused:<br>
> + self._manager.restart()<br>
> + else:<br>
> + if not self._manager.is_playing:<br>
> + self._manager.say_selected_text()<br>
> + else:<br>
> + self._manager.pause()<br>
<br>
</div>This could be streamlined by using elif.<br>
<div class="im"><br>
<br>
> + def __stop_clicked_cb(self, widget):<br>
> + self._manager.stop()<br>
<br>
</div>Why do we need a Stop button in addition to Play/Pause? And where does<br>
this callback get invoked? AFAICT the hook-up is commented out.<br>
<br></blockquote><div><br>If you have a long text been played, and want finish or start again <br>from the beginning, you need a stop button. <br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
[extensions/globalkey/speech.py]<br>
[...]<br>
> +BOUND_KEYS = ['<alt>s']<br>
<br>
This will collide with shortcuts from existing applications and<br>
activities, especially since GTK uses <Alt> for mnemonics [7]. To give a<br>
concrete example of a non-mnemonic case: the Scan activity [8] uses<br>
<Alt>+S to start or stop scanning [9].<br>
<br>
Why didn't you keep the existing shortcut (<Alt><Shift>S)?<br>
<br></blockquote><div><br>I have changed it by Simon suggestion. Please read the other reviews.<br>Scan activity? Is not in ASLO yet. You can change it ;)<br><br><br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
[src/jarabe/model/speech.py]<br>
> +import gconf<br>
<br>
Like in extensions/deviceicon/speech.py, the gconf import should be<br>
moved to the block of third-party module imports.<br>
<div class="im"><br>
> +<br>
> +import gst<br>
> +import gtk<br>
> +import gobject<br>
> +<br>
> +import os<br>
> +from gettext import gettext as _<br>
> +import logging<br>
<br>
</div>Standard Python library modules should be listed first, before<br>
third-party modules (PEP-8 section "Imports", second item).<br>
<div class="im"><br>
<br>
> +# TRANS: The language pitch (range [0 - 99], default 50 for English)<br>
> +# Look at <a href="http://espeak.sourceforge.net/commands.html" target="_blank">http://espeak.sourceforge.net/commands.html</a> for details<br>
> +DEFAULT_PITCH = int(_('50'))<br>
> +<br>
> +<br>
> +# TRANS: The diction speed, in average words per minute (range [80 - 390],<br>
> +# default 170 for English).<br>
> +# Look at <a href="http://espeak.sourceforge.net/commands.html" target="_blank">http://espeak.sourceforge.net/commands.html</a> for details<br>
> +DEFAULT_RATE = int(_('170'))<br>
<br>
</div>Why did you choose a different default for rate than espeak did (170 vs.<br>
175)?<br></blockquote><div><br>Hmmm, I can't find why :) Changing to 175<br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Do these values really need to be localised? AFAICT from the document<br>
you referenced, the upstream default is always the same and espeak<br>
voices will interpret the value as needed.<br>
<br></blockquote><div><br>Ok, I will remove i18n here.<br><br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
[...]<br>
[class SpeechManager]<br>
<div class="im">> + def __init__(self, **kwargs):<br>
> + gobject.GObject.__init__(self, **kwargs)<br>
> + self._player = AudioGrabGst()<br>
> + self._player.connect('play', self._emit_signal, 'play')<br>
> + self._player.connect('stop', self._emit_signal, 'stop')<br>
> + self._player.connect('pause', self._emit_signal, 'pause')<br>
> + logging.debug('SpeechManager setting default parameters')<br>
<br>
</div>Does this log message actually aid in debugging? The values always get<br>
set on instantiation and before instantiation there's no SpeechManager<br>
to access.<br>
<br></blockquote><div><br><br>Not needed now. Removed.<br> <br></div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
[...]<br>
<div class="im">> + try:<br>
> + self._loading = True<br>
> + self.restore()<br>
> + self._loading = False<br>
> + except:<br>
> + pass<br>
<br>
</div>Please don't catch exceptions unconditionally except in very specific<br>
cases. I'm not even sure we should catch exceptions here at all, as<br>
anything that could happen in self.restore() would be the result of a<br>
severe problem and should be reported to the user, rather than ignored<br>
silently.<br>
<br>
You're leaving self._loading set if restoring the settings fails, which<br>
will also prevent changes to be saved later on. But you can get rid of<br>
self._loading completely if you set the internal variables (self._pitch<br>
and self._rate) in self.restore() directly rather than using the<br>
accessors.<br>
<div class="im"><br>
<br></div></blockquote><div><br>It's true. Thanks.<br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="im">
> + def _emit_signal(self, player, signal):<br>
> + self._is_playing = (signal == 'play')<br>
> + self._is_paused = (signal == 'pause')<br>
> + self.emit(signal)<br>
<br>
</div>This method does more than just emitting a signal. Something like<br>
_update_state() would be a better name.<br>
<br>
<br></blockquote><div><br>Ok<br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
[...]<br>
<div class="im">> + def save(self):<br>
> + client = gconf.client_get_default()<br>
> + client.set_int('/desktop/sugar/speech/pitch', self.get_pitch())<br>
> + client.set_int('/desktop/sugar/speech/rate',<br>
> + self.get_rate())<br>
<br>
</div>You can access the private variables directly here; that would get rid<br>
of the line break.<br>
<br></blockquote><div><br>Ok.<br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Also, if we save on every change, we should listen for gconf updates so<br>
that outside changes (e.g. using gconf-editor or gconftool-2) will be<br>
used by Sugar instead of getting overwritten on the next invocation of<br>
save().<br>
<br></blockquote><div><br>We are not doing this in sound or network settings, right?<br><br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> +class AudioGrabGst(gobject.GObject):<br>
<br>
What's the purpose of this class? What does it grab? Please consider<br>
renaming the class and / or providing a useful docstring.<br>
<div class="im"><br>
> +<br>
> + def __init__(self):<br>
> + gobject.GObject.__init__(self)<br>
> + self._pipeline = None<br>
> + self._quiet = True<br>
<br>
</div>What do we need self._quiet for? AFAICT it's only set, never read.<br></blockquote><div><br>It's true. Removed.<br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div class="im"><br>
<br>
> + def restart_sound_device(self):<br>
> + if self._pipeline is None:<br>
> + return<br>
<br>
</div>Calling restart_sound_device() shouldn't be allowed to be called if<br>
there's no pipeline set up. The UI should deactivate the buttons while<br>
there's no text to be spoken. restart_sound_device() et al. should<br>
throw an error when called out-of-order.<br>
<br></blockquote><div><br>The UI is deactivating the buttons, but I think does not harm have a control here.<br>I will add a log message.<br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
[...]<br>
> + def make_pipeline(self, cmd):<br>
<br>
Re. "cmd": Please don't abbreviate unless you have a good reason to do<br>
so [6].<br>
<br></blockquote><div><br>Ok.<br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
[...]<br>
<div class="im">> + def __pipe_message_cb(self, bus, message):<br>
> + logging.error('Gst message %s' % message)<br>
<br>
</div>Why do you mark every GStreamer message as error? AFAICT you will<br>
receive these messages as part of normal operation.<br>
<br>
Also please pass parameters to the logging function instead of doing the<br>
formatting yourself. To quote [5]: "This is more robust and formatting<br>
only happens if we actually log something."<br>
<div class="im"><br></div></blockquote><div><br>Ok. Is only for testing I will remove it in the final version.<br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div class="im">
> + if message.structure.get_name() == 'espeak-mark' and \<br>
> + message.structure['mark'] == 'end':<br>
> + self.emit('stop')<br>
<br>
</div>Do we need to set the pipeline state to gst.STATE_NULL like in<br>
self.stop_sound_device() or does that happen automatically?<br>
<div class="im"><br></div></blockquote><div><br>Automatically? How?<br><br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="im">
<br>
> + def speak(self, pitch, rate, voice_name, text):<br>
<br>
</div>Why do we need to pass the parameters in every time, rather than<br>
settings the properties a) on init and b) when the gconf / UI ?<br>
<div class="im"><br></div></blockquote><div><br>This class does not have this variables. Should copy almost the same code in<br>the SpeechManager class. I am looking if are really needed two classes or if<br>I can move part of this to sugar-toolkit, to enable the activities to use tts.<br>
</div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="im">
<br>
> + # XXX workaround for <a href="http://bugs.sugarlabs.org/ticket/1801" target="_blank">http://bugs.sugarlabs.org/ticket/1801</a><br>
<br>
</div>Please get rid of the XXX, it does not add any value. If we intend to<br>
remove this some time after SL#1801 gets fixed, we should mark it as<br>
TODO.<br>
<div class="im"><br></div></blockquote><div><br>Ok.<br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="im">
<br>
> + if not [i for i in text if i.isalnum()]:<br>
> + return<br>
<br>
</div>The user will be rather puzzled why saying some texts works and others<br>
simply get ignored. We should filter out the individual characters that<br>
gst-plugins-espeak doesn't like, rather than bailing out completely.<br>
<br></blockquote><div><br>I understand the text is ignored if there are only non alpha chars.<br>But yes, the code is a little cryptic. <br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
[...]<br>
> + def get_all_voices(self):<br>
<br>
This should probably be a private method (i.e. be called<br>
_get_all_voices()).<br>
<div class="im"><br></div></blockquote><div><br>Like before, I am trying to see what is needed by activities, <br>and moving to sugar-toolkit.<br><br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div class="im">
<br>
> + all_voices = {}<br>
> + for i in gst.element_factory_make('espeak').props.voices:<br>
> + name, language, dialect = i<br>
<br>
</div> for voice in gst.element_factory_make('espeak').props.voices:<br>
<br>
Please consider using "voice" or a similar speaking name in place of<br>
"i". The latter is traditionally used for _i_ntegers (or _i_ndexes,<br>
which are also integers).<br>
<div class="im"><br></div></blockquote><div><br>Ok<br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="im">
<br>
> + #if name in ('en-rhotic','english_rp','english_wmids'):<br>
> + # these voices don't produce sound<br>
> + # continue<br>
<br>
</div>Please remove dead code.<br>
<div><br></div></blockquote><div><br>Ok.<br><br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><br> </div></blockquote><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div class="im">
> + all_voices[language] = name<br>
<br>
</div>Are the voices in ...props.voices ordered such that the preferred voice<br>
for a given language is listed last? Can the user influence the<br>
ordering? If not, we should make the voice configurable (which is easy,<br>
given that we already pass it through SpeechManager), even if just as a<br>
gconf setting with no UI in Sugar.<br>
<br>
<br></blockquote><div><br>Yes. I am tempted of adding the voice selection to the UI too.<br>In particular, there are 8 different english voices, two spanish and two portuguese,<br>and I found difficult found the right voice every time.<br>
<br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
[...]<br>
<div class="im">> + def get_default_voice(self):<br>
> + """Try to figure out the default voice, from the current locale ($LANG)<br>
> + Fall back to espeak's voice called Default."""<br>
> + voices = self.get_all_voices()<br>
> +<br>
> + try:<br>
> + lang = os.environ['LANG']<br>
> + if lang.find('.') > --1:<br>
> + lang = lang[0:lang.find('.')]<br>
> + lang = lang.replace('_', '-').lower()<br>
> + except:<br>
> + lang = ""<br>
> +<br>
> + best = "default"<br>
> +<br>
> + try:<br>
> + best = voices[lang]<br>
> + except:<br>
> + try:<br>
> + lang = lang[0:lang.find('-')]<br>
> + best = voices[lang]<br>
> + except:<br>
> + pass<br>
<br>
</div>The following would be more robust, not masking coding mistakes (like<br>
"--1") by catching exceptions unconditionally. Not to mention that it's<br>
a lot less code:<br>
<br>
locale = os.environ.get('LANG', '')<br>
language_location = locale.split('.', 1)[0].replace('_', '-').lower()<br>
language = language_location.split('-')[0]<br>
return voices.get(language_location) or voices.get(language) or 'default'<br>
<div class="im"><br></div></blockquote><div><br>Thanks. I found other problems making the language list, <br>I will try to do a better list and search of the best voice.<br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div class="im">
<br>
> +def get_speech_manager():<br>
> + global _speech_manager<br>
> +<br>
> + if _speech_manager == None:<br>
<br>
</div>PEP8, "Programming Recommendations", second item: "Comparisons to<br>
singletons like None should always be done with 'is' or 'is not', never<br>
the equality operators."<br>
<br>
<br></blockquote><div><br>Ok. <br><br>Thanks by the exhaustive review. I will do another try this week,<br>to have a good candidate before the deadline.<br><br>Gonzalo<br> </div><blockquote class="gmail_quote" style="margin:0pt 0pt 0pt 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Sascha<br>
<br>
[1] <a href="https://wiki.sugarlabs.org/go/Development_Team/Code_Review#Dependencies" target="_blank">https://wiki.sugarlabs.org/go/Development_Team/Code_Review#Dependencies</a><br>
[2] <a href="https://wiki.sugarlabs.org/go/Development_Team/Code_guidelines#Tools" target="_blank">https://wiki.sugarlabs.org/go/Development_Team/Code_guidelines#Tools</a><br>
[3] <a href="https://wiki.sugarlabs.org/go/Development_Team/Code_Review#Proposal" target="_blank">https://wiki.sugarlabs.org/go/Development_Team/Code_Review#Proposal</a><br>
[4] <a href="https://wiki.sugarlabs.org/go/Development_Team/Code_guidelines" target="_blank">https://wiki.sugarlabs.org/go/Development_Team/Code_guidelines</a><br>
[5] <a href="https://wiki.sugarlabs.org/go/Development_Team/Code_guidelines#Miscellaneous" target="_blank">https://wiki.sugarlabs.org/go/Development_Team/Code_guidelines#Miscellaneous</a><br>
[6] <a href="https://wiki.sugarlabs.org/go/Development_Team/Code_guidelines#Code_style" target="_blank">https://wiki.sugarlabs.org/go/Development_Team/Code_guidelines#Code_style</a><br>
[7] <a href="http://www.gtk.org/api/2.6/gtk/GtkButton.html#gtk-button-new-with-mnemonic" target="_blank">http://www.gtk.org/api/2.6/gtk/GtkButton.html#gtk-button-new-with-mnemonic</a><br>
[8] <a href="http://git.sugarlabs.org/scan-activity" target="_blank">http://git.sugarlabs.org/scan-activity</a><br>
[9] <a href="http://git.sugarlabs.org/scan-activity/scan-activity/blobs/master/scan.py#line655" target="_blank">http://git.sugarlabs.org/scan-activity/scan-activity/blobs/master/scan.py#line655</a><br>
[10] <a href="http://wiki.laptop.org/go/Speech_Server" target="_blank">http://wiki.laptop.org/go/Speech_Server</a><br>
<span class="HOEnZb"><font color="#888888">--<br>
<a href="http://sascha.silbe.org/" target="_blank">http://sascha.silbe.org/</a><br>
<a href="http://www.infra-silbe.de/" target="_blank">http://www.infra-silbe.de/</a><br>
</font></span></blockquote></div><br><br clear="all"><br>-- <br>Gonzalo Odiard<br>SugarLabs Argentina<br><br>