[Asterisk-code-review] pbx: Add support for autohints. (asterisk[master])

[Richard Mudgett]

Richard Mudgett has posted comments on this change.

Change subject: pbx: Add support for autohints.
......................................................................


Patch Set 3: Code-Review-1

(5 comments)

https://gerrit.asterisk.org/#/c/2499/3/CHANGES
File CHANGES:

Line 164:    device.
s/device/the device/


https://gerrit.asterisk.org/#/c/2499/3/main/pbx.c
File main/pbx.c:

Line 2394: struct fake_context /* this struct is purely for matching in the hashtab */
Considering I heard you scream all the way from Canada, a note here and at ast_context that they need to be in sync or crashyness is invoked would seem appropriate.

Though a better fix (in another patch) would be to actually fix the issue by defining fake_context as a structure containing ast_context.

struct fake_context {
  struct ast_context con;
  char buf[256];
}


PS3, Line 6346: 		/* Create any autohint contexts */
              : 		iter = ast_hashtab_start_traversal(exttable);
              : 		while ((tmp = ast_hashtab_next(iter))) {
              : 			if (!tmp->autohints) {
              : 				continue;
              : 			}
              : 			context_create_autohint_placeholder(tmp);
              : 		}
              : 		ast_hashtab_end_traversal(iter);
package into a routine.  See below


PS3, Line 6478: 	/* Remove all autohints as the below iteration will recreate them */
              : 	ao2_callback(autohints, OBJ_UNLINK | OBJ_NODATA | OBJ_MULTIPLE, NULL, NULL);
              : 
              : 	/* Create all applicable autohint contexts */
              : 	iter = ast_hashtab_start_traversal(contexts_table);
              : 	while ((tmp = ast_hashtab_next(iter))) {
              : 		if (!tmp->autohints) {
              : 			continue;
              : 		}
              : 		context_create_autohint_placeholder(tmp);
              : 	}
              : 	ast_hashtab_end_traversal(iter);
Looks like this should be packaged into its own function and you then pass in either contexts_table or exttable depending upon which of the two locations to call it from.


https://gerrit.asterisk.org/#/c/2499/3/pbx/pbx_config.c
File pbx/pbx_config.c:

PS3, Line 1904: 				if (ast_true(v->value)) {
              : 					ast_context_enable_autohints(con);
              : 				}
This only allows autohints to be enabled.  So the first time autohints is enabled in the context, they are enabled without the ability to change your mind with a later autohints setting.

autohints=no
autohints=yes
autohints=no ; Has no effect because it was turned on earlier.

This could be allowed by changing ast_context_enable_autohints() to ast_context_set_autohints(con, enable)


-- 
To view, visit https://gerrit.asterisk.org/2499
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I7e444c7da41b7b7d33374420fec658beeb18584e
Gerrit-PatchSet: 3
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-Reviewer: Matthew Fredrickson <creslin at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes