Skip to content

Update SaveManager.lua #64

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.

Sign up for GitHub

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
ATGFAIL wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update SaveManager.lua #64

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
51 changes: 33 additions & 18 deletions Addons/SaveManager.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,28 +292,43 @@ local SaveManager = {} do
SaveManager.Options.SaveManager_ConfigList:SetValue(nil)
end})

local AutoloadButton
AutoloadButton = section:AddButton({Title = "Set as autoload", Description = "Current autoload config: none", Callback = function()
local name = SaveManager.Options.SaveManager_ConfigList.Value
writefile(self.Folder .. "/settings/autoload.txt", name)
AutoloadButton:SetDesc("Current autoload config: " .. name)
self.Library:Notify({
Title = "Interface",
Content = "Config loader",
SubContent = string.format("Set %q to auto load", name),
Duration = 7
})
end})
local Toggle = section:AddToggle("AutoloadToggle", {
Title = "Auto Load Config",
Default = false,
Description = "Toggle to set/unset autoload config"
})

Toggle:OnChanged(function(state)
local name = SaveManager.Options.SaveManager_ConfigList.Value
local autoloadPath = self.Folder .. "/settings/autoload.txt"

if state then
writefile(autoloadPath, name)
self.Library:Notify({
Title = "Interface",
Content = "Config loader",
SubContent = string.format("Set %q to auto load", name),
Duration = 7
})
Comment on lines +301 to +312
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Oct 7, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Validate config selection before writing to autoload.

The handler writes name to the autoload file without verifying that a config is actually selected. If SaveManager_ConfigList.Value is nil or empty, this will write invalid data to autoload.txt, causing LoadAutoloadConfig() to fail silently or load nothing.

Apply this diff to add validation:

 Toggle:OnChanged(function(state)
     local name = SaveManager.Options.SaveManager_ConfigList.Value
     local autoloadPath = self.Folder .. "/settings/autoload.txt"
 
     if state then
+        if not name or name == "" then
+            self.Library:Notify({
+                Title = "Interface",
+                Content = "Config loader",
+                SubContent = "Please select a config first",
+                Duration = 7
+            })
+            Toggle:SetValue(false)
+            return
+        end
         writefile(autoloadPath, name)
🤖 Prompt for AI Agents 
In Addons/SaveManager.lua around lines 301 to 312, the Toggle OnChanged handler
writes SaveManager.Options.SaveManager_ConfigList.Value directly to autoload.txt
without validating it; this can write nil/empty values. Before calling
writefile, check that name is a non-empty string (not nil and not just
whitespace); if invalid, do not write and instead call self.Library:Notify with
an error/warning message indicating no config selected; only proceed to
writefile and show the success notification when validation passes.

else
if isfile(autoloadPath) then
delfile(autoloadPath)
end
self.Library:Notify({
Title = "Interface",
Content = "Config loader",
SubContent = "Autoload has been disabled",
Duration = 7
})
end
end)
Comment on lines +301 to +324
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Oct 7, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

OnChanged fires on initialization, causing unwanted notification.

Based on the Toggle:OnChanged implementation in src/Elements/Toggle.lua (lines 62-65), the handler is invoked immediately with the current toggle value. Since the toggle defaults to false, users will see an "Autoload has been disabled" notification on every UI load, even though autoload was never active in that session.

Consider one of these approaches:

Option 1: Add a flag to suppress the initial notification:

+local initializing = true
 Toggle:OnChanged(function(state)
     local name = SaveManager.Options.SaveManager_ConfigList.Value
     local autoloadPath = self.Folder .. "/settings/autoload.txt"
 
     if state then
         writefile(autoloadPath, name)
         self.Library:Notify({
             Title = "Interface",
             Content = "Config loader",
             SubContent = string.format("Set %q to auto load", name),
             Duration = 7
         })
     else
+        if initializing then
+            initializing = false
+            return
+        end
         if isfile(autoloadPath) then
             delfile(autoloadPath)
         end

Option 2: Initialize the toggle state based on whether autoload.txt exists before registering the handler.


if isfile(self.Folder .. "/settings/autoload.txt") then
local name = readfile(self.Folder .. "/settings/autoload.txt")
AutoloadButton:SetDesc("Current autoload config: " .. name)
end
-- ตั้งค่าเริ่มต้นเป็นปิดไว้ก่อน
Options.AutoloadToggle:SetValue(false)
Comment on lines +295 to +327
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Oct 7, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

Toggle state doesn't reflect actual autoload status on initialization.

The toggle always starts as false, even if autoload.txt exists. This creates a mismatch between the UI state and the actual autoload configuration. Users won't know that autoload is active until they manually check or toggle the control.

Consider initializing the toggle based on file existence:

 local Toggle = section:AddToggle("AutoloadToggle", {
     Title = "Auto Load Config",
-    Default = false,
+    Default = isfile(self.Folder .. "/settings/autoload.txt"),
     Description = "Toggle to set/unset autoload config"
 })

Then register the OnChanged handler. This ensures the UI accurately represents the autoload state.

🤖 Prompt for AI Agents 
In Addons/SaveManager.lua around lines 295-327, the autoload toggle always
initializes to false and uses Options.AutoloadToggle:SetValue(false), causing a
UI/config mismatch; fix by computing local autoloadPath = self.Folder ..
"/settings/autoload.txt" first, check isfile(autoloadPath) and set the toggle
state accordingly via Toggle:SetValue(true) if the file exists (or
Toggle:SetValue(false) if not), and only then register the Toggle:OnChanged
handler (i.e., move the OnChanged hookup after setting the initial value) so the
UI reflects actual autoload status on startup and you use the correct Toggle
reference instead of Options.AutoloadToggle.

Comment on lines +326 to +327
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Oct 7, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Remove redundant SetValue call.

Line 327 calls SetValue(false) after the OnChanged handler has already been registered. Since the toggle's Default is already false (line 297) and OnChanged invokes the handler immediately, this second call is unnecessary and will trigger the handler again, potentially showing duplicate notifications.

Apply this diff to remove the redundant call:

 end)
 
--- ตั้งค่าเริ่มต้นเป็นปิดไว้ก่อน
-Options.AutoloadToggle:SetValue(false)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
-- ตั้งค่าเริ่มต้นเป็นปิดไว้ก่อน
Options.AutoloadToggle:SetValue(false)
end)
🤖 Prompt for AI Agents 
In Addons/SaveManager.lua around lines 326-327, the call
Options.AutoloadToggle:SetValue(false) is redundant because Default is already
false and the OnChanged handler triggers immediately when registered; remove
this SetValue(false) invocation to avoid invoking the handler twice (thus
preventing duplicate notifications) and keep only the Default configuration and
the OnChanged registration.


SaveManager:SetIgnoreIndexes({ "SaveManager_ConfigList", "SaveManager_ConfigName" })
end

SaveManager:BuildFolderTree()
end

return SaveManager
return SaveManager