[asterisk-dev] [Code Review]: Revamp app_confbridge into something resembling a state machine

[Terry Wilson]

> On Aug. 22, 2012, 11:38 a.m., Mark Michelson wrote:
> > /branches/10/apps/confbridge/include/confbridge.h, line 353
> > <https://reviewboard.asterisk.org/r/2072/diff/4/?file=31066#file31066line353>
> >
> >     The param is "conference_bridge" not "conference bridge"

It says \param conference_bridge already


> On Aug. 22, 2012, 11:38 a.m., Mark Michelson wrote:
> > /branches/10/apps/confbridge/include/confbridge.h, line 237
> > <https://reviewboard.asterisk.org/r/2072/diff/4/?file=31066#file31066line237>
> >
> >     This is a real stretch, but since we're including pjlib in Asterisk now, they have a type called pj_list. I'd feel warmer and fuzzier if this were called something like post_join_list just so there's no confusion.

good call. done.


> On Aug. 22, 2012, 11:38 a.m., Mark Michelson wrote:
> > /branches/10/apps/app_confbridge.c, line 278
> > <https://reviewboard.asterisk.org/r/2072/diff/4/?file=31057#file31057line278>
> >
> >     Coding guidelines require spaces around the '=' sign.

doh, fixed.


> On Aug. 22, 2012, 11:38 a.m., Mark Michelson wrote:
> > /branches/10/apps/confbridge/include/confbridge.h, line 234
> > <https://reviewboard.asterisk.org/r/2072/diff/4/?file=31066#file31066line234>
> >
> >     This isn't used anywhere. Remove.

fixed.


> On Aug. 22, 2012, 11:38 a.m., Mark Michelson wrote:
> > /branches/10/apps/app_confbridge.c, line 61
> > <https://reviewboard.asterisk.org/r/2072/diff/4/?file=31057#file31057line61>
> >
> >     It looks like you aren't actually using a taskprocessor anywhere here, so you can remove this #include

fixed.


> On Aug. 22, 2012, 11:38 a.m., Mark Michelson wrote:
> > /branches/10/apps/confbridge/include/confbridge.h, line 358
> > <https://reviewboard.asterisk.org/r/2072/diff/4/?file=31066#file31066line358>
> >
> >     The param is "conference_bridge" not "conference bridge user"

fixed.


> On Aug. 22, 2012, 11:38 a.m., Mark Michelson wrote:
> > /branches/10/apps/confbridge/include/confbridge.h, line 368
> > <https://reviewboard.asterisk.org/r/2072/diff/4/?file=31066#file31066line368>
> >
> >     The param is "conference_bridge" not "cbu"
> >     
> >     There are a few more errors along these lines. I'm not going to point out the rest of them.

Fixed this one, although I didn't notice any other mistakes


- Terry


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2072/#review6948
-----------------------------------------------------------


On Aug. 21, 2012, 11:28 a.m., Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2072/
> -----------------------------------------------------------
> 
> (Updated Aug. 21, 2012, 11:28 a.m.)
> 
> 
> Review request for Asterisk Developers and Neil Tallim.
> 
> 
> Summary
> -------
> 
> 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?
> 
> 
> This addresses bugs ASTERISK-19562, ASTERISK-19726 and ASTERISK-20181.
>     https://issues.asterisk.org/jira/browse/ASTERISK-19562
>     https://issues.asterisk.org/jira/browse/ASTERISK-19726
>     https://issues.asterisk.org/jira/browse/ASTERISK-20181
> 
> 
> Diffs
> -----
> 
>   /branches/10/apps/app_confbridge.c 371580 
>   /branches/10/apps/confbridge/conf_state.c PRE-CREATION 
>   /branches/10/apps/confbridge/conf_state_empty.c PRE-CREATION 
>   /branches/10/apps/confbridge/conf_state_inactive.c PRE-CREATION 
>   /branches/10/apps/confbridge/conf_state_marked.c PRE-CREATION 
>   /branches/10/apps/confbridge/conf_state_multi.c PRE-CREATION 
>   /branches/10/apps/confbridge/conf_state_single.c PRE-CREATION 
>   /branches/10/apps/confbridge/conf_state_single_marked.c PRE-CREATION 
>   /branches/10/apps/confbridge/include/conf_state.h PRE-CREATION 
>   /branches/10/apps/confbridge/include/confbridge.h 371580 
> 
> Diff: https://reviewboard.asterisk.org/r/2072/diff
> 
> 
> Testing
> -------
> 
> Lots of calls. Need to do more formalized testing.
> 
> 
> Thanks,
> 
> Terry
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20120823/ed5c5f81/attachment-0001.htm>