[Asterisk-code-review] chan_pjsip: Add option for moh allows only answered channel (asterisk[master])

[N A]

Attention is currently required from: sungtae kim.
N A has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/18781 )

Change subject: chan_pjsip: Add option for moh allows only answered channel
......................................................................


Patch Set 4: Code-Review-1

(5 comments)

Commit Message:

https://gerrit.asterisk.org/c/asterisk/+/18781/comment/e17c8288_f4344d23 
PS4, Line 7: chan_pjsip: Add option for moh allows only answered channel
This title is unclear to me. It sounds like this option ignores hold if the channel is unanswered.

Maybe "chan_pjsip: Add option to not play music on hold on unanswered channels"?


https://gerrit.asterisk.org/c/asterisk/+/18781/comment/c5c6e19a_d47a2d16 
PS4, Line 11: 
I don't follow the description. The code seems to ignore and not do MOH if it's not answered, rather than processing it at a later point. Maybe you mean "only if... has" rather than "when... is"?


Patchset:

PS4: 
A few more nit picks... mostly just clearer wording to help users understand this option.


File channels/chan_pjsip.c:

https://gerrit.asterisk.org/c/asterisk/+/18781/comment/af444eb6_2cdcd4ff 
PS4, Line 1771: 			ast_log(LOG_DEBUG, "The session '%s' is not able to oh hold with endpoint '%s'.\n",
1. I think you have a typo here, extraneous "oh"?
2. Also use ast_debug with a level number, not ast_log(LOG_DEBUG
3. This code only executes when the channel *is* up, is it supposed to be when it's *not* up?
4. Finally, if this option is intentionally ignoring the hold, don't say "not able to hold" because that makes it sound like it tried to and failed.


File configs/samples/pjsip.conf.sample:

https://gerrit.asterisk.org/c/asterisk/+/18781/comment/88111ee1_0aa0e2f6 
PS4, Line 678: ;moh_answeredonly=yes   ; Allows the only answered channel able to be music on hold.
This phrasing is unclear to me and a casual reader would not understand this. Maybe "Only allow answered channels to have music on hold; enabling this will prevent MOH on unanswered channels".



-- 
To view, visit https://gerrit.asterisk.org/c/asterisk/+/18781
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Change-Id: I3c9b9101e4dc85338154b8004cecdd3edc227474
Gerrit-Change-Number: 18781
Gerrit-PatchSet: 4
Gerrit-Owner: sungtae kim <pchero21 at gmail.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: N A <mail at interlinked.x10host.com>
Gerrit-Attention: sungtae kim <pchero21 at gmail.com>
Gerrit-Comment-Date: Tue, 12 Jul 2022 16:29:12 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20220712/ac44473b/attachment-0001.html>