til_settings: rework setting get/add for bare values

The core thing here is rather than turning a bare value into a
key as I was doing before - we just leave the bare value as a
bare value and its setting must be located positionally via
get_value_by_idx since there's no key.

Existing callers that used to get_key() positionally now
get_value_by_idx() positionally all the same, except it's the
value instead of the key.  This is mostly done for things like
the module or fb name at the front of a settings instance.

The impetus for this change is partially just
cosmetic/ergonomics, but it's also rather strange for what's
really a key-less value to be treated as a value-less key.  It
was also awkward to talk/reason about on the road to recursive
settings where bare values would be supported as a standalone
settings instance if properly escaped...

This also adds unescaping of keys, and adds a dependency on the
somewhat linux-specific open_memstream() which may need changing
in the future (see comments).

6 files changed, 80 insertions, 60 deletions

diff --git a/src/main.c b/src/main.c
index efc9d12..5165d71 100644
--- a/src/main.c
+++ b/src/main.c
@@ -83,7 +83,7 @@ static int setup_video(const til_settings_t *settings, til_setting_t **res_setti
 	til_setting_t	*setting;
 	const char	*video;
 
-	video = til_settings_get_key(settings, 0, &setting);
+	video = til_settings_get_value_by_idx(settings, 0, &setting);
 	if (!video || !setting->desc) {
 		til_setting_desc_t	*desc;
 		const char		*values[] = {
@@ -377,8 +377,8 @@ int main(int argc, const char *argv[])
 	exit_if(r && print_setup_as_args(&setup, !rototiller.args.gogogo) < 0,
 		"unable to print setup");
 
-	exit_if(!(rototiller.module = til_lookup_module(til_settings_get_key(setup.module_settings, 0, NULL))),
-		"unable to lookup module from settings \"%s\"", til_settings_get_key(setup.module_settings, 0, NULL));
+	exit_if(!(rototiller.module = til_lookup_module(til_settings_get_value_by_idx(setup.module_settings, 0, NULL))),
+		"unable to lookup module from settings \"%s\"", til_settings_get_value_by_idx(setup.module_settings, 0, NULL));
 
 	exit_if((r = til_fb_new(fb_ops, setup.video_setup, NUM_FB_PAGES, &rototiller.fb)) < 0,
 		"unable to create fb: %s", strerror(-r));
diff --git a/src/sdl_fb.c b/src/sdl_fb.c
index 469fcb1..9eabbaf 100644
--- a/src/sdl_fb.c
+++ b/src/sdl_fb.c
@@ -73,7 +73,7 @@ static int sdl_fb_setup(const til_settings_t *settings, til_setting_t **res_sett
 							res_desc);
 		if (r)
 			return r;
-	} else if ((size = til_settings_get_value(settings, "size", res_setting)) && !(*res_setting)->desc) {
+	} else if ((size = til_settings_get_value_by_key(settings, "size", res_setting)) && !(*res_setting)->desc) {
 		/* if fullscreen=on AND size=WxH is specified, we'll do a more legacy style SDL fullscreen
 		 * where it tries to change the video mode.  But if size is unspecified, it'll be a desktop
 		 * style fullscreen where it just uses a fullscreen window in the existing video mode, and
diff --git a/src/setup.c b/src/setup.c
index f85ac10..7c13ea0 100644
--- a/src/setup.c
+++ b/src/setup.c
@@ -6,21 +6,6 @@
 #include "til_setup.h"
 #include "til_util.h"
 
-/* add key=value, but if key is NULL, use value as key. */
-static int add_value(til_settings_t *settings, const char *key, const char *value)
-{
-	assert(settings);
-
-	if (!key) {
-		key = value;
-		value = NULL;
-	}
-
-	assert(key);
-
-	return til_settings_add_value(settings, key, value, NULL);
-}
-
 
 /* returns negative on error, otherwise number of additions made to settings */
 int setup_interactively(til_settings_t *settings, int (*setup_func)(const til_settings_t *settings, til_setting_t **res_setting, const til_setting_desc_t **res_desc, til_setup_t **res_setup), int defaults, til_setup_t **res_setup, const til_setting_desc_t **res_failed_desc)
@@ -115,7 +100,7 @@ int setup_interactively(til_settings_t *settings, int (*setup_func)(const til_se
 
 		if (*buf == '\n') {
 			/* accept preferred */
-			add_value(settings, desc->key, desc->preferred);
+			til_settings_add_value(settings, desc->key, desc->preferred, NULL);
 		} else {
 			buf[strlen(buf) - 1] = '\0';
 
@@ -131,7 +116,7 @@ int setup_interactively(til_settings_t *settings, int (*setup_func)(const til_se
 
 				for (found = i = 0; desc->values[i]; i++) {
 					if (i == j) {
-						add_value(settings, desc->key, desc->values[i]);
+						til_settings_add_value(settings, desc->key, desc->values[i], NULL);
 						found = 1;
 						break;
 					}
@@ -146,7 +131,7 @@ int setup_interactively(til_settings_t *settings, int (*setup_func)(const til_se
 
 			} else {
 				/* use typed input as setting, TODO: apply regex */
-				add_value(settings, desc->key, buf);
+				til_settings_add_value(settings, desc->key, buf, NULL);
 			}
 		}
 _next:
diff --git a/src/til.c b/src/til.c
index 9053c2d..6c40207 100644
--- a/src/til.c
+++ b/src/til.c
@@ -270,7 +270,7 @@ int til_module_setup(const til_settings_t *settings, til_setting_t **res_setting
 	const til_module_t	*module;
 	const char		*name;
 
-	name = til_settings_get_key(settings, 0, &setting);
+	name = til_settings_get_value_by_idx(settings, 0, &setting);
 	if (!name) {
 		const char		*values[nelems(modules) + 1] = {};
 		const char		*annotations[nelems(modules) + 1] = {};
diff --git a/src/til_settings.c b/src/til_settings.c
index b4da4fd..84feed4 100644
--- a/src/til_settings.c
+++ b/src/til_settings.c
@@ -38,6 +38,7 @@ typedef struct til_settings_t {
 
 typedef enum til_settings_fsm_state_t {
 	TIL_SETTINGS_FSM_STATE_KEY,
+	TIL_SETTINGS_FSM_STATE_KEY_ESCAPED,
 	TIL_SETTINGS_FSM_STATE_EQUAL,
 	TIL_SETTINGS_FSM_STATE_VALUE,
 	TIL_SETTINGS_FSM_STATE_VALUE_ESCAPED,
@@ -45,27 +46,39 @@ typedef enum til_settings_fsm_state_t {
 } til_settings_fsm_state_t;
 
 
-static int add_value(til_settings_t *settings, const char *key, const char *value, const til_setting_desc_t *desc)
+static til_setting_t * add_setting(til_settings_t *settings, const char *key, const char *value, const til_setting_desc_t *desc)
 {
+	til_setting_t	**new_settings;
+	til_setting_t	*s;
+
 	assert(settings);
 
+	s = calloc(1, sizeof(til_setting_t));
+	if (!s)
+		return NULL;
+
+	new_settings = realloc(settings->settings, (settings->num + 1) * sizeof(til_setting_t *));
+	if (!new_settings) {
+		free(s);
+		return NULL;
+	}
+
+	settings->settings = new_settings;
+	settings->settings[settings->num] = s;
+	settings->settings[settings->num]->key = key;
+	settings->settings[settings->num]->value = value;
+	settings->settings[settings->num]->desc = desc;
 	settings->num++;
-	/* TODO errors */
-	settings->settings = realloc(settings->settings, settings->num * sizeof(til_setting_t *));
-	settings->settings[settings->num - 1] = calloc(1, sizeof(til_setting_t));
-	settings->settings[settings->num - 1]->key = key;
-	settings->settings[settings->num - 1]->value = value;
-	settings->settings[settings->num - 1]->desc = desc;
 
-	return 0;
+	return s;
 }
 
 
 /* split settings_string into a data structure */
 til_settings_t * til_settings_new(const char *label, const char *settings_string)
 {
-	til_settings_fsm_state_t	state = TIL_SETTINGS_FSM_STATE_KEY;
-	const char			*p, *token;
+	til_settings_fsm_state_t	state = TIL_SETTINGS_FSM_STATE_COMMA;
+	const char			*p;
 	til_settings_t			*settings;
 	FILE				*value_fp;
 	char				*value_buf;
@@ -84,31 +97,44 @@ til_settings_t * til_settings_new(const char *label, const char *settings_string
 	if (!settings_string)
 		return settings;
 
-	for (token = p = settings_string; ;p++) {
+	for (p = settings_string;;p++) {
 
 		switch (state) {
 		case TIL_SETTINGS_FSM_STATE_COMMA:
-			token = p;
+			value_fp = open_memstream(&value_buf, &value_sz); /* TODO FIXME: open_memstream() isn't portable */
+			if (!value_fp)
+				goto _err;
+
 			state = TIL_SETTINGS_FSM_STATE_KEY;
-			break;
+			/* fallthrough */
 
 		case TIL_SETTINGS_FSM_STATE_KEY:
-			if (*p == '=' || *p == ',' || *p == '\0') {
-				add_value(settings, strndup(token, p - token), NULL, NULL);
+			if (*p == '\\')
+				state = TIL_SETTINGS_FSM_STATE_KEY_ESCAPED;
+			else if (*p == '=' || *p == ',' || *p == '\0') {
+				fclose(value_fp);
 
-				if (*p == '=')
+				if (*p == '=') { /* key= */
+					(void) add_setting(settings, value_buf, NULL, NULL);
 					state = TIL_SETTINGS_FSM_STATE_EQUAL;
-				else if (*p == ',')
+				} else { /* bare value */
+					(void) add_setting(settings, NULL, value_buf, NULL);
 					state = TIL_SETTINGS_FSM_STATE_COMMA;
-			}
+				}
+			} else
+				fputc(*p, value_fp);
+			break;
+
+		case TIL_SETTINGS_FSM_STATE_KEY_ESCAPED:
+			fputc(*p, value_fp);
+			state = TIL_SETTINGS_FSM_STATE_KEY;
 			break;
 
 		case TIL_SETTINGS_FSM_STATE_EQUAL:
-			value_fp = open_memstream(&value_buf, &value_sz);
+			value_fp = open_memstream(&value_buf, &value_sz); /* TODO FIXME: open_memstream() isn't portable */
 			if (!value_fp)
 				goto _err;
 
-			token = p;
 			state = TIL_SETTINGS_FSM_STATE_VALUE;
 			/* fallthrough, necessary to not leave NULL values for empty "key=\0" settings */
 
@@ -125,6 +151,13 @@ til_settings_t * til_settings_new(const char *label, const char *settings_string
 			break;
 
 		case TIL_SETTINGS_FSM_STATE_VALUE_ESCAPED:
+			/* TODO: note currently we just pass-through literally whatever char was escaped,
+			 * but in cases like \n it should really be turned into 0xa so we can actually have
+			 * a setting contain a raw newline post-unescaping (imagine a marquee module supporting
+			 * arbitray text to be drawn, newlines would be ok yeah? should it be responsible for
+			 * unescaping "\n" itself or just look for '\n' (0xa) to insert linefeeds?  I think
+			 * the latter...)  But until there's a real need for that, this can just stay as-is.
+			 */
 			fputc(*p, value_fp);
 			state = TIL_SETTINGS_FSM_STATE_VALUE;
 			break;
@@ -166,13 +199,16 @@ til_settings_t * til_settings_free(til_settings_t *settings)
 }
 
 
-/* find key= in settings, return dup of value side or NULL if missing */
-const char * til_settings_get_value(const til_settings_t *settings, const char *key, til_setting_t **res_setting)
+/* find key= in settings, return value NULL if missing, optionally store setting @res_setting if found */
+const char * til_settings_get_value_by_key(const til_settings_t *settings, const char *key, til_setting_t **res_setting)
 {
 	assert(settings);
 	assert(key);
 
 	for (int i = 0; i < settings->num; i++) {
+		if (!settings->settings[i]->key)
+			continue;
+
 		if (!strcasecmp(key, settings->settings[i]->key)) {
 			if (res_setting)
 				*res_setting = settings->settings[i];
@@ -185,16 +221,16 @@ const char * til_settings_get_value(const til_settings_t *settings, const char *
 }
 
 
-/* return positional key from settings */
-const char * til_settings_get_key(const til_settings_t *settings, unsigned pos, til_setting_t **res_setting)
+/* return positional value from settings, NULL if missing, optionally store setting @res_setting if found */
+const char * til_settings_get_value_by_idx(const til_settings_t *settings, unsigned idx, til_setting_t **res_setting)
 {
 	assert(settings);
 
-	if (pos < settings->num) {
+	if (idx < settings->num) {
 		if (res_setting)
-			*res_setting = settings->settings[pos];
+			*res_setting = settings->settings[idx];
 
-		return settings->settings[pos]->key;
+		return settings->settings[idx]->value;
 	}
 
 	return NULL;
@@ -216,7 +252,7 @@ int til_settings_get_and_describe_value(const til_settings_t *settings, const ti
 	assert(desc);
 	assert(res_value);
 
-	value = til_settings_get_value(settings, desc->key, &setting);
+	value = til_settings_get_value_by_key(settings, desc->key, &setting);
 	if (!value || !setting->desc) {
 		int	r;
 
@@ -243,17 +279,16 @@ int til_settings_get_and_describe_value(const til_settings_t *settings, const ti
 }
 
 
-/* add key=value to the settings,
- * or just key if value is NULL.
+/* add key,value as a new setting to settings,
+ * NULL keys and/or values are passed through as-is
  * desc may be NULL, it's simply passed along as a passenger.
  */
-/* returns < 0 on error */
-int til_settings_add_value(til_settings_t *settings, const char *key, const char *value, const til_setting_desc_t *desc)
+/* returns the added setting, or NULL on error (ENOMEM) */
+til_setting_t * til_settings_add_value(til_settings_t *settings, const char *key, const char *value, const til_setting_desc_t *desc)
 {
 	assert(settings);
-	assert(key);
 
-	return add_value(settings, strdup(key), value ? strdup(value) : NULL, desc);
+	return add_setting(settings, key ? strdup(key) : NULL, value ? strdup(value) : NULL, desc);
 }
 
 
diff --git a/src/til_settings.h b/src/til_settings.h
index 8911e6c..c9da235 100644
--- a/src/til_settings.h
+++ b/src/til_settings.h
@@ -37,9 +37,9 @@ struct til_setting_t {
 
 til_settings_t * til_settings_new(const char *label, const char *settings);
 til_settings_t * til_settings_free(til_settings_t *settings);
-const char * til_settings_get_value(const til_settings_t *settings, const char *key, til_setting_t **res_setting);
-const char * til_settings_get_key(const til_settings_t *settings, unsigned pos, til_setting_t **res_setting);
-int til_settings_add_value(til_settings_t *settings, const char *key, const char *value, const til_setting_desc_t *desc);
+const char * til_settings_get_value_by_key(const til_settings_t *settings, const char *key, til_setting_t **res_setting);
+const char * til_settings_get_value_by_idx(const til_settings_t *settings, unsigned idx, til_setting_t **res_setting);
+til_setting_t * til_settings_add_value(til_settings_t *settings, const char *key, const char *value, const til_setting_desc_t *desc);
 void til_settings_reset_descs(til_settings_t *settings);
 int til_settings_get_and_describe_value(const til_settings_t *settings, const til_setting_desc_t *desc, const char **res_value, til_setting_t **res_setting, const til_setting_desc_t **res_desc);
 char * til_settings_as_arg(const til_settings_t *settings);