Merge pull request #364 from johnjohto/mods-stray-folder-notice

Adopt mods dropped beside the game instead of ignoring them

src/core/SaveData.lua

@@ -110,11 +110,14 @@ local function makePortableFs(dir)
   }
 end
 
-local function detectPortable()
+-- Every folder a player might reasonably call "the game folder", best first:
-  if portableChecked then return portableBase end
+-- the packaged-app container, the folder holding the executable, then the
-  portableChecked = true
+-- source itself.  Portable mode is the case where one of these holds the
-  portableBase = false
+-- marker; the list itself is just locations, marker or not, which is also
-  if not (love and love.filesystem) then return false end
+-- 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
@@ -122,7 +125,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()
@@ -150,15 +153,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 <gamedir>` run drops portable.txt in the game folder).
-  -- First one holding the marker wins.  Built by appending so a nil (e.g. no
+  -- Built by appending so a nil (e.g. no .app in the path) never truncates
-  -- .app in the path) never truncates the ipairs scan.
+  -- 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 sbd and sbd ~= "" then candidates[#candidates + 1] = sbd end
-  if src then candidates[#candidates + 1] = src end
+  if src and src ~= "" then candidates[#candidates + 1] = src end
-  for _, base in ipairs(candidates) do
+  return candidates
-    if base ~= "" and pathExists(base .. SEP .. PORTABLE_MARKER) then
+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

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

src/import/RomImporter.lua

@@ -2708,6 +2708,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
 

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
+-- The same split decides where a mod is FOUND, and that has a sharp edge: a
--- no filesystem, so the engine tier can table-drive it; the discovery,
+-- non-portable install never reads the game folder at all, so a mod unzipped
--- install, and uninstall paths reach for love.filesystem and SaveData.
+-- 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 <gamedir>` 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

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")