[asterisk-dev] [Code Review] 2971: app_confbridge: Allow dynamically created menus via CONFBRIDGE function

[rmudgett]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2971/#review10089
-----------------------------------------------------------



/trunk/apps/app_confbridge.c
<https://reviewboard.asterisk.org/r/2971/#comment19362>

    Change to:
    Set a custom dynamic bridge, user, or menu profile on a channel for the ConfBridge application using the same options defined in confbridge.conf.



/trunk/apps/app_confbridge.c
<https://reviewboard.asterisk.org/r/2971/#comment19366>

    The bridge, user, and menu could now be NULL when trying to output this message for bridge, user, and menu.
    
    Three instances in this function.  One for bridge, user, and menu.



/trunk/apps/confbridge/conf_config_parser.c
<https://reviewboard.asterisk.org/r/2971/#comment19365>

    Blank lines are separators and useful for grouping blocks of code as a larger operation.  What are you trying to convey by separating the set and test here?
    



/trunk/apps/confbridge/conf_config_parser.c
<https://reviewboard.asterisk.org/r/2971/#comment19363>

    Since an effort is made to ensure that b_profile.sounds in not NULL, if this fails then cleanup needs to be done.  The best thing to do is likely to remove and destroy the datastore.



/trunk/apps/confbridge/conf_config_parser.c
<https://reviewboard.asterisk.org/r/2971/#comment19364>

    Why are you calling the destructor manually?  Just ao2_cleanup() and set to NULL.



/trunk/apps/confbridge/conf_config_parser.c
<https://reviewboard.asterisk.org/r/2971/#comment19367>

    Stray blank lines.  These blank lines are separating logically grouped code.



/trunk/apps/confbridge/conf_config_parser.c
<https://reviewboard.asterisk.org/r/2971/#comment19368>

    Stray blank lines.  These blank lines are separating logically grouped code.



/trunk/apps/confbridge/conf_config_parser.c
<https://reviewboard.asterisk.org/r/2971/#comment19361>

    Move the if test after the pvt->menu assignment otherwise you will crash in menu_hook_destroy().  Also separating this code with a blank line is not necessary because this block of code is doing initialization.


- rmudgett


On Oct. 31, 2013, 10:03 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2971/
> -----------------------------------------------------------
> 
> (Updated Oct. 31, 2013, 10:03 p.m.)
> 
> 
> Review request for Asterisk Developers, Matt Jordan, Mark Michelson, and rmudgett.
> 
> 
> Bugs: ASTERISK-22760
>     https://issues.asterisk.org/jira/browse/ASTERISK-22760
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Adds the ability to dynamically create menus using the CONFBRIDGE dialplan function. This includes the ability to use the existing options as well as to specify templates with the 'template' option.
> 
> I suppose it's worth mentioning that there is one minor bit of awkwardness. Both the channel datastore and the conference user will end up with an allocation of the menu. This is probably for the best for a couple reasons though. For instance, the CONFBRIDGE function can modify the contents of the datastore allocated menu while the user is still in the conference if it is set via AMI. We probably don't want the menu to be modified in place during this time (even if it would be cool to be able to edit a menu while the conference is in progress).
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/confbridge/include/confbridge.h 402126 
>   /trunk/apps/confbridge/conf_config_parser.c 402126 
>   /trunk/apps/app_confbridge.c 402126 
>   /trunk/CHANGES 402126 
> 
> Diff: https://reviewboard.asterisk.org/r/2971/diff/
> 
> 
> Testing
> -------
> 
> The following extensions were tested for successful entry with the expected options as well as not having any obvious memory leaks:
> 
> exten => 6060,1,NoOp
> exten => 6060,n,Set(CONFBRIDGE(menu,*)=playback_and_continue(conf-usermenu))
> exten => 6060,n,Set(CONFBRIDGE(menu,0)=leave_conference)
> exten => 6060,n,Set(CONFBRIDGE(menu,*9)=increase_volume)
> exten => 6060,n,Set(CONFBRIDGE(menu,*7)=decrease_volume)
> exten => 6060,n,Answer()
> exten => 6060,n,Confbridge(this_conference)
> 
> exten => 6061,1,Answer()
> exten => 6061,n,Confbridge(this_conference)
> 
> exten => 6062,1,Answer()
> exten => 6062,n,Confbridge(this_conference,,,sample_user_menu)
> 
> exten => 6063,1,NoOp
> exten => 6063,n,Set(CONFBRIDGE(menu,template)=sample_user_menu)
> exten => 6063,n,Set(CONFBRIDGE(menu,0)=leave_conference)
> exten => 6063,n,Answer()
> exten => 6063,n,Confbridge(this_conference)
> 
> With the following sample_user_menu configuration in confbridge.conf
> 
> [sample_user_menu]
> type=menu
> *=playback_and_continue(conf-usermenu)
> *1=toggle_mute
> 1=toggle_mute
> *4=decrease_listening_volume
> 4=decrease_listening_volume
> *6=increase_listening_volume
> 6=increase_listening_volume
> *7=decrease_talking_volume
> 7=decrease_talking_volume
> *8=leave_conference
> 8=leave_conference
> *9=increase_talking_volume
> 9=increase_talking_volume
> 
> 
> Thanks,
> 
> jrose
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20131031/cb4f75d2/attachment-0001.html>