Future-proof 5.0.0 for fixing #4183

[ghost]

Issue type

• Something between feature request and bug report.

Minetest version

5f1cd555cd9d1c64426e173b30b5b792d211c835 (git master as of this writing)

Summary

#6898 is a compatibility-breaking change. Since it doesn't seem likely that it's accepted for 5.0.0, I suggest adding this future-proofing change, that will allow to seamlessly include #6898 into the 5.x series at a future point in time.

diff --git a/builtin/mainmenu/dlg_config_world.lua b/builtin/mainmenu/dlg_config_world.lua
index 140eb60d..15d5e16f 100644
--- a/builtin/mainmenu/dlg_config_world.lua
+++ b/builtin/mainmenu/dlg_config_world.lua
@@ -134,7 +134,7 @@ local function handle_buttons(this, fields)
 					not mod.is_game_content then
 				if modname_valid(mod.name) then
 					worldfile:set("load_mod_" .. mod.name,
-							tostring(mod.enabled))
+						mod.enabled and "true" or "false")
 				elseif mod.enabled then
 					gamedata.errormessage = fgettext_ne("Failed to enable mo" ..
 							"d \"$1\" as it contains disallowed characters. " ..
diff --git a/builtin/mainmenu/pkgmgr.lua b/builtin/mainmenu/pkgmgr.lua
index eb062ccc..c650efc0 100644
--- a/builtin/mainmenu/pkgmgr.lua
+++ b/builtin/mainmenu/pkgmgr.lua
@@ -366,7 +366,8 @@ function pkgmgr.get_worldconfig(worldpath)
 		if key == "gameid" then
 			worldconfig.id = value
 		elseif key:sub(0, 9) == "load_mod_" then
-			worldconfig.global_mods[key] = core.is_yes(value)
+			worldconfig.global_mods[key] = value ~= "false" and value ~= "nil"
+				and value
 		else
 			worldconfig[key] = value
 		end
@@ -565,7 +566,7 @@ function pkgmgr.preparemodlist(data)
 				end
 			end
 			if element ~= nil then
-				element.enabled = core.is_yes(value)
+				element.enabled = value ~= "false" and value ~= "nil" and value
 			else
 				core.log("info", "Mod: " .. key .. " " .. dump(value) .. " but not found")
 			end
diff --git a/src/content/mods.cpp b/src/content/mods.cpp
index a3e70676..5140c520 100644
--- a/src/content/mods.cpp
+++ b/src/content/mods.cpp
@@ -270,7 +270,8 @@ void ModConfiguration::addModsFromConfig(
 	conf.readConfigFile(settings_path.c_str());
 	std::vector<std::string> names = conf.getNames();
 	for (const std::string &name : names) {
-		if (name.compare(0, 9, "load_mod_") == 0 && conf.getBool(name))
+		if (name.compare(0, 9, "load_mod_") == 0 && conf.get(name) != "false"
+				&& conf.get(name) != "nil")
 			load_mod_names.insert(name.substr(9));
 	}
 

(Edited to fix the patch, to include 'nil' as false, and to not generate 'nil')

[rubenwardy]

Does the patch work if the key isn't in the config? eg: on a new world

[ghost]

I believe so, but to be honest, I haven't tested it. However, note that the C++ side checks the name in a loop that iterates through the existing names, therefore the name is guaranteed to exist; and the Lua side checks whether 'value' is not false, and nonexistent values are false in Lua.

[ghost]

Tested now, and found/fixed a problem. On save, the UI was saving "nil" instead of "false" for mods that didn't have a setting yet. To strengthen the check, it now checks for both strings "false" and "nil", which will help migrating older worlds that might contain "nil". And the code now consistently saves either "false" or "true".

[paramat]

So, this depends on concept approval of the #6898 changes, which already had 1 approval, please can core devs consider that?

[ghost]

This is not strictly necessary for fixing #4183 actually. It is necessary if the approach in #6898 is used, namely to reuse load_mod_XXXX to hold the path of the mod; but an alternative approach would be to keep load_mod_XXXX as is, and add a new setting per mod to hold the path, like: mod_path_XXXX = <path>.

So, the decision to adopt this approach, and therefore to implement the above patch, depends on the decision of whether to reuse load_mod_XXXX to hold the path, or to make a separate mod_path_XXXX setting per mod.

[SmallJoker]

I support the patch provided above. Checking for "nil" does not look very great, but it's acceptable to make it a smooth transition. Distinguishing mods by modpacks would really be a great thing to see in 5.0.0.

[paramat]

Yes so technically not a blocker but still needs consideration before 5.0.0.

[ghost]

To test the patch (and merge if it gets enough approvals):

# Remove the remote with name 'pgimeno' if it exists
git remote remove pgimeno
# Add the remote repository with name 'pgimeno'
git remote add pgimeno https://gitlab.com/pgimeno/minetest
# Fetch this branch's commits from the 'pgimeno' remote
git fetch pgimeno futureproof-load-mod
# Create a local branch that tracks the remote one
git checkout futureproof-load-mod

If the last command does not work, either git is too old, or I made a blunder on how exactly it's supposed to work. In that case:

# Check out the commit corresponding to the remote's branch
git checkout pgimeno/futureproof-load-mod
# Create a local branch for it and switch to it
git checkout -b futureproof-load-mod

That branch can now be pushed to MS-GitHub and turned into a PR if desired, or it can be cherry-picked into Master if pushing it directly is preferred.

[ghost]

Why is this not a blocker? Without it, the opportunity of implementing an important bugfix in the 5.x series will be missed.

[paramat]

It's only a blocker if the approach in #6898 is concept-approved, but i now see 2 core devs have expressed support for this approach (one support was hiding in emoticons, i wish core devs wouldn't use those for official support, it's so easy to miss).
It needed consideration anyway so i probably should have left it as a blocker.

[SmallJoker]

#8055 merged