[asterisk-dev] [Code Review] 2679: bridge_holding + app_bridgewait: Add more entertainment options, change default to music on hold

[rmudgett]

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



/trunk/apps/app_bridgewait.c
<https://reviewboard.asterisk.org/r/2679/#comment18029>

    Change this to "Put the channel on hold".  Saying "hold control frames" is an implementation detail wording.



/trunk/apps/app_bridgewait.c
<https://reviewboard.asterisk.org/r/2679/#comment18030>

    Probably should add return -1 here since it is an error condition.



/trunk/bridges/bridge_holding.c
<https://reviewboard.asterisk.org/r/2679/#comment18032>

    These should be removed since this is what the patch is for.



/trunk/bridges/bridge_holding.c
<https://reviewboard.asterisk.org/r/2679/#comment18031>

    You aren't accounting for NULL moh_class here.


- rmudgett


On July 16, 2013, 4:59 p.m., jrose wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2679/
> -----------------------------------------------------------
> 
> (Updated July 16, 2013, 4:59 p.m.)
> 
> 
> Review request for Asterisk Developers, Matt Jordan and rmudgett.
> 
> 
> Bugs: ASTERISK-21923
>     https://issues.asterisk.org/jira/browse/ASTERISK-21923
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> First, the arguments to app_bridgewait are changed by this patch.
> 
> Before it was just bridgewait(options)
> Now it's bridgewait(role,options)
> 
> Combined with the named waiting bridge containers patch, this will soon become
> bridgewait(name,role,options)
> 
> The announcer option and the various idle mode options have been squashed. Now the role (announcer or participant) is specified in the role argument (defaulting to participant) while how participants act while idling in the holding bridge is set by the 'e' option which can specify an entertainment method.
> 
> New entertainment methods have been added to quell the bugbug comments. The 'e' option can now specify the following:
> s - use a silence generator
> h - Issue hold indications to be handled by the channel driver (respects MOH class argument)
> n - provide no entertainment (used to be the default)
> 
> in addition to the existing:
> m - Play music on hold directly (default) (respects MOH class argument)
> r - Ring ring
> 
> Hopefully these changes make the app more self-explanatory to use since it isn't possible to use conflicting options now.
> 
> Between this review and https://reviewboard.asterisk.org/r/2642/diff/#index_header all BUGBUG comments in app_bridgewait should be quelled.
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_bridgewait.c 394249 
>   /trunk/bridges/bridge_holding.c 394249 
>   /trunk/main/bridging_roles.c 394249 
> 
> Diff: https://reviewboard.asterisk.org/r/2679/diff/
> 
> 
> Testing
> -------
> 
> * Tested all new entertainment methods to make sure they produced the anticipated audio (or just RTP in the case of silence and nothing in the case of nothing)
> * Tested all options to BridgeWait to make sure they didn't break
> * Tested to make sure roles were applied properly and that unknown roles would be appropriately rejected.
> 
> 
> Thanks,
> 
> jrose
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130716/9cf65c3f/attachment.htm>