<p> Attention is currently required from: sungtae kim. </p>
<p>Patch set 4:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4; color: #000000;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/18781">View Change</a></p><p>5 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="null">Commit Message:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18781/comment/e17c8288_f4344d23">Patch Set #4, Line 7:</a> <code style="font-family:monospace,monospace">chan_pjsip: Add option for moh allows only answered channel</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This title is unclear to me. It sounds like this option ignores hold if the channel is unanswered.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Maybe "chan_pjsip: Add option to not play music on hold on unanswered channels"?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18781/comment/c5c6e19a_d47a2d16">Patch Set #4, Line 11:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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"?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">Patchset:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18781?tab=comments">Patch Set #4:</a> </p><p style="white-space: pre-wrap; word-wrap: break-word;">A few more nit picks... mostly just clearer wording to help users understand this option.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File channels/chan_pjsip.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18781/comment/af444eb6_2cdcd4ff">Patch Set #4, Line 1771:</a> <code style="font-family:monospace,monospace">                      ast_log(LOG_DEBUG, "The session '%s' is not able to oh hold with endpoint '%s'.\n",</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">1. I think you have a typo here, extraneous "oh"?<br>2. Also use ast_debug with a level number, not ast_log(LOG_DEBUG<br>3. This code only executes when the channel *is* up, is it supposed to be when it's *not* up?<br>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.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="null">File configs/samples/pjsip.conf.sample:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18781/comment/88111ee1_0aa0e2f6">Patch Set #4, Line 678:</a> <code style="font-family:monospace,monospace">;moh_answeredonly=yes   ; Allows the only answered channel able to be music on hold.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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".</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/18781">change 18781</a>. To unsubscribe, or for help writing mail filters, 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/c/asterisk/+/18781"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I3c9b9101e4dc85338154b8004cecdd3edc227474 </div>
<div style="display:none"> Gerrit-Change-Number: 18781 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </div>
<div style="display:none"> Gerrit-Owner: sungtae kim <pchero21@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: N A <mail@interlinked.x10host.com> </div>
<div style="display:none"> Gerrit-Attention: sungtae kim <pchero21@gmail.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 12 Jul 2022 16:29:12 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>