[asterisk-dev] [Code Review]: ConfbridgeActionExec AMI Command

[Matt Jordan]

> On May 11, 2012, 9:55 a.m., Olle E Johansson wrote:
> > Just a short note "ConfbridgeActionExec" is too complex as a name. "ConfbridgeAction" or "ConfbridgeExec" - but not a hybrid, please :-)
> > 
> > /O

I was going to change this to ConfbridgeAction last night, but this morning I thought about it some more.  Typically, names of AMI Actions that involve an application include the application name and some verb indicating the action to be taken, e.g., ConfbridgeListRooms, MeetmeMute, QueueAdd, etc.  There are, of course, exceptions to this rule, but in general this approach seems to make sense from the perspective that an AMI action *does* something, as opposed to it representing an object (which would take a noun form).

Where this is all going is that neither ConfbridgeAction nor ConfbridgeExec tell you what the action is doing.  ConfbridgeAction implies an object as opposed to an action that is performed.  ConfbridgeExec does imply that something is performed, but not what.  I'll admit that ConfbridgeActionExec doesn't provide a lot more information, but at least the concept of actions exist in the ConfBridge application, and since this command - by definition - is generic, there's only so much information that can be conveyed.

As far as the issue of name complexity goes, looking at our current AMI actions, this is not the most egregious example of a long AMI action name (that prize goes to ConfbridgeSetSingleVideoSrc).  While I'm not a proponent of continuing bad practices on the basis of historical precedent, I'm not convinced this is a bad practice - at the very least, the action name tells you exactly what it does.

So - I'm inclined to keep it as ConfbridgeActionExec, unless there is a compelling reason that I'm missing or sufficient outcry.


- Matt


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


On May 5, 2012, 11:50 p.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1902/
> -----------------------------------------------------------
> 
> (Updated May 5, 2012, 11:50 p.m.)
> 
> 
> Review request for Asterisk Developers and Joshua Colp.
> 
> 
> Summary
> -------
> 
> Currently, most actions that are executed upon a channel in a conference (increasing/decreasing listening volume, playing back a sound on a channel, etc.) are handled via DTMF menus associated with a user profile.  While this suffices for scenarios where a user initiates an action upon themselves, it does not account for scenarios where a third party application wants to interact with the channels in a ConfBridge.  While there are a handful of AMI commands that allow for external interaction with the channels in the conference, they are currently limited to a subset of the defined actions.
> 
> Rather then add individual AMI commands for each confbridge action, this patch adds a single new AMI command for ConfBridge, ConfbridgeActionExec.  The command lets you execute any of the ConfBridge actions on a channel in the conference.
> 
> In order to facilitate this, a new mechanism has been added to the bridging layer to allow for a callback function to be executed at the next convenient moment on a bridged channel's thread.  This lets a user of the bridging layer defer execution of some function until such a time that the bridging layer determines that it is safe to execute that action on the channel's thread.
> 
> Example Usage:
> 
> Action: ConfbridgeActionExec
> Conference: 1
> Channel: Local/blah at foo
> Actions: increase_listening_volume,playback(tt-monkeys)
> 
> This would playback monkeys to the offending channel.  Very loudly.
> 
> 
> Diffs
> -----
> 
>   /trunk/CHANGES 364579 
>   /trunk/apps/app_confbridge.c 364788 
>   /trunk/main/bridging.c 364579 
>   /trunk/apps/confbridge/conf_config_parser.c 364579 
>   /trunk/apps/confbridge/include/confbridge.h 364579 
>   /trunk/include/asterisk/bridging.h 364579 
> 
> Diff: https://reviewboard.asterisk.org/r/1902/diff
> 
> 
> Testing
> -------
> 
> Initial testing using a test in the Asterisk Test Suite verified proper behavior of the AMI command.  The channel was properly suspended from the bridging layer, the playback confbridge action was executed, and the channel was placed back into the bridging layer.  All of this was similar to what would occur if the same action was triggered using a DTMF menu.
> 
> Note that a test case is being written to handle the various actions, but will be posted under a separate review.
> 
> 
> Thanks,
> 
> Matt
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120512/3618703d/attachment.htm>