<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/2578/">https://reviewboard.asterisk.org/r/2578/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On June 1st, 2013, 12:10 a.m. UTC, <b>rmudgett</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<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/2578/diff/2/?file=38835#file38835line86" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/main/features_config.c</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">86</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="cm"><span class="tb"> </span><span class="tb"> </span><span class="tb"> </span>digit sequence mapped to the specified feature for this channel. Thi</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Typo: Thi</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">In case you're curious why this issue was "dropped" rather than "fixed" it's because in my working copy, the word is spelled correctly. I think some weak sed-fu to get rid of red blobs in the diff is what resulted in the s getting lopped off here.</pre>
<br />
<p>- Mark</p>
<br />
<p>On May 30th, 2013, 6:46 p.m. UTC, Mark Michelson wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/static/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, jrose and rmudgett.</div>
<div>By Mark Michelson.</div>
<p style="color: grey;"><i>Updated May 30, 2013, 6:46 p.m.</i></p>
<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-21542">ASTERISK-21542</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
Asterisk
</div>
<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;">This change separates features configuration out of features.c into a separate features_config.c file. In addition, a features_config.h file has been added as a means of getting configured values. The configuration changes are as follows:
* Configuration for features uses the config framework now.
* The FEATURE and FEATUREMAP functions use the same handlers as the configuration. Therefore, any configuration item available in the config file is also available for the FEATURE and FEATUREMAP functions
* Because the FEATURE and FEATUREMAP functions are more complete, feature configuration retrieval is done per channel rather than globally. Global configuration can be retrieved, but this is a rare occurrence.
* The CLI command to show feature configuration has been moved into the features_config.c file as well
* Three new options for attended transfers were added: atxfercomplete, atxferabort, and atxferthreeway. Now the key combinations for completing, aborting, and conferencing an attended transfer are no longer hard coded.
Since feature configuration is handled differently than it used to be, it meant that several items from features.h and features.c could be removed altogether:
* All functions having to do with configuration parsing have been removed from features.c. This includes functions that were used to modify the dialplan due to parking options being parsed.
* Functions that retrieved call features have been removed. This includes the removal of ast_call_feature, its associated locks, the builtin_features array, and functions used to find dynamic features.
The result of removing some of the above meant that there was quite a bit of dead code in features.c. However, I did not remove this code and instead opted to comment it out for several reasons. The following bits have been commented out rather than being removed.
* All builtin feature operations in features.c have been commented out. These items had already been "dead" for a while, but now that nothing references them, they had to be commented out in order to compile with devmode enabled. I left these commented out since there are currently tasks to make the builtin features feature-complete. Leaving the old code around as an example is not a bad thing.
* The unit tests in features.c have been commented out. The new parking code currently does not have unit tests, so keeping the old unit tests around but commented out allows for them to be used as a guide when writing new tests.
There was a tangential change as well: an AST_FEATURES_DTMF_MASK constant was added that can be used to tell if a builtin feature requires DTMF. This was necessary for setting up bridge features since the ast_call_feature structure was removed.
Things I'm looking for in this review:
* Are there items in features.c that I have commented out that should just plain be removed instead?
* Are there any items in features.c that I removed that should not have been?</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;">Tested configuration parsing. Ensured that general features, featuremap items, applicationmap items, and featuregroups were set and retrievable properly.
Tested that the FEATURE and FEATUREMAP dialplan functions overrode the global configuration in features.conf
Tested that items in the featuremap and applicationmap could be triggered mid-call. This includes the new attended transfer options.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/trunk/apps/app_dial.c <span style="color: grey">(390144)</span></li>
<li>/trunk/bridges/bridge_builtin_features.c <span style="color: grey">(390144)</span></li>
<li>/trunk/channels/chan_dahdi.c <span style="color: grey">(390144)</span></li>
<li>/trunk/channels/chan_mgcp.c <span style="color: grey">(390144)</span></li>
<li>/trunk/channels/chan_misdn.c <span style="color: grey">(390144)</span></li>
<li>/trunk/channels/chan_sip.c <span style="color: grey">(390144)</span></li>
<li>/trunk/channels/chan_unistim.c <span style="color: grey">(390144)</span></li>
<li>/trunk/channels/sig_analog.c <span style="color: grey">(390144)</span></li>
<li>/trunk/channels/sip/include/sip.h <span style="color: grey">(390144)</span></li>
<li>/trunk/include/asterisk/bridging_features.h <span style="color: grey">(390144)</span></li>
<li>/trunk/include/asterisk/channel.h <span style="color: grey">(390144)</span></li>
<li>/trunk/include/asterisk/features.h <span style="color: grey">(390144)</span></li>
<li>/trunk/include/asterisk/features_config.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/main/bridging.c <span style="color: grey">(390144)</span></li>
<li>/trunk/main/features.c <span style="color: grey">(390144)</span></li>
<li>/trunk/main/features_config.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/trunk/main/manager.c <span style="color: grey">(390144)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2578/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>