From 0dddb32305c4343a8a5367ae9f976805f1ef1f66 Mon Sep 17 00:00:00 2001 From: johnjohto Date: Tue, 28 Jul 2026 21:34:05 -0400 Subject: [PATCH] Adopt mods dropped beside the game instead of ignoring them love.filesystem looks for "mods/" in two places: the save directory, and -- portable installs only -- the game folder, which CacheFs mounts. So a player who unzips a mod next to the executable of an ordinary install, which is where very nearly every other game would want it, gets no error and no mod. The panel just comes up empty, with nothing on screen to suggest the files are sitting in the wrong folder twenty centimetres away. That is a hard failure to self-diagnose, and it is worse behind a launcher: the install lives somewhere the player never opens, so "the game's mods folder" is a guess to begin with. The mods panel now looks in those folders before its first listing and copies what it finds into the tree the game really reads, reporting what it took in the notice line. It happens on open rather than behind a button because the failure being fixed is one where nothing suggests there is anything to press. Looking is scoped: CacheFs.withMounted puts the folder on the read path at its own mount point, runs the scan, and takes it straight back off. Nothing a stray folder contains can shadow a game file or change what the running game resolves, which is what makes it safe to point at a folder whose contents nobody has validated. Adoption skips ids the game can already see, so it is idempotent and never nags twice, and it leaves the loose folder alone -- deleting files outside the save directory on the player's behalf is not this code's call to make. Which strays are worth taking is pure (LauncherMods.pickStrays), matching how deriveList and locateRoot are already split out, so the engine tier covers the rules without needing love. SaveData.gameFolders is the old detectPortable candidate list lifted out unchanged -- portable mode is just the case where one of those folders holds the marker. Claude-Session: https://claude.ai/code/session_01JvEthuoNBPfxpvHUD9Pd4N --- src/core/SaveData.lua | 36 ++++--- src/import/CacheFs.lua | 33 ++++++- src/import/RomImporter.lua | 24 +++++ src/mods/LauncherMods.lua | 141 ++++++++++++++++++++++++++- tests/engine/launcher_mods_tests.lua | 48 +++++++++ 5 files changed, 264 insertions(+), 18 deletions(-) diff --git a/src/core/SaveData.lua b/src/core/SaveData.lua index d8277c9f..0a92535f 100644 --- a/src/core/SaveData.lua +++ b/src/core/SaveData.lua @@ -109,11 +109,14 @@ local function makePortableFs(dir) } end -local function detectPortable() - if portableChecked then return portableBase end - portableChecked = true - portableBase = false - if not (love and love.filesystem) then return false end +-- Every folder a player might reasonably call "the game folder", best first: +-- the packaged-app container, the folder holding the executable, then the +-- source itself. Portable mode is the case where one of these holds the +-- marker; the list itself is just locations, marker or not, which is also +-- what the mods panel needs to notice a mod dropped beside the game by hand +-- (LauncherMods.strays). Empty on Android/iOS and outside LOVE. +function SaveData.gameFolders() + if not (love and love.filesystem) then return {} end -- Desktop only: portable mode carries the save (and, since issue #74, the -- ROM cache) in the game folder next to the executable/source. On -- Android/iOS the source is a read-only package with no such folder, so @@ -121,7 +124,7 @@ local function detectPortable() if love.system and love.system.getOS then local osName = love.system.getOS() if osName ~= "Windows" and osName ~= "Linux" and osName ~= "OS X" then - return false + return {} end end local src = love.filesystem.getSource and love.filesystem.getSource() @@ -149,15 +152,24 @@ local function detectPortable() -- Order: the packaged-app containing folder (macOS .app / Linux AppImage), -- then the source-base directory (next to a packaged .exe), then the source -- itself (a `love ` run drops portable.txt in the game folder). - -- First one holding the marker wins. Built by appending so a nil (e.g. no - -- .app in the path) never truncates the ipairs scan. + -- Built by appending so a nil (e.g. no .app in the path) never truncates + -- the ipairs scan. local candidates = {} local appDir = appContainer(src) or appContainer(sbd) or appImageContainer() if appDir then candidates[#candidates + 1] = appDir end - if sbd then candidates[#candidates + 1] = sbd end - if src then candidates[#candidates + 1] = src end - for _, base in ipairs(candidates) do - if base ~= "" and pathExists(base .. SEP .. PORTABLE_MARKER) then + if sbd and sbd ~= "" then candidates[#candidates + 1] = sbd end + if src and src ~= "" then candidates[#candidates + 1] = src end + return candidates +end + +-- The game folder carrying portable.txt, or false. First candidate holding +-- the marker wins. +local function detectPortable() + if portableChecked then return portableBase end + portableChecked = true + portableBase = false + for _, base in ipairs(SaveData.gameFolders()) do + if pathExists(base .. SEP .. PORTABLE_MARKER) then portableBase = base break end diff --git a/src/import/CacheFs.lua b/src/import/CacheFs.lua index c85ed8d0..ab69f39a 100644 --- a/src/import/CacheFs.lua +++ b/src/import/CacheFs.lua @@ -126,9 +126,9 @@ local function resolveMount() if okl and lib then local oks, fn = pcall(function() return lib.PHYSFS_mount end) if oks and fn then - physfsMountFn = function(d, append) + physfsMountFn = function(d, mountPoint, append) if append == nil then append = true end - local okr, ret = pcall(fn, d, "", append and 1 or 0) + local okr, ret = pcall(fn, d, mountPoint or "", append and 1 or 0) return okr and ret ~= 0 end break @@ -145,7 +145,7 @@ end local function mountReadable(dir, append) local fn = resolveMount() if not fn then return false end - return fn(dir, append) + return fn(dir, "", append) end -- PHYSFS_unmount, resolved the same way PHYSFS_mount is. Only @@ -385,4 +385,31 @@ function CacheFs.unmountVersion(version) return done end +-- Mount `dir` at `mountPoint` for the length of `fn()`, then take it back off +-- the read path and hand back whatever fn returned. +-- +-- Every other mount here is permanent and lands at the physfs root: this one +-- exists to *look* at a folder the game has deliberately not mounted, which +-- is a different job. The mods panel uses it to read a mods/ folder sitting +-- beside the executable of a non-portable install (LauncherMods.strays). +-- Because it unmounts again, and because a non-empty mountPoint keeps the +-- tree in its own corner of the namespace while it is up, a folder inspected +-- this way can never shadow a game file or change what the running game +-- resolves -- which is what makes it safe to point at a folder whose contents +-- nobody has validated. +-- +-- Returns nil when the mount is unavailable (no ffi, no PHYSFS symbol, or the +-- mount was refused), which callers must treat as "could not look", not as +-- "nothing there". An error inside fn still unmounts before it propagates. +function CacheFs.withMounted(dir, mountPoint, fn) + if not dir or dir == "" then return nil end + local mount, unmount = resolveMount(), resolveUnmount() + if not (mount and unmount) then return nil end + if not mount(dir, mountPoint, true) then return nil end + local ok, res = pcall(fn) + unmount(dir) + if not ok then error(res, 0) end + return res +end + return CacheFs diff --git a/src/import/RomImporter.lua b/src/import/RomImporter.lua index 99618938..b2235b0b 100644 --- a/src/import/RomImporter.lua +++ b/src/import/RomImporter.lua @@ -2697,6 +2697,30 @@ end -- so a still list costs nothing after the first paint. function RomImporter:_refreshMods() local LauncherMods = require("src.mods.LauncherMods") + -- Once per session, ahead of the first listing: pull in any mod the player + -- unzipped beside the executable, which an ordinary (non-portable) install + -- has no way to read. It happens here rather than behind a button because + -- the failure being fixed is one where nothing on screen suggests there is + -- anything to press -- the panel just comes up empty. Guarded so a toggle + -- or a delete does not re-scan; adoptStrays is idempotent regardless. + if not self.modStraysChecked then + self.modStraysChecked = true + local imported, failed = {}, {} + for _, s in ipairs(LauncherMods.adoptStrays() or {}) do + table.insert(s.err and failed or imported, s.id) + end + -- the failure wins the notice: an import that worked speaks for itself in + -- the list right below it, one that did not is the only word they get + if #imported > 0 then + self.modNotice = { ok = true, + text = "Imported from the game folder: " .. table.concat(imported, ", ") } + end + if #failed > 0 then + self.modNotice = { ok = false, + text = "Found beside the game but could not import: " + .. table.concat(failed, ", ") } + end + end self.mods = LauncherMods.list() or {} end diff --git a/src/mods/LauncherMods.lua b/src/mods/LauncherMods.lua index 2e81e0a2..84f35610 100644 --- a/src/mods/LauncherMods.lua +++ b/src/mods/LauncherMods.lua @@ -16,9 +16,19 @@ -- fused build), which is why those mods still loaded while landing in the -- wrong place. -- --- Split in two: the pure derivation (deriveList, locateRoot) has no love and --- no filesystem, so the engine tier can table-drive it; the discovery, --- install, and uninstall paths reach for love.filesystem and SaveData. +-- The same split decides where a mod is FOUND, and that has a sharp edge: a +-- non-portable install never reads the game folder at all, so a mod unzipped +-- next to the executable -- where most games would want it -- is not wrong so +-- much as invisible, with an empty panel and no error to explain it. +-- adoptStrays looks in those folders anyway (a scoped mount that comes down +-- again, CacheFs.withMounted) and copies what it finds into the tree the game +-- really reads, so the mistake costs a line of notice rather than a support +-- thread. +-- +-- Split in two: the pure derivation (deriveList, locateRoot, pickStrays) has +-- no love and no filesystem, so the engine tier can table-drive it; the +-- discovery, install, uninstall, and stray-scan paths reach for +-- love.filesystem and SaveData. local Manifest = require("src.mods.Manifest") local ManagerState = require("src.mods.ManagerState") @@ -136,6 +146,28 @@ function LauncherMods.locateRoot(paths) return nil, "no manifest.json found in the .zip" end +-- pickStrays(found, installed) -> the rows worth adopting, pure. +-- found is an array of { id, name, folder, path } in scan order (game folder +-- order, then directory order); installed is the id -> true set of what the +-- game can already see. An installed id is dropped -- the player has a +-- working copy and the loose folder is just where they first put it -- and a +-- duplicate id across two game folders keeps the first, the same first-wins +-- rule discover() uses. Sorted by id so the notice reads the same every time. +function LauncherMods.pickStrays(found, installed) + installed = installed or {} + local out, seen = {}, {} + for _, row in ipairs(found or {}) do + local id = row.id + if id and not installed[id] and not seen[id] then + seen[id] = true + out[#out + 1] = { id = id, name = row.name or id, + folder = row.folder, path = row.path } + end + end + table.sort(out, function(a, b) return a.id < b.id end) + return out +end + -- ------- discovery (love.filesystem) local function decodeManifest(raw, path) @@ -301,6 +333,109 @@ local function removeTree(path) fs.remove(path) end +-- ------- strays: mods dropped beside the game that it cannot see + +-- love.filesystem looks in two places for "mods/": the save directory, and -- +-- portable installs only -- the game folder, which CacheFs mounts. A player +-- who unzips a mod next to the executable of an ordinary install, which is +-- where very nearly every other game would want it, gets no error and no mod. +-- The MODS panel simply stays empty, and there is nothing on screen to +-- suggest the files are twenty centimetres away in the wrong folder. +-- +-- The scan mounts each game folder at a private mount point just long enough +-- to list mods/ inside it and drops it again (CacheFs.withMounted), so the +-- read path the game actually runs on is never touched and a stray can never +-- shadow a real file. +local STRAY_MOUNT = "stray_scan" + +-- Run fn(mountedModsRoot) for each game folder that has a readable mods/ +-- directory, one mount at a time. Folders that are already the physfs source +-- are skipped: their mods/ is discoverable by definition, so anything there is +-- installed already and not a stray (this is every `love ` dev run). +local function eachStrayRoot(fn) + local SaveData_ = require("src.core.SaveData") + local fs = love and love.filesystem + if not fs then return end + local source = fs.getSource and fs.getSource() + local seen = {} + for _, folder in ipairs(SaveData_.gameFolders() or {}) do + if not seen[folder] and folder ~= source then + seen[folder] = true + CacheFs.withMounted(folder, STRAY_MOUNT, function() + local root = STRAY_MOUNT .. "/mods" + if fs.getInfo(root) then fn(root, folder) end + end) + end + end +end + +-- Every valid mod folder sitting in a game folder's mods/, in scan order. +-- Only reads. The rows carry the mounted path, which is live for the length +-- of the mount and dead after it -- copying has to happen inside the same +-- scan, which is why adoption is a flag here rather than a second pass. +local function findStrays(fs, adopt, installed) + local found, adopted = {}, {} + eachStrayRoot(function(root, folder) + local batch = {} + for _, name in ipairs(fs.getDirectoryItems(root)) do + local path = root .. "/" .. name + local info = fs.getInfo(path) + if info and info.type == "directory" then + local raw = fs.read(path .. "/manifest.json") + local manifest = raw and decodeManifest(raw, path) + if manifest then + batch[#batch + 1] = { id = manifest.id, + name = manifest.name or manifest.id, + folder = folder, path = path } + end + end + end + -- filtered per mount, so a copy only ever runs for a row that survived + -- the pure rules -- and so the second game folder sees the first one's + -- ids as taken + for _, row in ipairs(LauncherMods.pickStrays(batch, installed)) do + if adopt then + -- same root pin installZip uses: the mods tree is shared by Red and + -- Blue, never version-prefixed (#330) + local savedPrefix = CacheFs.prefix + CacheFs.prefix = "" + local dest = "mods/" .. row.id + local copied, copyErr = copyTree(row.path, dest) + if not copied then removeTree(dest) end + CacheFs.prefix = savedPrefix + if not copied then row.err = copyErr or "could not copy the files" end + end + installed[row.id] = true + row.path = nil -- dead once this mount comes down + adopted[#adopted + 1] = row + found[#found + 1] = row + end + end) + return LauncherMods.pickStrays(found, {}) +end + +-- The strays, optionally adopted. A folder whose id the game can already see +-- is left out: the player has a working copy, and the loose one is just where +-- they first put it. Rows that failed to copy come back with .err set. +local function scanStrays(adopt) + local fs = love and love.filesystem + if not fs then return {} end + local installed = {} + for _, m in ipairs(discover()) do installed[m.id] = true end + return findStrays(fs, adopt, installed) +end + +-- strays() -> the rows, nothing copied. +function LauncherMods.strays() return scanStrays(false) end + +-- adoptStrays() -> the rows, each one copied into the mods tree the game +-- really reads (rows carrying .err failed). Idempotent: a second call finds +-- the ids installed and returns nothing, so the panel can run this on every +-- open without duplicating anything or nagging twice. The loose folder is +-- deliberately left where it is -- deleting files outside the save directory +-- on the player's behalf is not this function's call to make. +function LauncherMods.adoptStrays() return scanStrays(true) end + -- installZip(source) -> true, id | nil, errString -- source is an external path or a love DroppedFile. The archive is validated -- BEFORE anything is copied; every path unmounts and clears the staged temp diff --git a/tests/engine/launcher_mods_tests.lua b/tests/engine/launcher_mods_tests.lua index 9329944b..2f4044c5 100644 --- a/tests/engine/launcher_mods_tests.lua +++ b/tests/engine/launcher_mods_tests.lua @@ -230,4 +230,52 @@ do "the copy uses the literal picked path") end +-- ------- pickStrays: which mods dropped beside the game are worth adopting + +do + -- the case this exists for: a player unzipped a mod next to the executable + -- of a non-portable install, where the game has no way to read it + local rows = LauncherMods.pickStrays({ + { id = "b_mod", name = "B", folder = "/game", path = "m/b_mod" }, + { id = "a_mod", name = "A", folder = "/game", path = "m/a_mod" }, + }, {}) + eq(#rows, 2, "an uninstalled stray is worth adopting") + eq(rows[1].id, "a_mod", "rows come back sorted by id") + eq(rows[2].id, "b_mod", "both of them") + eq(rows[1].path, "m/a_mod", "carrying the path the copy reads from") + eq(rows[1].folder, "/game", "and the folder it was found in, for the notice") +end + +do + -- already installed: the player has a working copy and the loose folder is + -- just where they first put it. Silence is right -- adopting would make a + -- second copy, and warning would nag on every open. + local rows = LauncherMods.pickStrays({ + { id = "have", name = "Have" }, + { id = "want", name = "Want" }, + }, { have = true }) + eq(#rows, 1, "a stray the game can already see is not a stray") + eq(rows[1].id, "want", "only the one it cannot see is adopted") +end + +do + -- two game folders can both hold the same id (a launcher install plus an + -- older manual one). First wins, matching discover()'s duplicate rule. + local rows = LauncherMods.pickStrays({ + { id = "dup", name = "First", folder = "/a" }, + { id = "dup", name = "Second", folder = "/b" }, + }, {}) + eq(#rows, 1, "a duplicate id across two game folders is adopted once") + eq(rows[1].name, "First", "and the first one found wins") +end + +do + eq(#LauncherMods.pickStrays({}, {}), 0, "no candidates, nothing to adopt") + eq(#LauncherMods.pickStrays(nil, nil), 0, "and nil is not an error") + eq(#LauncherMods.pickStrays({ { name = "no id" } }, {}), 0, + "a row with no id is dropped rather than crashing the panel") + local rows = LauncherMods.pickStrays({ { id = "bare" } }, {}) + eq(rows[1].name, "bare", "a nameless row falls back to its id") +end + T.finish("launcher_mods")