<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/2072/">https://reviewboard.asterisk.org/r/2072/</a>
</td>
</tr>
</table>
<br />
<p>Ship it!</p>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Overall I think this is a very sensible way to solve this problem. One thing that might be worth considering though is that you are handling state transition behavior as part of the events that take place when you are going to change state, and it might eventually be the case that there are multiple ways to perform the same transition. I think transition behavior as well as the state change itself should be put into a separate function from where the need for a state change is determined. That's something that could be broken out at any time though.</pre>
<br />
<div>
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/2072/diff/5/?file=31082#file31082line22" style="color: black; font-weight: bold; text-decoration: underline;">/branches/10/apps/confbridge/include/conf_state.h</a>
<span style="font-weight: normal;">
(Diff revision 5)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">struct conference_state {</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">22</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">conference_exit_fn</span> <span class="n">exit</span><span class="p">;</span> <span class="cm">/*!< Function to handle exiting from a state */</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I notice none of the states actually sets the exit callback. Got any plans for these? It completes the concept, but I think this is probably going to be unused to the point that it's just a waste of a pointer check that happens during every state transition. The performance concern is irrelevant since the memory involved is negligible and so is checking if a pointer isn't NULL, </pre>
</div>
<br />
<p>- jrose</p>
<br />
<p>On August 23rd, 2012, 11:01 a.m., Terry Wilson 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 and Neil Tallim.</div>
<div>By Terry Wilson.</div>
<p style="color: grey;"><i>Updated Aug. 23, 2012, 11:01 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;">Thank's to flan's tireless testing, issue reporting, and patches it became clear that app_confbridge had some complex logic and was missing some corner cases. In particular, it didn't handle mixing unmarked and waitmarked users very well and was inconsistent in how MOH and muting was applied. flan's patches seem to fix the issue, but they highlight how hard the code could be to maintain. In an attempt to make things easier to maintain and to more fully enumerate the various cases that exist, I have attempted to break up the logic into a state machine-like setup.
This should fix all of the issues in the linked bugs, as well as a couple of other issues. I still need to add some documentation, but I thought I'd give people a chance to look at it test it out, and comment.
One thing to note is that this solution involves treating waitmarked users without a marked user as essentially outside of the conference. For now, I have the announce_user_count stuff taking into account both the counts for waiting and active users which can lead to things like having a single waiting user followed by an unmarked user and the announcement that "there is one other user in the conference" (because we count the waiting user there) followed by "you are the only user in the conference" (because that prompt plays when you are the only active user) being played to the unmarked user. This is easy to fix, we just need to decide which way to go. Count users who are essentially outside the conference (they can't speak or hear) or not?</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;">Lots of calls. Need to do more formalized testing.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-19562">ASTERISK-19562</a>,
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-19726">ASTERISK-19726</a>,
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-20181">ASTERISK-20181</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/branches/10/apps/app_confbridge.c <span style="color: grey">(371641)</span></li>
<li>/branches/10/apps/confbridge/conf_state.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/branches/10/apps/confbridge/conf_state_empty.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/branches/10/apps/confbridge/conf_state_inactive.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/branches/10/apps/confbridge/conf_state_marked.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/branches/10/apps/confbridge/conf_state_multi.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/branches/10/apps/confbridge/conf_state_single.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/branches/10/apps/confbridge/conf_state_single_marked.c <span style="color: grey">(PRE-CREATION)</span></li>
<li>/branches/10/apps/confbridge/include/conf_state.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>/branches/10/apps/confbridge/include/confbridge.h <span style="color: grey">(371641)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/2072/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>