Assorted fixes/cleanups

Contains the following patches:
* [PATCH 01/10] Replace 'minetest' with 'core'
* [PATCH 02/10] Extend type checks
* [PATCH 03/10] Add examples
* [PATCH 04/10] Protect against invalid formspec inputs
* [PATCH 05/10] Unify some code
* [PATCH 06/10] Use dropdown index to check againt value validity
* [PATCH 07/10] Improve formspec and implement 'Reset' button
* [PATCH 08/10] Do not run callbacks if no value changed
* [PATCH 09/10] Add checkbox to show technical names
* [PATCH 10/10] Make formspec layout more compact

4 files changed, 276 insertions, 85 deletions

diff --git a/.luacheckrc b/.luacheckrc
index 1b67400..a5aea61 100644
--- a/.luacheckrc
+++ b/.luacheckrc
@@ -5,6 +5,9 @@ globals = {
 }
 
 read_globals = {
-	"minetest",
+	"dump",
+	table = { fields = { "indexof", "copy" } },
+
+	"core",
 	"unified_inventory",
 }
diff --git a/README b/README
index 71a4ca9..6b962ec 100644
--- a/README
+++ b/README
@@ -32,7 +32,7 @@ playersettings.register("yourmodname:yoursettingname",{
 	-- For "number" settings: The largest selectable value.
 	
 	integer = true,
-	-- For "integer" settings: If true, entered numbers will be rounded down to the nearest integer.
+	-- For "number" settings: If true, entered numbers will be rounded down to the nearest integer.
 	
 	values = "some text here",
 	-- For "string" settings: Text to be displayed under the entry field to assist the user in selecting valid values.
diff --git a/examples.lua b/examples.lua
new file mode 100644
index 0000000..eb439b8
--- /dev/null
+++ b/examples.lua
@@ -0,0 +1,64 @@
+core.log("error", "[playersettings] Examples are enabled!")
+
+local longdesc =  "Some helpful long description with [];\\[ escaped characters\nLine 2"
+
+-- Wrapper to add logging
+local function playersettings_register_logged(setting, def)
+	def.onjoin = function(player_name, setting_value)
+		core.log("action", ("Callback %s for player=%s: %s=%s"):format(
+			"onjoin", player_name, setting, dump(setting_value)))
+	end
+	def.onchange = function(player_name, old_value, new_value)
+		core.log("action", ("Callback %s for player=%s: old=%s, new=%s"):format(
+			"onchange", player_name, dump(old_value), dump(new_value)))
+		return true -- allow changing
+	end
+	def.afterchange = function(player_name, old_value, new_value)
+		core.log("action", ("Callback %s for player=%s: old=%s, new=%s"):format(
+			"afterchange", player_name, dump(old_value), dump(new_value)))
+	end
+
+	playersettings.register(setting, def)
+end
+
+playersettings_register_logged("playersettings:number", {
+	type = "number",
+	shortdesc = "Example: number",
+	longdesc = longdesc,
+	default = 5,
+	min = -0.123,
+	max = 10,
+})
+
+playersettings_register_logged("playersettings:number_integer", {
+	type = "number",
+	integer = true,
+	shortdesc = "Example: number (integer)",
+	longdesc = longdesc,
+	default = 5,
+	min = -10,
+	max = 20,
+})
+
+playersettings_register_logged("playersettings:string", {
+	type = "string",
+	shortdesc = "Example: string",
+	longdesc = longdesc,
+	default = "<default>",
+	values = "Input any value."
+})
+
+playersettings_register_logged("playersettings:boolean", {
+	type = "boolean",
+	shortdesc = "Example: boolean",
+	longdesc = longdesc,
+	default = true,
+})
+
+playersettings_register_logged("playersettings:enum", {
+	type = "enum",
+	shortdesc = "Example: enum",
+	longdesc = longdesc,
+	default = "apple",
+	values = { "apple", "banana", "batman" },
+})
diff --git a/init.lua b/init.lua
index b33c522..b1d1b2a 100644
--- a/init.lua
+++ b/init.lua
@@ -1,4 +1,4 @@
-local storage = minetest.get_mod_storage()
+local storage = core.get_mod_storage()
 
 playersettings = {}
 
@@ -6,45 +6,101 @@ playersettings.registered = {}
 
 playersettings.settingslist = {}
 
+-- per-player values for formspec creation
 playersettings.highlighted = {}
+playersettings.show_technical_names = {}
 
 playersettings.form = "formspec_version[4]"..
-	"size[15,15]" ..
-	"button[12.5,13.5;2,1;quit;Save and Exit]"..
-	"textlist[0.5,0.5;7,14;settinglist;%s;%d;false]"..
-	"label[8,2;%s]"..
-	"label[8,3;%s]"..
+	"size[15,12.5]" ..
+	"button[11, 9.5;3,1;bset;Set]"..
+	"button[12,11;2,1;breset;Reset]"..
+	"textlist[0.5,0.5;7,11;settinglist;%s;%d;false]"..
+	"checkbox[0.5,12;ctechnical;Show technical names;%s]"..
+	"     box[8,1;6.5,4;#222]"..
+	"textarea[8,1;6.5,4;;%s;%s]"..
 	"%s"
 
+local text_no_settings = [[
+No settings currently exist.
+
+To use the settings menu, one or more mods that provide some settings must be installed.
+]]
+
+playersettings.form_fallback = (
+	"formspec_version[4]"..
+	"size[10,4.5]"..
+	"textarea[0.5,0.5;9,2.5;;;" .. text_no_settings .."]"..
+	"button_exit[4,3;2,1;quit;Close]"
+)
+
+--- Returns: setting, def
+local function get_setting_at(index)
+	local settingname = playersettings.settingslist[index]
+	if not settingname then
+		return nil, nil
+	end
+	return settingname, playersettings.registered[settingname]
+end
+
+local function table_concat_escaped(list, delim)
+	local esc = {}
+	for i, v in ipairs(list) do
+		esc[i] = core.formspec_escape(v)
+	end
+	return table.concat(esc, delim)
+end
+
 function playersettings.openform(player)
-	if type(player) == "string" then player = minetest.get_player_by_name(player) end
+	if type(player) == "string" then player = core.get_player_by_name(player) end
 	local name = player:get_player_name()
 	if #playersettings.settingslist < 1 then
-		local form = "formspec_version[4]"..
-		             "size[12,3.5]"..
-		             "label[0.5,1;No settings currently exist.]"..
-		             "label[0.5,1.5;To use the settings menu, one or more mods that provide some settings must be installed.]"..
-		             "button_exit[5,2;2,1;quit;Close]"
-		minetest.show_formspec(name,"playersettings:nosettings",form)
+		core.show_formspec(name,"playersettings:nosettings", playersettings.form_fallback)
 		return
 	end
-	if not playersettings.highlighted[name] then playersettings.highlighted[name] = 1 end
-	local settingslist = ""
-	for _,setting in ipairs(playersettings.settingslist) do
-		settingslist = settingslist..minetest.formspec_escape(playersettings.registered[setting].shortdesc)..","
+
+	local settingslist
+	do
+		local list = {}
+		local technical = playersettings.show_technical_names[name]
+		for _,setting in ipairs(playersettings.settingslist) do
+			local def = playersettings.registered[setting]
+			if technical then
+				table.insert(list, ("%s (%s)"):format(setting, def.type))
+			else
+				table.insert(list, def.shortdesc)
+			end
+		end
+		settingslist = table_concat_escaped(list, ",")
 	end
-	settingslist = settingslist:sub(1,-2)
-	local settingname = playersettings.settingslist[playersettings.highlighted[name]]
-	local def = playersettings.registered[settingname]
+
+	local settingname, def = get_setting_at(playersettings.highlighted[name])
+	if not def then
+		playersettings.highlighted[name] = 1
+		settingname, def = get_setting_at(1)
+	end
+	local settingvalue = playersettings.get(name, settingname)
+	-- Engine bug? Even though this code uses the correct values, when pressing
+	--             "Set" and then "Reset", the shown value does not update.
+	--print(def.type, settingvalue)
+
 	local extras = ""
 	if def.type == "boolean" then
-		extras = "checkbox[8,8;checkbox;Enabled;%s]"
-		extras = extras:format(playersettings.get(name,settingname) and "true" or "false")
+		extras = (
+			"checkbox[8,6.5;checkbox;Enabled;%s]" ..
+			"field_close_on_enter[field;false]"
+		):format(
+			settingvalue and "true" or "false"
+		)
 	elseif def.type == "string" then
-		extras = "field[8,8;6,1;field;Enter value:;%s]label[8,9.5;Allowed values: %s]field_close_on_enter[field;false]"
-		extras = extras:format(minetest.formspec_escape(playersettings.get(name,settingname)),minetest.formspec_escape(def.values or "any string"))
+		extras = (
+			"field[8,6;6,1;field;Enter value:;%s]" ..
+			"textarea[8,7.5;6,2;;;Allowed values: %s]" ..
+			"field_close_on_enter[field;false]"
+		):format(
+			core.formspec_escape(settingvalue),
+			core.formspec_escape(def.values or "any string")
+		)
 	elseif def.type == "number" then
-		extras = "field[8,8;6,1;field;Enter value:;%s]label[8,9.5;Allowed values: %s]field_close_on_enter[field;false]"
 		local allowed = "any number"
 		if def.min and def.max then
 			allowed = "numbers "..def.min.." to "..def.max
@@ -56,56 +112,86 @@ function playersettings.openform(player)
 		if def.integer then
 			allowed = allowed.." (whole numbers only)"
 		end
-		extras = extras:format(minetest.formspec_escape(playersettings.get(name,settingname)),allowed)
+		extras = (
+			"field[8,6;6,1;field;Enter value:;%s]" ..
+			"textarea[8,7.5;6,2;;;Allowed values: %s]" ..
+			"field_close_on_enter[field;false]"
+		):format(
+			core.formspec_escape(settingvalue),
+			allowed
+		)
 	elseif def.type == "enum" then
-		extras = "dropdown[8,8;6,1;dropdown;%s;%d;false]"
-		local choices = ""
-		local current = playersettings.get(name,settingname)
-		local selected = 1
-		for k,v in ipairs(def.values) do
-			choices = choices..minetest.formspec_escape(v)..","
-			if v == current then selected = k end
-		end
-		choices = choices:sub(1,-2)
-		extras = extras:format(choices,selected)
-	end
-	local highlighted = playersettings.highlighted[name]
-	local shortdesc = minetest.formspec_escape(def.shortdesc)
-	local longdesc = minetest.formspec_escape(def.longdesc)
-	local form = playersettings.form:format(settingslist,highlighted,shortdesc,longdesc,extras)
-	minetest.show_formspec(name,"playersettings:settings",form)
+		local selected = table.indexof(def.values, settingvalue)
+		if selected == -1 then selected = 1 end
+		extras = ("dropdown[8,6;6,1;dropdown;%s;%d;true]"):format(
+			table_concat_escaped(table.copy(def.values), ","),
+			selected
+		)
+	end
+
+	local form = playersettings.form:format(
+		settingslist,
+		playersettings.highlighted[name],
+		playersettings.show_technical_names[name] and "true" or "false",
+		core.formspec_escape(def.shortdesc),
+		core.formspec_escape(def.longdesc),
+		extras
+	)
+	core.show_formspec(name,"playersettings:settings",form)
 end
 
 function playersettings.handleform(player,form,fields)
 	if form ~= "playersettings:settings" then return end
+
 	local name = player:get_player_name()
-	local settingname = playersettings.settingslist[playersettings.highlighted[name]]
-	local def = playersettings.registered[settingname]
-	if def.type == "boolean" and fields.checkbox then
-		playersettings.set(name,settingname,fields.checkbox == "true")
-	elseif def.type == "enum" and fields.dropdown then
-		playersettings.set(name,settingname,fields.dropdown)
-	elseif def.type == "number" and fields.field then
-		local value = tonumber(fields.field)
-		if value
-			and ((not def.max) or (value <= def.max))
-			and ((not def.min) or (value >= def.min)) then
-			if def.integer then value = math.floor(value) end
-			playersettings.set(name,settingname,value)
+
+	if fields.ctechnical then
+		playersettings.show_technical_names[name] = fields.ctechnical == "true"
+		playersettings.openform(player)
+		return
+	end
+
+	local settingname, def = get_setting_at(playersettings.highlighted[name])
+	if def then
+		if fields.breset then
+			local value = playersettings.getdefault(settingname)
+			playersettings.set(name, settingname, value)
+			playersettings.openform(player)
+			return
+		end
+
+		-- May also be triggered by `fields.bset`
+		if def.type == "boolean" and fields.checkbox then
+			playersettings.set(name,settingname,fields.checkbox == "true")
+		elseif def.type == "enum" and fields.dropdown then
+			local value = def.values[tonumber(fields.dropdown)]
+			if value then
+				playersettings.set(name, settingname, value)
+			end
+		elseif def.type == "number" and fields.field then
+			local value = tonumber(fields.field)
+			if value
+				and ((not def.max) or (value <= def.max))
+				and ((not def.min) or (value >= def.min)) then
+				if def.integer then value = math.floor(value) end
+				playersettings.set(name,settingname,value)
+			end
+		elseif def.type == "string" and fields.field then
+			playersettings.set(name,settingname,fields.field)
 		end
-	elseif def.type == "string" and fields.field then
-		playersettings.set(name,settingname,fields.field)
 	end
 	if fields.settinglist then
-		local exp = minetest.explode_textlist_event(fields.settinglist)
+		local exp = core.explode_textlist_event(fields.settinglist)
 		if exp.type == "CHG" then
 			playersettings.highlighted[name] = exp.index
 			playersettings.openform(player)
 		end
 	end
-	if fields.quit then
-		minetest.close_formspec(name,"playersettings:settings")
-	end
+end
+
+local function CHECK_TYPE(typename, value, readable)
+	local got = type(value)
+	assert(got == typename, ("Invalid %s (expected %s, got %s)"):format(readable, typename, got))
 end
 
 function playersettings.getdefault(setting)
@@ -121,10 +207,11 @@ function playersettings.getdefault(setting)
 end
 
 function playersettings.get(name,setting)
-	assert(type(name) == "string",string.format("Invalid player name (expected string, got %s)",type(name)))
-	assert(type(setting) == "string",string.format("Invalid setting name (expected string, got %s)",type(setting)))
+	CHECK_TYPE("string", name, "player name")
+	CHECK_TYPE("string", setting, "setting name")
 	assert(playersettings.registered[setting],"No such setting: "..setting)
-	local value = minetest.deserialize(storage:get_string(string.format("%s|%s",name,setting)))
+
+	local value = core.deserialize(storage:get_string(string.format("%s|%s",name,setting)))
 	if value ~= nil then
 		return value
 	else
@@ -133,22 +220,50 @@ function playersettings.get(name,setting)
 end
 
 function playersettings.set(name,setting,value)
-	assert(type(name) == "string",string.format("Invalid player name (expected string, got %s)",type(name)))
-	assert(type(setting) == "string",string.format("Invalid setting name (expected string, got %s)",type(setting)))
+	CHECK_TYPE("string", name, "player name")
+	CHECK_TYPE("string", setting, "setting name")
 	assert(playersettings.registered[setting],"No such setting: "..setting)
+
 	local old = playersettings.get(name,setting)
+	if old == value then
+		return
+	end
+
 	local def = playersettings.registered[setting]
+	if def.type == "enum" then
+		-- Prohibit unknown values
+		if table.indexof(def.values, value) == -1 then
+			return
+		end
+	end
 	if def.onchange then
 		if not def.onchange(name,old,value) then return end
 	end
-	storage:set_string(string.format("%s|%s",name,setting),minetest.serialize(value))
+	storage:set_string(string.format("%s|%s",name,setting),core.serialize(value))
 	if def.afterchange then def.afterchange(name,old,value) end
 end
 
 function playersettings.register(setting,def)
-	assert(type(setting) == "string",string.format("Invalid setting name (expected string, got %s)",type(setting)))
-	assert(not playersettings.registered[setting],string.format("Setting %s already defined",setting))
-	assert(type(def) == "table",string.format("Invalid setting definition (expected table, got %s)",type(def)))
+	CHECK_TYPE("string", setting, "setting name")
+	CHECK_TYPE("table", def, "setting definition")
+	assert(not playersettings.registered[setting],string.format("Setting %s is already defined",setting))
+
+	assert(type(def.type) == "string")
+	assert(type(def.shortdesc) == "string")
+	assert(type(def.longdesc)  == "string")
+	assert(not def.onjoin      or type(def.onjoin)      == "function")
+	assert(not def.onchange    or type(def.onchange)    == "function")
+	assert(not def.afterchange or type(def.afterchange) == "function")
+
+	-- Type-specific checks
+	if def.type == "string" then
+		assert(type(def.values) == "string")
+	elseif def.type == "enum" then
+		assert(type(def.values) == "table")
+	end
+	local default_type = (def.type == "enum") and "string" or def.type
+	assert(def.default == nil or type(def.default) == default_type)
+
 	table.insert(playersettings.settingslist,setting)
 	table.sort(playersettings.settingslist)
 	playersettings.registered[setting] = def
@@ -157,27 +272,36 @@ end
 function playersettings.onjoin(player)
 	local name = player:get_player_name()
 	for setting,def in pairs(playersettings.registered) do
-		if type(def.onjoin) == "function" then
+		if def.onjoin then
 			def.onjoin(name,playersettings.get(name,setting))
 		end
 	end
 end
 
-if minetest.global_exists("unified_inventory") then
-	unified_inventory.register_button("playersettings",
-		{
-			action = playersettings.openform,
-			tooltip = "Settings",
-			type = "image",
-			image = "playersettings_settings_button.png"
-		}
-	)
+local function on_leaveplayer(player)
+	-- Clean up memory
+	local name = player:get_player_name()
+	playersettings.highlighted[name] = nil
+	playersettings.show_technical_names[name] = nil
 end
 
-minetest.register_chatcommand("settings",{
+if core.global_exists("unified_inventory") then
+	unified_inventory.register_button("playersettings", {
+		action = playersettings.openform,
+		tooltip = "Settings",
+		type = "image",
+		image = "playersettings_settings_button.png"
+	})
+end
+
+core.register_chatcommand("settings",{
 	description = "Open player settings menu",
 	func = playersettings.openform,
 })
 
-minetest.register_on_joinplayer(playersettings.onjoin)
-minetest.register_on_player_receive_fields(playersettings.handleform)
+core.register_on_joinplayer(playersettings.onjoin)
+core.register_on_leaveplayer(on_leaveplayer)
+core.register_on_player_receive_fields(playersettings.handleform)
+
+-- For testing purposes only!
+--dofile(core.get_modpath(core.get_current_modname()) .. "/examples.lua")