<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__, &quot;$Revision$&quot;)</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.&lt;/para&gt;</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 &#39;timeleft&#39;</span>, the time remaining will be announced.&lt;/para&gt;</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&#39;s worth noting that contrary to documentation,
normal Asterisk doesn&#39;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&#39;s
features struct. Only one can be applied to a bridge channel at a time right
now and I&#39;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
&quot;timeleft&quot; macro sound filename.  This allows the LIMIT_CONNECT_FILE and
LIMIT_WARNING_FILE to be set to &quot;timeleft&quot; 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&#39;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&#39;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&#39;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>