[PATCH 01/13] Keymap_LoadRemapFile() refactor / rewrite

[Eero Tamminen <oak@xxxxxxxxxxxxxx>]

Partly inspired by changes in Vincent Barrilliot's patch.

Splitting keymap host (PC) & guest (ST) value reads to separate
functions both improves function readability and prepares it for
further changes in later commits. Switching from strtok() to
strtok_r() is also needed by later commits.

Other improvements:
- Declare variables in scope closer to their use
- Do all string content checks consistently with strlen
- File missing issue warrants a warning, not just debug msg
- Add separate debug log for loading of the keymap file
- Use ARRAY_SIZE() instead of define as loop limit
- Warn if rest of mappings have to be skipped
- Use Str_Trim() on results to skip white space before parsed items
- Check/warn about PC & ST scancode errors separately (with line number)
- If there are warnings, tell about them in an error dialog too
---
 src/keymap.c | 166 ++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 111 insertions(+), 55 deletions(-)

diff --git a/src/keymap.c b/src/keymap.c
index 83d03934..a0407d81 100644
--- a/src/keymap.c
+++ b/src/keymap.c
@@ -422,92 +422,148 @@ static uint8_t Keymap_RemapKeyToSTScanCode(const SDL_Keysym* pKeySym)
 }
 
 
+/*-----------------------------------------------------------------------*/
+/**
+ * Fill host (PC) key based on host "spec" string.
+ * Return true on success
+ */
+static bool HostSpecToKeymap(const char *spec, int *scancode)
+{
+	int key;
+	if (!spec)
+		return false;
+
+	key = atoi(spec);    /* Direct key code? */
+	if (key < 10)
+	{
+		/* If it's not a valid number >= 10, then
+		 * assume we've got a symbolic key name
+		 */
+		int offset = 0;
+		/* quoted character (e.g. comment line char)? */
+		if (*spec == '\\' && strlen(spec) == 2)
+			offset = 1;
+		key = Keymap_GetKeyFromName(spec+offset);
+	}
+	if (key < 8)
+	{
+		Log_Printf(LOG_WARN, "Invalid PC key: '%s' (%d >= 8)\n",
+			   spec, key);
+		return false;
+	}
+	*scancode = key;
+	return true;
+}
+
+
+/*-----------------------------------------------------------------------*/
+/**
+ * Fill guest (ST) key based on guest "spec" string.
+ * Return true on success
+ */
+static bool GuestSpecToKeymap(const char *spec, int *scancode)
+{
+	int key;
+	if (!spec)
+		return false;
+
+	key = atoi(spec);
+	if (key <= 0 || key > KBD_MAX_SCANCODE)
+	{
+		Log_Printf(LOG_WARN, "Invalid ST scancode: '%s' (0 > %d < %d)\n",
+			   spec, key, KBD_MAX_SCANCODE);
+		return false;
+	}
+	*scancode = key;
+	return true;
+}
+
+
 /*-----------------------------------------------------------------------*/
 /**
  * Load keyboard remap file
  */
 void Keymap_LoadRemapFile(const char *pszFileName)
 {
-	char szString[1024];
-	int STScanCode, PCKeyCode;
 	FILE *in;
-	int idx = 0;
+	int idx, linenro, fails;
 
 	/* Initialize table with default values */
 	memset(LoadedKeymap, 0, sizeof(LoadedKeymap));
 
-	if (!*pszFileName)
+	if (strlen(pszFileName) == 0)
 		return;
 
-	/* Attempt to load file */
+	/* Attempt to load mapping file */
 	if (!File_Exists(pszFileName))
 	{
-		Log_Printf(LOG_DEBUG, "Keymap_LoadRemapFile: '%s' not a file\n", pszFileName);
+		Log_Printf(LOG_WARN, "Keymap_LoadRemapFile: keymap file '%s' missing\n", pszFileName);
 		return;
 	}
 	in = fopen(pszFileName, "r");
 	if (!in)
 	{
-		Log_Printf(LOG_ERROR, "Keymap_LoadRemapFile: failed to "
-			   " open keymap file '%s'\n", pszFileName);
+		Log_Printf(LOG_ERROR, "Keymap_LoadRemapFile: failed to open keymap file '%s'\n", pszFileName);
 		return;
 	}
 
-	while (!feof(in) && idx < KBD_MAX_SCANCODE)
+	Log_Printf(LOG_DEBUG, "Keymap_LoadRemapFile: Loading '%s' keymap file\n", pszFileName);
+
+	idx = linenro = fails = 0;
+	while (!feof(in))
 	{
+		char *line, *saveptr, buf[1024];
+		const char *host, *guest;
+
+		if (idx >= ARRAY_SIZE(LoadedKeymap))
+		{
+			Log_Printf(LOG_WARN, "Mappings specified already for"
+				   "supported number (%d) of keys, skipping"
+				   "rest of '%s' at line %d\n",
+				   ARRAY_SIZE(LoadedKeymap), pszFileName, linenro);
+			fails++;
+			break;
+		}
+
 		/* Read line from file */
-		if (fgets(szString, sizeof(szString), in) == NULL)
+		if (fgets(buf, sizeof(buf), in) == NULL)
 			break;
+		linenro++;
+
 		/* Remove white-space from start of line */
-		Str_Trim(szString);
-		if (strlen(szString)>0)
+		line = Str_Trim(buf);
+
+		/* Ignore empty line and comments */
+		if (strlen(line) == 0 || line[0] == ';' || line[0] == '#')
+			continue;
+
+		/* get the host (PC) key spec */
+		host = Str_Trim(strtok_r(line, ",", &saveptr));
+		if (!HostSpecToKeymap(host, &LoadedKeymap[idx][0]))
 		{
-			char *p;
-			/* Is a comment? */
-			if (szString[0] == ';' || szString[0] == '#')
-				continue;
-			/* Cut out the values */
-			p = strtok(szString, ",");
-			if (!p)
-				continue;
-			Str_Trim(szString);
-			PCKeyCode = atoi(szString);    /* Direct key code? */
-			if (PCKeyCode < 10)
-			{
-				/* If it's not a valid number >= 10, then
-				 * assume we've got a symbolic key name
-				 */
-				int offset = 0;
-				/* quoted character (e.g. comment line char)? */
-				if (*szString == '\\' && strlen(szString) == 2)
-					offset = 1;
-				PCKeyCode = Keymap_GetKeyFromName(szString+offset);
-			}
-			p = strtok(NULL, "\n");
-			if (!p)
-				continue;
-			STScanCode = atoi(p);
-			/* Store into remap table, check both value within range */
-			if (STScanCode > 0 && STScanCode <= KBD_MAX_SCANCODE
-			    && PCKeyCode >= 8)
-			{
-				LOG_TRACE(TRACE_KEYMAP,
-				          "keymap from file: sym=%i --> scan=%i\n",
-				          PCKeyCode, STScanCode);
-				LoadedKeymap[idx][0] = PCKeyCode;
-				LoadedKeymap[idx][1] = STScanCode;
-				idx += 1;
-			}
-			else
-			{
-				Log_Printf(LOG_WARN, "Could not parse keymap file:"
-				           " '%s' (%d >= 8), '%s' (0 > %d <= %d)\n",
-					   szString, PCKeyCode, p, STScanCode, KBD_MAX_SCANCODE);
-			}
+			Log_Printf(LOG_WARN, "Failed to parse host (PC/SDL) part '%s' of line %d in: %s\n",
+				   host, linenro, pszFileName);
+			fails++;
+			continue;
 		}
-	}
 
+		/* Get the guest (ST) key spec */
+		guest = Str_Trim(strtok_r(NULL, "\n", &saveptr));
+		if (!GuestSpecToKeymap(guest, &LoadedKeymap[idx][1]))
+		{
+			Log_Printf(LOG_WARN, "Failed to parse guest (ST) part '%s' of line %d in: %s\n",
+				   guest, linenro, pszFileName);
+			fails++;
+			continue;
+		}
+		LOG_TRACE(TRACE_KEYMAP, "key mapping from file: host %s => guest %s\n",
+			  host, guest);
+		idx += 1;
+	}
 	fclose(in);
+
+	if (fails)
+		Log_AlertDlg(LOG_ERROR, "%d keymap file parsing failures\n(see console log for details)", fails);
 }
 
 
-- 
2.30.2


--------------6125158E9D8E923B9ED57408
Content-Type: text/plain; charset=UTF-8;
 name="keymap.txt"
Content-Transfer-Encoding: base64
Content-Disposition: attachment;
 filename="keymap.txt"

IyBzcGVjaWFsIHNjYW5jb2RlIG5hbWUgZXhhbXBsZToKIyAgICdmMScgLT4gJ2InCkYxLDQ4
CgojIHNwZWNpYWwga2V5cyBtYXBwZWQgZGlmZmVyZW50bHkgb24gVVMgJiBGSSBrZXlib2Fy
ZHMKIyAoYW5kIHVzaW5nIGhleCBkZWNpbWFscykKIyAgICcgLT4gJ2EnCiMgICDDtiAtPiAn
YicKJywgMHgxZQrDtiwgMHgzMAoKIyB3aXRoIG1vZGlmaWVyIG5hbWVzIGJlZm9yZSBrZXk6
CiMgICBsLXNoaWZ0ICsgJ2EnIC0+ICdCJwpMU0hJRlR8QSwgTFNISUZUfDQ4CgojIHdpdGgg
bW9kaWZpZXIgbmFtZXMgYWZ0ZXIga2V5OgojICAgci1zaGlmdCArICdhJyAtPiAnQycKQXxS
U0hJRlQsIDQ2fFJTSElGVAoKIyB0aGlzIG1hdGNoZXMgb25seSB3aGVuIG1vdmVkIGJlZm9y
ZSBlaXRoZXIgb3IgYm90aAojIG9mIHRoZSBMU0hJRlQvUlNISUZUIHNwZWNzIGFib3ZlLCB0
aGVuIGl0IG92ZXJyaWRlcwojIGZvbGxvd2luZyBzaGlmdCBzcGVjcyBmb3IgdGhlIHNhbWUg
c2NhbmNvZGUKIyAgIChhbnkpIHNoaWZ0ICsgJ2EnIC0+ICdEJwpBfFNISUZULCAzMnxMU0hJ
RlQKCiMgQWx0R3Igd2l0aCBrZXkgbm90IHVzZWQgYXMgSGF0YXJpIHNob3J0Y3V0OgojIGh0
dHBzOi8vaGF0YXJpLnR1eGZhbWlseS5vcmcvZG9jL21hbnVhbC5odG1sI0tleWJvYXJkX3No
b3J0Y3V0c19mb3JfdGhlX1NETF9HVUkKIyAgIEFsdEdyICsgJ2UnIC0+ICfCtScKRXxSQUxU
LCBBTFRfWFhYIHwgMjMwCgojIHdpdGggbnVtZXJpYyBTREwgc2NhbmNvZGUgKyBudW1sb2Nr
LCBleHRyYSBzcGFjaW5nIGFuZCBBbHQteHh4OgojICAgTnVtTG9jayArICdhJyAtPiAnwrUn
CjQgfCBOVU0gLCBBTFRfWFhYIHwgMjMwCgojIGJlY2F1c2UgdGhpcyBoYXMgbm8gbW9kaWZp
ZXIsIGl0IG5lZWRzIHRvIGJlCiMgc3BlY2lmaWVkIGFmdGVyIG1vZGlmaWVyIHNwZWNzIGZv
ciBzYW1lIHNjYW5jb2RlOgojICAgJ2EnIC0+ICdlJwpBLCAxOAo=
--------------6125158E9D8E923B9ED57408--