Skip to content

Feat: Remove popups when configs.xml or presets.xml have invalid values. Just notify on configs.xml#10

Merged
DaymareOn merged 2 commits intoDaymareOn:mainfrom
KaninHop:fix/silent-preset-mismatch-warn-config
Apr 24, 2026
Merged

Feat: Remove popups when configs.xml or presets.xml have invalid values. Just notify on configs.xml#10
DaymareOn merged 2 commits intoDaymareOn:mainfrom
KaninHop:fix/silent-preset-mismatch-warn-config

Conversation

@KaninHop
Copy link
Copy Markdown
Contributor

@KaninHop KaninHop commented Apr 22, 2026

Summary

Replace blocking Debug.MessageBox in tagDefaultValue with a non-blocking Debug.Notification, gated on a new warn flag threaded through getTagValue.

  • loadConfigFilewarn=true: missing tag in configs.xml still surfaces (corner notification), since storeConfigAndSmpReset will rewrite the file with defaults anyway.
  • configMatchesPresetwarn=false: silent. A preset lacking a tag (or being absent entirely) is a normal mismatch signal, not a user-facing error.

Motivation

On startup with only configs.xml present (no preset XMLs, or presets from an older FSMP version missing newer tags), the MCM fired a blocking popup per missing tag — e.g. "This tag isn't set in the loading file: logLevel". The popup was triggered by the preset-match path, where a miss is expected behavior, not an error.

Changes

  • Source/Scripts/FSMPM.psc
    • getTagValue(..., bool warn = false) — new param, forwarded to tagDefaultValue.
    • tagDefaultValue(tag, bool warn = false) — notification instead of message box, only when warn is true.
    • Call sites updated: loadConfigFile passes true, configMatchesPreset passes false.

Test plan

  • Recompile FSMPM.psc.
  • Launch with configs.xml present and no preset XMLs — no popup on load.
  • Launch with a preset XML missing a tag — no popup, mismatch still detected (preset not marked as loaded).
  • Remove a tag from configs.xml manually — corner notification appears naming the tag and default; storeConfigAndSmpReset writes the tag back.
  • Load a valid preset via MCM — preset applies, no spurious notifications.

AI Notice

  • used Claude to make the change, but I reviewed and understand the code.

Summary by CodeRabbit

  • Bug Fixes
    • Improved notification handling for missing configuration tags. Configuration file loading displays warnings when tags are absent, while preset matching handles missing tags silently. Enhanced user experience with non-blocking notifications.

KaninHop and others added 2 commits April 21, 2026 18:40
Replace blocking MessageBox in tagDefaultValue with a non-blocking
Notification, gated on a new warn flag. loadConfigFile passes warn=true
so a missing tag in configs.xml still surfaces; configMatchesPreset
passes warn=false since tag gaps in presets are expected during the
match check and should not interrupt the player.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 22, 2026

📝 Walkthrough

Walkthrough

The FSMPM.psc script was updated to add an optional warn parameter to getTagValue and tagDefaultValue functions, enabling callers to control whether debug notifications are displayed when configuration tags are missing. The notification behavior is now conditional rather than unconditional.

Changes

Cohort / File(s) Summary
Logging Configuration
Source/Scripts/FSMPM.psc
Extended getTagValue and tagDefaultValue function signatures with optional warn parameter to conditionally emit debug notifications when tags are absent; loadConfigFile passes warn=true while configMatchesPreset passes warn=false.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

Poem

🐰 A tag's absence need not cry,
With warnings now we choose when to notify—
Some calls demand the gentle note,
While others whisper, softly afloat! ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: removing popups (MessageBox) and replacing them with non-blocking notifications, with the warn flag controlling when notifications appear.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@KaninHop KaninHop marked this pull request as ready for review April 22, 2026 00:48
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
Source/Scripts/FSMPM.psc (1)

473-506: 🧹 Nitpick | 🔵 Trivial

LGTM — clean, minimal change that correctly threads warn through to tagDefaultValue.

Signature additions use optional params (backward-compatible), the notification message accurately attributes the source to configs.xml (only reachable with warn=true from loadConfigFile), and bLastTagFound semantics are preserved so configMatchesPreset still detects mismatches silently.

One minor consideration (non-blocking): if configs.xml is severely out-of-date/corrupt, up to 23 corner notifications could be emitted in quick succession from loadConfigFile. Since storeConfigAndSmpReset() immediately rewrites the file afterwards, you could consider aggregating into a single notification (e.g., collect missing tag names in loadConfigFile and emit one summary). Fine to leave as-is given the rarity of the scenario.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Source/Scripts/FSMPM.psc` around lines 473 - 506, The change is fine as-is:
getTagValue now threads the optional warn flag through to tagDefaultValue so
missing tags log a notification; keep this behavior. If you want to address the
non-blocking concern, modify loadConfigFile to collect missing tag names instead
of calling tagDefaultValue(warn=true) for each missing tag and emit a single
aggregated Debug.Notification after parsing (use getTagValue/tagDefaultValue to
detect missing tags but suppress per-tag notifications), then let
storeConfigAndSmpReset continue to rewrite the file as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@Source/Scripts/FSMPM.psc`:
- Around line 473-506: The change is fine as-is: getTagValue now threads the
optional warn flag through to tagDefaultValue so missing tags log a
notification; keep this behavior. If you want to address the non-blocking
concern, modify loadConfigFile to collect missing tag names instead of calling
tagDefaultValue(warn=true) for each missing tag and emit a single aggregated
Debug.Notification after parsing (use getTagValue/tagDefaultValue to detect
missing tags but suppress per-tag notifications), then let
storeConfigAndSmpReset continue to rewrite the file as before.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 0fe31de2-d4b2-45fd-90a7-6dae6280d689

📥 Commits

Reviewing files that changed from the base of the PR and between 0234f1c and 854eae9.

📒 Files selected for processing (3)
  • Scripts/FSMPM.pex
  • Scripts/FSMPMPlayerScript.pex
  • Source/Scripts/FSMPM.psc

@DaymareOn DaymareOn merged commit 3a4b80a into DaymareOn:main Apr 24, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants