Addon presets by MichalLabuda · Pull Request #1951 · Return-To-The-Roots/s25client

MichalLabuda · 2026-06-18T09:13:58Z

Summary

Implements addon preset save/load functionality, as requested in issue #665.
Closes #665
Players can now save their current addon configuration to a named .ini file and reload it later, instead of manually re-configuring addons each time.

Changes

New iwAddonPresets.h/.cpp - Two new dialog windows (iwSaveAddonPreset, iwLoadAddonPreset) built on a shared base class (iwAddonPresetsBase). The dialogs show a sortable list of saved presets, a name edit field, and buttons for the primary action and deletion (with confirmation). Double-clicking a row triggers the action directly.
iwAddons - Added Save and Load buttons in a new button row above the existing Apply/Abort row. Window height increased from 500 to 530 px to accommodate the extra row. New applyAddonStates() method applies a loaded preset back into the GUI, respecting read-only addons and falling back to defaults for unknown/out-of-range values.
files.h - New folder constant addonPresets pointing to <RTTR_USERDATA>/PRESETS.
const_gui_ids.h - New CGI_ADDON_PRESETS GUI ID for the preset dialogs.

Presets are stored as .ini files (using libsiedler2) in <RTTR_USERDATA>/PRESETS/. The save dialog validates the name (strips path components, rejects Windows reserved device names, prompts before overwrite). Load validates the file format and falls back to addon defaults for any missing entries.

This PR probably also allows to solve issue #587, which reports that addon configurations configured while hosting a game are not preserved - players can now explicitly save a preset before a session and reload it next time.

Edit: Resolves #587

Flamefire

Looks good, thanks!
Just some questions

…eset name

- Replace ad-hoc checks in iwSave/iwAddonPresets with isValidFileName()/isValidFileNameChar() from libutil. - Add fileNameOnly_ mode to ctrlEdit filtering invalid chars on both AddChar() and SetText(). - Fix double-space bug: remove KeyType::Space from Msg_KeyDown, space already arrives via char/text-input event. - Set maxLength=251 on filename edits (255-byte limit minus 4-char extension). - Skip makePortableFileName() sanitization for ServerType::Local+MapType::Savegame to prevent renamed duplicates. - Migrate iwAddonPresets Msg_TableSelectItem from boost::optional to std::optional. - Add unit and integration tests for all new behaviours.

MichalLabuda · 2026-06-30T21:52:08Z

Last push contained:

external/libutil bumped to latest
Replace ad-hoc checks in iwSave/iwAddonPresets with isValidFileName()/isValidFileNameChar() from libutil.
Add fileNameOnly_ mode to ctrlEdit filtering invalid chars on both AddChar() and SetText().
Fix double-space bug: remove KeyType::Space from Msg_KeyDown, space already arrives via char/text-input event.
Set maxLength=251 on filename edits (255-byte limit minus 4-char extension).
Skip makePortableFileName() sanitization for ServerType::Local+MapType::Savegame to prevent renamed duplicates.
Migrate iwAddonPresets Msg_TableSelectItem from boost::optional to std::optional.
Add unit and integration tests for all new behaviours.

Flamefire

Ok, thanks. Most comments are pretty much about the quality of the AI code. I tried to provide enough reasoning so you can catch that for future PRs too

- Replace `numberOnly_`/`fileNameOnly_` booleans with `EditType` enum and `SetType()` - Add `ctrlEdit::GetFileName(ext)` - trims, appends extension, validates, returns `GetFileNameResult` - Remove duplicated `GetSaveFilePath()` from `iwSave`/`iwAddonPresets`; use `GetFileName()` instead - Distinct error messages for empty vs. invalid filename in `iwSave`/`iwAddonPresets` - Update `dskOptions`/`iwTrade` callers to `SetType(EditType::Number)` - Move filename unit tests to `testControls.cpp`; add `AddonPresetSaveLoadAndOverwrite` integration test

- Close preset window together with iwAddons - Don't throw from preset window ctor if folder creation fails - Expose msgbox IDs as class constants, use in test - Free leaked overwrite msgbox in test - Reword test comments

MichalLabuda · 2026-07-14T02:01:41Z

Last few commits contained:

ctrlEdit: replaced bool flags with an EditType enum (Text/Number/Filename)
added ctrlEdit::GetFileName(): trims, optionally appends missing extension, validates - replaces the duplicated GetSaveFilePath() in iwSave/iwAddonPresets
RTTR_Assert that GetFileName() is only called on Filename edits
preset window now closes together with the addon window
preset folder creation no longer throws from the window ctor
tests:
- freed leaked overwrite msgbox, msgbox IDs as constants
- removed the iwSave filename test and the preset extension/length test (behavior moved into GetFileName(), unit-tested by EditGetFileName in testControls.cpp)
- new AddonPresetSaveLoadAndOverwrite integration test
- comment cleanups

Flamefire

Small polishing only. I would have used a std::optional but I guess GetFileNameResult is more explicit, so good idea there!

BTW: Please don't resolve conversations during a review. I usually use them to check what I reviewed before and how that was implemented. If you wan't it to keep track for yourself you could e.g. add a thumbs-up reaction for the done-comments.
Exceptions are where the comment is a simple suggestion with not further text which you applied as-is. GitHub auto-resolves those anyway when using the UI.

- Name edit, not table selection, is the action target - GetSelectedFilePath -> GetTargetFilePath/GetTargetFileOrNotify - Fail-closed if presets folder can't be created - ctrlEdit::GetFileName always appends ext - ListDir: guard on is_directory() - New fixture + tests for the above

MichalLabuda · 2026-07-16T18:13:52Z

@Flamefire I think I addressed all of your recent comments except for the addons window modal/non-modal in which I await your reply.

Last commits contained:

Resolve the target preset from the name edit, not the table selection,
so a typed/edited name wins over a stale selection (edit = source of truth)
Replace GetSelectedFilePath() with GetTargetFilePath() (name -> existing
file, else empty) and GetTargetFileOrNotify() (shows "Preset Not Found"
for a non-empty unresolved name; empty name is a silent no-op)
Create the presets folder in the ctor before building controls; on failure
show "Addon Presets Unavailable" and close the window instead of only logging
Move Msg_TableChooseItem (double-click acts) into the base class
Move control/msgbox IDs into a class enum; rename ID_msgbox* to ID_mb*
iwSaveAddonPreset: switch over FileNameStatus; return early on save success
LoadPresetsFromFile: log non-text entries and unparseable options
ctrlEdit::GetFileName: always append a non-empty ext (a name already ending
in it becomes a distinct ".ini.ini" file); drop now-unused includes
ListDir: guard on is_directory() rather than exists()
Tests: add AddonPresetFixture and cases for distinct-extension names,
edit-overrides-selection, delete + named confirmation, target-not-found,
empty-name no-op, and folder-unavailable

MichalLabuda · 2026-07-17T14:10:59Z

Corrected the clang-format and GCC issues that my MSVC build didn't catch. I've set up a local Linux dev env with clang-format, clang-tidy, and the gcc build at the same versions CI uses, so I should be able to catch these kinds of errors before pushing in the future.

Flamefire · 2026-07-19T12:06:18Z

I've set up a local Linux dev env with clang-format, clang-tidy, and the gcc build at the same versions CI uses, so I should be able to catch these kinds of errors before pushing in the future.

There are static clang-tools binaries also for Windows if you work rather there: https://github.com/muttleyxd/clang-tools-static-binaries/releases
No real need to test with all compilers if that is much work as that's what CI is for. At least for cases that are not easy to spot. So don't worry too much about that :)

The only genuine semantic trade-off is newest-first surfacing of independent modal bursts, which in practice means rare multi-MissionStatement scripts read in reverse; everything else is neutral or an improvement.

@Spikeone I'd say it makes sense that the last opened modal-window will be the top window as I'd say this is the most intuitive. Lua-msg-windows (mission statements) pause the game so I don't really see where this would cause an issue there. Could there even be a situation with multiple statements in a GF? Do we have any? Would it be an issue if they get reversed, i.e. newest/last-added first?
If not I'd say we do make the change. Especially e.g. for the case like: 1. Show progress/wait/settings window 2. show an error message --> You do indeed want the last one first/top.

Spikeone · 2026-07-19T16:27:13Z

@Spikeone I'd say it makes sense that the last opened modal-window will be the top window as I'd say this is the most intuitive. Lua-msg-windows (mission statements) pause the game so I don't really see where this would cause an issue there. Could there even be a situation with multiple statements in a GF? Do we have any? Would it be an issue if they get reversed, i.e. newest/last-added first?
If not I'd say we do make the change. Especially e.g. for the case like: 1. Show progress/wait/settings window 2. show an error message --> You do indeed want the last one first/top.

I agree that this is what I'd expect.

- WindowManager: new modal goes on top, non-modal behind all modals - iwAddonPresets, iwAddons: construct as modal windows - GetTargetFilePath: match preset by table's displayed name - GetTargetFileOrNotify: now const - iwSaveAddonPreset: overwrite prompt names the preset - iwAddons: Show instead of ReplaceWindow for preset windows - ctrlEdit::GetFileName: only trim trailing space when no ext given - ListDir: revert to exists() check, drop is_directory() requirement - iwSave::SaveLoad: switch on FileNameStatus - Tests for modal stacking, mouse blocking, and preset matching

MichalLabuda · 2026-07-24T00:11:20Z

My latest commit includes:

New modal windows now appear on top; non-modal windows stay behind all modals
Addon presets and Addons windows are now modal
Preset lookup now matches by the table's displayed name
Overwrite prompt now names the preset being overwritten
Addons uses Show instead of ReplaceWindow for preset windows
ctrlEdit trims trailing whitespace only when no extension is appended
ListDir reverts to an exists() check
iwSave switches on FileNameStatus
Added tests covering modal stacking, mouse blocking, and preset matching

MichalLabuda · 2026-08-10T17:30:19Z

@Flamefire I think all the comments are addressed. Any chance for a review?

Flamefire

Ok thanks!
Just 2 more changes, one optional to not introduce another instance of addonGuis_[i]->setStatus(*GetCtrl<ctrlGroup>(ID_grpAddonsStart + i), status):
the assumption there is that the same group/window is always passed as used in the constructor. We should instead store a reference to it and not pass it again. Also replace the existing usages too.

I'd like to see the Get[Target]FilePath(name) change to avoid the hidden dependency on GetCtrl<ctrlEdit>(ID_edtName) and make it clear why clearing that edit afterwards makes sense. Or what do you think?

Flamefire · 2026-08-14T17:44:11Z

+    bfs::path path = GetTargetFilePath();
+    if(!path.empty())
+        return path;
+
+    const std::string name = GetCtrl<ctrlEdit>(ID_edtName)->GetText();
+    if(name.empty())
+        return {};


This is confusing. I had to check multiple times in which situation name and/or path can be empty. I think it is better to pass the name to GetTargetFilePath()
The only other usage is the deletion in the msgbox result where you then do GetCtrl<ctrlEdit>(ID_edtName)->SetText("")

If you get the text from there first that line makes much more sense.
I'd also assert that only non-empty names are passed to GetTargetFilePath so an empty result is always: name not found in table.
I guess GetFilePath(name) is then enough: "Convert" a name to a path using the table.

Flamefire · 2026-08-14T17:57:38Z

+            const auto it = states.find(static_cast<unsigned>(addon->getId()));
+            const unsigned rawStatus = (it != states.end()) ? it->second : addon->getDefaultStatus();
+            const unsigned status = (rawStatus < addon->getNumOptions()) ? rawStatus : addon->getDefaultStatus();
+            addonGuis_[i]->setStatus(*GetCtrl<ctrlGroup>(ID_grpAddonsStart + i), status);


This looks odd, although it already exists...
Can you do a small cleanup commit and store the window/group in AddonGui and remove it from the set/getStatus functions?
Avoids the error-prone use of ID_grpAddonsStart +i in multiple cases. We'd need a getWindow (or so) function for UpdateView then.

MichalLabuda added 2 commits June 18, 2026 10:54

Addon presets

2dae906

Fix load/save addon preset window height

903624e

Flamefire requested changes Jun 18, 2026

View reviewed changes

Comment thread libs/s25main/ingameWindows/iwAddonPresets.cpp Outdated

Comment thread libs/s25main/ingameWindows/iwAddonPresets.cpp Outdated

Comment thread libs/s25main/ingameWindows/iwAddonPresets.cpp Outdated

Fix LoadPresetsFromFile name, explicitly reject path separators in pr…

8ca030e

…eset name

MichalLabuda requested a review from Flamefire June 22, 2026 14:07

Flamefire reviewed Jun 23, 2026

View reviewed changes

Comment thread libs/s25main/ingameWindows/iwAddonPresets.cpp Outdated

Merge remote-tracking branch 'origin/master' into addon-presets

79b516a

MichalLabuda mentioned this pull request Jun 27, 2026

Add reserved name handling and add user filename validation Return-To-The-Roots/libutil#42

Merged

MichalLabuda added 3 commits June 30, 2026 14:28

Bump libutil submodule to latest master

15e9bd2

Merge remote-tracking branch 'origin/master' into addon-presets

ef88aa5

MichalLabuda requested a review from Flamefire June 30, 2026 21:52

Flamefire requested changes Jul 1, 2026

View reviewed changes

MichalLabuda added 5 commits July 13, 2026 23:45

Merge remote-tracking branch 'origin/master' into addon-presets

133b0dd

ctrlEdit: cover case-insensitive ext test

1b1679f

ctrlEdit: assert Filename type in GetFileName()

2cc6a38

Fix addon preset window lifetime issues

aaa399f

- Close preset window together with iwAddons - Don't throw from preset window ctor if folder creation fails - Expose msgbox IDs as class constants, use in test - Free leaked overwrite msgbox in test - Reword test comments

MichalLabuda requested a review from Flamefire July 14, 2026 02:01

Flamefire requested changes Jul 14, 2026

View reviewed changes

MichalLabuda added 2 commits July 16, 2026 03:08

Test selection/deselection driving addon preset name edit

b6cdb7f

MichalLabuda requested a review from Flamefire July 16, 2026 18:14

MichalLabuda added 2 commits July 16, 2026 20:54

Merge branch 'master' into addon-presets

e71bbc8

Fix GCC -Werror build and apply clang-format

997c45d

Flamefire requested changes Jul 19, 2026

View reviewed changes

Comment thread libs/s25main/ingameWindows/iwAddonPresets.cpp

Comment thread tests/s25Main/UI/testWindows.cpp Outdated

Comment thread libs/s25main/ListDir.cpp Outdated

MichalLabuda added 2 commits July 24, 2026 01:22

Merge branch 'master' into addon-presets

6e5f30a

MichalLabuda requested a review from Flamefire July 24, 2026 00:24

Merge branch 'master' into addon-presets

5ae47af

Flamefire requested changes Aug 14, 2026

View reviewed changes

Uh oh!

Conversation

MichalLabuda commented Jun 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Uh oh!

Flamefire left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MichalLabuda commented Jun 30, 2026

Uh oh!

Flamefire left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MichalLabuda commented Jul 14, 2026

Uh oh!

Flamefire left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MichalLabuda commented Jul 16, 2026

Uh oh!

MichalLabuda commented Jul 17, 2026

Uh oh!

Flamefire commented Jul 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Spikeone commented Jul 19, 2026

Uh oh!

MichalLabuda commented Jul 24, 2026

Uh oh!

MichalLabuda commented Aug 10, 2026

Uh oh!

Flamefire left a comment

Choose a reason for hiding this comment

Uh oh!

Flamefire Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

Flamefire Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MichalLabuda commented Jun 18, 2026 •

edited

Loading

Flamefire commented Jul 19, 2026 •

edited

Loading