wikipbx-dev team mailing list archive

[Stas Shtin <antisvin@xxxxxxxxx>]

On 26.07.2010 20:02, Dmytro Mishchenko wrote:
> Looks nice, but for now I prefer we stick with basic div hiding
> technique. We'll see how it can be used on IVR construction and then
> apply it to extension actions too.
>   
Yeah, like I said, it's purely optional.
>
>> Now on to the code.
>>
>> 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.

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.
>> 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.

>> 5. Don't store strings in class definitions, i.e.:
>>
>>     labeltxt = "Alias this domain to Dialout SIP profile?"
>>     help_text = ("When checked, this account's domain will become an " +
>>                  "alias for the dialout sip profile.")
>>     aliased = forms.BooleanField(initial=True, required=False,
>>                                  label=labeltxt,
>>                                  help_text=help_text)
>>
>> should be:
>>
>>     aliased = forms.BooleanField(
>>         initial=True, required=False,
>>         label="Alias this domain to Dialout SIP profile?",
>>         help_text=("When checked, this account's domain will become an "
>>                    "alias for the dialout sip profile."))
>>
>> Such reordering doesn't do anything useful, just makes code harder to
>> follow. There may be left some places where such style is used, but I'll
>> get rid of it eventually. Also note that there's no need to concatenate
>> strings with + sign in this case, as python joins strings inside
>> parentheses anyway.
>>
>>     
> It's some old wikipbx code. I've fixed it anyway.
>   
Sorry, I haven't realized that you were editing existing code there.

Stas.