[asterisk-dev] [Code Review] [confbridge] Behavioural correction for hold-music state when users join/part conferences in varying combinations

[Neil Tallim]

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



/trunk/apps/app_confbridge.c
<https://reviewboard.asterisk.org/r/1869/#comment11582>

    While true, the decision only makes sense when the second participant joins. By the time a third has joined, nothing else in the logic of the confbridge app will alter the state in any meaningful way.
    
    Making it > 1 won't hurt anything, but it will result in unnecessary checks. 



/trunk/apps/app_confbridge.c
<https://reviewboard.asterisk.org/r/1869/#comment11583>

    They can right now, yes, but another change I was working on in parallel (explicit toggling of MoH via AMI/CLI) requires them to be separate because it introduces another variable that needs to be checked.
    
    That patch was not combined with this one because it's a feature, but a consistency correction.



/trunk/apps/app_confbridge.c
<https://reviewboard.asterisk.org/r/1869/#comment11584>

    I was planning to submit a refactoring patch to do just that once my other patches (now eight in total) were approved or rejected. If I were to make that change now, I fear I might break other pending submissions.



/trunk/apps/app_confbridge.c
<https://reviewboard.asterisk.org/r/1869/#comment11585>

    While the Asterisk styleguide does say these should not be mixed, the published confbridge code did it in many places. In keeping with the style of the module, I followed suit. I have no objection to restructuring to be compliant, but another patch will be required to enforce full-file consistency, so I would like to leave that for a refactoring patch to be submitted after the other patches I have pending have been reviewed, to avoid diff-fuzz.
    
    
    As for the number of participants, I disagree: the test needs to be whether we have any other participants who don't require a marked user present in order to speak, or else some people who should be able to converse will be incorrectly set to listening to MoH (and that would cause another problem addressed by the patch I submitted regarding MoH not being consistent with mute).
    
    That the number is counted instead of just setting a value to 1 like a fake-boolean may be misleading, however, since if even one exists, the condition is satisfied.



/trunk/apps/app_confbridge.c
<https://reviewboard.asterisk.org/r/1869/#comment11586>

    This is because I was working across multiple systems and, in some of them, gEdit wasn't set up with consistent tab/space replacement. I probably should have just determined what the file's normal indentation rules were and run astyle over it before generating the diff or something. I'll do that with any future patches.



/trunk/apps/app_confbridge.c
<https://reviewboard.asterisk.org/r/1869/#comment11587>

    1. As with the previous instance, these serve as a hook for another patch that adds a feature.
    
    2. The value differentiates between users with WAITMARKED and those without. According to the config file and ConfBridge wiki entry by Malcolm Davenport, which I'm considering as official documentation, users without that flag set do NOT need to wait for a marked user to begin speaking. The changes in this patch make it such that, if one such user is in a conference with no eligible speech partners, they hear MoH as a comfort-noise thing, but as soon as someone they can speak to, be it a marked user or another non-WAITMARKED user, joins, they're able to converse freely.
    
    3. I've personally always found "continue" statements surprising when reading code. I always feel I need to do a double-take to understand why the loop is progressing, hence why they're commented. I have no problem with removing them if you believe the logic leading up to them is clear, however.


- Neil


On May 9, 2012, 4:35 p.m., Neil Tallim wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1869/
> -----------------------------------------------------------
> 
> (Updated May 9, 2012, 4:35 p.m.)
> 
> 
> Review request for Asterisk Developers, David Vossel and jcolp.
> 
> 
> Summary
> -------
> 
> This is the "everything" patch attached to issue https://issues.asterisk.org/jira/browse/ASTERISK-19562
> 
> Details about what it does may be found in the issue's description.
> 
> 
> Diffs
> -----
> 
>   /trunk/apps/app_confbridge.c 359977 
> 
> Diff: https://reviewboard.asterisk.org/r/1869/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Neil
> 
>

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