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

[jrose]

> On Oct. 30, 2013, 11:45 p.m., rmudgett wrote:
> > /trunk/apps/confbridge/conf_config_parser.c, lines 2282-2283
> > <https://reviewboard.asterisk.org/r/2971/diff/2/?file=47806#file47806line2282>
> >
> >     Init to NULL not necessary.  If you move these to the respective closest enclosing blocks where they are used it is easier to see it.

It's really just because I split this off of another function that used this datastore (conf_find_bridge_profile). Cleaned up.


> On Oct. 30, 2013, 11:45 p.m., rmudgett wrote:
> > /trunk/apps/confbridge/conf_config_parser.c, lines 2287-2289
> > <https://reviewboard.asterisk.org/r/2971/diff/2/?file=47806#file47806line2287>
> >
> >     This can go back to right before the ao2_find().  The only reason it needed to be moved before was because you were allocating created_menu; which is no more.


> On Oct. 30, 2013, 11:45 p.m., rmudgett wrote:
> > /trunk/apps/confbridge/conf_config_parser.c, lines 1883-1884
> > <https://reviewboard.asterisk.org/r/2971/diff/2/?file=47806#file47806line1883>
> >
> >     Hopefully you've already fixed setting the dst->actions next pointer to NULL.
> >     
> >     However, both dst->entries and dst->actions need their next pointers set to NULL.

I'm not quite sure if dst->entries really needs to have anything poked on here. Whichever function calls this one is currently responsible for making sure the conf_menu_entry is already set up properly and everything that calls this function currently either allocs a fresh menu entry or provides one that is NULLED out in advance.

That might not have been the case in the older code though.  I'm not sure.  I don't think this is a problem in what I'm about to post though.


- jrose


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


On Oct. 30, 2013, 8:19 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2971/
> -----------------------------------------------------------
> 
> (Updated Oct. 30, 2013, 8:19 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/e5802d4a/attachment-0001.html>