Skip to content

Commit 30255ee

Browse files
carterscodeclaude
andcommitted
fix(verify): publish what Verify finds, and give the user a way to fix it
Clicking "Verify all" reported a drifted setting while the Status page kept showing 0, and there was nothing to click to put the setting back. Two independent causes. Verify never published what it found. It ran its own CheckDrift loop, logged a snapshot and showed a count in a MessageBox, and never touched MonitorService. The Status page reads MonitorService.CurrentDrift, so it kept showing whatever the last background poll published -- up to ten minutes stale for a stable setting. Verify is a full re-read of every monitor, so its findings are now published as a full scan via PublishManualScan. Nothing is applied and no notification is raised: Verify still only reports. Verify and Status were also counting different populations. The background scan filters CheckDrift on IsMonitored; Verify did not, so it counted unmonitored settings too. That is the reported case exactly -- on the reporter's machine all ten drifted settings are unmonitored, so Verify counted ten and Status correctly counted zero. Only monitored drift is published (the Status count is defined as monitored settings), and the window now states the split in words rather than leaving the user to reconcile two numbers. A third defect in the same family, found while fixing these: RecordVerifiedApplies seeded _lastVerified but left the published snapshot alone, so fixing a setting by hand left it in the count until its tier was next scanned. The auto-apply path already dropped verified ids via appliedAndVerified; the manual path did not. Verified applies now leave the snapshot too, which is what makes the new Fix button move the number. Publishing is no longer single-writer, so the read-modify-write takes a lock and the event is raised outside it -- a UI handler that marshals to the dispatcher would otherwise deadlock the next publisher. VerifyResultsWindow replaces the MessageBox: every drifted setting with its current and desired value, where it lives, how it is enforced, and a badge on the unmonitored ones. "Fix these settings" runs the same verified-apply path as the Settings Apply button -- apply, re-read, log, ApplyResultsWindow, and the shared unowned RebootPrompt. Verified by opening the window in the running app, not only by the suite. The construction test deliberately does not claim to cover the row template: with a broken StaticResource in it the test still passed under construction, Measure/Arrange, Show(), and a drained dispatcher queue, because an ItemsControl never generates containers in that host. That limitation is written into the test rather than left implied. 739 stable / 721 beta tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent f2a5010 commit 30255ee

5 files changed

Lines changed: 735 additions & 22 deletions

File tree

src/GamerGuardian/Services/MonitorService.cs

Lines changed: 93 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -93,11 +93,20 @@ public sealed class MonitorService : IDisposable
9393

9494
/// <summary>Published snapshot, and also the accumulator across ticks (each
9595
/// publish replaces it with a freshly built dictionary, so it is never mutated
96-
/// in place). Single-writer: only <see cref="TickAsync"/> publishes, and the
97-
/// <see cref="_running"/> guard means ticks never overlap. Volatile so a reader
98-
/// on another thread sees the newest one.</summary>
96+
/// in place). Volatile so a reader on another thread sees the newest one.
97+
///
98+
/// <para>Three writers, not one: the poll tick, a manual Verify
99+
/// (<see cref="PublishManualScan"/>), and a verified apply
100+
/// (<see cref="RecordVerifiedApplies"/>). The latter two run on the UI thread
101+
/// and can land mid-tick, so every publish takes <see cref="_publishLock"/> —
102+
/// the <see cref="_running"/> guard only serialises ticks against each
103+
/// other.</para></summary>
99104
private volatile IReadOnlyDictionary<string, DriftItem> _publishedDrift = EmptyDrift;
100105

106+
/// <summary>Serialises the read-modify-write in <see cref="Publish"/>. Distinct
107+
/// from <see cref="_lock"/>, which guards the tick re-entrancy flag.</summary>
108+
private readonly object _publishLock = new();
109+
101110
public void SetPaused(bool paused)
102111
{
103112
if (_userPaused == paused) return;
@@ -205,15 +214,43 @@ private void OnDisplaySettingsChanged(object? sender, EventArgs e)
205214
/// Without this, the very first background tick after a manual Apply would
206215
/// misread "no prior verified value" and skip the EXTRESET detection until
207216
/// the second tick after the eventual revert.
217+
///
218+
/// <para>Also drops the verified ids from the published drift snapshot. A
219+
/// setting the user just fixed is not drifting any more, and leaving it in the
220+
/// snapshot meant the Status count kept reporting it until that setting's tier
221+
/// was next scanned — up to ten minutes for a stable setting. The auto-apply
222+
/// path already did this via <c>appliedAndVerified</c>; the manual path did
223+
/// not.</para>
208224
/// </summary>
209225
public void RecordVerifiedApplies(IEnumerable<ApplyResult> results)
210226
{
211227
var now = DateTimeOffset.UtcNow;
228+
var verified = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
212229
foreach (var r in results)
213230
{
214231
if (!r.Verified) continue;
215232
_lastVerified[r.SettingId] = new LastVerified(r.RawAfter, r.After, now);
233+
verified.Add(r.SettingId);
216234
}
235+
236+
if (verified.Count > 0) Publish(previous => WithoutResolved(previous, verified));
237+
}
238+
239+
/// <summary>
240+
/// The snapshot minus the settings just fixed. Pure so it can be unit-tested
241+
/// without timers or the registry, like <see cref="MergeDrift"/>.
242+
///
243+
/// <para>Deliberately not expressed as a <see cref="MergeDrift"/> call: that
244+
/// takes a tier and would either replace the whole snapshot (<c>tier: null</c>)
245+
/// or only the matching tier. This removes ids and keeps everything else,
246+
/// whatever tier it belongs to.</para>
247+
/// </summary>
248+
public static Dictionary<string, DriftItem> WithoutResolved(
249+
IReadOnlyDictionary<string, DriftItem> previous, IReadOnlySet<string> resolvedIds)
250+
{
251+
var merged = new Dictionary<string, DriftItem>(previous, StringComparer.OrdinalIgnoreCase);
252+
foreach (var id in resolvedIds) merged.Remove(id);
253+
return merged;
217254
}
218255

219256
/// <summary>
@@ -449,20 +486,65 @@ private async Task TickAsync(MonitorTier? tier)
449486
///
450487
/// <para>Pure bookkeeping: it observes the scan, it never influences it.</para>
451488
/// </summary>
452-
private void PublishDrift(MonitorTier? tier, List<DriftItem> drifted, HashSet<string> appliedAndVerified)
489+
private void PublishDrift(MonitorTier? tier, List<DriftItem> drifted, HashSet<string> appliedAndVerified) =>
490+
Publish(previous => MergeDrift(previous, tier, drifted, appliedAndVerified));
491+
492+
/// <summary>
493+
/// Swaps in a new snapshot and raises <see cref="DriftChanged"/> when the set of
494+
/// drifted ids actually changed.
495+
///
496+
/// <para>The read-modify-write is locked because a manual Verify or a verified
497+
/// apply can publish from the UI thread while a poll tick is publishing from the
498+
/// timer thread; without it one of the two updates is silently lost. The event is
499+
/// raised <b>outside</b> the lock — a handler that marshals to the dispatcher and
500+
/// blocks would otherwise deadlock the next publisher.</para>
501+
/// </summary>
502+
private void Publish(Func<IReadOnlyDictionary<string, DriftItem>, Dictionary<string, DriftItem>> mutate)
453503
{
454-
var previous = _publishedDrift;
455-
var snapshot = MergeDrift(previous, tier, drifted, appliedAndVerified);
504+
IReadOnlyDictionary<string, DriftItem> snapshot;
505+
bool changed;
506+
lock (_publishLock)
507+
{
508+
var previous = _publishedDrift;
509+
snapshot = mutate(previous);
456510

457-
// A count surface only cares about which ids are drifting, so an unchanged
458-
// id set raises nothing and the UI doesn't re-render on every quiet poll.
459-
bool changed = snapshot.Count != previous.Count
460-
|| !snapshot.Keys.All(previous.ContainsKey);
511+
// A count surface only cares about which ids are drifting, so an
512+
// unchanged id set raises nothing and the UI doesn't re-render on every
513+
// quiet poll.
514+
changed = snapshot.Count != previous.Count
515+
|| !snapshot.Keys.All(previous.ContainsKey);
461516

462-
_publishedDrift = snapshot;
517+
_publishedDrift = snapshot;
518+
}
463519
if (changed) DriftChanged?.Invoke(snapshot);
464520
}
465521

522+
/// <summary>
523+
/// Publishes the findings of a manual, full re-read of every monitor — the
524+
/// "Verify all" button.
525+
///
526+
/// <para>Verify checks every setting, so this is a full scan and replaces the
527+
/// snapshot wholesale, exactly as a <c>tier: null</c> tick does. Nothing is
528+
/// applied and no notification is raised: Verify's contract is that it reports
529+
/// and never changes anything.</para>
530+
///
531+
/// <para>Without this the two surfaces disagreed. Verify ran its own drift check
532+
/// and told the user a setting had drifted, while the Status count kept showing
533+
/// whatever the last poll published — reporting 0 for up to ten minutes after
534+
/// Verify had just said otherwise.</para>
535+
/// </summary>
536+
/// <param name="monitoredDrift">Drifted items that are monitored. Unmonitored
537+
/// settings must not be passed: the published set is what the Status count
538+
/// reports, and that count is defined as monitored settings only.</param>
539+
public void PublishManualScan(IEnumerable<DriftItem> monitoredDrift)
540+
{
541+
var drifted = monitoredDrift.Where(d => d.IsMonitored).ToList();
542+
Publish(previous => MergeDrift(previous, tier: null, drifted, EmptyApplied));
543+
}
544+
545+
private static readonly IReadOnlySet<string> EmptyApplied =
546+
new HashSet<string>(StringComparer.OrdinalIgnoreCase);
547+
466548
/// <summary>
467549
/// The merge rule, pure so it can be unit-tested without timers or the registry
468550
/// (same approach as <see cref="SelectNotifiable"/>). Returns a new dictionary;

src/GamerGuardian/UI/SettingsWindow.xaml.cs

Lines changed: 48 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1935,16 +1935,24 @@ private async Task RunCpuActionAsync(
19351935
}
19361936

19371937
/// <summary>
1938-
/// Re-reads every monitored setting against the committed config and
1939-
/// writes a [SNAPSHOT] entry to changes.log. Nothing is applied. A status
1940-
/// popup tells the user where to look.
1938+
/// Re-reads every setting against the committed config and writes a [SNAPSHOT]
1939+
/// entry to changes.log. Nothing is applied here.
1940+
///
1941+
/// <para>Verify is a full re-read of every monitor, so its monitored findings are
1942+
/// published to the <see cref="MonitorService"/> as a full scan. Without that the
1943+
/// two surfaces contradicted each other: Verify would report a drifted setting
1944+
/// while the Status count kept showing the last poll's number — 0, for up to ten
1945+
/// minutes.</para>
1946+
///
1947+
/// <para>When something has drifted the user gets a list and a way to fix it,
1948+
/// rather than a count in a MessageBox with nowhere to go.</para>
19411949
/// </summary>
19421950
private void VerifyAllButton_Click(object sender, RoutedEventArgs e)
19431951
{
19441952
try
19451953
{
19461954
var rows = new List<(string, string, string, string, bool)>();
1947-
int drifting = 0;
1955+
var drifted = new List<DriftItem>();
19481956
foreach (var m in _monitors)
19491957
{
19501958
IEnumerable<DriftItem> items;
@@ -1953,16 +1961,27 @@ private void VerifyAllButton_Click(object sender, RoutedEventArgs e)
19531961
foreach (var d in items)
19541962
{
19551963
rows.Add((d.SettingId, d.DisplayLabel, d.CurrentValue, d.DesiredValue, false));
1956-
drifting++;
1964+
drifted.Add(d);
19571965
}
19581966
}
19591967
ChangeLogger.LogStateSnapshot(rows);
1960-
var msg = drifting == 0
1961-
? "All monitored settings match your preferences. Snapshot written to changes.log."
1962-
: $"{drifting} setting(s) currently drifting from your preferences. Snapshot written to changes.log -- nothing was applied.";
1963-
System.Windows.MessageBox.Show(this, msg, "GamerGuardian -- Verify all",
1964-
System.Windows.MessageBoxButton.OK,
1965-
drifting == 0 ? System.Windows.MessageBoxImage.Information : System.Windows.MessageBoxImage.Warning);
1968+
1969+
// Only monitored drift is published: the Status count is defined as
1970+
// monitored settings, and Verify checks everything.
1971+
_monitorService?.PublishManualScan(drifted);
1972+
1973+
if (drifted.Count == 0)
1974+
{
1975+
System.Windows.MessageBox.Show(this,
1976+
"Everything matches your preferences. Snapshot written to changes.log.",
1977+
"GamerGuardian -- Verify all",
1978+
System.Windows.MessageBoxButton.OK, System.Windows.MessageBoxImage.Information);
1979+
return;
1980+
}
1981+
1982+
var win = new VerifyResultsWindow(drifted, _monitors, _config, _monitorService) { Owner = this };
1983+
win.Fixed += OnVerifyFixApplied;
1984+
win.ShowDialog();
19661985
}
19671986
catch (Exception ex)
19681987
{
@@ -1971,6 +1990,24 @@ private void VerifyAllButton_Click(object sender, RoutedEventArgs e)
19711990
}
19721991
}
19731992

1993+
/// <summary>Reload the form after a Verify fix — the values on screen were read
1994+
/// before the apply and are now stale.</summary>
1995+
private void OnVerifyFixApplied()
1996+
{
1997+
try
1998+
{
1999+
RebaseDraftFromConfig();
2000+
LoadGlobals();
2001+
LoadDisplays();
2002+
LoadServices();
2003+
LoadWindowsAi();
2004+
LoadCpuTabs();
2005+
UpdatePendingStatus();
2006+
Saved?.Invoke();
2007+
}
2008+
catch { /* a refresh failure must not break the fix that already succeeded */ }
2009+
}
2010+
19742011
private void CancelButton_Click(object sender, RoutedEventArgs e)
19752012
{
19762013
// Discards the draft entirely. _config (and on-disk config.json) are
Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
<ui:FluentWindow x:Class="GamerGuardian.UI.VerifyResultsWindow"
2+
xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
3+
xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
4+
xmlns:ui="http://schemas.lepo.co/wpfui/2022/xaml"
5+
Title="Verify results"
6+
Width="660" Height="600"
7+
MinWidth="520" MinHeight="380"
8+
WindowStartupLocation="CenterOwner"
9+
ResizeMode="CanResize"
10+
ShowInTaskbar="True"
11+
WindowBackdropType="None"
12+
ExtendsContentIntoTitleBar="True"
13+
WindowCornerPreference="Round">
14+
<!-- The window background is not usable here: WPF-UI overwrites
15+
FluentWindow.Background at load to let the backdrop show through, so a
16+
value set in XAML is discarded. Painting the root panel is what actually
17+
puts the palette behind the title bar and the footer. -->
18+
<Grid Background="{DynamicResource ApplicationBackgroundBrush}"
19+
TextElement.Foreground="{DynamicResource TextFillColorPrimaryBrush}">
20+
<Grid.RowDefinitions>
21+
<RowDefinition Height="Auto"/>
22+
<RowDefinition Height="*"/>
23+
</Grid.RowDefinitions>
24+
25+
<ui:TitleBar Grid.Row="0" Title="Verify results">
26+
<ui:TitleBar.Icon>
27+
<ui:SymbolIcon Symbol="ClipboardCheckmark24"/>
28+
</ui:TitleBar.Icon>
29+
</ui:TitleBar>
30+
31+
<DockPanel Grid.Row="1" Margin="20,8,20,16" LastChildFill="True">
32+
<Grid DockPanel.Dock="Bottom" Margin="0,12,0,0">
33+
<Grid.ColumnDefinitions>
34+
<ColumnDefinition Width="Auto"/>
35+
<ColumnDefinition Width="*"/>
36+
<ColumnDefinition Width="Auto"/>
37+
</Grid.ColumnDefinitions>
38+
<ui:Button Grid.Column="0" Content="Open change log" MinWidth="140" Click="OpenLogButton_Click"/>
39+
<StackPanel Grid.Column="2" Orientation="Horizontal" HorizontalAlignment="Right">
40+
<!-- The whole point of the window: Verify used to report a count
41+
and leave the user with nowhere to go. -->
42+
<ui:Button x:Name="FixButton" Content="Fix these settings"
43+
Appearance="Primary" MinWidth="150" Margin="0,0,8,0"
44+
Click="FixButton_Click"/>
45+
<ui:Button x:Name="CloseButton" Content="Close" MinWidth="90" Click="CloseButton_Click"/>
46+
</StackPanel>
47+
</Grid>
48+
49+
<StackPanel DockPanel.Dock="Top" Margin="0,0,0,12">
50+
<TextBlock x:Name="HeaderText" FontSize="16" FontWeight="SemiBold" TextWrapping="Wrap"/>
51+
<TextBlock x:Name="SubText"
52+
Foreground="{DynamicResource TextFillColorSecondaryBrush}"
53+
FontSize="12" Margin="0,3,0,0"
54+
TextWrapping="Wrap"/>
55+
</StackPanel>
56+
57+
<ScrollViewer VerticalScrollBarVisibility="Auto">
58+
<ItemsControl x:Name="ItemsList">
59+
<ItemsControl.ItemTemplate>
60+
<DataTemplate>
61+
<ui:Card Padding="14" Margin="0,0,0,8">
62+
<StackPanel>
63+
<Grid>
64+
<Grid.ColumnDefinitions>
65+
<ColumnDefinition Width="*"/>
66+
<ColumnDefinition Width="Auto"/>
67+
</Grid.ColumnDefinitions>
68+
<TextBlock Grid.Column="0" Text="{Binding Description}"
69+
FontWeight="SemiBold" VerticalAlignment="Center"
70+
TextWrapping="Wrap"/>
71+
<!-- An unmonitored setting is exactly the case that made
72+
Verify and the Status count disagree, so it is labelled
73+
rather than silently mixed in. -->
74+
<Border Grid.Column="1" Visibility="{Binding UnmonitoredBadgeVisibility}"
75+
Background="{DynamicResource SystemFillColorNeutralBackgroundBrush}"
76+
CornerRadius="3" Padding="6,1" Margin="8,0,0,0"
77+
VerticalAlignment="Center">
78+
<TextBlock Text="not monitored"
79+
Foreground="{DynamicResource TextFillColorSecondaryBrush}"
80+
FontSize="10" FontWeight="SemiBold"/>
81+
</Border>
82+
</Grid>
83+
<TextBlock Margin="0,4,0,0" FontSize="12">
84+
<Run Text="Now: "/><Run Text="{Binding CurrentValue, Mode=OneWay}"
85+
Foreground="{DynamicResource SystemFillColorCautionBrush}" FontWeight="SemiBold"/>
86+
<Run Text=""/>
87+
<Run Text="You want: "/><Run Text="{Binding DesiredValue, Mode=OneWay}"
88+
Foreground="{DynamicResource TextFillColorSecondaryBrush}"/>
89+
</TextBlock>
90+
<TextBlock Margin="0,6,0,0" FontSize="11"
91+
Foreground="{DynamicResource TextFillColorTertiaryBrush}"
92+
TextWrapping="Wrap"
93+
Text="{Binding SectionNote}"/>
94+
</StackPanel>
95+
</ui:Card>
96+
</DataTemplate>
97+
</ItemsControl.ItemTemplate>
98+
</ItemsControl>
99+
</ScrollViewer>
100+
</DockPanel>
101+
</Grid>
102+
</ui:FluentWindow>

0 commit comments

Comments
 (0)