wikipbx-dev team mailing list archive

[Dmytro Mishchenko <wikipbx@xxxxxxxxxxxxx>]

My comments bellow...

>>> 1. I see that you've added context name to account model. I think you're
>>> only using it for preventing users from hanging up calls not belonging
>>> to him. Ok, but we don't really need to ask user for context name for
>>> this sole purpose. It's better to simply generate context name based on
>>> account ID. I suggest removing that field from the model and use
>>> something like this:
>>>
>>>     @property
>>>     def context(self):
>>>         return '__account_%s' % self.id
>>>
>>> This way account.context still returns a unique string like it does in
>>> your branch, but we don't ask it from the user.
>>>
>>>     
>> For current purpose it'll work in the way you suggest. I've made this
>> change.
>> However now, when we search for Account by context, first we need to
>> parse string received from dialplan and then perform query by Account id.
>>   
> Where does it happen in the code? I haven't found it after a quick search.
> 

This operation performed when we receive CDR. It's another way to
determine to what account we should assign received CDR.

I've replaced regexp with sting slicing.

> Anyway, it's actually an easy thing to do. Something like:
> 
> if context_string.startswith('__account_'):
>     account = Account.objects.get(id=context_string[10:])
> 
> Python string have method for matching values in the beginning and end,
> so you don't have to use regexes that are about 10 times slower.
> 
>>> 2. This code:
>>>
>>> class ActionSingleForm(forms.Form):
>>>     def __init__(self, account, template, action, *args, **kwargs):
>>>         # ...
>>>         voicetypes =(
>>> ('Allison','Allison'),('Mila','Mila'),('William','William'))
>>>
>>>         self.voicetypes = voicetypes
>>>         # ...
>>>         self.fields[tv.id].choices = voicetypes
>>>
>>> should be written like this:
>>>
>>> class ActionSingleForm(forms.Form):
>>>     VOICE_CHOICES = [(voice, voice) for voice in ('Allison', 'Mila',
>>> 'William')]
>>>
>>>     def __init__(self, account, template, action, *args, **kwargs):
>>>         # ...
>>>         self.fields[tv.id].choices = self.VOICE_CHOICES
>>>
>>>     
>> I've changed it as you suggest.
>>
>> This list depends from licenses installed for TTS engine and I expected
>> it to be loaded for some external source DB or config file.
>> May be we can configure it in setting.py. What do you think?
>>   
> Yes, this really belongs to the settings. Don't forget to add a default
> value to settings_template.py. I think that the default value should be
> an empty tuple, and if no values are added then TTS action should be
> hidden as this means that it's not configured yet.
> 
> There are a few issues related to the TTS action. We don't use
> freeswitch's mod_cepstral for TTS in wikipbx. Instead of that we have a
> python function that runs TTS executable (swift) directly. The benefit
> here is that we can cache multiple calls to generate the same string, so
> if there's more than one call with the same TTS string, we don't need
> more licenses for cepstral. Or we can record TTS when the action is
> created, this way cepstral is not needed later at all.
> 
> Wikipbx supports festival TTS as well, and I'd prefer to keep an option
> for using it. It could be achieved by using mod_flite or by using
> approach I've described above. See wikipbx.ttsutil module for
> TTS-related functions.


I've moved TTS voices configuration to setting.py file.
I didn't tested TTS functionality with any other engines except
cepstral. My current changes don't limit possibility of using other
engines in any way. They are only about restructuring existing
functionality we have in v 0.8.

>>> The difference is:
>>>
>>> * follows DRY principle (Don't Repeat Yourself)
>>> * static data is shared between all class instances, rather than created
>>> each time
>>>
>>> Talking about choices, code like:
>>>     SELECTION_CHOICES = ((0, 'Audio file'), (1, 'Extensions'),  (2,
>>> 'Gateway'),  (3, 'IVR script'), (4, 'Local endpoint'), (5, 'Voice type') )
>>>
>>> can be written like:
>>>     SELECTION_CHOICES = tuple(enumerate('Audio file', 'Extensions',
>>> 'Gateway', 'IVR script', 'Local endpoint', 'Voice type'))
>>>
>>> Looks much cleaner and less awkward typing.
>>>
>>>     
>> Later on in the code we refer to these choices as:
>> ...
>>             elif self.kind == 3:
>>                 # IVR
>> ...
>> If we'll have a long list written with enumerate
>> it'll require us to count indexes manually, next time we decide to add
>> another elif.
>>   
> Actually, counting choices takes almost as much time time as finding the
> number. The problem is that either way you have to look elsewhere to
> understand what that code means  - so it's a readability issue. In order
> to have a more readable code you'd have to do something like the following:
> 
> 1. Add meaningful variables for all selections (better to put it in
> wikipbx.statics to avoid polluting models file):
> ACTION_AUDIO = 0
> ACTION_EXTENSIONS = 1
> etc.
> 
> 2. Now put them in a dictionary in the same file:
> ACTION_CHOICES = {ACTION_AUDIO: 'Audio file', ACTION_EXTENSIONS:
> 'Extensions', ...}
> 
> 3. Our model would contain: choices=statics.ACTION_CHOICES.items()
> 
> 4. Now that comparison looks like:
> 
>             elif self.kind == statics.ACTION_IVR:
> 
> That would result in a more readable code.
> 


These constants moved to statics.py.

Dmitry