Merge pull request #953 from ShaneMcGovernIE/shanemcgovernie-fix-reset-settings-bugs

saveOptions three-way merge: stop partial writes dropping launcher keys (#932)

src/core/SaveData.lua

@@ -358,6 +358,22 @@ local function readTable(fs, name)
   return SaveSerializer.decode(body)
 end
 
+-- Deep-copy a value folded in from the on-disk decode so the returned
+-- options table never aliases the file's nested tables (SaveData must not
+-- depend on src/mods/Merge.lua for this).  Options data is plain tables of
+-- strings/numbers/booleans/tables, so a cycle guard is belt-and-braces.
+local function deepCopy(v, seen)
+  if type(v) ~= "table" then return v end
+  seen = seen or {}
+  if seen[v] then return seen[v] end
+  local copy = {}
+  seen[v] = copy
+  for k, val in pairs(v) do
+    copy[deepCopy(k, seen)] = deepCopy(val, seen)
+  end
+  return copy
+end
+
 -- the stub filesystem some headless harnesses inject has no remove; a
 -- lingering tmp/bak there is harmless
 local function remove(fs, name)
@@ -371,13 +387,48 @@ end
 -- options round-trip headless (no love global).
 function SaveData.saveOptions(opts, fs)
   fs = persistFs(fs)
+  -- #932: options.lua is a WHOLE-FILE rewrite, so a caller that hands over a
+  -- PARTIAL table (just the keys it changed) would silently drop every key it
+  -- does not mention -- launcher-only keys like lastVersion, and keys the
+  -- launcher set (battleBg, tilt...) all fall back to defaults.  Read the
+  -- on-disk file FIRST and fold caller-absent values underneath, so a delta
+  -- write changes only what it names.
+  --
+  -- A table holding EVERY defaultOptions key is a full snapshot
+  -- (loadOptions() results, game.save.options, the RESET REBINDS /
+  -- activeProfile-drop paths) and stays authoritative: its absent keys are
+  -- deliberate deletions, so nothing folds for it.  Partial tables get every
+  -- on-disk key they do not provide folded in (deep-copied so the caller's
+  -- table is never aliased).  This is the reconciling rule: bindings and
+  -- activeProfile -- not defaultOptions members -- can be deleted by their
+  -- sites precisely because those sites always write full tables.
+  local onDisk = readTable(fs, OPTIONS_FILENAME)
+  local isFull = type(opts) == "table"
+  if isFull then
+    for k in pairs(SaveData.defaultOptions()) do
+      if opts[k] == nil then isFull = false break end
+    end
+  end
+  if not isFull then
+    local merged = {}
+    if type(opts) == "table" then
+      for k, v in pairs(opts) do merged[k] = v end
+    end
+    if type(onDisk) == "table" then
+      for k, v in pairs(onDisk) do
+        if k ~= "modOptions" and merged[k] == nil then
+          merged[k] = deepCopy(v)
+        end
+      end
+    end
+    opts = merged
+  end
   opts = SaveData.mergeOptions(opts)
   -- modOptions is per-mod nested state: fold the on-disk sub-tree
   -- underneath (newest value winning per key) so one caller's partial
   -- write cannot clobber another mod's persisted keys.  Every other
   -- option stays on the shallow path.
-  local onDisk = readTable(fs, OPTIONS_FILENAME)
-  if onDisk and type(onDisk.modOptions) == "table" then
+  if type(onDisk) == "table" and type(onDisk.modOptions) == "table" then
     local merged = {}
     for modId, bucket in pairs(onDisk.modOptions) do
       merged[modId] = bucket

tests/engine/options_partial_write_bug932.lua

@@ -0,0 +1,100 @@
+-- #932 "Bugs reset settings": a caller that hands saveOptions a PARTIAL
+-- table (only the keys it changed) used to drop every key it did not
+-- mention -- launcher-only keys like lastVersion, and keys the launcher set
+-- (battleBg, tilt) all fell back to defaults.  saveOptions now reads the
+-- on-disk file first and folds caller-absent values underneath, so a delta
+-- write changes only what it names.
+--
+-- This suite pins the three-way merge against injected filesystem stubs
+-- (the same { getInfo, read, write, remove } shape the other engine suites
+-- use).  It is ROM-free (T2 engine tier).
+--   luajit tests/engine/options_partial_write_bug932.lua
+
+package.path = "./?.lua;./?/init.lua;" .. package.path
+
+local T = require("tests.harness")
+local check, eq = T.check, T.eq
+love = love or require("tests.love_stub")
+
+local SaveData = require("src.core.SaveData")
+
+local OPTIONS = "options.lua"
+
+local function memfs()
+  local files = {}
+  return {
+    files = files,
+    write = function(path, content) files[path] = content return true end,
+    read = function(path) return files[path] end,
+    remove = function(path) files[path] = nil return true end,
+    getInfo = function(path)
+      if files[path] ~= nil then return { type = "file" } end
+      return nil
+    end,
+  }
+end
+
+-- Seed a save dir with the full snapshot a launcher would write: defaults,
+-- plus the keys the issue cares about.  lastVersion is launcher-only (not a
+-- defaultOptions member) and must survive ANY write that does not name it.
+local function seed(fs)
+  local seed = SaveData.defaultOptions()
+  seed.battleBg = "world"
+  seed.lastVersion = "blue"
+  seed.tilt = 1
+  seed.mods = { foo = true }
+  seed.modOptions = { alpha = { keep = true, x = 1 } }
+  check(SaveData.saveOptions(seed, fs) ~= nil, "seeding lands")
+end
+
+-- ---- launcher-only keys survive a delta write
+
+local fs = memfs()
+seed(fs)
+
+-- loader-style partial write: only the mods bucket it manages.
+SaveData.saveOptions({ mods = { foo = true } }, fs)
+local opts = SaveData.loadOptions(fs)
+eq(opts.battleBg, "world", "a partial write keeps battleBg the launcher set")
+eq(opts.lastVersion, "blue", "a partial write keeps lastVersion (#932)")
+eq(opts.tilt, 1, "a partial write keeps tilt the launcher set")
+
+-- ---- caller-present keys still win
+
+SaveData.saveOptions({ battleBg = "black" }, fs)
+eq(SaveData.loadOptions(fs).battleBg, "black",
+  "a key the caller DOES provide wins over the on-disk value")
+eq(SaveData.loadOptions(fs).lastVersion, "blue",
+  "...while the launcher-only key is still carried")
+
+-- ---- modOptions per-mod deep merge stays intact
+
+SaveData.saveOptions({ modOptions = { alpha = { x = 5 } } }, fs)
+local after = SaveData.loadOptions(fs)
+eq(after.modOptions.alpha.x, 5, "newest alpha value wins the per-mod merge")
+eq(after.modOptions.alpha.keep, true, "alpha's untouched keys survive")
+eq(after.modOptions.beta, nil, "no beta was invented by the merge")
+
+-- ---- full-table writes stay authoritative (bindings/activeProfile drops)
+
+-- The fold must NOT resurrect a key a full snapshot deliberately deletes:
+-- the RESET REBINDS path nils bindings and the mod manager nils
+-- activeProfile, always on full loadOptions tables.
+fs = memfs()
+seed(fs)
+SaveData.saveOptions({ bindings = { a = 1 } }, fs)
+eq(SaveData.loadOptions(fs).bindings.a, 1, "bindings is not a default member")
+
+local full = SaveData.loadOptions(fs)
+full.bindings = nil
+full.activeProfile = nil
+SaveData.saveOptions(full, fs)
+local reopened = SaveData.loadOptions(fs)
+eq(reopened.bindings, nil,
+  "a full-snapshot deletion of bindings is NOT resurrected by the fold")
+eq(reopened.activeProfile, nil,
+  "a full-snapshot deletion of activeProfile is NOT resurrected")
+eq(reopened.battleBg, "world",
+  "the rest of the full snapshot is still what it was")
+
+T.finish("options_partial_write_bug932")

tests/engine/options_write_readback_bug828.lua

@@ -204,23 +204,26 @@ eq(reopened.lastVersion, "blue",
   "launcher-only keys the game never reads are carried through its write")
 
 -- The corollary, and the reason the copy has to come from loadOptions: a
--- caller that writes a partial literal instead of a loaded table drops every
--- key it does not mention, because mergeOptions only fills DEFAULTS in around
--- what it is handed (SaveData.mergeOptions).  Nothing on the boot path does
--- this today; the assertion is the guard rail if someone shortcuts it.
+-- caller that writes a partial literal instead of a loaded table would drop
+-- every key it does not mention.  Since #932 that drop is closed by a
+-- three-way merge -- saveOptions folds on-disk values the caller's table
+-- does not carry (lastVersion here), defaults-filling only what neither side
+-- has -- so even a delta write keeps the launcher's key alive.  Nothing on
+-- the boot path writes partials today; the assertion is the guard rail if
+-- someone shortcuts it.
 SaveData.saveOptions({ battleLayout = "og" }, hop)
-eq(SaveData.loadOptions(hop).lastVersion, nil,
-  "a partial write drops launcher-only keys, so the game must write the "
-  .. "table loadOptions handed it")
+eq(SaveData.loadOptions(hop).lastVersion, "blue",
+  "a partial write no longer drops launcher-only keys (#932)")
 
--- Known gap, deliberately not asserted: a copy taken BEFORE the launcher's
--- write and flushed after it still wins, because saveOptions merges only
--- modOptions from disk and every other key is last-writer-wins.  Measured,
--- not guessed (og beats a newer wide).  No shipping path holds an options
--- table across a launcher write -- HostShell.restart replaces the process on
--- the way back to the launcher (#785, #575) and LauncherSettings.open notes
--- its own cached table is only true while its modal covers the launcher --
--- so closing that gap needs a three-way merge (baseline vs caller vs disk),
--- not a straight "disk wins", which would throw away real in-game changes.
+-- Known gap, deliberately not asserted: a FULL copy taken BEFORE the
+-- launcher's write and flushed after it still wins -- a table holding every
+-- defaultOptions key is authoritative, so its og is never folded against a
+-- newer wide on disk (#932 closes the PARTIAL-write drop, not this).
+-- Measured, not guessed.  No shipping path holds an options table across a
+-- launcher write -- HostShell.restart replaces the process on the way back
+-- to the launcher (#785, #575) and LauncherSettings.open notes its own
+-- cached table is only true while its modal covers the launcher -- so
+-- closing that gap needs a real three-way baseline (vs caller vs disk), not
+-- a straight "disk wins", which would throw away real in-game changes.
 
 T.finish("options_write_readback_bug828")