Skip to content

Add a client side mod dialog #16001

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions builtin/mainmenu/content/pkgmgr.lua
Original file line number Diff line number Diff line change
Expand Up @@ -711,6 +711,53 @@ function pkgmgr.preparemodlist(data)
return retval
end

function pkgmgr.prepareclientmodlist(data)
local retval = {}

local clientmods = {}

--read clientmods
local modpath = core.get_clientmodpath()

if modpath ~= nil and modpath ~= "" then
pkgmgr.get_mods(modpath, "clientmods", clientmods)
end

for i=1,#clientmods,1 do
clientmods[i].type = "mod"
clientmods[i].loc = "global"
clientmods[i].is_clientside = true
retval[#retval + 1] = clientmods[i]
end
Comment on lines +726 to +731
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tables are references, thus this shorter version is possible:

Suggested change
for i=1,#clientmods,1 do
clientmods[i].type = "mod"
clientmods[i].loc = "global"
clientmods[i].is_clientside = true
retval[#retval + 1] = clientmods[i]
end
for _, mod in ipairs(clientmods) do
mod.type = "mod"
mod.loc = "global"
mod.is_clientside = true
end
retval = clientmods

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The retval variable becomes unnecessary, does it not?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, by how it's currently used, it's a duplicate.


--read mods configuration
local filename = modpath ..
DIR_DELIM .. "mods.conf"

local conffile = Settings(filename)
Comment on lines +734 to +737
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
local filename = modpath ..
DIR_DELIM .. "mods.conf"
local conffile = Settings(filename)
local conffile = Settings(modpath .. DIR_DELIM .. "mods.conf")


for key,value in pairs(conffile:to_table()) do
if key:sub(1, 9) == "load_mod_" then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do

local mod_name = key:match("^load_mod_(.+)")
if mod_name then
    ...
end

key = key:sub(10)
local element = nil
for i=1,#retval,1 do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bad style. Just do something like

Suggested change
for i=1,#retval,1 do
for _, client_mod in ipairs(client_mods) do

and give element a better name (such as found_client_mod or whatever).

though really i feel you should have client mods indexed in a table by name. then you wouldn't need a linear search here - the code would be cleaner.

if retval[i].name == key and
not retval[i].is_modpack then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can go on the same line. if you do need continuation lines, indent them by two tabs so they don't blend in with the next line.

element = retval[i]
break
end
end
if element ~= nil then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if element ~= nil then
if element then

element.enabled = value ~= "false" and value ~= "nil" and value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
element.enabled = value ~= "false" and value ~= "nil" and value
element.enabled = value ~= "false" and value ~= "nil"

i don't see and value adding any value, it just creates type confusion: the value is a string and thus necessarily truthy.

else
core.log("info", "Clientmod: " .. key .. " " .. dump(value) .. " but not found")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
core.log("info", "Clientmod: " .. key .. " " .. dump(value) .. " but not found")
core.log("info", "Client Mod: Cannot find " .. key .. ", " .. dump(value))

end
end
end

return retval
end

function pkgmgr.compare_package(a, b)
return a and b and a.name == b.name and a.path == b.path
end
Expand Down Expand Up @@ -749,6 +796,8 @@ function pkgmgr.reload_global_mods()
end
pkgmgr.global_mods = filterlist.create(pkgmgr.preparemodlist,
pkgmgr.comparemod, is_equal, nil, {})
pkgmgr.clientmods = filterlist.create(pkgmgr.prepareclientmodlist,
pkgmgr.comparemod, is_equal, nil, {})
pkgmgr.global_mods:add_sort_mechanism("alphabetic", sort_mod_list)
pkgmgr.global_mods:set_sortmode("alphabetic")
end
Expand Down
193 changes: 193 additions & 0 deletions builtin/mainmenu/dlg_csm.lua
Original file line number Diff line number Diff line change
@@ -0,0 +1,193 @@
--Minetest
--Copyright (C) 2025 ProunceDev <[email protected]>
--
--This program is free software; you can redistribute it and/or modify
--it under the terms of the GNU Lesser General Public License as published by
--the Free Software Foundation; either version 2.1 of the License, or
--(at your option) any later version.
--
--This program is distributed in the hope that it will be useful,
--but WITHOUT ANY WARRANTY; without even the implied warranty of
--MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
--GNU Lesser General Public License for more details.
--
--You should have received a copy of the GNU Lesser General Public License along
--with this program; if not, write to the Free Software Foundation, Inc.,
--51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
Comment on lines +4 to +16
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: you may use the short SPDX-License-Identifier notation instead to avoid this boilerplate. Ctrl+F in the other code for examples.


local packages_raw, packages, tab_selected_pkg

local function modname_valid(name)
return not name:find("[^a-z0-9_]")
end

local function get_formspec(dlgview, name, tabdata)
if pkgmgr.clientmods == nil then
pkgmgr.reload_global_mods()
end

if packages == nil then
packages_raw = {}
table.insert_all(packages_raw, pkgmgr.clientmods:get_list())

local function get_data()
return packages_raw
end

local function is_equal(element, uid) --uid match
return (element.type == "game" and element.id == uid) or
element.name == uid
end

packages = filterlist.create(get_data, pkgmgr.compare_package, is_equal, nil, {})

local filename = core.get_clientmodpath() .. DIR_DELIM .. "mods.conf"
local conffile = Settings(filename)
local mods = conffile:to_table()

for i = 1, #packages_raw do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for i = 1, #packages_raw do
for _, mod in ipairs(packages_raw) do

but given the name of packages_raw, wouldn't pkg be a better name (it looks like this also includes things like modpacks)?

local mod = packages_raw[i]
if mod.is_clientside then
if modname_valid(mod.name) then
conffile:set("load_mod_" .. mod.name,
mod.enabled and "true" or "false")
elseif mod.enabled then
gamedata.errormessage = fgettext_ne("Failed to enable clientmod" ..
" \"$1\" as it contains disallowed characters. " ..
"Only characters [a-z0-9_] are allowed.",
mod.name)
end
mods["load_mod_" .. mod.name] = nil
Comment on lines +51 to +60
Copy link
Member

@SmallJoker SmallJoker Apr 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate of dlg_config_world.lua. Please re-use existing code. This dialogue already mentions that it's about Client Mods, thus the error message (Failed to enable mod [...] (that is used there) should be good enough.

Example:

function pkgmgr.foobar_insert_my_mods(mods_ret, packages, conffile, cb_may_add)
	for _, mod in ipairs(packages) do
		if cb_may_add(mod) then
			-- same code here
		end
	end
end


-- for CSM:
pkgmgr.foobar_insert_my_mods(mods, packages_raw, conffile, function(mod)
	return mod.is_clientside
end

-- for Server Mods: (dlg_config_world.lua)
pkgmgr.foobar_insert_my_mods(mods, packages_raw, conffile, function(mod)
	return not mod.is_modpack and not mod.is_game_content
end

end
end

-- Remove mods that are not present anymore
for key in pairs(mods) do
if key:sub(1, 9) == "load_mod_" then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if key:sub(1, 9) == "load_mod_" then
if key:find("^load_mod_") then

conffile:remove(key)
end
end
if not conffile:write() then
core.log("error", "Failed to write clientmod config file")
end
end

if tab_selected_pkg == nil then
Copy link
Contributor

@appgurueu appgurueu Apr 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if tab_selected_pkg == nil then
tab_selected_pkg = tab_selected_pkg or 1

then delete the rest of the if

tab_selected_pkg = 1
end

local use_technical_names = core.settings:get_bool("show_technical_names")


local fs = {
"size[6.5,7]",
"label[0.25,-0.1;", fgettext("Installed Client Mods"), "]",
"tablecolumns[color;tree;text]",
"table[0.15,0.5;6,5.7;pkglist;", pkgmgr.render_packagelist(packages, use_technical_names), ";", tab_selected_pkg, "]",
"button[0.15,5.5;3,3;back;", fgettext("Back"), "]"
}


local selected_pkg
if filterlist.size(packages) >= tab_selected_pkg then
selected_pkg = packages:get_list()[tab_selected_pkg]
end

if selected_pkg ~= nil then
if selected_pkg.type == "mod" then
if selected_pkg.is_clientside then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary if pyramid. just do if selected_pkg and selected_pkg.type == "mod" and selected_pkg.is_clientside then (wrap as needed).

if selected_pkg.enabled then
table.insert_all(fs, {
"button[3.36,5.5;3,3;btn_csm_mgr_disable_mod;", fgettext("Disable"), "]"
})
else
table.insert_all(fs, {
"button[3.36,5.5;3,3;btn_csm_mgr_enable_mod;", fgettext("Enable"), "]"
})
end
end
end
end
return table.concat(fs, "")
end

--------------------------------------------------------------------------------
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
--------------------------------------------------------------------------------

let's not do these anymore, same below.

local function handle_doubleclick(pkg, pkg_name)
pkgmgr.enable_mod({data = {list = packages, selected_mod = pkg_name}})
packages = nil
end

--------------------------------------------------------------------------------
local function handle_buttons(dlgview, fields, tabname, tabdata)
if fields["pkglist"] ~= nil then
local event = core.explode_table_event(fields["pkglist"])
tab_selected_pkg = event.row
if event.type == "DCL" then
handle_doubleclick(packages:get_list()[tab_selected_pkg], tab_selected_pkg)
end
return true
end

if fields.btn_csm_mgr_mp_enable ~= nil or fields.btn_csm_mgr_mp_disable ~= nil then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if fields.btn_csm_mgr_mp_enable ~= nil or fields.btn_csm_mgr_mp_disable ~= nil then
if fields.btn_csm_mgr_mp_enable or fields.btn_csm_mgr_mp_disable then

pkgmgr.enable_mod({data = {list = packages, selected_mod = tab_selected_pkg}}, fields.btn_csm_mgr_mp_enable ~= nil)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

packages = nil
return true
end

if fields.btn_csm_mgr_enable_mod ~= nil or fields.btn_csm_mgr_disable_mod ~= nil then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if fields.btn_csm_mgr_enable_mod ~= nil or fields.btn_csm_mgr_disable_mod ~= nil then
if fields.btn_csm_mgr_enable_mod or fields.btn_csm_mgr_disable_mod then

pkgmgr.enable_mod({data = {list = packages, selected_mod = tab_selected_pkg}}, fields.btn_csm_mgr_enable_mod ~= nil)
packages = nil
return true
end

if fields.back then
dlgview:delete()
return true
end

if fields["btn_csm_mgr_rename_modpack"] ~= nil then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if fields["btn_csm_mgr_rename_modpack"] ~= nil then
if fields.btn_csm_mgr_rename_modpack then

The same for the other occurrences. It makes the code look more consistent. Currently you're mixing two different styles.

local mod = packages:get_list()[tab_selected_pkg]
local dlg_renamemp = create_rename_modpack_dlg(mod)
dlg_renamemp:set_parent(dlgview)
dlgview:hide()
dlg_renamemp:show()
packages = nil
return true
end

if fields["btn_csm_mgr_delete_mod"] ~= nil then
local mod = packages:get_list()[tab_selected_pkg]
local dlg_delmod = create_delete_content_dlg(mod)
dlg_delmod:set_parent(dlgview)
dlgview:hide()
dlg_delmod:show()
packages = nil
return true
end

if fields.btn_csm_mgr_use_txp or fields.btn_csm_mgr_disable_txp then
local txp_path = ""
if fields.btn_csm_mgr_use_txp then
txp_path = packages:get_list()[tab_selected_pkg].path
end

core.settings:set("texture_path", txp_path)
packages = nil

mm_game_theme.init()
mm_game_theme.reset()
return true
end

return false
end

function create_csm_dlg()
mm_game_theme.set_engine()
return dialog_create(
"csm",
get_formspec,
handle_buttons,
pkgmgr.update_gamelist
)
end
1 change: 1 addition & 0 deletions builtin/mainmenu/init.lua
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ dofile(menupath .. DIR_DELIM .. "dlg_version_info.lua")
dofile(menupath .. DIR_DELIM .. "dlg_reinstall_mtg.lua")
dofile(menupath .. DIR_DELIM .. "dlg_clients_list.lua")
dofile(menupath .. DIR_DELIM .. "dlg_server_list_mods.lua")
dofile(menupath .. DIR_DELIM .. "dlg_csm.lua")

local tabs = {
content = dofile(menupath .. DIR_DELIM .. "tab_content.lua"),
Expand Down
11 changes: 10 additions & 1 deletion builtin/mainmenu/tab_content.lua
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,8 @@ local function get_formspec(tabview, name, tabdata)
pkgmgr.render_packagelist(packages, use_technical_names, update_icons),
";", tabdata.selected_pkg, "]",

"button[0.4,5.8;6.3,0.9;btn_contentdb;", contentdb_label, "]"
"button[0.4,5.8;3.1,0.9;btn_contentdb;", contentdb_label, "]",
"button[3.6,5.8;3.1,0.9;btn_csm;", fgettext("Client side mods"), "]"
}

local selected_pkg
Expand Down Expand Up @@ -229,6 +230,14 @@ local function handle_buttons(tabview, fields, tabname, tabdata)
return true
end

if fields.btn_csm then
local dlg = create_csm_dlg()
dlg:set_parent(tabview)
tabview:hide()
dlg:show()
return true
end

if fields.btn_mod_mgr_rename_modpack then
local mod = packages:get_list()[tabdata.selected_pkg]
local dlg_renamemp = create_rename_modpack_dlg(mod)
Expand Down
2 changes: 2 additions & 0 deletions src/script/scripting_mainmenu.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,8 @@ bool MainMenuScripting::mayModifyPath(const std::string &path)
return true;
if (fs::PathStartsWith(path, path_user + DIR_DELIM "worlds"))
return true;
if (fs::PathStartsWith(path, path_user + DIR_DELIM "clientmods"))
return true;

if (fs::PathStartsWith(path, fs::AbsolutePathPartial(porting::path_cache)))
return true;
Expand Down
Loading