Re: [Nvda-dev] [NVDA] #311: Customized synthDriver settings

[NVDA T. <tr...@nv...>]

#311: Customized synthDriver settings
-------------------------------------------------------------------------------+
 Reporter:  aleksey_s                                                          |       Owner:  aleksey_s
     Type:  enhancement                                                        |      Status:  accepted 
 Priority:  major                                                              |   Milestone:           
Component:  Speech                                                             |     Version:  trunk    
 Keywords:  custom settings, synthDrivers, synthSettingsRing, needsCodeReview  |          Os:           
Blockedby:                                                                     |    Blocking:           
-------------------------------------------------------------------------------+

Comment(by aleksey_s):

 Replying to [comment:6 jteh]:
 >  * I don't like the use of the data attribute for different purposes in
 the !SynthSetting class. Dynamic typing allows it, but imo, this is a bad
 abuse of dynamic typing.
 May be. I am still unsure where it is correct to use dynamic typing
 features, as my background is strong-typed languages. Anyway, here i
 decided to use this trick just to decrease  lines of code i was required
 to write.
 >Instead, I'd prefer different classes for different types of settings;
 i.e. an abstract !SynthSetting class with !NumericSynthSetting and
 !StringSynthSetting subclasses.
 I don't think having NumericSynthSetting which is just a class synonim to
 basic SynthSetting is good idea. This is the class which does nothing
 (incapsulates nothing), and use it only as pretty identifier in
 {{isinstance}}} seems strange to me. May be it is only a point of
 programming style. For synthSettingsRing i created SynthSetting and
 StringSynthSetting, but SynthSetting is used as numeric.
 >This way, you can have different attributes on the class for each type;
 !StringSynthSetting needs nothing, but !NumericSynthSetting would have a
 "step" attribute. You would then use isinstance instead of having an
 isNumeric method, which is acceptable in this case because you're checking
 the instance of the object itself, not its data.
 Well, it is shorter to write some.isNumeric() instead of
 isinstance(some,synthDriverHandler.NumericSynthSetting)
 >  * The docstring for !SynthDriver needs to be updated. The old stuff
 should be removed and the new stuff documented. This is important.
 I agree. But i think that native speaker can do it better. May i ask you
 to write it?
 >  * I'd probably prefer that the Rate, Pitch, etc. !SynthSetting
 instances on the base !SynthDriver be renamed to !RateSetting,
 !PitchSetting, etc. Right now, it's not immediately clear that they're
 setting types, not the settings themselves.
 They are capitalized.
 >  * _supportedSettings should probably just be supportedSettings; i.e.
 public, not private. Several external classes need to access it and it is
 something that others may wish to know about it.
 I am not sure this will not conflict with _get_supportedSettings() getter.
 >Being fussy, it should also probably be a tuple, not a list; there's
 probably no need for it to be mutable, unless you have a good reason.
 yes, I have. Look at sapi4 !SynthDriver, it dynamically changes set of
 supported settings.
 >  * Rather than prefixing the i18nName with "&" in the settings dialog to
 get the mnemonic, I'd suggest the reverse; i.e. include "&" in the
 i18nName where appropriate and then strip it out when you don't want it,
 such as in the synth settings ring. This way, you get more flexibility, no
 regression and minimal change to language strings.
 Good idea, thanks.
 >  * I understand why you've done it, but all of the code which caches
 setting controls in the voice settings dialog gets pretty complex. I
 wonder whether it's that much of a performance loss to just regenerate the
 controls we need, especially as we only have to do it when the voice
 changes. Hiding/showing controls may not actually be that much cheaper in
 the end.
 I was also not sure about performance so decided to write better code
 instead of simpler.
 >  * At present, overriding the step size for a setting (as has to be done
 for audiologic) is a little strange. One copies the base instance and
 modifies it. I think perhaps it might be better to have each base setting
 as a class instead. You would then do:[[BR]]
 >  {{{
 > supportedSettings = (RateSetting(step=5), PitchSetting(step=2),
 VolumeSetting(), ...)
 >  }}}
 >  The base !SynthSetting classes could still be used to create custom
 settings. This is something I need to think about further and we'll
 probably want to discuss it on IRC or some such.
 Again good idea. I can't imagine something against that.

-- 
Ticket URL: <http://www.nvda-project.org/ticket/311#comment:7>
NVDA <http://www.nvda-project.org/>
A free and open-source screen reader for Windows