Jason Parker's review
--------------------------------------

trunk/Makefile (Diff revision 1)

135 	

  ASTETCDIR=$(sysconfdir)/asterisk

135 	

  ASTETCDIR=/usr/local/etc/asterisk

    bad, bad, bad
    JAROTH - FIXED.  Not sure how that got there!

trunk/apps/app_voicemail.c (Diff revision 1)

243 	

	OPT_DTMFEXIT =         (1 << 7),

243 	

	OPT_MESSAGE_URGENT =   (1 << 7),

    Seems a little silly to renumber these for no apparent reason.
    JAROTH - I was using that number before there was a DTMFEXIT, so it was
             already in the code that way.  I changed the URGENT value as 
             suggested.

trunk/apps/app_voicemail.c (Diff revision 1)

1591 	

	/* Find all .txt files - even if they are not in sequence from 0000 */

1603 	

	/* Find all .txt files - even if they are not in sequence from, int urgent 0000 */

       JAROTH - Fixed.

trunk/apps/app_voicemail.c (Diff revision 1)

1605 	

	int vmcount = 0, is_urgent;

    No need for this variable.
    JAROTH - Fixed

trunk/apps/app_voicemail.c (Diff revision 1)

3087 	

			snprintf(arguments, sizeof(arguments), "%s %s %s %d %s&", externnotify, context, extension, newvoicemails, flag ? flag : "");

    S_OR(flag, "")
    JAROTH - Fixed.

trunk/apps/app_voicemail.c (Diff revision 1)
	

3150 	

	if (ast_test_flag(options, OPT_MESSAGE_URGENT)) {
		ast_copy_string(flag, "URGENT", sizeof(flag));
	} else if (ast_test_flag(options, OPT_MESSAGE_PRIORITY)) {
		ast_copy_string(flag, "PRIORITY", sizeof(flag));
	} else {
		flag[0] = '\0';
	}
	

    flag (why is it called flag?) (why is it a char?) is used in many places, such as format strings.  All of those places would look odd if flag is 0 length.
    JAROTH - It is called flag because right now it is only for the type "URGENT", but 
             could have additional values in the future.   I store the flag's value 
             here (ie. URGENT) for use in notifications.  I've fixed the problem with the
             empty string making the formatting look odd.

trunk/apps/app_voicemail.c (Diff revision 1)

3431 	

				category ? category : "");

    S_OR
    JAROTH - Fixed.

trunk/apps/app_voicemail.c (Diff revision 1)

4839 	

	char flag[10];

    Wasn't it char [80] earlier?
    JAROTH - Fixed.

trunk/include/asterisk/app.h (Diff revision 1)

115 	

/*! \brief Determine number of new/old messages in a mailbox */

115 	

/*! Determine number of urgent/new/old messages in a mailbox */

    removes \brief?
    JAROTH - Fixed.

trunk/main/manager.c (Diff revision 1)
	
2053 	

"	UrgMessages: <count>\n"

    No reason not to spell it out
    JAROTH - Fixed.
    
trunk/apps/app_voicemail.c (Diff revision 1)
	

244 	

	OPT_MESSAGE_PRIORITY = (1 << 8)

    Is this just not implemented?
    JAROTH - It is not implemented.  This was a carry-over from a preexisting patch
             that also added a priority flag.  I don't remember what the patch was
             or who did it.

-------------------------------------------
Mark Michelson's review:
------------------------------------------------

trunk/apps/app_voicemail.c (Diff revision 1)

2045 	

		fprintf(p, "Subject: New %s message %d in mailbox %s" ENDL, flag, msgnum + 1, mailbox);

    Minor quibble. A non-urgent message will have two spaces between "New" and "Message"
    JAROTH - Fixed as noted above.

trunk/apps/app_voicemail.c (Diff revision 1)

2542 	

	if (flag != NULL && !strcmp(flag,"URGENT")) {

    Should use !ast_strlen_zero(flag) instead of checking if it   is not NULL
    JAROTH - Fixed.

trunk/apps/app_voicemail.c (Diff revision 1)
	

2890 	

static int check_if_urgent(char *dir, char *filename)

    This whole function should be replaced using the ast_config API. This seems like a
    reinvention of the wheel to me.
    JAROTH - Fixed as noted in bug tracker.

trunk/apps/app_voicemail.c (Diff revision 1)	

2892 	

	unsigned char buf[256]; 

    Why unsigned? Every time buf is used in this function it has to be cast to a char *...
    JAROTH - No idea.  Fixed.

trunk/apps/app_voicemail.c (Diff revision 1)
	

2899 	

	ast_debug(1, "About to open file %s\n", path);

    I think either this debug comment should be removed or at least be at a higher level.
    JAROTH - Fixed. Also fixed other locations with same issue.

trunk/apps/app_voicemail.c (Diff revision 1)
	

2912 	

					ast_debug(1, "** buf is set to %s\n", buf);

    Same with this debug statement as well
    JAROTH - Fixed.

trunk/apps/app_voicemail.c (Diff revision 1)

2960 	

		if (!strncasecmp(de->d_name, "msg", 3)) {
			if (shortcircuit) {
				ret = 1;
				break;
			} else if (!strncasecmp(de->d_name + 8, "txt", 3)) {
				if (is_urgent == urgent) {	/* count urgent or non-urgent as specified */
					ret++;
				}
			}
		}

    This is problematic because if shortcirtcuit is set, then you will not accurately report
    if there are urgent messages. The function will return true as long as there's a message
    in the INBOX.
    JAROTH - This one took me some time to figure out what you meant, but I believe that
             I have a fix for this as well.


trunk/apps/app_voicemail.c (Diff revision 1)

3087 	

			snprintf(arguments, sizeof(arguments), "%s %s %s %d %s&", externnotify, context, extension, newvoicemails, flag ? flag : "");

    This concerns me somewhat since I don't know if this will break whatever it is
    people use for externnotify.
    JAROTH - Updated UPGRADE.txt as noted in bug tracker note.

trunk/apps/app_voicemail.c (Diff revision 1)	

4894 	

	/* Play the word urgent if we are listening to urgent messages */
	if (flag && strncmp(flag,"URGENT",6) == 0) {
		res = wait_file2(chan, vms, "vm-Urgent");	/* "urgent" */
	}

    While it doesn't hurt to just do a string comparison of the first 6 characters, 
    it isn't necessary. Since flag was set using ast_variable_retrieve, you don't have
    to worry about the trailing newline.

    The same goes for the comparison a few lines down.
    JAROTH - Fixed.  This was a carryover from the comparison to the flag in the
             IMAP message, where it DOES matter.

trunk/apps/app_voicemail.c (Diff revision 1)

5207 	

	if (box == NEW_FOLDER && urgent == 1) {
		pgm->unseen = 1;
		pgm->seen = 0;
		pgm->flagged = 1;
		pgm->unflagged = 0;
	} else if (box == NEW_FOLDER && urgent == 0) {
		pgm->unseen = 1;
		pgm->seen = 0;
		pgm->flagged = 0;
		pgm->unflagged = 1;
	} else if (box == OLD_FOLDER) {
		pgm->seen = 1;
		pgm->unseen = 0;
	}

    This section of code makes the assumption that there would be no urgent messages
    in the "old" folder. Also, it assumes that any flagged message is an urgent message.
    I guess the second assumption is valid for now, but if any further flags are added later,
    it would require modification.
    JAROTH - In this implementation, since URGENT is just a flag, the flag can be "carried"
             to any mailbox (IMAP or file-based).  So yes, there can be old urgent messages.
             They are just treated as old messages at this time.   As for "flagged" messages,
             RFC-3501 defines \Flagged as 'Message is "flagged" for urgent/special attention'. 


trunk/apps/app_voicemail.c (Diff revision 1)
	
2718 	

		/* look for urgent messages */
		if (fold == 2) {
                	pgm->flagged = 1;
                	pgm->unflagged = 0;
		}

    This (as well as any other places which use fold == 2 as a sign to look for urgent 
    messages is incorrect. folder_int returns 2 for the "Work" folder.
    JAROTH - Understood.  The notion of folder_int was not there in the version I was
             working from.  I have added the URGENT folder type and fixed this code.