Feature/settings#351
Conversation
erikdarlingdata
left a comment
erikdarlingdata
left a comment
There was a problem hiding this comment.
Review — Unified Settings Window
Solid feature work overall — sensible architecture, good dirty-state handling, defaults are reasonable, value clamping on load is nice. A few real bugs and several smaller issues below.
🔴 Bugs
1. Reset Section mutates MainWindow's shared _appSettings even on Cancel
MainWindow.Settings_Click passes _appSettings by reference into new SettingsWindow(_appSettings). Reset_Click (sections 0 and 1) then writes directly to _settings.QueryStoreSlicerDays = …, etc. — which mutates that same shared instance. If the user clicks Reset Section then Cancel, the in-memory MainWindow._appSettings is left with reset values that were never persisted to disk. ResetAll_Click is safe because it rebinds _settings = fresh;, but section reset isn't.
Fix: stage edits to a local working copy and only assign to _settings (and invoke SettingsSaved) inside Save_Click. Or pass a deep clone into the SettingsWindow ctor.
2. QuerySessionControl.Format.cs — broken indentation after the patch
The diff replaced the trailing braces with:
finally
{
FormatButton.IsEnabled = true;
}
}
}The brace count is correct (closes finally/method/class), but indentation is 20/20/16/12 spaces — clearly a botched edit. Compiles, but ugly and inconsistent with the rest of the file. Re-indent to 12/8/4.
3. SelectComboByTag silently leaves SelectedIndex = -1 on unknown tag
In QueryStoreGridControl, if userSettings.QueryStoreDefaultMetric is a stale or corrupted value, OrderByBox ends up with no selection. The GroupByBox call has a _ => \"query-hash\" fallback, but OrderByBox does not. Add a fallback or assert a default index.
4. Legacy format-settings file is orphaned when FormatOptions already exists
MigrateFormatSettings early-returns if settings.FormatOptions != null, leaving the old perfstudio_format_settings.json on disk forever. Minor, but worth deleting unconditionally when the file is present and either migrated or superseded.
🟡 Concerns
5. AppSettingsService.Load() is called many times per Query Store view
New call sites: QueryStoreGridControl ctor, QueryStoreOverviewControl ctor, QueryStoreHistoryControl.BuildColorMap (called on every history refresh), TimeRangeSlicerControl field initializer, QueryStoreHistoryWindow ctor. Each call hits disk + deserializes JSON. Not catastrophic, but BuildColorMap runs per refresh, which is wasteful. Consider caching AppSettings in-process (with invalidation via SettingsSaved).
6. TimeRangeSlicerControl._activeFilterTag initialized via field initializer
private string _activeFilterTag = AppSettingsService.Load().QueryStoreDefaultTimeRange;Field initializers run before the constructor body and before InitializeComponent — lower risk for a static disk read, but moving into the constructor is more conventional.
7. Reset Section for the Script Options section discards FormatOptions to null then rebuilds rows
Works because BuildScriptOptionsSection uses _settings.FormatOptions ?? new SqlFormatSettings(). Fine — but _isDirty = true is set inside Reset before the user explicitly intends to save, so a Cancel after that leaks intent. (Same bug class as #1.)
8. Hex color input has no validation on save
_colorTextBoxes.Select(tb => tb.Text ?? \"#555555\") persists whatever the user typed. Garbage values silently fall back to gray at draw time via the try { Color.Parse } catch swallow. At least show a red border or block save when any value fails to parse — currently the user gets no feedback.
9. AppSettingsService.MigrateFormatSettings calls Save(settings) inside Load()
A read operation that may write to disk is surprising. If Save throws inside MigrateFormatSettings, the catch swallows it and the old file stays, so the migration retries on every Load. Reasonable in practice but worth a comment.
10. Doc comment for AccuracyRatioDivergenceLimit was dropped in the move
The XML summary above it was removed when the field was relocated to the "App State" block. Re-attach.
🟢 Style nits
SettingsWindow.axaml.csuses tabs; the rest of the project appears to use 4 spaces. Confirm against.editorconfig.SettingsWindow.axamlResetAll/Reset buttons have an extra level of indentation in the source vs. the Save/Cancel buttons (visual only)._colorTextBoxes/_colorPreviewsare rebuilt on every_topDbCountBoxchange — fine, but previous handlers on old TextBoxes are unreferenced and GC'd, not actively detached.
✅ Things done well
- Sensible value clamping on load.
Debug.WriteLineinstead of swallowing — much better than the prior behavior.- Singleton pattern in
Settings_Clickis correct (UI-thread only, no race). - Migration logic is conservative (only fires when
FormatOptions == nulland old file exists). - Default color palette extracted as a single source of truth (
DefaultTopDbColors). - Removing
SqlFormatSettingsService.Saveeliminates the read-modify-write race against the shared file.
The bugs in #1 and #2 are worth addressing before merge; the rest can land as follow-ups.
fixes for remarks on PR#351 - SettingsWindow now clones settings via AppSettings.Clone() (JSON roundtrip) - Re-indented to correct 8/8/4 spacing - Added box.SelectedIndex = 0 fallback - File.Delete now runs unconditionally when the old file exists - Added static _cached field; Save() updates cache; Invalidate() available - Moved _activeFilterTag init into TimeRangeSlicerControl constructor - Fixed by clone approach - Added Color.Parse validation with red border feedback; blocks save on invalid - Added explanatory XML doc comment
|
Re-reviewed commit 375cf3b — looks great. All four 🔴 bugs and three of the four 🟡 concerns are fixed:
Nice work, ready from my side. |
2 participants
What does this PR do?
Add unified Settings window
Summary
Adds a non-modal Settings window (ALT+S) that centralises configuration previously scattered across hardcoded defaults. Settings persist to
%LocalAppData%\PerformanceStudio\appsettings.json.What changed
New files
Dialogs/SettingsWindow.axamlDialogs/SettingsWindow.axaml.csDialogs/FormatOptionRow.csModified files
Services/AppSettingsService.csAppSettingswith Query Store, Multi QS Overview, Query History, and Format Options properties. Added value clamping on load,Debug.WriteLineon deserialization failure, and legacy format-settings migration.Services/SqlFormatSettingsService.csSqlFormatSettingsServicefaçade (dead code after settings were unified).SqlFormatSettingsclass remains.MainWindow.axaml.csSettings_Clickhandler, ALT+S key binding, singleton window management.Deleted files
Dialogs/FormatOptionsWindow.axamlDialogs/FormatOptionsWindow.axaml.csSettings sections
Query Store
Multi QS Overview
Query History
Script Options (Format)
SqlFormatSettingssurface: keyword casing, SQL version, indentation, all newline/multiline/align flags.UI behaviour
ListBoxswitches between sections.SettingsSavedso the main window picks up changes immediately.Bug fixes included
SqlFormatSettingsService.Save(read-modify-write on shared file) by deleting the dead façade.FormatOptionRowchanged frompublictointernal;PropertyInfowas alreadyinternal.AppSettingsService.Load()now logs failures viaDebug.WriteLine.FormatOptionsWindow(both files) and the unusedSqlFormatSettingsServiceclass.Testing
Which component(s) does this affect?
How was this tested?
2026-05-28_00h42_30.mp4
2026-05-28_00h44_31.mp4
Describe the testing you've done. Include:
Checklist
dotnet build -c Debug)dotnet test)