From 666cdb40bd78869eaee8dd42113c82f56c2a6a40 Mon Sep 17 00:00:00 2001 From: Dan Guzek Date: Mon, 6 Apr 2015 21:01:52 -0400 Subject: [PATCH 1/4] IniFile fix: checking for false ~= checking for nil false is a perfectly valid setting for one of SM's ini files. The code here needs to ensure that both the key (string) and the value (string, numeric, boolean) read from file exist. It would previously halt if the value was set to false. This fixes that by explicitly checking for nil. --- Themes/_fallback/Scripts/01 IniFile.lua | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Themes/_fallback/Scripts/01 IniFile.lua b/Themes/_fallback/Scripts/01 IniFile.lua index c01743dd4d..e99c918e46 100644 --- a/Themes/_fallback/Scripts/01 IniFile.lua +++ b/Themes/_fallback/Scripts/01 IniFile.lua @@ -42,7 +42,7 @@ local RageFile = IniFile = { StrToKeyVal = function( str ) - local _, _, key, value = str:find( "(.+)=(.*)" ) + local key, value = str:match( "(.+)=(.*)" ) -- key is always a string, but value may be num, bool, or nil. -- do a few quick checks to see which one it is. @@ -80,7 +80,7 @@ IniFile = --ignore comments. if not str:find("^%s*#") then -- is this a section? - local _, _, sec = str:find( "%[(.+)%]" ) + local sec = str:match( "%[(.+)%]" ) -- if so, set focus there; otherwise, try to -- read a key/value pair (ignore blank lines) @@ -91,7 +91,7 @@ IniFile = --Warn( "Switching section to " .. sec ) else local k, v = IniFile.StrToKeyVal( str ) - if k and v then current[k] = v end + if k and v ~= nil then current[k] = v end end end end From f0e46ecb492e81afe52ecd7a57a86c74920e0861 Mon Sep 17 00:00:00 2001 From: Dan Guzek Date: Mon, 6 Apr 2015 21:08:12 -0400 Subject: [PATCH 2/4] fix ThemePrefs The ThemePrefs table contains several functions and single standalone variable, NeedsSaving. When a themer calls ThemePrefs.Save() the system will check the status of NeedsSaved to determine whether it really needs to save or not. This is by design to prevent unnecessary writing to disk. As far as I am able to discern, NeedsSaved would never be anything but false due to scoping within the ThemePrefs table. This commit properly scopes that variable as it used within the functions. It looks like ThemePrefs.ForceSave() was added as a workaround; it didn't check the status of NeedsSaved and just always saved, "no matter what." Though it seems safe to outright remove ForceSave(), I'm leaving it in for compatibility. Consider it deprecated I suppose. --- Themes/_fallback/Scripts/02 ThemePrefs.lua | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/Themes/_fallback/Scripts/02 ThemePrefs.lua b/Themes/_fallback/Scripts/02 ThemePrefs.lua index dec746da7f..3efbfe94bd 100644 --- a/Themes/_fallback/Scripts/02 ThemePrefs.lua +++ b/Themes/_fallback/Scripts/02 ThemePrefs.lua @@ -79,6 +79,7 @@ ThemePrefs = -- Only read from disk once, when _fallback calls this; we just -- need the base set once to add prefs onto. Init = function( prefs, bLoadFromDisk ) + -- If we don't have IniFile, we can't read/write from/to disk if not IniFile then Warn( GetString("IniFileMissing") ) end @@ -99,7 +100,7 @@ ThemePrefs = -- if the key doesn't exist, add it with our default value for k, tbl in pairs(prefs) do - if not PrefsTable[section][k] then + if PrefsTable[section][k] == nil then Trace( k .. " doesn't exist, creating" ) PrefsTable[section][k] = tbl.Default end @@ -116,16 +117,17 @@ ThemePrefs = Save = function() -- Trace( "ThemePrefs.Save" ) - if not IniFile then return false end - if not NeedsSaved then return end - NeedsSaved = false - IniFile.WriteFile( ThemePrefsPath, PrefsTable ) + if IniFile and ThemePrefs.NeedsSaved then + IniFile.WriteFile( ThemePrefsPath, PrefsTable ) + ThemePrefs.NeedsSaved = false + return + end end, -- for when you absolutely have to save, no matter what NeedsSaved says. ForceSave = function() if not IniFile then return false end - NeedsSaved = false + ThemePrefs.NeedsSaved = false IniFile.WriteFile( ThemePrefsPath, PrefsTable ) end, @@ -140,7 +142,11 @@ ThemePrefs = Set = function( name, value ) --Trace( ("ThemePrefs.Set(%s, %s)"):format(name, tostring(value)) ) local tbl = ResolveTable(name) - if tbl then tbl[name] = value; NeedsSaved = true; return end + if tbl then + ThemePrefs.NeedsSaved = true + tbl[name] = value + return + end Warn( "Set: "..GetString("UnknownPreference"):format(name) ) end, }; From ce8669f0491cf6bdd691d46918a42e3172460dab Mon Sep 17 00:00:00 2001 From: Dan Guzek Date: Mon, 6 Apr 2015 21:18:47 -0400 Subject: [PATCH 3/4] default theme doesn't need ThemePrefs.ForceSave() As noted in my previous commit message, I think ThemePrefs.ForceSave() can safely be deprecated. Since many novice themers look to default when learning, I think it's better to use ThemePrefs.Save() as the system was originally designed. I moved the ThemePrefs.Save() call out of ScreenTitleMenu decoration to ScreenOptionsService in. This seems safer to me; Pay/Freeplay never hit ScreenTitleMenu which could be another reason ThemePrefs would never get saved. Additionally, moving the call to ScreenOptionsService in also seems to have fixed a bug where switching themes via the Appearance Options service menu would cause the new theme to inherit all of the previous theme's ini settings. I'm still not entirely sure why. I've done a fair amount of testing with this. I've tried deleting the ThemePrefs.ini file outright and booting fresh into both default and Simply Love. In both cases, the ThemePrefs.ini file is written when StepMania exits or when the user hit ScreenOptionsService, whichever comes first. I've tried switching between various themes, including those that did not use ThemePrefs. I haven't seen any sections being created erroneously and themes that do use ThemePrefs don't seem to inherit the previous theme's settings any longer. Still, it *very* possible that I've missed some edge case(s) and is probably worth a few people testing before merging. --- .../default/BGAnimations/ScreenOptionsService in.lua | 5 +++++ .../BGAnimations/ScreenTitleMenu decorations.lua | 11 ----------- 2 files changed, 5 insertions(+), 11 deletions(-) create mode 100644 Themes/default/BGAnimations/ScreenOptionsService in.lua diff --git a/Themes/default/BGAnimations/ScreenOptionsService in.lua b/Themes/default/BGAnimations/ScreenOptionsService in.lua new file mode 100644 index 0000000000..c5355f0dde --- /dev/null +++ b/Themes/default/BGAnimations/ScreenOptionsService in.lua @@ -0,0 +1,5 @@ +return Def.Actor{ + StartTransitioningCommand=function(self) + ThemePrefs.Save() + end +} \ No newline at end of file diff --git a/Themes/default/BGAnimations/ScreenTitleMenu decorations.lua b/Themes/default/BGAnimations/ScreenTitleMenu decorations.lua index f2288395f7..e942af2272 100644 --- a/Themes/default/BGAnimations/ScreenTitleMenu decorations.lua +++ b/Themes/default/BGAnimations/ScreenTitleMenu decorations.lua @@ -2,17 +2,6 @@ InitUserPrefs(); local t = Def.ActorFrame {} -t[#t+1] = Def.ActorFrame { - OnCommand=function(self) - if not FILEMAN:DoesFileExist("Save/ThemePrefs.ini") then - Trace("ThemePrefs doesn't exist; creating file") - ThemePrefs.ForceSave() - end - - ThemePrefs.Save() - end; -}; - t[#t+1] = StandardDecorationFromFileOptional("Footer","Footer"); t[#t+1] = StandardDecorationFromFileOptional("Logo","Logo"); t[#t+1] = StandardDecorationFromFileOptional("VersionInfo","VersionInfo"); From 3d7cb7305b6ffb75ab35a74b86f4d33784221ef5 Mon Sep 17 00:00:00 2001 From: Dan Guzek Date: Mon, 6 Apr 2015 21:21:23 -0400 Subject: [PATCH 4/4] change broadcasting of ThemePref changes I moved a MESSAGEMAN:Broadcast() call so that it only broadcasts the setting that has been saved. This seems more sensible to me, but it is possible I'm misinterpreting the original design here. Feel free to merge or not merge this commit. :) --- Themes/_fallback/Scripts/02 ThemePrefsRows.lua | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/Themes/_fallback/Scripts/02 ThemePrefsRows.lua b/Themes/_fallback/Scripts/02 ThemePrefsRows.lua index 21ad8973c2..d6e00381bb 100644 --- a/Themes/_fallback/Scripts/02 ThemePrefsRows.lua +++ b/Themes/_fallback/Scripts/02 ThemePrefsRows.lua @@ -57,8 +57,11 @@ local function DefaultSave( pref, choices, values ) return function(self, list, pn) for i=1, #choices do - if list[i] then ThemePrefs.Set( pref, values[i] ) break end - MESSAGEMAN:Broadcast( msg, params ) + if list[i] then + ThemePrefs.Set( pref, values[i] ) + MESSAGEMAN:Broadcast( msg, params ) + break + end end end end