EQ Client Settings overhaul — Phase 2+ (sub-forms on the engine) + v3.24.49 fix-forward#16
Merged
Merged
Conversation
v3.24.48 raised the MaxFPS/MaxBGFPS cap 99->999 in only 2 of 6 places (schema + EQ-Client NUD). AppConfig.Validate, SeedFromIni (x4), and the Process Manager NUD + FpsForDisplay still clamped at 99, so MaxFPS=100 (EQ's stock default) snapped back to 99 on next load / on opening the Process Manager. All cap sites now reference EqClientIniSchema.MaxFpsCap (one source, no Section-F drift). Also: the enforce self-test now proves a Bucket-2 numeric (MaxFPS) left in the ini survives relaunch untouched (eqgame-wins regression lock); the now-vestigial AANoConfirm first-run import was aligned to the corrected schema polarity (inert today - no readers post-Phase-1 - but closes the drift bomb). STEP 0 of the Phase-2 verifier swarm (6 agents Sonnet+Opus on diff 584039c..13c2f30) flagged this as the one shipped regression. Build clean; --test-eqclient-{schema,inidoc,save,enforce} all pass.
|
Warning Review limit reached
More reviews will be available in 7 minutes and 4 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughThis PR unifies FPS cap handling by introducing a shared ChangesFPS Cap Unification
🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
UI/DiagRender.cs (1)
587-590: 💤 Low valueOptional: verify the [Defaults] mirror is also untouched.
The schema defines
MaxFPSwith a[Defaults]mirror, and the fixture seeds only[Options] MaxFPS=123without a corresponding[Defaults]entry. The current assertion verifies[Options]remains"123", provingEnforceOverridesdidn't touch the canonical section.For additional robustness, consider also asserting that
[Defaults] MaxFPSremains absent (or seed both sections in the fixture and verify both are preserved). This would guard against a regression that writes only the mirror section.Optional enhancement
Eq("Options", "MaxFPS", "123"); +// Verify the [Defaults] mirror was not written by EnforceOverrides either: +Eq("Defaults", "MaxFPS", null); // or seed fixture with "[Defaults]\nMaxFPS=123" and assert "123"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@UI/DiagRender.cs` around lines 587 - 590, Add an additional assertion to ensure the [Defaults] mirror for MaxFPS is not created/modified by EnforceOverrides: check that there is no [Defaults] MaxFPS entry (or if you prefer, seed both sections in the fixture and assert both remain unchanged). Specifically, alongside the existing Eq("Options","MaxFPS","123") assertion, add a check referencing the same fixture/schema that verifies the absence (or preserved value) of the [Defaults] MaxFPS mirror to guard against regressions that would write the mirror section; use the same test helpers that produced Eq(...) so the check integrates with the current test flow and EnforceOverrides verification.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Line 9: Update the changelog entry so all CLI flags use their full long-form
names for clarity: replace the shorthand fragments '-inidoc', '-save',
'-enforce' with the long forms '--inidoc', '--save', '--enforce' to match the
existing '--test-eqclient-schema' usage in the same line; ensure the final line
reads something like "Verified: clean build; `--test-eqclient-schema` /
`--inidoc` / `--save` / `--enforce` all pass."
---
Nitpick comments:
In `@UI/DiagRender.cs`:
- Around line 587-590: Add an additional assertion to ensure the [Defaults]
mirror for MaxFPS is not created/modified by EnforceOverrides: check that there
is no [Defaults] MaxFPS entry (or if you prefer, seed both sections in the
fixture and assert both remain unchanged). Specifically, alongside the existing
Eq("Options","MaxFPS","123") assertion, add a check referencing the same
fixture/schema that verifies the absence (or preserved value) of the [Defaults]
MaxFPS mirror to guard against regressions that would write the mirror section;
use the same test helpers that produced Eq(...) so the check integrates with the
current test flow and EnforceOverrides verification.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a51ff9e9-760b-4877-bf6e-71988460dd35
📒 Files selected for processing (7)
CHANGELOG.mdConfig/AppConfig.csConfig/EqClientIniSchema.csEQSwitch.csprojUI/DiagRender.csUI/EQClientSettingsForm.csUI/ProcessManagerForm.cs
|
|
||
| **Hardening** — the launch-enforce self-test now also proves a Bucket-2 numeric (`MaxFPS`) left in the INI survives a relaunch untouched (the "eqgame wins" guarantee), locking the Phase-1 win against regression; and the now-vestigial `AANoConfirm` first-run import was aligned to the corrected polarity. | ||
|
|
||
| Verified: clean build; `--test-eqclient-schema` / `-inidoc` / `-save` / `-enforce` all pass. |
There was a problem hiding this comment.
Use full test flag names to avoid operator confusion.
Line 9 mixes one full flag with shorthand fragments (-inidoc, -save, -enforce), which can mislead copy/paste execution. Use the full flag forms consistently.
Proposed wording update
-Verified: clean build; `--test-eqclient-schema` / `-inidoc` / `-save` / `-enforce` all pass.
+Verified: clean build; `--test-eqclient-schema` / `--test-eqclient-inidoc` / `--test-eqclient-save` / `--test-eqclient-enforce` all pass.📝 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
| Verified: clean build; `--test-eqclient-schema` / `-inidoc` / `-save` / `-enforce` all pass. | |
| Verified: clean build; `--test-eqclient-schema` / `--test-eqclient-inidoc` / `--test-eqclient-save` / `--test-eqclient-enforce` all pass. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` at line 9, Update the changelog entry so all CLI flags use
their full long-form names for clarity: replace the shorthand fragments
'-inidoc', '-save', '-enforce' with the long forms '--inidoc', '--save',
'--enforce' to match the existing '--test-eqclient-schema' usage in the same
line; ensure the final line reads something like "Verified: clean build;
`--test-eqclient-schema` / `--inidoc` / `--save` / `--enforce` all pass."
Extract the Phase-1 binding loop into a shared engine (UI/EqClientBindings: LoadInto / SaveChanged) and migrate the main form + ChatSpam onto it, so the six EQ Client Settings forms can't drift in how they read / snapshot / touch-gate (spec S3 - one engine, not N hand-synced parsers).
EQChatSpamForm was the write-all offender: every Save stamped all 22 filters and EnforceOverrides re-stamped them at launch. Now:
- display = live read of eqclient.ini via the schema (was a config-dict seed),
- Save = touch-gated (only changed filters written to [Options]),
- dropped its launch re-stamp from EQClientSettingsForm.EnforceOverrides (eqgame wins after first set),
- 4 (Key,Label,Default) arrays collapse to grouped key-lists; label/default/polarity now live once in EqClientIniSchema.
Main form refactored onto the shared engine with no behaviour change (--test-eqclient-save still green).
Verify: clean build; --test-eqclient-{schema,inidoc,save,enforce,chatspam-save} all pass; --diag-render-form EQChatSpamForm renders without crash. New --test-eqclient-chatspam-save proves the touch-gate (absent/untouched keys NOT inserted = no write-all; unmanaged preserved; no ghost).
Owner
Author
|
Progress checkpoint (2026-06-06):
Build clean; all 5 |
EQModelsForm migrated onto the shared EqClientBindings engine: live-read display, touch-gated save (only changed models), launch re-stamp dropped from EnforceOverrides (eqgame wins after first set). 32 (Key,Label) entries collapse to grouped key-lists; quick All-Luclin/All-Classic buttons now filter _bindings via RaceKeySet instead of a parallel checkbox dict. ModelOverrides config dict + _initialValues snapshot retired (engine's per-binding snapshot replaces them). Verify: clean build; all 5 --test-eqclient-* pass (no regression); --diag-render-form EQModelsForm renders (validates all 32 keys resolve in the schema).
EQKeymapsForm migrated onto the shared EqClientBindings engine via the numeric path (EQ scan codes are decimals:0 numerics; modifier flags in the high bits are opaque to the engine). The live-decoded key-name label stays as cosmetic UI driven by ValueChanged + an explicit post-load sync. Keymaps already had no launch re-stamp (was eqgame-wins), so this is a pure consistency wire-up - no behaviour change. _nudValues/_initialValues dicts retired. Verify: clean build; all 5 --test-eqclient-* pass; Keymaps/Models/ChatSpam all render via --diag-render-form.
Extended EqClientBindings with a third control type: TrackBar sliders (Slider + SliderScale; INI float = Slider.Value / scale). EQParticlesForm migrated onto the engine - opacity/density sliders (scale 100 over the schema 0..1 floats), clip/filter/FogScale/LODBias numerics, SameResolution checkbox - all live-read + touch-gated; launch re-stamp dropped from EnforceOverrides (only VideoMode's dict remains, Phase 5). Slider keeps its coarse-but-safe round-trip: a finer on-disk float snapshots to the slider's resolution and is left untouched on save unless dragged. ParticleOverrides config dict now inert. New --test-eqclient-particles-save proves the slider->float write (drag -> 0.750000), an untouched numeric preserved exactly, an absent managed key NOT inserted (touch-gated), unmanaged preserved, no ghost. Verify: clean build; all 6 --test-eqclient-* pass; --diag-render-form EQParticlesForm renders.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Continuation of the EQ Client Settings overhaul. Spec:
docs/specs/2026-06-06-eqclient-settings-overhaul.md. Phase 0+1 shipped as v3.24.48 onmain; this branch carries the fix-forward from the Phase-1 verifier swarm plus Phases 2–10.Safety / no limbo release: releases here are tag-gated — nothing ships until this PR is merged and a
vX.Y.Ztag is pushed.mainstays at the working v3.24.48 the whole time, so a half-finished branch can never become a bad release.Done
584039c..13c2f30. The two high-risk areas (EnforceOverrides narrowing → Operational-only; SaveSettings touch-gating) verified clean. Engine is section-correct, ANSI-preserving, InvariantCulture throughout. One shipped regression found.MaxFPS=100(EQ stock) snapped back to 99 on reload / on opening Process Manager. Unified every cap site to oneEqClientIniSchema.MaxFpsCapconstant. + enforce-test regression lock (a stripped Bucket-2 numeric must survive relaunch) + AANoConfirm seed-polarity alignment. Build clean; all 4--test-eqclient-*pass.In progress / planned
EQChatSpamFormonto the schema engine: register controls → schema keys, live-read display, touch-gated save (it currently writes all keys — the bug), drop its launch re-stamp.ConfiguredKeys.Defaults to EQ-stock + deferred popup.WindowedModeX/YOffsetcanonical →[Defaults].Sub-forms not yet migrated stay fully functional throughout — each phase ends green before the next starts.
Summary by CodeRabbit
Bug Fixes
Refactor
Chores