[asterisk-dev] [Code Review]: Make the FollowMe application optionally update the connected line information when the accepting endpoint is bridged.

[rmudgett]

> On Jan. 9, 2012, 2:24 p.m., Mark Michelson wrote:
> > /branches/1.8/apps/app_followme.c, lines 794-796
> > <https://reviewboard.asterisk.org/r/1656/diff/1/?file=22891#file22891line794>
> >
> >     The way this is coded doesn't match the description of the 'I' option. The documentation says that any connected line change will be blocked. This only blocks connected line changes received from outbound channels. So either update the documentation to say what it actually does or change the way this behaves to match what the documentation says it does. For what it's worth, app_dial in Asterisk 1.8.X blocks connected line changes on the incoming channel as well. I'd suggest making this do the same.

app_dial does not block connected line updates from the caller.  The only difference between what FollowMe and Dial do for connected line is that FollowMe saves the update until we have an accepting answerer instead of passing it immediately.

The 'I' option in FollowMe, Dial, and Queue only prevents the called channel from overwriting any manually setup CONNECTEDLINE() values for the initial connect.  Any subsequent connected line updates are passed.


> On Jan. 9, 2012, 2:24 p.m., Mark Michelson wrote:
> > /branches/1.8/apps/app_followme.c, lines 767-770
> > <https://reviewboard.asterisk.org/r/1656/diff/1/?file=22891#file22891line767>
> >
> >     I think this change makes the message less clear. The reason is that saying "SIP/1000 placed on hold" can sanely be interpreted to mean Asterisk is placing SIP/1000 on hold. What the message is trying to convey is that SIP/1000 has placed Asterisk on hold. So either add back "Call on" or change it to something like "%s has placed us on hold".

Changed to:
SIP/1000 placed call on hold
SIP/1000 removed call from hold

Like the control frames reported above, these are also otherwise ignored since we are not bridged yet.


> On Jan. 9, 2012, 2:24 p.m., Mark Michelson wrote:
> > /branches/1.8/apps/app_followme.c, lines 755-764
> > <https://reviewboard.asterisk.org/r/1656/diff/1/?file=22891#file22891line755>
> >
> >     Did you make these changes because there is the possibility that winner and caller are the same channel?

No.  The message output is wrong.  I made these changes because the control frame is not passed.  It is simply ignored because we are not bridged yet.


- rmudgett


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


On Jan. 6, 2012, 4:52 p.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1656/
> -----------------------------------------------------------
> 
> (Updated Jan. 6, 2012, 4:52 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Like Dial and Queue, FollowMe needs to deal with AST_CONTROL_CONNECTED_LINE information so when the parties are initially bridged, the connected line information will be correct.
> 
> Added the 'I' option just like the app_dial and app_queue 'I' option.
> 
> 
> This addresses bug ASTERISK-18969.
>     https://issues.asterisk.org/jira/browse/ASTERISK-18969
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/CHANGES 349955 
>   /branches/1.8/apps/app_dial.c 349955 
>   /branches/1.8/apps/app_followme.c 349955 
> 
> Diff: https://reviewboard.asterisk.org/r/1656/diff
> 
> 
> Testing
> -------
> 
> Tested with and without the new FollowMe 'I' option.  Connected line is updated or blocked in a similar manner to app_dial and app_queue.
> 
> 
> Thanks,
> 
> rmudgett
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120109/3322c4cf/attachment-0001.htm>