Re: [asterisk-dev] [Code Review] 4477: app_confbridge (11): file playback blocks dtmf
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4477/ --- (Updated March 26, 2015, noon) Status -- This change has been marked as submitted. Review request for Asterisk Developers. Changes --- Committed in revision 433445 Bugs: ASTERISK-24864 https://issues.asterisk.org/jira/browse/ASTERISK-24864 Repository: Asterisk Description --- Attempting to execute DTMF in a confbridge while file playback (prompt, announcement, etc) is occurring is not allowed. You have to wait until the sound file has completed before entering DTMF. This patch fixes it so that app_confbridge now monitors for dtmf key presses during file playback. If a key is pressed playback stops and it executes the matched menu option. Diffs - branches/11/apps/app_confbridge.c 433195 Diff: https://reviewboard.asterisk.org/r/4477/diff/ Testing --- Manual testing done. Setup a basic conference bridge that allowed both regular and admin users to enter. Ran through various menu options to make sure the sound file playback would stop (no longer have to wait) and a new option was executed when appropriate. Also ran the app_confbridge testsuite tests to make sure they still passed. Thanks, Kevin Harwell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4477: app_confbridge (11): file playback blocks dtmf
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4477/#review14786 --- Ship it! Ship It! - Joshua Colp On March 19, 2015, 9:59 p.m., Kevin Harwell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4477/ --- (Updated March 19, 2015, 9:59 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24864 https://issues.asterisk.org/jira/browse/ASTERISK-24864 Repository: Asterisk Description --- Attempting to execute DTMF in a confbridge while file playback (prompt, announcement, etc) is occurring is not allowed. You have to wait until the sound file has completed before entering DTMF. This patch fixes it so that app_confbridge now monitors for dtmf key presses during file playback. If a key is pressed playback stops and it executes the matched menu option. Diffs - branches/11/apps/app_confbridge.c 433195 Diff: https://reviewboard.asterisk.org/r/4477/diff/ Testing --- Manual testing done. Setup a basic conference bridge that allowed both regular and admin users to enter. Ran through various menu options to make sure the sound file playback would stop (no longer have to wait) and a new option was executed when appropriate. Also ran the app_confbridge testsuite tests to make sure they still passed. Thanks, Kevin Harwell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4477: app_confbridge (11): file playback blocks dtmf
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4477/#review14797 --- Ship it! - rmudgett On March 19, 2015, 4:59 p.m., Kevin Harwell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4477/ --- (Updated March 19, 2015, 4:59 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24864 https://issues.asterisk.org/jira/browse/ASTERISK-24864 Repository: Asterisk Description --- Attempting to execute DTMF in a confbridge while file playback (prompt, announcement, etc) is occurring is not allowed. You have to wait until the sound file has completed before entering DTMF. This patch fixes it so that app_confbridge now monitors for dtmf key presses during file playback. If a key is pressed playback stops and it executes the matched menu option. Diffs - branches/11/apps/app_confbridge.c 433195 Diff: https://reviewboard.asterisk.org/r/4477/diff/ Testing --- Manual testing done. Setup a basic conference bridge that allowed both regular and admin users to enter. Ran through various menu options to make sure the sound file playback would stop (no longer have to wait) and a new option was executed when appropriate. Also ran the app_confbridge testsuite tests to make sure they still passed. Thanks, Kevin Harwell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4477: app_confbridge (11): file playback blocks dtmf
On March 18, 2015, 12:39 p.m., rmudgett wrote: branches/11/apps/app_confbridge.c, lines 1847-1853 https://reviewboard.asterisk.org/r/4477/diff/2/?file=72633#file72633line1847 Swaping which plays first will allow the channel's prompt to be interruptable. Although doing this will make it kind of awkward that there will be a delay in the user hearing the prompt. Good suggestion, but I think swapping it and having a delayed playback is more ackward than not being allowed to interrupt the playback. - Kevin --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4477/#review14733 --- On March 17, 2015, 1 p.m., Kevin Harwell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4477/ --- (Updated March 17, 2015, 1 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24864 https://issues.asterisk.org/jira/browse/ASTERISK-24864 Repository: Asterisk Description --- Attempting to execute DTMF in a confbridge while file playback (prompt, announcement, etc) is occurring is not allowed. You have to wait until the sound file has completed before entering DTMF. This patch fixes it so that app_confbridge now monitors for dtmf key presses during file playback. If a key is pressed playback stops and it executes the matched menu option. Diffs - branches/11/apps/confbridge/include/confbridge.h 432991 branches/11/apps/confbridge/conf_state_multi_marked.c 432991 branches/11/apps/app_confbridge.c 432991 Diff: https://reviewboard.asterisk.org/r/4477/diff/ Testing --- Manual testing done. Setup a basic conference bridge that allowed both regular and admin users to enter. Ran through various menu options to make sure the sound file playback would stop (no longer have to wait) and a new option was executed when appropriate. Also ran the app_confbridge testsuite tests to make sure they still passed. Thanks, Kevin Harwell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4477: app_confbridge (11): file playback blocks dtmf
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4477/ --- (Updated March 19, 2015, 4:59 p.m.) Review request for Asterisk Developers. Changes --- Addressed review findings. Bugs: ASTERISK-24864 https://issues.asterisk.org/jira/browse/ASTERISK-24864 Repository: Asterisk Description --- Attempting to execute DTMF in a confbridge while file playback (prompt, announcement, etc) is occurring is not allowed. You have to wait until the sound file has completed before entering DTMF. This patch fixes it so that app_confbridge now monitors for dtmf key presses during file playback. If a key is pressed playback stops and it executes the matched menu option. Diffs (updated) - branches/11/apps/app_confbridge.c 433195 Diff: https://reviewboard.asterisk.org/r/4477/diff/ Testing --- Manual testing done. Setup a basic conference bridge that allowed both regular and admin users to enter. Ran through various menu options to make sure the sound file playback would stop (no longer have to wait) and a new option was executed when appropriate. Also ran the app_confbridge testsuite tests to make sure they still passed. Thanks, Kevin Harwell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4477: app_confbridge (11): file playback blocks dtmf
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4477/#review14733 --- branches/11/apps/app_confbridge.c https://reviewboard.asterisk.org/r/4477/#comment25310 I think some more thought needs to go into the parameters passed to this function because the bridge_channel and channel parameters are a bit redundant. branches/11/apps/app_confbridge.c https://reviewboard.asterisk.org/r/4477/#comment25305 No init needed with below change. branches/11/apps/app_confbridge.c https://reviewboard.asterisk.org/r/4477/#comment25304 Add stop_digits = AST_DIGIT_NONE; here. branches/11/apps/app_confbridge.c https://reviewboard.asterisk.org/r/4477/#comment25306 Please get out of the habit of assigning in if tests: digit = ast_stream_and_wait(...) if (digit 0) branches/11/apps/app_confbridge.c https://reviewboard.asterisk.org/r/4477/#comment25311 The bridge_channel parameter really acts as a flag for if the prompts are interruptable or not. The caller sort of already knows this so it should be a flag instead of a pointer. branches/11/apps/app_confbridge.c https://reviewboard.asterisk.org/r/4477/#comment25309 Revert this part. It is not interruptable because you currently have no way of telling if the return of play_file() was because the file could not play or was interrupted by DTMF. branches/11/apps/app_confbridge.c https://reviewboard.asterisk.org/r/4477/#comment25302 This function plays to the bridge so its playback cannot not be interrupted since the bridge has no menu. Only users have menus. Revert this change. This will simplify the patch because of the ripple effect. branches/11/apps/app_confbridge.c https://reviewboard.asterisk.org/r/4477/#comment25303 Revert this too for the same reason. branches/11/apps/app_confbridge.c https://reviewboard.asterisk.org/r/4477/#comment25312 This is really awkward: conference_bridge_user bridge_channel chan All indicate the channel. In this case the function is always called when the channel is in the bridge so the prompt is always interruptable. branches/11/apps/app_confbridge.c https://reviewboard.asterisk.org/r/4477/#comment25313 Similar situation here as with action_toggle_mute(). bridge_channel is not even used. branches/11/apps/app_confbridge.c https://reviewboard.asterisk.org/r/4477/#comment25314 Swaping which plays first will allow the channel's prompt to be interruptable. Although doing this will make it kind of awkward that there will be a delay in the user hearing the prompt. branches/11/apps/app_confbridge.c https://reviewboard.asterisk.org/r/4477/#comment25316 The way that this function is written will cause an interrupting DTMF to repeatedly interrupt a series of playback files: play_meno_play_mepick_me A DTMF interrupt during play_me will push the digit back and then start playing no_play_me which will interrupt and push the digit back a second time to start playing pick_me which will interrupt and push the digit back a third time. branches/11/apps/app_confbridge.c https://reviewboard.asterisk.org/r/4477/#comment25315 The warning message is already generated by play_file(). branches/11/apps/app_confbridge.c https://reviewboard.asterisk.org/r/4477/#comment25317 Similar situation here as with action_toggle_mute(). bridge_channel is not really used as conference_bridge_user-chan could be used instead. branches/11/apps/app_confbridge.c https://reviewboard.asterisk.org/r/4477/#comment25318 Similar situation here as with action_toggle_mute(). - rmudgett On March 17, 2015, 1 p.m., Kevin Harwell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4477/ --- (Updated March 17, 2015, 1 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24864 https://issues.asterisk.org/jira/browse/ASTERISK-24864 Repository: Asterisk Description --- Attempting to execute DTMF in a confbridge while file playback (prompt, announcement, etc) is occurring is not allowed. You have to wait until the sound file has completed before entering DTMF. This patch fixes it so that app_confbridge now monitors for dtmf key presses during file playback. If a key is pressed playback stops and it executes the matched menu option. Diffs - branches/11/apps/confbridge/include/confbridge.h 432991 branches/11/apps/confbridge/conf_state_multi_marked.c 432991 branches/11/apps/app_confbridge.c 432991 Diff: https://reviewboard.asterisk.org/r/4477/diff/ Testing --- Manual
Re: [asterisk-dev] [Code Review] 4477: app_confbridge (11): file playback blocks dtmf
On March 18, 2015, 12:39 p.m., rmudgett wrote: branches/11/apps/app_confbridge.c, line 676 https://reviewboard.asterisk.org/r/4477/diff/2/?file=72633#file72633line676 Please get out of the habit of assigning in if tests: digit = ast_stream_and_wait(...) if (digit 0) As an aside: it is not against our coding guidelines to do this. Yes, it may be preferable to not do the assignment in the conditional: * It's usually more readable * It's usually safer (although a compiler will typically catch it when you mess it up However, this is a personal stylistic point. People don't have to necessarily agree on this particular issue. - Matt --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4477/#review14733 --- On March 17, 2015, 1 p.m., Kevin Harwell wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4477/ --- (Updated March 17, 2015, 1 p.m.) Review request for Asterisk Developers. Bugs: ASTERISK-24864 https://issues.asterisk.org/jira/browse/ASTERISK-24864 Repository: Asterisk Description --- Attempting to execute DTMF in a confbridge while file playback (prompt, announcement, etc) is occurring is not allowed. You have to wait until the sound file has completed before entering DTMF. This patch fixes it so that app_confbridge now monitors for dtmf key presses during file playback. If a key is pressed playback stops and it executes the matched menu option. Diffs - branches/11/apps/confbridge/include/confbridge.h 432991 branches/11/apps/confbridge/conf_state_multi_marked.c 432991 branches/11/apps/app_confbridge.c 432991 Diff: https://reviewboard.asterisk.org/r/4477/diff/ Testing --- Manual testing done. Setup a basic conference bridge that allowed both regular and admin users to enter. Ran through various menu options to make sure the sound file playback would stop (no longer have to wait) and a new option was executed when appropriate. Also ran the app_confbridge testsuite tests to make sure they still passed. Thanks, Kevin Harwell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
Re: [asterisk-dev] [Code Review] 4477: app_confbridge (11): file playback blocks dtmf
--- This is an automatically generated e-mail. To reply, visit: https://reviewboard.asterisk.org/r/4477/ --- (Updated March 17, 2015, 1 p.m.) Review request for Asterisk Developers. Changes --- Addressed some of the review feedback when appropriate, but mainly the fix involves a new approach. Instead of calling back into the menu select code this instead re-queues the pressed dtmf that then gets read out of the queue again and processed accordingly. Summary (updated) - app_confbridge (11): file playback blocks dtmf Bugs: ASTERISK-24864 https://issues.asterisk.org/jira/browse/ASTERISK-24864 Repository: Asterisk Description --- Attempting to execute DTMF in a confbridge while file playback (prompt, announcement, etc) is occurring is not allowed. You have to wait until the sound file has completed before entering DTMF. This patch fixes it so that app_confbridge now monitors for dtmf key presses during file playback. If a key is pressed playback stops and it executes the matched menu option. Diffs (updated) - branches/11/apps/confbridge/include/confbridge.h 432991 branches/11/apps/confbridge/conf_state_multi_marked.c 432991 branches/11/apps/app_confbridge.c 432991 Diff: https://reviewboard.asterisk.org/r/4477/diff/ Testing --- Manual testing done. Setup a basic conference bridge that allowed both regular and admin users to enter. Ran through various menu options to make sure the sound file playback would stop (no longer have to wait) and a new option was executed when appropriate. Also ran the app_confbridge testsuite tests to make sure they still passed. Thanks, Kevin Harwell -- _ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
