<p>Richard Mudgett <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/9060">View Change</a></p><p>Patch set 3:</p><p style="white-space: pre-wrap; word-wrap: break-word;">The patch is fine as far functionality is concerned.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The sticking point is the policy that new features need testsuite tests when added to released branches.  The patch falls into a gray area because it now sends out current state information on an existing AMI event.  What is the needed test to check?  The ability to determine talking status?  The presence of the Talking AMI header in the event?</p><p style="white-space: pre-wrap; word-wrap: break-word;">Also since this is changing an AMI event, the AMI version needs to be bumped (but NOT by this patch) appropriately when the next Asterisk release is made.</p><p style="white-space: pre-wrap; word-wrap: break-word;">A note in CHANGES in the appropriate place is needed to document that the AMI event now has the new header available.  Where the note needs to go depends if the patch is going into v13/v15 or just in master.</p><p>(1 comment)</p><ul style="list-style: none; padding-left: 20px;"><li><p><a href="https://gerrit.asterisk.org/#/c/9060/3/apps/app_confbridge.c">File apps/app_confbridge.c:</a></p><ul style="list-style: none; padding-left: 20px;"><li><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/#/c/9060/3/apps/app_confbridge.c@3540">Patch Set #3, Line 3540:</a> <code style="font-family:monospace,monospace">           "Talking: %s\r\n"</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This is shown even when talk_detection_events=no. Should this only be shown</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">No.  This should always be present just like Muted is always present.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/9060">change 9060</a>. To unsubscribe, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/9060"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: 13 </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I19b5284f34966c3fda94f5b99a7e40e6b89767c6 </div>
<div style="display:none"> Gerrit-Change-Number: 9060 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: William McCall <william.mccall@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Matthew Fredrickson <creslin@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: William McCall <william.mccall@gmail.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 31 May 2018 13:01:52 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>