Improve configuration handling

This commit improves and simplifies weltschmerz's configuration
handling. Obtaining and parsing the KeyFile is split out into
ConfigReader, which is fully agnostic of the specific configuration
values.

Config now contains all configuration values in the form of primitive
types or class instances, and no longer delegates access to the KeyFile
structure directly. This means that the configuration file is read once,
and then kept in the Config instance.

Indirectly this commit also fixes a bug where weltschmerz would segfault
if one of the palette entries contained an invalid value.

5 files changed, 259 insertions, 155 deletions

diff --git a/Makefile b/Makefile
index ad973c7..5822595 100644
--- a/Makefile
+++ b/Makefile
@@ -7,7 +7,7 @@ VALAC ?= valac
 
 weltschmerz: weltschmerz.vala terminal.vala config.vala resources.c
 	${VALAC} -X -lm --pkg posix --pkg gtk+-3.0 --pkg vte-2.91 --gresources resources.xml \
-		weltschmerz.vala terminal.vala config.vala resources.c
+		weltschmerz.vala terminal.vala config.vala configreader.vala resources.c
 
 resources.c: resources.xml terminal.ui terminal.css
 	glib-compile-resources $< --target=$@ --generate-source
diff --git a/config.vala b/config.vala
index c36d533..991d015 100644
--- a/config.vala
+++ b/config.vala
@@ -1,108 +1,71 @@
 class Config {
 
-	KeyFile? keyfile = new KeyFile();
-	string[] warnings = {};
-
-	public Config() {
-		var path = Path.build_filename(Environment.get_user_config_dir(), PROGRAM_NAME, "config");
-		try {
-			keyfile.load_from_file(path, NONE);
-		} catch (Error e) {
-			// We want to ignore a legitimately missing file, since we fall back to defaults.
-			if (!(e is FileError.NOENT)) {
-				append_warning(path + ": " + e.message);
-				keyfile = null;
-			}
-		}
+	public bool autohide_mouse;
+	public Vte.CursorShape cursor_shape;
+	public Pango.FontDescription font;
+	public int scrollback;
+	public bool scrollbar;
+
+	public Gdk.RGBA? foreground;
+	public Gdk.RGBA? background;
+	public Gdk.RGBA? cursor_foreground;
+	public Gdk.RGBA? cursor_background;
+	public Gdk.RGBA? selection_foreground;
+	public Gdk.RGBA? selection_background;
+	public Gdk.RGBA? bold;
+
+	public Gdk.RGBA[] palette = new Gdk.RGBA[16];
+
+	struct PaletteEntry {
+		string name;
+		string normal;
+		string bright;
 	}
 
-	public void append_warning(string message) {
-		warning(message);
+	const PaletteEntry[] DEFAULT_PALETTE = {
+		{ "black",   "black",    "grey50"  },
+		{ "red",     "red3",     "red"     },
+		{ "green",   "green3",   "green"   },
+		{ "yellow",  "yellow3",  "yellow"  },
+		{ "blue",    "blue2",    "#5c5cff" },
+		{ "magenta", "magenta3", "magenta" },
+		{ "cyan",    "cyan3",    "cyan"    },
+		{ "white",   "grey90",   "white"   },
+	};
 
-		warnings += "• " + Markup.escape_text(message);
-	}
-
-	void check_error(KeyFileError e) {
-		if (!(e is KeyFileError.KEY_NOT_FOUND || e is KeyFileError.GROUP_NOT_FOUND)) {
-			append_warning(e.message);
-		}
-	}
-
-	public string[] done() {
-		if (keyfile == null) {
-			return warnings;
-		}
-
-		string[] keys = {};
-		try {
-			foreach(var group in keyfile.get_groups()) {
-				foreach(var key in keyfile.get_keys(group)) {
-					keys += string.join(".", group, key);
-				}
-			}
-		} catch (KeyFileError e) {
-			// purposefully ignored
-		}
-
-		if (keys.length > 0) {
-			string k = keys.length > 1 ? "keys" : "key";
-			append_warning("Unknown %s in config: %s".printf(k, string.joinv(", ", keys)));
-		}
+	ConfigReader reader;
 
-		return warnings;
+	public Config() {
+		reader = new ConfigReader(Path.build_filename(Environment.get_user_config_dir(), PROGRAM_NAME, "config"));
+		load();
 	}
 
-	public string? value(string group, string key, string? fallback) {
-		if (keyfile == null) { return fallback; }
-
-		try {
-			string value = keyfile.get_value(group, key);
-			keyfile.remove_key(group, key);
-			return value;
-		} catch (KeyFileError e) {
-			check_error(e);
-			return fallback;
+	public void load() {
+		autohide_mouse = reader.read_boolean("misc", "autohide-mouse", false);
+		cursor_shape = reader.read_cursor("misc", "cursor-shape", "block");
+		font = Pango.FontDescription.from_string(reader.read_string("misc", "font", "Monospace 12"));
+		scrollback = reader.read_integer("misc", "scrollback", 10000);
+		scrollbar = reader.read_boolean("misc", "scrollbar", true);
+
+		foreground = reader.read_colour("colours", "foreground", null);
+		background = reader.read_colour("colours", "background", null);
+		cursor_foreground = reader.read_colour("colours", "cursor.foreground", null);
+		cursor_background = reader.read_colour("colours", "cursor.background", null);
+		selection_foreground = reader.read_colour("colours", "cursor.foreground", null);
+		selection_background = reader.read_colour("colours", "cursor.background", null);
+		bold = reader.read_colour("colours", "bold", null);
+
+		for (int i = 0; i < DEFAULT_PALETTE.length; i++) {
+			var entry = DEFAULT_PALETTE[i];
+			palette[i] = reader.read_colour("colours", "normal." + entry.name, entry.normal);
+			palette[i + 8] = reader.read_colour("colours", "bright." + entry.name, entry.bright);
 		}
-	}
 
-	public int? integer(string group, string key, int? fallback) {
-		if (keyfile == null) { return fallback; }
-
-		try {
-			int integer = keyfile.get_integer(group, key);
-			keyfile.remove_key(group, key);
-			return integer;
-		} catch (KeyFileError e) {
-			check_error(e);
-			return fallback;
-		}
+		reader.log_unknown_keys();
 	}
 
-	public bool? boolean(string group, string key, bool? fallback) {
-		if (keyfile == null) { return fallback; }
-
-		try {
-			bool boolean = keyfile.get_boolean(group, key);
-			keyfile.remove_key(group, key);
-			return boolean;
-		} catch (KeyFileError e) {
-			check_error(e);
-			return fallback;
-		}
+	public string[] get_warnings() {
+		return reader.get_warnings();
 	}
 
-	public Gdk.RGBA? colour(string key, string? fallback) {
-		string value = value("colours", key, fallback);
-		if (value == null) {
-			return null;
-		}
-
-		var rgba = Gdk.RGBA();
-		if (!rgba.parse(value)) {
-			append_warning("invalid colour: " + value);
-			return null;
-		}
-
-		return rgba;
-	}
 }
diff --git a/configreader.vala b/configreader.vala
new file mode 100644
index 0000000..a57a7ab
--- /dev/null
+++ b/configreader.vala
@@ -0,0 +1,188 @@
+// What follows is arguably the least readily understandable part of
+// weltschmerz; some pointers and design notes shall follow.
+//
+// The purpose of this class is to read the key-value configuration file from
+// disk and turn it into an in-memory representation of GLib's KeyFile
+// structure. We do not stray from this format, yet the error handling and some
+// design implications make it a bit harder than *just* parsing a file.
+//
+// In particular, a core design decision is to provide reasonable defaults to
+// the user and to, wherever possible, fall back to a configuration that is at
+// least useable. Short of a bug in GLib's KeyFile implementation, this code
+// *should* gracefully deal with any sort of input: a missing configuration
+// file, a corrupted or wrongly encoded configuration file, etc.
+//
+// Another decision is to report any noteworthy error to the user, and to make
+// sure that issues with the configuration file are reported as accurately as
+// possible. This includes particularly the reporting of entries that were
+// *not* accessed by the program at all, which can indicate a small oversight
+// like a typo, or a deprecated config entry.
+//
+// Most of the information in this file based on the official documentation,
+// but a few parts (the ones there is no documentation for) are based on my
+// reading of the GLib C code and live testing. Therefore, I cannot guarantee
+// complete accuracy.
+//
+// Notable GLib resources:
+// https://valadoc.org/glib-2.0/GLib.KeyFile.html
+// https://valadoc.org/glib-2.0/GLib.KeyFileError.html
+
+class ConfigReader {
+	KeyFile keyfile = new KeyFile();
+	string[] warnings;
+
+	public ConfigReader (string path) {
+		try {
+			keyfile.load_from_file(path, NONE);
+		} catch (Error e) {
+			// The GLib documentation does not make this clear, but the only
+			// KeyFileErrors that will be reported when loading the file from
+			// disk are KeyFileError.PARSE and KeyFileError.UNKNOWN_ENCODING
+			//
+			// KeyFileError.NOT_FOUND will never be returned, FileError.NOENT
+			// takes its place instead. Since we can gracefully fall back to
+			// default values, we specifically ignore this error.
+			if (e is FileError.NOENT)
+				return;
+
+			// We want to warn the user about any other error, and set keyfile
+			// to null.  At this point the file has already been fully parsed
+			// by GLib, but we do not trust it to have read anything correctly
+			// at this point, so we destroy the in-memory representation.
+			append_warning(e.message);
+			keyfile = null;
+		}
+	}
+
+	public string[] get_warnings() {
+		return warnings;
+	}
+
+	void append_warning(string message) {
+		warning(message);
+		warnings += "• " + Markup.escape_text(message);
+	}
+
+	// This method is called from all three read_* methods if they encountered
+	// any KeyFileErrors whilst accessing keys in keyfile. At this point, the
+	// following KeyFileErrors are possible: INVALID_VALUE, UNKNOWN_ENCODING,
+	// KEY_NOT_FOUND, and GROUP_NOT_FOUND.
+	//
+	// The latter two are insignificant since we can fall back to default
+	// values.
+	//
+	// INVALID_VALUE is treated specially here to remove a potentially
+	// confusing warning message about unknown keys (see log_unknown_keys).
+	//
+	// Finally, UNKNOWN_ENCODING is treated normally.
+	void handle_error(KeyFileError e, string group, string key) {
+		if (e is KeyFileError.INVALID_VALUE) {
+			try {
+				// Remove a known key with an invalid value
+				keyfile.remove_key(group, key);
+			} catch (KeyFileError e) {
+				debug("Could not remove existing and valid key entry");
+			}
+		} else if (e is KeyFileError.UNKNOWN_ENCODING) {
+			append_warning(e.message);
+		}
+	}
+
+	// In order to keep track of entries that were not accessed by the program
+	// at all, we remove all successfully read entries from the KeyFile. Any
+	// entries that are left over have not been used and can then be reported
+	// to the user.
+	public void log_unknown_keys() {
+		if (keyfile == null)
+			return;
+
+		string[] keys = {};
+		try {
+			foreach(var group in keyfile.get_groups()) {
+				foreach(var key in keyfile.get_keys(group)) {
+					keys += string.join(".", group, key);
+				}
+			}
+		} catch (KeyFileError e) {}
+
+		if (keys.length > 0) {
+			string k = keys.length > 1 ? "keys" : "key";
+			append_warning("Unknown %s in config: %s".printf(k, string.joinv(", ", keys)));
+		}
+	}
+
+	public string? read_string(string group, string key, string? default) {
+		if (keyfile == null)
+			return default;
+
+		try {
+			string str = keyfile.get_string(group, key);
+			keyfile.remove_key(group, key);
+			return str;
+		} catch (KeyFileError e) {
+			handle_error(e, group, key);
+			return default;
+		}
+	}
+
+	public int? read_integer(string group, string key, int? default) {
+		if (keyfile == null)
+			return default;
+
+		try {
+			int integer = keyfile.get_integer(group, key);
+			keyfile.remove_key(group, key);
+			return integer;
+		} catch (KeyFileError e) {
+			handle_error(e, group, key);
+			return default;
+		}
+	}
+
+	public bool? read_boolean(string group, string key, bool? default) {
+		if (keyfile == null)
+			return default;
+
+		try {
+			bool boolean = keyfile.get_boolean(group, key);
+			keyfile.remove_key(group, key);
+			return boolean;
+		} catch (KeyFileError e) {
+			handle_error(e, group, key);
+			return default;
+		}
+	}
+
+	public Gdk.RGBA? read_colour(string group, string key, string? default) {
+		string str = read_string(group, key, default);
+
+		if (str == null)
+			return null;
+
+		var rgba = Gdk.RGBA();
+		if (!rgba.parse(str)) {
+			append_warning("invalid colour '%s' in %s.%s".printf(str, group, key));
+
+			rgba.parse(default);
+			return rgba;
+		}
+
+		return rgba;
+	}
+
+	public Vte.CursorShape read_cursor(string group, string key, string default) {
+		string cursor = read_string(group, key, default);
+
+		switch (cursor) {
+			case "block":
+				return BLOCK;
+			case "beam":
+				return IBEAM;
+			case "underline":
+				return UNDERLINE;
+			default:
+				append_warning("invalid cursor shape '%s' in %s.%s".printf(cursor, group, key));
+				return BLOCK;
+		}
+	}
+}
diff --git a/meson.build b/meson.build
index ea90630..485ce8e 100644
--- a/meson.build
+++ b/meson.build
@@ -11,7 +11,7 @@ dependencies = [
   dependency('vte-2.91'),
 ]
 
-sources = files('weltschmerz.vala', 'terminal.vala', 'config.vala')
+sources = files('weltschmerz.vala', 'terminal.vala', 'config.vala', 'configreader.vala')
 sources += gnome.compile_resources('resources', 'resources.xml')
 
 executable('weltschmerz', sources, dependencies: dependencies, install: true)
diff --git a/terminal.vala b/terminal.vala
index a7cfb6b..99c21ce 100644
--- a/terminal.vala
+++ b/terminal.vala
@@ -18,23 +18,6 @@ class Terminal : Gtk.Overlay {
 	const double FONT_SCALE_MAX = 3.583180799999999;  // 1.2 ^ 7
 	const double FONT_SCALE_FACTOR = 1.2;
 
-	struct PaletteEntry {
-		string name;
-		string normal;
-		string bright;
-	}
-
-	const PaletteEntry[] DEFAULT_PALETTE = {
-		{ "black",   "black",    "grey50"  },
-		{ "red",     "red3",     "red"     },
-		{ "green",   "green3",   "green"   },
-		{ "yellow",  "yellow3",  "yellow"  },
-		{ "blue",    "blue2",    "#5c5cff" },
-		{ "magenta", "magenta3", "magenta" },
-		{ "cyan",    "cyan3",    "cyan"    },
-		{ "white",   "grey90",   "white"   },
-	};
-
 	public Gtk.Window window { get; construct set; }
 	[GtkChild] Gtk.Button search_button_down;
 	[GtkChild] Gtk.Button search_button_up;
@@ -83,10 +66,10 @@ class Terminal : Gtk.Overlay {
 	public void load_config(bool reload) {
 		var conf = new Config();
 
-		Gtk.PolicyType policy = conf.boolean("misc", "scrollbar", true) ? Gtk.PolicyType.AUTOMATIC : Gtk.PolicyType.NEVER;
+		Gtk.PolicyType policy = conf.scrollbar ? Gtk.PolicyType.AUTOMATIC : Gtk.PolicyType.NEVER;
 		scrolled_window.set_policy(policy, policy);
 
-		vte.set_font(Pango.FontDescription.from_string(conf.value("misc", "font", "Monospace 12")));
+		vte.set_font(conf.font);
 		var geometry = Gdk.Geometry() {
 			// This must be kept in sync with the padding size in terminal.css
 			base_width = 2 * 2,
@@ -96,50 +79,20 @@ class Terminal : Gtk.Overlay {
 		};
 		window.set_geometry_hints(null, geometry, BASE_SIZE | RESIZE_INC);
 
-		vte.set_mouse_autohide(conf.boolean("misc", "autohide-mouse", false));
-		vte.set_scrollback_lines(conf.integer("misc", "scrollback", 10000));
-
-		var cursor = conf.value("misc", "cursor-shape", "block");
-		switch (cursor) {
-			case "block":
-				vte.set_cursor_shape(BLOCK);
-				break;
-			case "beam":
-				vte.set_cursor_shape(IBEAM);
-				break;
-			case "underline":
-				vte.set_cursor_shape(UNDERLINE);
-				break;
-			default:
-				conf.append_warning("invalid cursor '%s'".printf(cursor));
-				vte.set_cursor_shape(BLOCK);
-				break;
-		}
-
-		var foreground = conf.colour("foreground", null);
-		var background = conf.colour("background", null);
-
-		var palette = new Gdk.RGBA[16];
-		for (int i = 0; i < DEFAULT_PALETTE.length; i++) {
-			var entry = DEFAULT_PALETTE[i];
-			palette[i] = conf.colour("normal." + entry.name, entry.normal);
-			palette[i + 8] = conf.colour("bright." + entry.name, entry.bright);
-		}
-
-		vte.set_colors(foreground, background, palette);
-
-		vte.set_color_bold(conf.colour("bold", null));
-
-		vte.set_color_cursor_foreground(conf.colour("cursor.foreground", null));
-		vte.set_color_cursor(conf.colour("cursor.background", null));
+		vte.set_mouse_autohide(conf.autohide_mouse);
+		vte.set_scrollback_lines(conf.scrollback);
+		vte.set_cursor_shape(conf.cursor_shape);
 
-		vte.set_color_highlight_foreground(conf.colour("selection.foreground", null));
-		vte.set_color_highlight(conf.colour("selection.background", null));
+		vte.set_colors(conf.foreground, conf.background, conf.palette);
+		vte.set_color_bold(conf.bold);
+		vte.set_color_cursor_foreground(conf.cursor_foreground);
+		vte.set_color_cursor(conf.cursor_background);
+		vte.set_color_highlight_foreground(conf.selection_foreground);
+		vte.set_color_highlight(conf.selection_background);
 
-		var warnings = conf.done();
-		if (warnings.length > 0) {
+		if (conf.get_warnings().length > 0) {
 			string header = "<b>Configuration loaded with warnings:</b>\n";
-			infobar_show(header + string.joinv("\n", warnings), Gtk.MessageType.WARNING);
+			infobar_show(header + string.joinv("\n", conf.get_warnings()), Gtk.MessageType.WARNING);
 		} else if (reload) {
 			infobar_show("Configuration loaded successfully.", Gtk.MessageType.INFO, 3);
 		}