Summary:	ASTERISK-16227: [patch] app_recordkey - new record application that returns key pressed
Reporter:	Cal Woodruff (cal)	Labels:
Date Opened:	2010-06-08 12:27:34	Date Closed:	2017-12-14 12:01:31.000-0600
Priority:	Major	Regression?	No
Status:	Closed/Complete	Components:	Applications/NewFeature
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) app_recordkeys-1.4.c ( 1) app_recordkeys-1.6.c ( 2) app_recordkeys-tested.zip
Description:	I tweaked the app_record application such that it returns a key. Here is an example of how I am using it: ; RecordKey stops on any digit rather than just #. It returns the digit as in ${EXTEN} exten => s,2,RecordKey(${newmsg}.${rectype},${silence},${msg_length}) exten => _X,1,Set(seccode=${EXTEN}) exten => _X,2,Read(secrest,,3) exten => _X,3,Set(seccode=${seccode}${secrest}) exten => _X,4,AGI(ll-login.pl,${seccode}) exten => _X,5,Playback(ll-en-invalid) exten => _X,6,Goto(lifeline,s,2) exten => _X,100,Goto(geninstr,s,1) The reason for this application is to make it easier for our voicemail clients to enter their login information. The behavior matches that of our old dialogic equipment. ****** ADDITIONAL INFORMATION ****** I have a few questions: Will this cause any performance problems? What is the best way to grab more than one digit?
Comments:	By: Cal Woodruff (cal) 2010-06-08 12:30:22 I was not able to commit to svn yet so I have uploaded the application file. It is mostly exactly the same as app_record.c. By: Leif Madsen (lmadsen) 2010-06-09 10:46:35 Waiting on license acknowledgement. However, I do not like the way this works as it is not intuitive. I would prefer that this store the digits pressed like in the Read() application. In fact it should probably would very much like the Read() application, except that it records the file rather than playing it back. I can see it working more like this: exten => start,1,RecordKeys(variable_name,file.extension,silence,msg_length,...) By: Cal Woodruff (cal) 2010-06-09 15:20:50 I agree with your point about usability. I'll have a look at Read and try and merge the functionality. By: Cal Woodruff (cal) 2010-06-09 19:22:26 I have uploaded a preliminary version of app_recordkeys. It would be used this way: ; record and get a 4 digit extension exten => s,1,RecordKeys(4,${file}.${extension},${timeout},${duration}) exten => _XXXX,1,Macro(dosomething) I have a newer version of the c file that accepts termination keys (ie you can input variables shorter than the requested length). The above version will only stop on timeout or the requested number of digits. It wouldn't be difficult to extend this to fill arbitrary variables as lmadsen described above. By: Paul Belanger (pabelanger) 2010-06-09 22:02:28 Any new features would need to be applied against trunk, not 1.4.  Why a new application?  Could this not be added to app_record, since you have 'tweaked' it? By: Cal Woodruff (cal) 2010-06-09 23:14:46 For RecordKeys the arguments aren't the same as it requires the number of digits to work properly as well as the other required arguments. For RecordKey it could be a drop in replacement for Record but the behavior seemed different enough that it might cause problems for dial plans that require # or * to be pressed and discarded rather than saved. Other than that I was being careful as this is the first change I've made. I can add this to trunk. I'll have a look at the differences if any between 1.4's record and 1.6's record before I try anything. I'm currently working on a system with 1.4 on centos 5.4. By: Cal Woodruff (cal) 2010-06-10 12:44:30 I have uploaded more recent sanity checked revisions of these applications for asterisk 1.4 and 1.6 (trunk). I have not tested all options yet. By: Cal Woodruff (cal) 2010-06-13 18:57:18 I've uploaded a final revision that can work like Record with no key capture (it will stop recording if any key is pressed but won't save the keys to ${EXTEN}). There are versions for asterisk 1.4 and 1.6 I have built and done sanity checks on both versions of these RecordKeys apps. The other uploads can be discarded (I am not able to remove them). By: Cal Woodruff (cal) 2010-06-13 22:51:52 app_recordkeys-tested.zip: I did testing of all new features and fixed an error that caused a segfault when numkeys was left blank. Other files can be discarded. By: Paul Belanger (pabelanger) 2010-06-14 07:31:53 Removed previous version. There is no need to post a version for 1.4, since it will not get merged it that branch, simply post the trunk version. I would also make a post to asterisk-dev list and / or IRC to get some feedback about this application. I still think this application could be added into app_record.c, not replacing the current functionally but create a 2nd function. IE: Dial and RetryDial, 2 apps both reside in app_dial.c By: Cal Woodruff (cal) 2010-06-14 12:51:15 The current version of RecordKeys will more or less work as a replacement for Record as has been suggested. I put the numkeys argument at the end and made it optional. The behavior is somewhat different from Record in that it ends on any key press and does not automatically fail and delete the recorded file if the line is hung up. If you want it to delete the file on hang up use the "d" option. I'm using a different application name simply to cover myself if these changes don't get incorporated into the main code base as I am using this application in production. It should be tested more thoroughly but I have tested my changes to the options etc. By: Paul Belanger (pabelanger) 2010-06-14 13:47:40 I would suggest moving this into http://reviewboard.digium.com/ for more feedback.  I suspect you will run into resistance replacing 'Record'. Either way, thanks for the patch. :) By: Leif Madsen (lmadsen) 2010-06-14 14:39:51 I'm not entirely sure why this needs to be outside of Record() at all. Can't the options just be tacked onto the end as optional arguments? Record(${filename}.${ext}[,timeout[,duration[,num_of_keys]]]) etc..? By: Leif Madsen (lmadsen) 2010-06-14 14:40:19 Additionally, you need to upload your files as text files, not compressed archives. By: Cal Woodruff (cal) 2010-06-14 19:09:14 Thanks for removing all those files. I've uploaded versions as text files for 1.4 and 1.6. I won't argue one way or the other about making RecordKeys a separate application or not. Not deleting files by default is important and an important difference. Here's a synopsis of how it can be used: ;put no keys into ${EXTEN} but stop on any key exten => s,1,RecordKeys(${file},${sil},${duration}); ;put exactly 4 keys into ${EXTEN} exten => s,1,RecordKeys(${file},${sil},${duration},,4); ;put up to 8 keys into ${EXTEN} optionally stop early with # exten => s,1,RecordKeys(${file},${sil},${duration},p,8); ;put up to 8 keys into ${EXTEN} optionally stop early with * exten => s,1,RecordKeys(${file},${sil},${duration},t,8); ;optionally delete the file on hang up exten => s,1,RecordKeys(${file},${sil},${duration},d,4); Note there is no x option but I can put that back in. I am in the process of modernizing a legacy dialogic voicemail application. The Record application's behavior was an issue because it was too different from how Dialogic's api works and was likely to confuse some of our clients so that was the motivation for making a version of Record that handles key input differently. I appreciate the comments I have received its certainly a much better application as a result. By: Leif Madsen (lmadsen) 2011-01-05 15:15:47.000-0600 Marked as Confirmed. Probably needs to be reviewed and completed. By: Leif Madsen (lmadsen) 2011-07-27 13:59:01.542-0500 Does the reporter have interest in moving this forward? Probably need to get it reviewed and tested, then we can push this potentially into Asterisk 10 or Asterisk 11. By: Cal Woodruff (cal) 2011-07-27 15:18:55.513-0500 Yes, if we could find some way to incorporate this functionality that would be great. I've been using the 1.4 and 1.6 patch in the field and it all seems to work. By: Leif Madsen (lmadsen) 2011-08-11 13:19:18.173-0500 Well I certainly think we'll need to do the following: 1) port to trunk and make sure it works there as it's a new feature, and I feel as if it might not make it for Asterisk 10 2) once ported and working, make sure any issues brought up here that were pointed out for how the application/function should work are resolved 3) post to reviewboard for review (and ideally testing) by the community. Once we get a ship it, then we can look at moving this towards merge. By: Leif Madsen (lmadsen) 2011-09-14 08:53:30.569-0500 Suspended due to lack of activity. Please request a bug marshal in #asterisk-bugs on the IRC network irc.freenode.net to reopen the issue should you have the additional information requested.  Further information can be found at http://www.asterisk.org/developers/bug-guidelines By: Cal Woodruff (cal) 2011-09-18 17:52:04.332-0500 I can work on this. I have been very busy with school for the last month. By: Matt Jordan (mjordan) 2015-03-14 21:01:37.478-0500 I don't think anyone ever saw the comment here - sorry :-( If you're still interested in having this included, I'll post the links that will help you get this patch up onto review board. By: Matt Jordan (mjordan) 2015-03-14 21:02:00.376-0500 Thanks for the contribution! If you'd like your contribution to be included faster, you should submit your patch for code review by the Asterisk Developer Community. To do so, please follow the Code Review [1] instructions on the wiki. Be sure to: * Verify that your patch conforms to the Coding Guidelines [2] * Review the Code Review Checklist [3] for common items reviewers will look for * If necessary, provide tests for the Asterisk Test Suite that verify the correctness of your patch [4] * As this is a new feature, please read the New Feature Guidelines [5] * Make sure your new feature applies cleanly to Asterisk trunk When ready, submit your patch and any tests to Review Board [6] for code review. Thanks! [1] https://wiki.asterisk.org/wiki/display/AST/Code+Review [2] https://wiki.asterisk.org/wiki/display/AST/Coding+Guidelines [3] https://wiki.asterisk.org/wiki/display/AST/Code+Review+Checklist [4] https://wiki.asterisk.org/wiki/display/AST/Asterisk+Test+Suite+Documentation [5] https://wiki.asterisk.org/wiki/display/AST/New+Feature+Guidelines [6] https://wiki.asterisk.org/wiki/display/AST/Review+Board+Usage By: Corey Farrell (coreyfarrell) 2017-12-14 12:01:31.767-0600 Suspending due to lack of activity.  The code was never posted for review, unless someone takes that initiative this patch is being considered abandoned. As others have pointed out I think this new feature could be accomplished by adding options to the existing Record app.  I'm -1 on duplicating the entire Record app.