[Asterisk-code-review] app queue.c: Fix announcements when announce-to-first-user n... (asterisk[13])

[Richard Mudgett]

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/6634 )

Change subject: app_queue.c: Fix announcements when announce-to-first-user not enabled.
......................................................................


Patch Set 1:

> We change the behaviour quite a bit with patch set 1. With this
 > change in mind, I think res = say_periodic_announcement(&qe,
 > ringing) should be within a if (makeannouncement) block, and
 > makeannouncement = 1 should be removed? If we do those two
 > modifications, then the only change compared to now is we check
 > qe.parent->announce_to_first_user at most once per Queue()
 > execution, which is fine, but there is also the following change:
 > Before we would never call say_position in the first loop
 > iteration, and with this change we might. I don't know whether
 > say_position would ever do anything in the first loop iteration but
 > it seems the whole purpose of the makeannouncement variable before
 > was to prevent this? Actually the existing makeannouncement
 > variable is very spurious, can we find out what its purpose was? Is
 > the goal of this commit just a code refactoring to improve
 > readability or performance? I'm personally already happy with the
 > current behaviour of app_queue as of ASTERISK-27126

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.

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.

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.


-- 
To view, visit https://gerrit.asterisk.org/6634
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieaeb7dbea8ae7073086b775fbafe0625b000b10a
Gerrit-Change-Number: 6634
Gerrit-PatchSet: 1
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Stefan Engström <stefanen at kth.se>
Gerrit-Comment-Date: Mon, 02 Oct 2017 18:38:34 +0000
Gerrit-HasComments: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20171002/8119e506/attachment.html>