Summary:	ASTERISK-11213: [patch] multiple bugs in Directory application
Reporter:	Dmitry Andrianov (dimas)	Labels:
Date Opened:	2008-01-11 14:06:47.000-0600	Date Closed:	2008-01-14 16:11:21.000-0600
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Applications/app_directory
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) dir.patch ( 1) dir2.patch ( 2) dir3.patch
Description:	app_directory contained some duplicate code even before addition of 'm' option. Addition of that option doubled amount of that code. Worst of all, there are minor differences between these code block and bugs caused by these differences. 1. There is a memory leak. In the 'menu' mode, result of the convert(pos) function is not freed while it should be. 2. In the 'menu' mode check for OPT_LISTBYFIRSTNAME flag ('f' option) is not negated as result, application works in the mode opposite to what user expect (checking last name when user wants the first nd vice versa). 3. select_item function plays message for user using res = func1() || func2() || func3()... construct. This construct loses the actual value returned by ast_waitstream() for example so at the end, res does not contain digit user dialed while listening to the message. 4. (also in 1.4) application announces entries from voicemail.conf/realtime separately from entries from users.conf. I see no reason why doing so instead of building combined list. 5. Alot of duplicated code as already mentioned. Attached patch does major refactoring of app_directory and I hope it fixes all the issues above. ****** ADDITIONAL INFORMATION ****** The patch split the huge piece of code into smaller functions. Now app_directory reads list of candidate extensions (from both voicemail.conf/realtime and users.conf). Then the list is sorted to guarantee persistent ordering and then finally, the list is presented to user in two different modes (depending on 'm' option). I believe the patch makes code easier to understand and maintain because it is more "linear" and less nested now.
Comments:	By: Mark Michelson (mmichelson) 2008-01-11 16:45:29.000-0600 This patch is awesome! do_directory was a big ugly function and definitely needed an overhaul. Great work! ... ... Unfortunately, this doesn't work. I tried calling the directory and pressing '1' to dial someone, but instead, the directory restarted (i.e. 'dir-intro' played and I had to enter the last three letters of the last name again). It was not possible to actually call anyone. I'll retest after you sort out this error. As a secondary, minor issue I think the sort could be improved for the situation where you search by last name. Instead of just using the last name as the key, I would suggest using the last name and appending the first name on as well so that people with the same last name will be listed in proper alphabetical order. By: Dmitry Andrianov (dimas) 2008-01-12 08:29:52.000-0600 Oops. I thought I tested everything. Sorry. New patch attached. Regarding the sort improvement - I'm not sure why you suggesting it because even the first patch was doing exactly what you want - the key is lastname+fullname and since fullname starts with the first name, it effectively enforces sorting by last name and then by first name. (when 'f' option i specified key = fullname) By: Dmitry Andrianov (dimas) 2008-01-13 05:59:03.000-0600 Updated the patch - fixed compiler warning. (This also fixes couple of bugs) By: Mark Michelson (mmichelson) 2008-01-14 14:52:02.000-0600 Ah, you're right about the sorting. I didn't notice the part where you added the first name onto the end if searching by last name. Sorry about that. I retested using dir3.patch and everything appears to be working as expected right now. I also ran with valgrind in case there might have been any uncaught memory errors, and there don't appear to be any. I plan on committing this pretty much as is. There are two places where you use // style comments instead of /*...*/,  so I'm going to change that. This is an excellent patch, and I thank you very much for taking the time to make it and correct the problems encountered. By: Digium Subversion (svnbot) 2008-01-14 16:10:20.000-0600 Repository: asterisk Revision: 98888 U   trunk/apps/app_directory.c ------------------------------------------------------------------------ r98888 | mmichelson | 2008-01-14 16:10:19 -0600 (Mon, 14 Jan 2008) | 24 lines Big improvement for app_directory. This patch breaks the do_directory function up so that it is more easily parsed by the human brain. It also fixes some errors. I'll quote dimas from the original bug description: "app_directory contained some duplicate code even before addition of 'm' option. Addition of that option doubled amount of that code. Worst of all, there are minor differences between these code block and bugs caused by these differences. 1. There is a memory leak. In the 'menu' mode, result of the convert(pos) function is not freed while it should be. 2. In the 'menu' mode check for OPT_LISTBYFIRSTNAME flag ('f' option) is not negated as result, application works in the mode opposite to what user expect (checking last name when user wants the first nd vice versa). 3. select_item function plays message for user using res = func1() || func2() || func3()... construct. This construct loses the actual value returned by ast_waitstream() for example so at the end, res does not contain digit user dialed while listening to the message. 4. (also in 1.4) application announces entries from voicemail.conf/realtime separately from entries from users.conf. I see no reason why doing so instead of building combined list. 5. Alot of duplicated code as already mentioned." This was tested by dimas and I (I tested under valgrind). A word of caution: any bug fixes that happen in app_directory in 1.4 will almost certainly not merge cleanly into trunk as a result of this, but it is well worth it. Huge thanks to dimas for this wonderful submission. (closes issue ASTERISK-11213) Reported by: dimas Patches:      dir3.patch uploaded by dimas (license 88)  Tested by: putnopvut, dimas ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=98888