Addon presets#1951
Conversation
Flamefire
left a comment
Flamefire
left a comment
There was a problem hiding this comment.
Looks good, thanks!
Just some questions
- 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.
|
Last push contained:
|
Flamefire
left a comment
Flamefire
left a comment
There was a problem hiding this comment.
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
| // For local savegame loads the filename comes from our own server and is already a valid | ||
| // filesystem name — sanitizing it (spaces -> underscores) would create a renamed duplicate | ||
| // alongside the original save file. | ||
| const std::string portFilename = |
There was a problem hiding this comment.
I actually don't agree with this one, do we really need a special case? Does makePortableFileName modify a name accepted by isValidFilename? If so why was this not a problem before?
There was a problem hiding this comment.
I guess people just accepted the way it worked or were used to saving games with very short names (since the column was so narrow anyway) consisting only of letters and numbers. However, if for your save game filename, you use spaces, brackets or any other valid filename character that is replaced with underscore by makePortableFileName, then you get a duplicate filename when you load that save game. Examples below.
- 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
|
Last few commits contained:
|
Flamefire
left a comment
Flamefire
left a comment
There was a problem hiding this comment.
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.
|
|
||
| void iwSaveAddonPreset::Msg_TableChooseItem(const unsigned /*ctrl_id*/, const unsigned /*selection*/) | ||
| { | ||
| DoAction(); |
There was a problem hiding this comment.
This can be in the base class can't it?
| : IngameWindow(CGI_ADDONS, IngameWindow::posLastOrCenter, Extent(700, 500), _("Addon Settings"), | ||
| LOADER.GetImageN("resource", 41), true, CloseBehavior::Custom, parent), | ||
| : IngameWindow(CGI_ADDONS, IngameWindow::posLastOrCenter, Extent(700, 530), _("Addon Settings"), | ||
| LOADER.GetImageN("resource", 41), false, CloseBehavior::Custom, parent), |
There was a problem hiding this comment.
Why did you make this non-Modal? The reason here was that you have to choose an action (Apply or Discard)
There was a problem hiding this comment.
The problem was that WindowManager::DoShow inserts every new window (including modal) behind all previously open modals (FIFO for modals). So, modal iwAddons trapped its own Save/Load preset child behind it - no input, uncloseable directly. Making iwAddons non-modal was the workaround.
However, I'd rather fix that than keep the workaround, which has a real bug: with both windows non-modal you can open the Save preset window, change addon settings afterwards, and pressing Save then writes the settings captured when the window was opened, not the current ones.
Simplest fix but app-wide change: LIFO modal stacking - in DoShow, insert modal windows at windows.end() (non-modals keep current behavior). Newest modal is topmost, closing it reveals the previous one. This flips the FIFO ordering tested in ModalWindowPlacement so this test would have to be rewritten unless there was some reason behind the intentional FIFO stacking that I don't see. With this change addons and presets windows could be made modal.
I asked the AI (Fable 5) to perform a deep analysis for potential breaking consequences of changing the stacking model from FIFO to LIFO, full analysis report is at the bottom of this message. In general it found that one change would be in case when there would be multiple MissionStatements from LUA open at once (a same-frame burst, or in unpaused multiplayer a new one arriving while an earlier is still open) - then messages would be in reverse order (newest first). Second change would be possible focus-stealing if some async modal would pop-up. Positive change, apart from the modal windows chaining required for addons presets, would be the reduced deadlock potential.
So in general, I would be willing to introduce this change and update the ModalWindowPlacement test accordingly.
Alternatives I considered:
- an optional
openerargument onShow(), used once at insertion to place a child modal on top of its opener - keeps FIFO for unrelated modals, but adds API surface and every spawning site must remember to opt in. Window::parent_can't be used to set parent window because ofGetDrawPos()parent relative position calculation. Using this member would require a lot of work to redesign this and could quickly get messy.- adding new member
parentWindow_- would help but then it requires some good explanations in the comments why this member is introduced when you already haveparent_available, it just isn't pretty, and as a stored raw pointer it can dangle if the opener closes first. - change nothing, keep iwAddons non-modal - works, but loses the forced Apply/Discard choice and also introduces the stale-settings save bug.
Full report: consequences of FIFO → LIFO modal stacking
TL;DR
LIFO preserves the invariant everything else keys off ("if any modal is open, windows.back() is modal"), so input routing, activation and close handling are untouched; msgbox-result handlers are id-keyed and order-independent. The only semantic change is that independent queued modals surface newest-first — practically relevant only if multiple lua MissionStatements are open at once (a same-frame burst, or in unpaused multiplayer a new one arriving while an earlier is still open; reading order reverses — shipped campaigns show one per event). Bonus: chained windows (msgbox→msgbox, error over iwPleaseWait) currently spend a frame hidden behind the dying modal with the mouse warped onto a hidden button — LIFO makes those immediately correct. Test-wise only ModalWindowPlacement and one assertion in the replace test need updating.
The change
In DoShow (WindowManager.cpp): modal windows get inserted at windows.end() instead of before the first modal. Non-modal windows keep the current rule (inserted before the first modal). One-line change.
What provably does NOT change
The critical invariant — if any modal is open, windows.back() is modal — holds under both schemes, because non-modals are still filed behind all modals. Everything that depends on it was checked and is untouched:
- Input routing:
Msg_LeftDown/Msg_RightDown/findAndActivateWindowonly testwindows.back()->IsModal()— they don't care which modal is at the back. SetActiveWindowImpl: still refuses to activate non-topmost windows while a modal exists; under LIFO the newly shown modal is topmost, so it activates cleanly.DoClose: activates the newwindows.back()when the topmost closes — this is what unwinds the LIFO stack correctly.ReplaceWindow/ToggleWindow: useFindNonModalWindow, so modals were never replaced/toggled; unchanged.dskGameInterface::SetActive(dskGameInterface.cpp:212): checksGetTopMostWindow()->IsModal()— invariant preserved.- Esc / Alt+W / right-click close: operate on the topmost window; behavior is identical whenever at most one modal is open, which is every case these paths were designed for.
iwMissionStatement's pause logic (iwMissionStatement.cpp:59): pauses on every activation, unpauses on Continue — works under both orders, since each stacked statement re-pauses when it becomes active.- Msgbox result handlers: all
Msg_MsgBoxResultimplementations are keyed bymsgboxidand mutually independent; the ones that switch desktops on acknowledgment destroy all remaining windows anyway (DoDesktopSwitchclears the list), so ack-order reversal can't break logic.
What changes
- The fix itself: a modal opened from a modal is now on top and usable.
- Transient chains get better: msgbox→msgbox chains,
iwConnecting::CI_Error, and dskSelectMap's please-wait→error currently spend one frame with the new window behind the dying modal (andiwMsgboxeven warps the mouse cursor onto its still-hidden default button, iwMsgbox.cpp:91). Under LIFO the new window is immediately topmost — strictly more correct. - Independent async modals surface newest-first instead of oldest-first. Two sub-cases:
- Reading order: the only realistic producer of a modal burst is Lua's
MissionStatement(LuaInterfaceGame.cpp:308) — likely why theModalWindowPlacementtest usesCGI_MISSION_STATEMENT. If a script fires two statements in one game-frame batch, the player now reads them in reverse order. Shipped campaign scripts (data/RTTR/campaigns/roman/*.lua) show one statement per event, and in paused single-player statements can't accumulate at all (no game frames while paused), so the exposure is: multiplayer or multi-event-per-frame scripts, rare. - Focus stealing: an async error msgbox now pops over a modal the user is interacting with (e.g. typing in
iwDirectIPConnect) instead of hiding behind it; a stray Enter could hit the msgbox's default button. This is standard toolkit behavior, and FIFO already half-misbehaves here via the mouse-warp above.
- Reading order: the only realistic producer of a modal burst is Lua's
- Deadlock/soft-lock surface shrinks. Today a msgbox spawned behind a button-less
CloseBehavior::Custommodal (iwPleaseWait) is unreachable until code closes the modal; under LIFO it's clickable immediately. No new lock scenario is introduced: the topmost modal is always the newest, which its creator by definition just chose to show.
Test impact
ModalWindowPlacement(testWindowManager.cpp:399): must be rewritten — the modal segment of the expected order reverses ({wnd3, wnd2, wnd, wnd6, wnd4}-style instead of{wnd, wnd2, wnd3, ...}, with wnd5 topmost).- The "modal windows are not replaced" case (testWindowManager.cpp:385-395): one assertion flips (
REQUIRE_WINDOW_ACTIVE(wnd)→wnd2). EscClosesWindowandRightclickClosesWindowpass unchanged — they never stack two modals.
Verdict
No hidden coupling to FIFO exists anywhere in the input, activation, or close machinery — it all keys off "is the back window modal", which LIFO preserves. 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.
- 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
|
@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:
|
|
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. |
There are static clang-tools binaries also for Windows if you work rather there: https://github.com/muttleyxd/clang-tools-static-binaries/releases
@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? |
| table.DeleteAllItems(); | ||
|
|
||
| for(const auto& file : ListDir(GetPresetsDir(), "ini")) | ||
| table.AddRow({file.stem().string(), file.string()}); |
There was a problem hiding this comment.
Do you still use the 2nd column anywhere?
| base.Msg_EditEnter(0); | ||
| } | ||
|
|
||
| // Returns the states passed to the callback, or empty if it was not invoked (name not found). |
There was a problem hiding this comment.
AI described "how"... The caller doesn't care about any callback. This loads the given preset and returns its settings/values/...
| std::vector<bfs::path> result; | ||
|
|
||
| if(!bfs::exists(path)) | ||
| if(!bfs::is_directory(path)) |
There was a problem hiding this comment.
Not sure, what happens if it is a non-directory without it? Probably an exception, isn't it?
I'd say that is fine as we do not handle the case where we expect a directory where the user put a file.
I actually use a trick based on that in other software: Add a read-only file where an app would place a folder to ensure it doesn't (usually env-vars can be set, this ensures they take effect)
There was a problem hiding this comment.
I changed it because before, when it was only LOG-on-presets-folder-creation-failed-and-continue, the code reached ListDir and it crashed because for testing I added a file named PRESETS. Thought that it would prevent crashes for the future but also would silently allow for someone to miss potential error situation because it just returns empty now. On the other hand it's a protection against the crash if someone missed something... You're choice - for my case in this PR it is no longer necessary as with current state it doesn't even open presets window if folder creation fails.
There was a problem hiding this comment.
On the other hand it's a protection against the crash if someone missed something...
I'm Ok with the crash: If this fires then we are in a situation that is unhandled, like what you just mentioned: We shouldn't try to list a directory which is not a directory and/or a user shouldn't create a file where we expect a directory. I mean: That would need to be a deliberate act, not something that could "just happen"
I agree that this is what I'd expect. |
3 participants
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
.inifile and reload it later, instead of manually re-configuring addons each time.Changes
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. NewapplyAddonStates()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 constantaddonPresetspointing to<RTTR_USERDATA>/PRESETS.const_gui_ids.h- NewCGI_ADDON_PRESETSGUI ID for the preset dialogs.Presets are stored as
.inifiles (usinglibsiedler2) 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