Settings: Add pending changes tracking and apply confirmation dialog#14425
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an “Apply changes” workflow to the WinUI-based WSL Settings app by tracking configuration edits as pending, and only committing them to disk after explicit user confirmation (optionally followed by wsl --shutdown).
Changes:
- Introduces pending-changes tracking in
IWslConfigService/WslConfigService, with a newPendingChangesChangedevent and commit/clear APIs. - Updates Settings view models/pages to react to pending state (
HasPendingChanges) and show an “Apply changes” button. - Adds an apply confirmation dialog that summarizes pending edits and offers “Shutdown WSL now” vs “Later”.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/windows/wslsettings/Views/Settings/SettingsApplyHelper.cs | New helper that builds and shows the apply/confirm dialog and triggers commit + optional shutdown. |
| src/windows/wslsettings/Views/Settings/DeveloperPage.xaml(.cs) | Adds “Apply changes” button and hooks pending-changes notifications. |
| src/windows/wslsettings/Views/Settings/FileSystemPage.xaml(.cs) | Adds “Apply changes” button and hooks pending-changes notifications. |
| src/windows/wslsettings/Views/Settings/MemAndProcPage.xaml(.cs) | Adds “Apply changes” button and hooks pending-changes notifications. |
| src/windows/wslsettings/Views/Settings/NetworkingPage.xaml(.cs) | Adds “Apply changes” button and hooks pending-changes notifications. |
| src/windows/wslsettings/Views/Settings/OptionalFeaturesPage.xaml(.cs) | Adds “Apply changes” button and hooks pending-changes notifications. |
| src/windows/wslsettings/ViewModels/Settings/WslConfigSettingViewModel.cs | Adds HasPendingChanges property + handler to update UI when pending state changes. |
| src/windows/wslsettings/Services/WslConfigService.cs | Implements pending dictionary, commit/clear APIs, and new pending-changes event. |
| src/windows/wslsettings/Contracts/Services/IWslConfigService.cs | Extends contract with pending-changes APIs and WslConfigPendingChange model. |
| src/windows/wslsettings/CMakeLists.txt | Adds the new helper source file to the build. |
| localization/strings/en-US/Resources.resw | Adds localized strings for apply dialog/buttons and error messages. |
|
This seems like a lot of code to handle this. Instead could we just keep track of the config struct when we launch and compare it to the current config (and then update that initial struct when we click apply / shutdown / whatever). |
| var changeLines = new List<string>(pendingChanges.Count); | ||
| foreach (var change in pendingChanges) | ||
| { | ||
| changeLines.Add($"- {GetSettingDisplayName(change.ConfigEntry)}: {FormatSettingValue(change.PendingSetting)}"); |
There was a problem hiding this comment.
The pending-changes lines are constructed with hardcoded punctuation/formatting ("- {name}: {value}"). This makes part of the user-facing dialog content non-localizable and may not read well in all locales. Consider using a localized format string/resource (or a structured UI like a ListView) to format each entry.
| changeLines.Add($"- {GetSettingDisplayName(change.ConfigEntry)}: {FormatSettingValue(change.PendingSetting)}"); | |
| changeLines.Add(string.Format( | |
| "Settings_ApplyChangesDialogChangeLineFormat".GetLocalized(), | |
| GetSettingDisplayName(change.ConfigEntry), | |
| FormatSettingValue(change.PendingSetting))); |
There was a problem hiding this comment.
The name and value are both already localized. The only hardcoded pieces are the bullet dash and colon. Won't fix for now unless reviewers say otherwise.
| @@ -27,41 +33,147 @@ public WslConfigService() | |||
| _wslConfigFileSystemWatcher.Renamed += OnWslConfigFileChanged; | |||
|
|
|||
| _wslConfigFileSystemWatcher!.EnableRaisingEvents = true; | |||
|
|
|||
| // Read all current config values into both snapshots so they start in sync | |||
| lock (_wslCoreConfigInterfaceLockObj!) | |||
| { | |||
| RefreshSnapshots_NoLock(); | |||
| } | |||
| } | |||
|
|
|||
| ~WslConfigService() | |||
| { | |||
| DisposeSnapshot(_baselineSnapshot); | |||
| DisposeSnapshot(_workingSnapshot); | |||
| WslCoreConfigInterface.FreeWslConfig(_wslConfig); | |||
| WslCoreConfigInterface.FreeWslConfig(_wslConfigDefaults); | |||
| } | |||
There was a problem hiding this comment.
WslConfigService creates a FileSystemWatcher but never disposes it. Since this service is a singleton, the watcher will keep OS handles for the app lifetime; and even at shutdown the finalizer currently doesn't dispose it. Consider implementing IDisposable and disposing/unsubscribing the watcher (and disabling events) when the app shuts down.
…/microsoft/WSL into user/dabhatti/wslSettingsRestart
| public required IWslConfigSetting PendingSetting { get; init; } | ||
| } | ||
|
|
||
| public interface IWslConfigSetting |
There was a problem hiding this comment.
GetPendingChanges() returns WslConfigPendingChange objects containing IWslConfigSetting instances that wrap unmanaged allocations (LibWsl.WslConfigSetting implements IDisposable). Since IWslConfigSetting doesn’t expose disposal, callers (like the Apply dialog) can’t deterministically free these clones, which can lead to avoidable unmanaged-memory growth. Consider changing WslConfigPendingChange to carry managed value snapshots (e.g., entry + strongly-typed value fields), or expose a disposable type (e.g., WslConfigSettingManaged / IDisposable) with clear ownership semantics.
| public interface IWslConfigSetting | |
| public interface IWslConfigSetting : System.IDisposable |
There was a problem hiding this comment.
Pre-existing. Can take it up in future PR
| public IReadOnlyList<WslConfigPendingChange> GetPendingChanges() | ||
| { | ||
| lock (_wslCoreConfigInterfaceLockObj!) | ||
| { | ||
| var changes = new List<WslConfigPendingChange>(_pendingValues.Count); | ||
| foreach (var (entry, value) in _pendingValues) | ||
| { | ||
| changes.Add(new WslConfigPendingChange | ||
| { | ||
| ConfigEntry = entry, | ||
| PendingValue = value, | ||
| }); | ||
| } | ||
| return changes; | ||
| } |
There was a problem hiding this comment.
GetPendingChanges() builds the UI list by iterating _pendingValues directly. Dictionary enumeration order isn’t a contractual UI ordering, so the apply dialog can show changes in a surprising order across runs/edits. Consider returning the list in a stable order (e.g., by WslConfigEntry or by localized display name) so the dialog content is predictable.
There was a problem hiding this comment.
I'm not sure if this is a real issue.
| event WslConfigChangedEventHandler WslConfigChanged; | ||
| public delegate void PendingChangesChangedEventHandler(); |
There was a problem hiding this comment.
The new PendingChangesChanged event doesn’t document what “changed” means (e.g., any edit to the pending set vs only empty/non-empty transitions). Since consumers may rely on this for more than button visibility later, add an XML doc comment clarifying the exact semantics (and/or consider a more specific name like HasPendingChangesChanged).
| event WslConfigChangedEventHandler WslConfigChanged; | |
| public delegate void PendingChangesChangedEventHandler(); | |
| event WslConfigChangedEventHandler WslConfigChanged; | |
| /// <summary> | |
| /// Raised whenever the in-memory collection of pending changes is modified. | |
| /// </summary> | |
| /// <remarks> | |
| /// This event is fired after a pending change has been added, updated, or removed. It includes | |
| /// transitions where <see cref="HasPendingChanges"/> changes value, as well as modifications to | |
| /// the contents returned by <see cref="GetPendingChanges"/> while remaining non-empty. Handlers | |
| /// should query <see cref="HasPendingChanges"/> and/or <see cref="GetPendingChanges"/> to | |
| /// observe the new state. | |
| /// </remarks> | |
| public delegate void PendingChangesChangedEventHandler(); | |
| /// <summary> | |
| /// Occurs when the in-memory collection of pending changes is modified. | |
| /// </summary> |
| var changeLines = new List<string>(pendingChanges.Count); | ||
| foreach (var change in pendingChanges) | ||
| { | ||
| changeLines.Add($"- {GetSettingDisplayName(change.ConfigEntry)}: {FormatValue(change.ConfigEntry, change.PendingValue)}"); | ||
| } |
There was a problem hiding this comment.
The pending-changes list is displayed in whatever order GetPendingChanges() returns. If that order isn’t stable, the dialog text can jump around between edits, making it harder to review changes. Consider sorting pendingChanges (or sorting in the service) before building changeLines so the dialog always shows a consistent order.
benhillis
left a comment
benhillis
left a comment
There was a problem hiding this comment.
A couple high level comments:
- The apply button code is copy / pasted a bunch of times
- I think it would be cleaner to use a second native config handle to keep track of the pending config changes, and when the user clicks save write that data.
benhillis
left a comment
benhillis
left a comment
There was a problem hiding this comment.
Sorry for the delay in getting this review. I think this looks like a good improvement to the settings app.
|
@copilot resolve the merge conflicts in this pull request |
…ttingsRestart # Conflicts: # src/windows/wslsettings/Services/WslConfigService.cs Co-authored-by: dabhattimsft <47045043+dabhattimsft@users.noreply.github.com>
Resolved in ccf2f77. Merged master into the PR branch; the only conflict was in |
![github-code-quality[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
…14425) * initial commit * cp * cp * pr feedback * cp * pr feedback * pr feedback * cp * pr feedback * refactor * pr feedback * cp * Move Apply Changes button to ShellPage to deduplicate across settings pages * PR feedback 1 --------- Co-authored-by: Darshak Bhatti <dabhatti@micorsoft.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Summary of the Pull Request
Adds "Apply Changes" workflow to the WSL Settings app. Settings changes are now tracked as pending and only written to disk when the user confirms, giving clear visibility into what will change.
PR Checklist
Detailed Description of the Pull Request / Additional comments
SetWslConfigSetting no longer writes to disk immediately. Instead it records changes in a pending dictionary, comparing against the persisted value to auto-remove no-op edits.
New PendingChangesChanged event so the UI can react when the pending set becomes non-empty or empty.
Validation Steps Performed
Manually tested