<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/2328/">https://reviewboard.asterisk.org/r/2328/</a>
</td>
</tr>
</table>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/2328/diff/4/?file=33451#file33451line270" style="color: black; font-weight: bold; text-decoration: underline;">/team/group/bridge_construction/apps/app_dial.c</a>
<span style="font-weight: normal;">
(Diff revision 4)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">ASTERISK_FILE_VERSION(__FILE__, "$Revision$")</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">270</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> If <span class="hl">not </span>set, the time remaining will be announced.</para></span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">270</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"> If set<span class="hl"> to 'timeleft'</span>, the time remaining will be announced.</para></span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Odd, I could have sworn I reverted this file...</pre>
</div>
<br />
<p>- jrose</p>
<br />
<p>On February 14th, 2013, 2:02 p.m., jrose wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers, Mark Michelson, rmudgett, and Matt Jordan.</div>
<div>By jrose.</div>
<p style="color: grey;"><i>Updated Feb. 14, 2013, 2:02 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Richard had a shopping list of improvements to be made. Here is the copypasta.
1) Support for ${LIMIT_CONNECT_FILE} is missing. This file is played at
the beginning of the bridge. This can be implemented as a one time
interval hook with an immediate timeout that fires as soon as the channel
joins the bridge.
* Addressed as described. It's worth noting that contrary to documentation,
normal Asterisk doesn't actually say the remaining time at the beginning of the
conference if no LIMIT_CONNECT_FILE is specified. This patch changes things to
match with the documentation.
2) The interval_hook_time_cmp() needs to compare a sequence number of the
order of hooks added in addition to the interval_trip_time. This will
ensure that when timeouts happen at the same time, the oldest interval
expiring at that time will execute first.
* Implemented as suggested.
3) Need to add the test events associated with interval hooks. Notably
the end bridge timeout event. There may be other timeout events that I am
not aware of.
* Added to the limits duration callback. The other relevant test event was
for the S option and for L with only the duration argument, and they are
currently executed through a different mechanism that is unaffected by this
work. That might be changed soon.
4) ast_bridge_features_set_limits() must not save a pointer to the limits
struct in the hook. In this case the peer channel will likely have a
stale pointer when ast_bridge_call() is changed to not expect to get the
peer back. Also transfers could move the peer channel to another bridge.
ast_bridge_features_set_limits() should copy the limits information into
its own structure so it is independent.
* ast_bridge_features_set_limits now copies the contents of the limits
struct into a freshly allocated struct and applies it to the bridge channel's
features struct. Only one can be applied to a bridge channel at a time right
now and I'm unsure if that should be changed or if a mechanism for overwriting
it should be provided.
5) I think it is a good feature of the interval hooks to know about the
"timeleft" macro sound filename. This allows the LIMIT_CONNECT_FILE and
LIMIT_WARNING_FILE to be set to "timeleft" and get the time left output.
* timeleft macro sound name is now read by the hook rather than reinterpreted
at the consumer level.
6) I don't particularly like the fact that struct
ast_bridge_features_limits has fixed sized sound filenames.
* Changed to use string fields. It might still be limited from a practical
standpoint by the dial application and/or features.c.
7) I don't think that ast_bridge_features_interval_hook() needs to have a
strict flag. It should always do that if it is a repeating interval.
* Strict flag removed and all intervals are now treated strictly.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Every single scenario I've been testing all along, which covers everything in the bridge test as well as some extra stuff involving setting of specific sound file variables.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-20872">ASTERISK-20872</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/team/group/bridge_construction/apps/app_dial.c <span style="color: grey">(381469)</span></li>
<li>/team/group/bridge_construction/include/asterisk/bridging.h <span style="color: grey">(381469)</span></li>
<li>/team/group/bridge_construction/include/asterisk/bridging_features.h <span style="color: grey">(381469)</span></li>
<li>/team/group/bridge_construction/main/bridging.c <span style="color: grey">(381469)</span></li>
<li>/team/group/bridge_construction/main/features.c <span style="color: grey">(381469)</span></li>
<li>/team/jrose/bridge_projects/bridges/bridge_builtin_interval_features.c <span style="color: grey">(381469)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2328/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>