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

[Stefan Engström]

Stefan Engström 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


-- 
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: Stefan Engström <stefanen at kth.se>
Gerrit-Comment-Date: Fri, 29 Sep 2017 17:26:49 +0000
Gerrit-HasComments: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20170929/aefcb2a2/attachment.html>