<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/2048/">https://reviewboard.asterisk.org/r/2048/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On July 31st, 2012, 11:07 a.m., <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">From what I can tell, all the changes here are good. The problem is that the chan_sip.c portion of the diff has a lot of unrelated changes. I looked through and found the changes you made to extensionstate_update(). Did you make any other changes in the latest version of the diff?</pre>
</blockquote>
<p>On August 1st, 2012, 3:52 a.m., <b>Guenther Kelleter</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The unrelated changes in chan_sip.c are not completely unrelated:-)
Determination of the oldest ringing channel of all watched devices was moved from pbx.c to chan_sip extension_state_update(). The extra argument "queued" is required for proper operation when a notification was queued and is sent after the current notification dialog was acked.
Along the way I fixed the prototype of extension_state_update() to use const to avoid having to cast away const of arguments, which is plain ugly.
The first change in state_notify_build_xml() is a bug fix and is required because the passed data->state can be -1 or -2 when the hint goes away and the final notification is generated. Without this fix state -1 and -2 would be regarded as AST_EXTENSION_RINGING which is obviously wrong.
There are no other unrelated changes.</pre>
</blockquote>
<p>On August 2nd, 2012, 11:51 a.m., <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I had meant the changes for ICE support and such. I meant that your diff wasn't "clean" . You had pulled in changes from unrelated commits.</pre>
</blockquote>
<p>On August 2nd, 2012, 12:18 p.m., <b>Thomas Arimont</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Mark, I cannot see any ICE support (and such) related changes in the latest diff? What exactly do you mean?</pre>
</blockquote>
<p>On August 2nd, 2012, 12:35 p.m., <b>Mark Michelson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Well that's weird. When I made my comment two days ago, there was a bunch of extra stuff in chan_sip.c that did not belong. It doesn't appear to be there now.</pre>
</blockquote>
<p>On August 3rd, 2012, 1:56 a.m., <b>Guenther Kelleter</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Maybe you compared diff 1 against diff 2 in the "View Diff" tab ("changes between r2 and 1")? You can't do that if the base revisions of the two diffs are different because the reviewboard seems to compare the base revision's files with diffs applied, not the pure diffs. It looks like other changes crept in then.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Yes, that's what I did. That kind of makes the review board feature useless.</pre>
<br />
<p>- Mark</p>
<br />
<p>On August 3rd, 2012, 6:53 a.m., Guenther Kelleter wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers, Mark Michelson, Matt Jordan, and Thomas Arimont.</div>
<div>By Guenther Kelleter.</div>
<p style="color: grey;"><i>Updated Aug. 3, 2012, 6:53 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">(For Digium: this is the implementation of the feature described in chapter 2 of the document SIP_NOTIFY_dialog_event_infos_DATUS_v1_4.odt)
This patch extends the extension state callbacks so that monitoring channels (as chan_sip) get more information of the devices which are responsible for an extension state change. The additional information is needed by chan_sip to present names/numbers of the caller and callee in an early-state SIP notification. Users of extenstion state callback not interested in the additional information are not affected by the changes.
Motivation: to present the involved party's name/number in an early-state nofification (used by the notified device as a pickup offer) one after another so that a user can see which call he will pick up in an undirected pickup.
Such a pickup offer to a user shall indicate the same call (number/name-A calls number/name-B) as the call which would be picked up when an undirected pickup is executed.
Users interested in additional state info must use the new functions ast_extension_state_add_extended() resp. ast_extension_state_add_destroy_extended() to register an extended state callback. When the callback is registered this way, an extra member device_state_info of struct ast_state_cb_info is passed to the callback in addition to the aggregated extension state. This container holds an object for every device of the monitored extension hint consisting of the device name, the device state and a channel reference to the channel which (presumably) caused the device state.
The information is used by chan_sip for early-state notifications. When the state of a device changes and the new state contains AST_EVENT_RINGING, an early-state notification is sent to the subscribed devices with the caller/callee names/numbers of the oldest ringing channel of the monitored extension.
The notified user may then invoke a direct pickup, which will pickup exactly this channel.
Users of the old non-extended callbacks will only be called when the aggregated state did change (same behavior as before). Users of the extended callback will also be called when the state is unchanged but does contain AST_EVENT_RINGING. That could be the case if two channels are ringing at one device and one of them hangs up, so the aggregated state does not change. This way the monitoring channel can create a new early-state notification with the now ringing party-ids.
This patch requires the channel creation time member of ast_channel and the changes for the direct pickup to pick up the oldest ringing channel, which are both introduced in review https://reviewboard.asterisk.org/r/2043/.
</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Checked the name/number of caller/callee displayed on the notified phone.
Checked that a second call at an already ringing device does not invoke a notification as long as the first call is ringing, instead it is sent when the first ringing channel is picked up by some other party or hung up.
Checked that the picked up caller is the same as advertized in the early-state notification when more than one calls are ringing on the subscribed extension.
Ref counting for the containers and channels checked.
Checked that a proper notification is sent for newly subscribed watchers and resubscriptions, as required by RFC 3265.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/trunk/channels/sip/include/sip.h <span style="color: grey">(370760)</span></li>
<li>/trunk/include/asterisk/pbx.h <span style="color: grey">(370760)</span></li>
<li>/trunk/main/pbx.c <span style="color: grey">(370760)</span></li>
<li>/trunk/channels/chan_sip.c <span style="color: grey">(370760)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2048/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>