<p>Richard Mudgett <strong>posted comments</strong> on this change.</p><p><a href="https://gerrit.asterisk.org/6634">View Change</a></p><p>Patch set 1:</p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">We change the behaviour quite a bit with patch set 1. With this<br>change in mind, I think res = say_periodic_announcement(&qe,<br>ringing) should be within a if (makeannouncement) block, and<br>makeannouncement = 1 should be removed? If we do those two<br>modifications, then the only change compared to now is we check<br>qe.parent->announce_to_first_user at most once per Queue()<br>execution, which is fine, but there is also the following change:<br>Before we would never call say_position in the first loop<br>iteration, and with this change we might. I don't know whether<br>say_position would ever do anything in the first loop iteration but<br>it seems the whole purpose of the makeannouncement variable before<br>was to prevent this? Actually the existing makeannouncement<br>variable is very spurious, can we find out what its purpose was? Is<br>the goal of this commit just a code refactoring to improve<br>readability or performance? I'm personally already happy with the<br>current behaviour of app_queue as of ASTERISK-27126</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">The goal of this patch is to fix the regression caused by the merged ASTERISK-27216 patch and to achieve the aim of that patch.  The use of qe.parent->announce_to_first_user in the merged patch results in absolutely no position or periodic announcements unless the announce-to-first-user configuration option is enabled.  By default the option is not enabled.  I'm inferring that you have the option enabled.</p><p style="white-space: pre-wrap; word-wrap: break-word;">The behaviour of the makeannouncement flag is to suppress the queue position announcement on the first time through the loop.  Looking at the code where makeannouncement was originally added in 2004 appears to confirm this.  However, the patch for ASTERISK-2672 where makeannouncement first appears makes no mention of why the code needed to do it.  It wasn't the focus of the ASTERISK-2672 patch.  The announce-to-first-user configuration option wants to allow this first announcement.  That is why makeannouncement is now defaulted to the announce-to-first-user setting in the current patch.</p><p style="white-space: pre-wrap; word-wrap: break-word;">No, say_periodic_announcement() should not be placed within the makeannouncement block.  It wasn't there before the merged ASTERISK-27216 patch and it doesn't announce on the first time anyway because the period timer (qe.last_periodic_announce_time) was just started when the channel joins the queue.</p><ul style="list-style: none; padding-left: 20px;"></ul><p>To view, visit <a href="https://gerrit.asterisk.org/6634">change 6634</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/6634"/><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: Ieaeb7dbea8ae7073086b775fbafe0625b000b10a </div>
<div style="display:none"> Gerrit-Change-Number: 6634 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins2 </div>
<div style="display:none"> Gerrit-Reviewer: Richard Mudgett <rmudgett@digium.com> </div>
<div style="display:none"> Gerrit-Reviewer: Stefan Engström <stefanen@kth.se> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 02 Oct 2017 18:38:34 +0000 </div>
<div style="display:none"> Gerrit-HasComments: No </div>